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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1336441779.6190.136.camel@yhuang-dev>
Date:	Tue, 08 May 2012 09:49:39 +0800
From:	Huang Ying <ying.huang@...el.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	huang ying <huang.ying.caritas@...il.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>, ming.m.lin@...el.com,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	Zheng Yan <zheng.z.yan@...el.com>
Subject: Re: [RFC v2 4/5] ACPI, PM, Specify lowest allowed state for device
 sleep state

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@...k.pl> 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@...el.com>
> > >> ---
> > >>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/