On Monday, 2 July 2007 07:49, David Brownell wrote:
> On Saturday 30 June 2007, Rafael J. Wysocki wrote:
> > From: Shaohua Li <[EMAIL PROTECTED]>, Rafael J. Wysocki <[EMAIL PROTECTED]>
> >
> > Based on the David Brownell's patch at
> > http://marc.info/?l=linux-acpi&m=117873972806360&w=2
>
> I hope someone's going to refresh the PCI glue calling this, then...
> making pci_choose_state() work was the goal of that patch!!
>
> Also the updates to teach how the PCI root bridge wakeup capabilities
> fit into the equation, for non-mainboard devices (all kinds of add-on
> cards that talk PCI) ... that stuff was unclear to me, which is most
> of why I left it out of that patch.
>
> Correct me if I'm wrong here, but nothing else in this patch set
> depends on this particular patch ... yes?
That's correct.
> > Add a helper routine returning the lowest power (highest number) ACPI device
> > power state that given device can be in while the system is in the sleep
> > state
> > indicated by acpi_target_sleep_state .
> >
> > Signed-off-by: Rafael J. Wysocki <[EMAIL PROTECTED]>
> > ---
> > drivers/acpi/sleep/main.c | 51
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > include/acpi/acpi_bus.h | 2 +
> > 2 files changed, 53 insertions(+)
> >
> > Index: linux-2.6.22-rc6-mm1/drivers/acpi/sleep/main.c
> > ===================================================================
> > --- linux-2.6.22-rc6-mm1.orig/drivers/acpi/sleep/main.c 2007-06-30
> > 12:18:47.000000000 +0200
> > +++ linux-2.6.22-rc6-mm1/drivers/acpi/sleep/main.c 2007-06-30
> > 12:18:58.000000000 +0200
> > @@ -255,6 +255,57 @@ static struct hibernation_ops acpi_hiber
> > };
> > #endif /* CONFIG_SOFTWARE_SUSPEND */
> >
> > +/**
> > + * acpi_pm_device_sleep_state - return the lowest power (highest number)
> > + * ACPI device power state given device can be
> > + * in while the system is in the sleep state
> > + * indicated by %acpi_target_sleep_state
> > + * @handle: Represents the device the state is evaluated for
> > + */
> > +
> > +int acpi_pm_device_sleep_state(acpi_handle handle)
>
> Yeah, moving this from the PCI glue to the ACPI core is probably
> better. But I'd like to see the comment use standard kerneldoc ...
> that is, the descriptive paragraph follows a set of (one line)
> summaries of the function and its parameter(s). The description
> needs to cover the fault returns too.
OK
> Also, recall that this was originally PCI-specific. A generalized
> approach would return a range of states, not a single state that
> embeds a particular policy that may not be universally appropriate...
Via pointers and the return value equal to 0 on success?
> > +{
> > + char acpi_method[] = "_SxD";
> > + unsigned long d_min, d_max;
> > + struct acpi_device *dev;
> > +
> > + if (!handle || ACPI_FAILURE(acpi_bus_get_device(handle, &dev))) {
> > + printk(KERN_ERR "ACPI handle has no context!\n");
>
> The description above should cover this fault return ... the caller
> would normally need some default to apply in such cases, too.
>
> (Example: PCI could just look at the PCI PM resource and see what
> states are supported there. Choosing PCI_D2, where it's available,
> could eliminate various driver re-initialization problems. That
> might make some video code work better, from what I'm told ...)
OK
> > + return -ENODEV;
> > + }
> > + acpi_method[2] = '0' + acpi_target_sleep_state;
> > + /*
> > + * If the sleep state is S0, we will return D3, but if the device has
> > + * _S0W, we will use the value from _S0W
>
> There was also the assumption that if it can wake, it can do
> so from D3 *or* there will be an _SxD method... remembering
> that lots of systems don't have ACPI 3.0 _SxW methods.
>
> But returning D3 is not necessarily best, here...
>
>
> > + */
> > + d_min = ACPI_STATE_D3;
> > + d_max = ACPI_STATE_D3;
>
> Other than the lack of empty lines separating code paragraphs ...
> I recall choosing "d_min = ACPI_STATE_D3" with specific reference
> to PCI. It's not clear to me that's appropriate for non-PCI
> devices.
>
> The logic was that PCI devices can all support PCI_D0 and PCI_D3.
> For non-PCI devices we can't actually know that. So "min" should
> probably default to ACPI_STATE_D0. If this returns a range, then
> such issues can stay out of this code.
The ACPI spec says that all devices must support D0 and D3.
> > + /*
> > + * If present, _SxD methods give the minimum D-state we may use
> > + * for each S-state ... with lowest latency state switching.
> > + *
> > + * We rely on acpi_evaluate_integer() not clobbering the integer
> > + * provided -- that's our fault recovery, we ignore retval.
> > + */
> > + if (acpi_target_sleep_state > ACPI_STATE_S0)
> > + acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
>
> ... "lowest latency" implies avoiding ACPI D3. Thing is, in my
> testing with PCI, most devices didn't have _SxD methods, so the
> only way to return anything other than PCI_D0 (i.e. some state
> that saved power) was to force a different default. D3 was the
> only option that always worked.
I think that usually D3 will be returned here.
> > +
> > + /*
> > + * If _PRW says we can wake from the upcoming system state, the _SxD
> > + * value can wake ... and we'll assume a wakeup-aware driver. If _SxW
> > + * methods exist (ACPI 3.x), they give the lowest power D-state that
> > + * can also wake the system. _S0W can be valid.
> > + */
> > + if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> > + (dev->wakeup.state.enabled &&
>
> This used device_may_wakeup() before. That ACPI flag is not a
> direct analogue ... without looking again at the issues, I'd
> say the right solution is to phase out the ACPI-only flags in
> new code.
Hm, I'm not sure. This is an ACPI routine after all ...
> Maybe just expect the caller to pass the results
> of device_may_wakeup() in ... or update the signature to accept
> a "struct device", and fetch the handle from there. (That'd
> be a better match for most callers, I'd think...)
In that case it would be nicer to pass 'struct acpi_device *' rahter than
'struct device *', IMO.
> > + dev->wakeup.sleep_state <= acpi_target_sleep_state)) {
> > + d_max = d_min;
>
> None of my systems happend to have _SxW methods to execute.
> So with few _SxD methods, most of the time d_max never changed.
> All the more reason to return both min and max...
>
>
> > + acpi_method[3] = 'W';
> > + acpi_evaluate_integer(handle, acpi_method, NULL, &d_max);
> > + }
> > + return d_max;
>
> ... so there's a range of from d_min to d_max that would be OK
> with ACPI, but this particular routine is only showing one end
> of the range. For a general purpose routine, that doesn't seem
> like it's obviously the best answer.
We can return a range.
Greetings,
Rafael
--
"Premature optimization is the root of all evil." - Donald Knuth
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
