On Monday, 2 July 2007 07:49, David Brownell wrote: > On Saturday 30 June 2007, Rafael J. Wysocki wrote: > > From: Shaohua Li <shaohua.li@xxxxxxxxx>, Rafael J. Wysocki <rjw@xxxxxxx> > > > > 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 <rjw@xxxxxxx> > > --- > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
