Re: [RFC v2 4/5] ACPI, PM, Specify lowest allowed state for device sleep state

From: Rafael J. Wysocki
Date: Mon May 07 2012 - 17:10:40 EST

On Saturday, May 05, 2012, huang ying wrote:
> On Sat, May 5, 2012 at 4:10 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> > On Friday, May 04, 2012, Huang Ying wrote:
> >> Lower device sleep state can save more power, but has more exit
> >> latency too. Sometimes, to satisfy some power QoS and other
> >> requirement, we need to constrain the lowest device sleep state.
> >>
> >> In this patch, a parameter to specify lowest allowed state for
> >> acpi_pm_device_sleep_state is added. So that the caller can enforce
> >> the constraint via the parameter.
> >>
> >> Signed-off-by: Huang Ying <ying.huang@xxxxxxxxx>
> >> ---
> >> drivers/acpi/sleep.c | 18 +++++++++++++++---
> >> drivers/pci/pci-acpi.c | 3 ++-
> >> drivers/pnp/pnpacpi/core.c | 4 ++--
> >> include/acpi/acpi_bus.h | 6 +++---
> >> 4 files changed, 22 insertions(+), 9 deletions(-)
> >>
> >> --- a/drivers/acpi/sleep.c
> >> +++ b/drivers/acpi/sleep.c
> >> @@ -677,6 +677,7 @@ int acpi_suspend(u32 acpi_state)
> >> * @dev: device to examine; its driver model wakeup flags control
> >> * whether it should be able to wake up the system
> >> * @d_min_p: used to store the upper limit of allowed states range
> >> + * @d_max_in: specify the lowest allowed states
> >> * Return value: preferred power state of the device on success, -ENODEV on
> >> * failure (ie. if there's no 'struct acpi_device' for @dev)
> >> *
> >> @@ -693,7 +694,7 @@ int acpi_suspend(u32 acpi_state)
> >> * via @wake.
> >> */
> >>
> >> -int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p)
> >> +int acpi_pm_device_sleep_state(struct device *dev, int *d_min_p, int d_max_in)
> >> {
> >> acpi_handle handle = DEVICE_ACPI_HANDLE(dev);
> >> struct acpi_device *adev;
> >> @@ -704,11 +705,14 @@ int acpi_pm_device_sleep_state(struct de
> >> printk(KERN_DEBUG "ACPI handle has no context!\n");
> >> return -ENODEV;
> >> }
> >> + d_max_in = clamp_t(int, d_max_in, ACPI_STATE_D0, ACPI_STATE_D3);
> >
> > Shouldn't that be clamp_val(), rather?
>
> Yes. clamp_val() is sufficient here.
>
> >> 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
> >> + * If the sleep state is S0, the lowest limit from ACPI is D3,
> >> + * but if the device has _S0W, we will use the value from _S0W
> >> + * as the lowest limit from ACPI. Finally, we will constrain
> >> + * the lowest limit with the specified one.
> >> */
> >> d_min = ACPI_STATE_D0;
> >> d_max = ACPI_STATE_D3;
> >> @@ -754,6 +758,14 @@ int acpi_pm_device_sleep_state(struct de
> >>
> >> if (d_min_p)
> >> *d_min_p = d_min;
> >> + /* constrain d_max with specified lowest limit (max number) */
> >> + if (d_max > d_max_in) {
> >> + d_max = d_max_in;
> >> + for (;d_max > d_min; d_max--) {
> >
> > Well, why didn't you do
> >
> > + for (d_max = d_max_in; d_max > d_min; d_max--)
>
> Because I think it is possible that d_max < d_max_in.

I mean:

+ if (d_max > d_max_in) {
+ for (d_max = d_max_in; d_max > d_min; d_max--) {

The assignment followed by the for () loop without the start instruction looks
odd.

> >> + if (adev->power.states[d_max].flags.valid)
> >> + break;
> >> + }
> >> + }
> >
> > And what if d_min > d_max_in ?
>
> I think that means something bad happens. Maybe we can do something as follow
>
> if (d_min > d_max_in) {
> pr_warning("acpi_pm_device_sleep_state: the specified lowest
> state is higher than the highest state from ACPI!");
> d_max_in = d_min;

Well, what about returning -EINVAL in that case?

> }
> if (d_max > d_max_in) {
> ...
> }
>
> >> return d_max;
> >> }
> >> #endif /* CONFIG_PM */
> >> --- a/drivers/pci/pci-acpi.c
> >> +++ b/drivers/pci/pci-acpi.c
> >> @@ -189,7 +189,8 @@ static pci_power_t acpi_pci_choose_state
> >> {
> >> int acpi_state;
> >>
> >> - acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL);
> >> + acpi_state = acpi_pm_device_sleep_state(&pdev->dev, NULL,
> >> + ACPI_STATE_D3);
> >> if (acpi_state < 0)
> >> return PCI_POWER_ERROR;
> >>
> >> --- a/drivers/pnp/pnpacpi/core.c
> >> +++ b/drivers/pnp/pnpacpi/core.c
> >> @@ -170,8 +170,8 @@ static int pnpacpi_suspend(struct pnp_de
> >> }
> >>
> >> if (acpi_bus_power_manageable(handle)) {
> >> - int power_state = acpi_pm_device_sleep_state(&dev->dev, NULL);
> >> -
> >> + int power_state = acpi_pm_device_sleep_state(&dev->dev, NULL,
> >> + ACPI_STATE_D3);
> >> if (power_state < 0)
> >> power_state = (state.event == PM_EVENT_ON) ?
> >> ACPI_STATE_D0 : ACPI_STATE_D3;
> >> --- a/include/acpi/acpi_bus.h
> >> +++ b/include/acpi/acpi_bus.h
> >> @@ -383,13 +383,13 @@ int acpi_enable_wakeup_device_power(stru
> >> int acpi_disable_wakeup_device_power(struct acpi_device *dev);
> >>
> >> #ifdef CONFIG_PM
> >> -int acpi_pm_device_sleep_state(struct device *, int *);
> >> +int acpi_pm_device_sleep_state(struct device *, int *, int);
> >> #else
> >> -static inline int acpi_pm_device_sleep_state(struct device *d, int *p)
> >> +static inline int acpi_pm_device_sleep_state(struct device *d, int *p, int m)
> >> {
> >> if (p)
> >> *p = ACPI_STATE_D0;
> >> - return ACPI_STATE_D3;
> >> + return m == ACPI_STATE_D3 ? m : ACPI_STATE_D0;
> >
> > Shouldn't m be returned (so long as it is between D0 and D3 inclusive)? IOW:
> >
> > + return (m >= ACPI_STATE_D0 && m <= ACPI_STATE_D3) ? m : ACPI_STATE_D0;
>
> My original idea is that only D0 and D3 is guaranteed to be valid for
> the device. If that need not to be considered here, you one is
> better.

No, it need not.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/