[PATCH] ACPI: Fix acpi_pm_device_sleep_state()

[PATCH] ACPI: Fix acpi_pm_device_sleep_state()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Fix acpi_pm_device_sleep_state() to return the value returned
by _SxD if the device is supposed to wake up the system from
given sleep state and the evaluation of _SxW fails (e.g. _SxW
is not present).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep/main.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -472,11 +472,20 @@ int acpi_pm_device_sleep_state(struct de
 	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
 	    (wake && adev->wakeup.state.enabled &&
 	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
+		acpi_status status;
+
 		acpi_method[3] = 'W';
-		acpi_evaluate_integer(handle, acpi_method, NULL, &d_max);
-		/* Sanity check */
-		if (d_max < d_min)
+		status = acpi_evaluate_integer(handle, acpi_method, NULL,
+						&d_max);
+		if (ACPI_FAILURE(status)) {
+			d_max = d_min;
+		} else if (d_max < d_min) {
+			/* Warn the user of the broken DSDT */
+			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
+				acpi_method);
+			/* Sanitize it */
 			d_min = d_max;
+		}
 	}
 
 	if (d_min_p)

Re: [PATCH] ACPI: Fix acpi_pm_device_sleep_state()

[Len Brown]

On Thursday 10 January 2008 18:10, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Fix acpi_pm_device_sleep_state() to return the value returned
> by _SxD if the device is supposed to wake up the system from
> given sleep state and the evaluation of _SxW fails (e.g. _SxW
> is not present).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/sleep/main.c |   15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/sleep/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/sleep/main.c
> +++ linux-2.6/drivers/acpi/sleep/main.c
> @@ -472,11 +472,20 @@ int acpi_pm_device_sleep_state(struct de
>  	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
>  	    (wake && adev->wakeup.state.enabled &&
>  	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
> +		acpi_status status;
> +
>  		acpi_method[3] = 'W';
> -		acpi_evaluate_integer(handle, acpi_method, NULL, &d_max);
> -		/* Sanity check */
> -		if (d_max < d_min)
> +		status = acpi_evaluate_integer(handle, acpi_method, NULL,
> +						&d_max);
> +		if (ACPI_FAILURE(status)) {
> +			d_max = d_min;
> +		} else if (d_max < d_min) {
> +			/* Warn the user of the broken DSDT */
> +			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
> +				acpi_method);
> +			/* Sanitize it */
>  			d_min = d_max;
> +		}
>  	}
>  
>  	if (d_min_p)
> 

this patch looks correct, it fixes the bug where 
_SxD=2, _PRW=x, _SxW was absent, we would return D3 as legal,
when we should have returned D2.

applied to acpi test.

BTW.  We currently don't use *d_min_p
and I can't imagine when and why we ever would,
so it would be fine with me if you simplify by deleting it.

Also, the last line of this block comment can be deleted,
though that was true before this patch also:

acpi_pci_choose_state() block comment can now be updated to delete this line:

* currently we simply return _SxD, if present.

thanks,
-Len

Re: [PATCH] ACPI: Fix acpi_pm_device_sleep_state()

[Rafael J. Wysocki]

On Saturday, 12 of January 2008, Len Brown wrote:
> On Thursday 10 January 2008 18:10, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Fix acpi_pm_device_sleep_state() to return the value returned
> > by _SxD if the device is supposed to wake up the system from
> > given sleep state and the evaluation of _SxW fails (e.g. _SxW
> > is not present).
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/acpi/sleep/main.c |   15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > Index: linux-2.6/drivers/acpi/sleep/main.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/sleep/main.c
> > +++ linux-2.6/drivers/acpi/sleep/main.c
> > @@ -472,11 +472,20 @@ int acpi_pm_device_sleep_state(struct de
> >  	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> >  	    (wake && adev->wakeup.state.enabled &&
> >  	     adev->wakeup.sleep_state <= acpi_target_sleep_state)) {
> > +		acpi_status status;
> > +
> >  		acpi_method[3] = 'W';
> > -		acpi_evaluate_integer(handle, acpi_method, NULL, &d_max);
> > -		/* Sanity check */
> > -		if (d_max < d_min)
> > +		status = acpi_evaluate_integer(handle, acpi_method, NULL,
> > +						&d_max);
> > +		if (ACPI_FAILURE(status)) {
> > +			d_max = d_min;
> > +		} else if (d_max < d_min) {
> > +			/* Warn the user of the broken DSDT */
> > +			printk(KERN_WARNING "ACPI: Wrong value from %s\n",
> > +				acpi_method);
> > +			/* Sanitize it */
> >  			d_min = d_max;
> > +		}
> >  	}
> >  
> >  	if (d_min_p)
> > 
> 
> this patch looks correct, it fixes the bug where 
> _SxD=2, _PRW=x, _SxW was absent, we would return D3 as legal,
> when we should have returned D2.
> 
> applied to acpi test.
> 
> BTW.  We currently don't use *d_min_p
> and I can't imagine when and why we ever would,
> so it would be fine with me if you simplify by deleting it.

It was requested by someone.  I can imagine that if _SxW > _SxD, a driver can
decide which state to choose on the basis of some policy we don't know of.

> Also, the last line of this block comment can be deleted,
> though that was true before this patch also:

Not sure what you mean here ...
 
> acpi_pci_choose_state() block comment can now be updated to delete this line:
> 
> * currently we simply return _SxD, if present.

Yes, I'll do that in a separate patch.

Thanks,
Rafael

Re: [PATCH] ACPI: Fix acpi_pm_device_sleep_state()

[Len Brown]

> > BTW.  We currently don't use *d_min_p
> > and I can't imagine when and why we ever would,
> > so it would be fine with me if you simplify by deleting it.
> 
> It was requested by someone.  I can imagine that if _SxW > _SxD, a driver can
> decide which state to choose on the basis of some policy we don't know of.

I think that for the S3 and S4 states, we'll always use the deepest
state that the constraints allow while not breaking system wake.

I haven't thought much about "device wake" -- ie _PRW in S0.
Maybe some non-trivial device specific policy would be useful there
some day.  Of course the device would have to know the relative latency
of the different D-states for such a policy to make a useful.
And I guess they can infer that Dx state latency on a class basis...

-Len

> > Also, the last line of this block comment can be deleted,
> > though that was true before this patch also:
> 
> Not sure what you mean here ...
>  
> > acpi_pci_choose_state() block comment can now be updated to delete this line:
> > 
> > * currently we simply return _SxD, if present.
> 
> Yes, I'll do that in a separate patch.
> 
> Thanks,
> Rafael
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>