Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

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

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


ying.huang at intel

May 4, 2012, 1:13 AM

Post #1 of 5 (123 views)
Permalink
[RFC v2 4/5] ACPI, PM, Specify lowest allowed state for device sleep state

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 [at] intel>
---
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);

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--) {
+ if (adev->power.states[d_max].flags.valid)
+ break;
+ }
+ }
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;
}
#endif

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


rjw at sisk

May 4, 2012, 1:10 PM

Post #2 of 5 (114 views)
Permalink
Re: [RFC v2 4/5] ACPI, PM, Specify lowest allowed state for device sleep state [In reply to]

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 [at] intel>
> ---
> 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?

>
> 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--)

> + if (adev->power.states[d_max].flags.valid)
> + break;
> + }
> + }

And what if d_min > 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;

> }
> #endif

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


huang.ying.caritas at gmail

May 5, 2012, 12:25 AM

Post #3 of 5 (112 views)
Permalink
Re: [RFC v2 4/5] ACPI, PM, Specify lowest allowed state for device sleep state [In reply to]

On Sat, May 5, 2012 at 4:10 AM, Rafael J. Wysocki <rjw [at] sisk> 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 [at] intel>
>> ---
>>  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.

>> +                     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;
}
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.

Best Regards,
Huang Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


rjw at sisk

May 7, 2012, 2:15 PM

Post #4 of 5 (113 views)
Permalink
Re: [RFC v2 4/5] ACPI, PM, Specify lowest allowed state for device sleep state [In reply to]

On Saturday, May 05, 2012, huang ying wrote:
> On Sat, May 5, 2012 at 4:10 AM, Rafael J. Wysocki <rjw [at] sisk> 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 [at] intel>
> >> ---
> >> 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 [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


ying.huang at intel

May 7, 2012, 6:49 PM

Post #5 of 5 (110 views)
Permalink
Re: [RFC v2 4/5] ACPI, PM, Specify lowest allowed state for device sleep state [In reply to]

On Mon, 2012-05-07 at 23:15 +0200, Rafael J. Wysocki wrote:
> On Saturday, May 05, 2012, huang ying wrote:
> > On Sat, May 5, 2012 at 4:10 AM, Rafael J. Wysocki <rjw [at] sisk> 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 [at] intel>
> > >> ---
> > >> 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.

Oh, Yes. I will change this.

> > >> + 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?

Yes. That is reasonable because it's a invalid parameter.

Best Regards,
Huang Ying


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

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.