[PATCH] ACPI / PM: Add check preventing transitioning to non-D0 state from D3.

[PATCH] ACPI / PM: Add check preventing transitioning to non-D0 state from D3.

[Lv Zheng]

No power transitioning from D3 state to a non-D0 state is allowed.
This patch also cleans up device power updating code in the
acpi_device_set_power() as it should already been updated in the
acpi_power_transition().

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/bus.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 07a20ee..12c1b51 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -293,6 +293,12 @@ int acpi_device_set_power(struct acpi_device *device, int state)
 			      " state than parent\n");
 		return -ENODEV;
 	}
+	if (device->parent->power.state >= ACPI_STATE_D3_HOT &&
+	    state != ACPI_STATE_D0) {
+		printk(KERN_WARNING PREFIX
+			      "Cannot transition to non-D0 state from D3\n");
+		return -ENODEV;
+	}
 
 	/* For D3cold we should execute _PS3, not _PS4. */
 	if (state == ACPI_STATE_D3_COLD)
@@ -341,7 +347,6 @@ int acpi_device_set_power(struct acpi_device *device, int state)
 			      "Device [%s] failed to transition to %s\n",
 			      device->pnp.bus_id, state_string(state));
 	else {
-		device->power.state = state;
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Device [%s] transitioned to %s\n",
 				  device->pnp.bus_id, state_string(state)));
-- 
1.7.10

Re: [PATCH] ACPI / PM: Add check preventing transitioning to non-D0 state from D3.

[Rafael J. Wysocki]

On Thursday, November 15, 2012 12:25:01 PM Lv Zheng wrote:
> No power transitioning from D3 state to a non-D0 state is allowed.
> This patch also cleans up device power updating code in the
> acpi_device_set_power() as it should already been updated in the
> acpi_power_transition().
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/bus.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 07a20ee..12c1b51 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -293,6 +293,12 @@ int acpi_device_set_power(struct acpi_device *device, int state)
>  			      " state than parent\n");
>  		return -ENODEV;
>  	}
> +	if (device->parent->power.state >= ACPI_STATE_D3_HOT &&

Why parent?  The device's own power state shouldn't be D3, right?

> +	    state != ACPI_STATE_D0) {
> +		printk(KERN_WARNING PREFIX
> +			      "Cannot transition to non-D0 state from D3\n");
> +		return -ENODEV;
> +	}

I fixed up the check and moved it into the (state < device->power.state)
block, because otherwise it would prevent transitions from D3_HOT to
D3_COLD from being carried out, but they are allowed.

>  	/* For D3cold we should execute _PS3, not _PS4. */
>  	if (state == ACPI_STATE_D3_COLD)
> @@ -341,7 +347,6 @@ int acpi_device_set_power(struct acpi_device *device, int state)
>  			      "Device [%s] failed to transition to %s\n",
>  			      device->pnp.bus_id, state_string(state));
>  	else {
> -		device->power.state = state;
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  				  "Device [%s] transitioned to %s\n",
>  				  device->pnp.bus_id, state_string(state)));
> 

Please check the linux-next branch of linux-pm.git and see if that's what you
wanted. :-)

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH] ACPI / PM: Add check preventing transitioning to non-D0 state from D3.

[Zheng, Lv]

> Please check the linux-next branch of linux-pm.git and see if that's what you
> wanted. :-)

Lots of bugs in my patch...
Thanks for fixing them.  I found it in the linux-next branch.

Best regards/Lv Zheng

Re: [PATCH] ACPI / PM: Add check preventing transitioning to non-D0 state from D3.

[Peter Wu]

Hi Lv Zheng,

I encounter a regression with your patch (Linux 3.8-rc5). On my Nvidia Optimus 
laptop, I use the bbswitch[1] kernel module to trigger a _PS3 ACPI method call 
to turn the video card off.

After this patch, I got the following in my kernel log:

    pci 0000:01:00.0: Refused to change power state, currently in D0
    ACPI: Cannot transition to non-D0 state from D3
    bbswitch: Succesfully loaded. Discrete card 0000:01:00.0 is on

The expected output would be "Discrete card 0000:01:00.0 is  off". Printing the 
contents of (acpi_device) device->power.state shows FF (ACPI_STATE_UNKNOWN). 
Should this condition be excluded from your check or is my hacky module 
outdated?

I currently workaround this issue by checking for ACPI_STATE_UNKNOWN. If that 
is the value, I assume on (overwrite device->power.state with ACPI_STATE_D0). 
Then I call pci_set_power_state(pci_dev, PCI_D3cold).

Regards,
Peter

 [1]: https://github.com/Bumblebee-Project/bbswitch

On Thursday 15 November 2012 12:25:01 Lv Zheng wrote:
> No power transitioning from D3 state to a non-D0 state is allowed.
> This patch also cleans up device power updating code in the
> acpi_device_set_power() as it should already been updated in the
> acpi_power_transition().
> 
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/bus.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 07a20ee..12c1b51 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -293,6 +293,12 @@ int acpi_device_set_power(struct acpi_device *device,
> int state) " state than parent\n");
>  		return -ENODEV;
>  	}
> +	if (device->parent->power.state >= ACPI_STATE_D3_HOT &&
> +	    state != ACPI_STATE_D0) {
> +		printk(KERN_WARNING PREFIX
> +			      "Cannot transition to non-D0 state from D3\n");
> +		return -ENODEV;
> +	}
> 
>  	/* For D3cold we should execute _PS3, not _PS4. */
>  	if (state == ACPI_STATE_D3_COLD)
> @@ -341,7 +347,6 @@ int acpi_device_set_power(struct acpi_device *device,
> int state) "Device [%s] failed to transition to %s\n",
>  			      device->pnp.bus_id, state_string(state));
>  	else {
> -		device->power.state = state;
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  				  "Device [%s] transitioned to %s\n",
>  				  device->pnp.bus_id, state_string(state)));

Re: [PATCH] ACPI / PM: Add check preventing transitioning to non-D0 state from D3.

[Rafael J. Wysocki]

On Wednesday, January 30, 2013 06:53:46 PM Peter Wu wrote:
> Hi Lv Zheng,
> 
> I encounter a regression with your patch (Linux 3.8-rc5). On my Nvidia Optimus 
> laptop, I use the bbswitch[1] kernel module to trigger a _PS3 ACPI method call 
> to turn the video card off.
> 
> After this patch, I got the following in my kernel log:
> 
>     pci 0000:01:00.0: Refused to change power state, currently in D0
>     ACPI: Cannot transition to non-D0 state from D3
>     bbswitch: Succesfully loaded. Discrete card 0000:01:00.0 is on
> 
> The expected output would be "Discrete card 0000:01:00.0 is  off". Printing the 
> contents of (acpi_device) device->power.state shows FF (ACPI_STATE_UNKNOWN). 
> Should this condition be excluded from your check or is my hacky module 
> outdated?
> 
> I currently workaround this issue by checking for ACPI_STATE_UNKNOWN. If that 
> is the value, I assume on (overwrite device->power.state with ACPI_STATE_D0). 
> Then I call pci_set_power_state(pci_dev, PCI_D3cold).

You shouldn't ever be transitioning from an uknown state to D3cold directly.
Please first transition to D0 and then to D3cold.

Can you please test the linux-next branch of the linux-pm.git tree and check
if you see this problem in there too?

Rafael


>  [1]: https://github.com/Bumblebee-Project/bbswitch
> 
> On Thursday 15 November 2012 12:25:01 Lv Zheng wrote:
> > No power transitioning from D3 state to a non-D0 state is allowed.
> > This patch also cleans up device power updating code in the
> > acpi_device_set_power() as it should already been updated in the
> > acpi_power_transition().
> > 
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > ---
> >  drivers/acpi/bus.c |    7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 07a20ee..12c1b51 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -293,6 +293,12 @@ int acpi_device_set_power(struct acpi_device *device,
> > int state) " state than parent\n");
> >  		return -ENODEV;
> >  	}
> > +	if (device->parent->power.state >= ACPI_STATE_D3_HOT &&
> > +	    state != ACPI_STATE_D0) {
> > +		printk(KERN_WARNING PREFIX
> > +			      "Cannot transition to non-D0 state from D3\n");
> > +		return -ENODEV;
> > +	}
> > 
> >  	/* For D3cold we should execute _PS3, not _PS4. */
> >  	if (state == ACPI_STATE_D3_COLD)
> > @@ -341,7 +347,6 @@ int acpi_device_set_power(struct acpi_device *device,
> > int state) "Device [%s] failed to transition to %s\n",
> >  			      device->pnp.bus_id, state_string(state));
> >  	else {
> > -		device->power.state = state;
> >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >  				  "Device [%s] transitioned to %s\n",
> >  				  device->pnp.bus_id, state_string(state)));
> --
> 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
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / PM: Add check preventing transitioning to non-D0 state from D3.

[Rafael J. Wysocki]

On Wednesday, January 30, 2013 10:55:17 PM Rafael J. Wysocki wrote:
> On Wednesday, January 30, 2013 06:53:46 PM Peter Wu wrote:
> > Hi Lv Zheng,
> > 
> > I encounter a regression with your patch (Linux 3.8-rc5). On my Nvidia Optimus 
> > laptop, I use the bbswitch[1] kernel module to trigger a _PS3 ACPI method call 
> > to turn the video card off.
> > 
> > After this patch, I got the following in my kernel log:
> > 
> >     pci 0000:01:00.0: Refused to change power state, currently in D0
> >     ACPI: Cannot transition to non-D0 state from D3
> >     bbswitch: Succesfully loaded. Discrete card 0000:01:00.0 is on
> > 
> > The expected output would be "Discrete card 0000:01:00.0 is  off". Printing the 
> > contents of (acpi_device) device->power.state shows FF (ACPI_STATE_UNKNOWN). 
> > Should this condition be excluded from your check or is my hacky module 
> > outdated?
> > 
> > I currently workaround this issue by checking for ACPI_STATE_UNKNOWN. If that 
> > is the value, I assume on (overwrite device->power.state with ACPI_STATE_D0). 
> > Then I call pci_set_power_state(pci_dev, PCI_D3cold).
> 
> You shouldn't ever be transitioning from an uknown state to D3cold directly.
> Please first transition to D0 and then to D3cold.
> 
> Can you please test the linux-next branch of the linux-pm.git tree and check
> if you see this problem in there too?

To be more precise, I wonder why you get ACPI_STATE_UNKNOWN in
device->power.state if the device is power-manageable.  In theory
acpi_bus_init_power() should change that to something meaningful and if it
doesn't, then there's a bug either in acpi_bus_init_power() or in the BIOS.

We may just need to try to force D0 in acpi_bus_init_power() if _PSC is missing,
for example.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / PM: Add check preventing transitioning to non-D0 state from D3.

[Rafael J. Wysocki]

On Thursday, January 31, 2013 12:27:25 AM Rafael J. Wysocki wrote:
> On Wednesday, January 30, 2013 10:55:17 PM Rafael J. Wysocki wrote:
> > On Wednesday, January 30, 2013 06:53:46 PM Peter Wu wrote:
> > > Hi Lv Zheng,
> > > 
> > > I encounter a regression with your patch (Linux 3.8-rc5). On my Nvidia Optimus 
> > > laptop, I use the bbswitch[1] kernel module to trigger a _PS3 ACPI method call 
> > > to turn the video card off.
> > > 
> > > After this patch, I got the following in my kernel log:
> > > 
> > >     pci 0000:01:00.0: Refused to change power state, currently in D0
> > >     ACPI: Cannot transition to non-D0 state from D3
> > >     bbswitch: Succesfully loaded. Discrete card 0000:01:00.0 is on
> > > 
> > > The expected output would be "Discrete card 0000:01:00.0 is  off". Printing the 
> > > contents of (acpi_device) device->power.state shows FF (ACPI_STATE_UNKNOWN). 
> > > Should this condition be excluded from your check or is my hacky module 
> > > outdated?
> > > 
> > > I currently workaround this issue by checking for ACPI_STATE_UNKNOWN. If that 
> > > is the value, I assume on (overwrite device->power.state with ACPI_STATE_D0). 
> > > Then I call pci_set_power_state(pci_dev, PCI_D3cold).
> > 
> > You shouldn't ever be transitioning from an uknown state to D3cold directly.
> > Please first transition to D0 and then to D3cold.
> > 
> > Can you please test the linux-next branch of the linux-pm.git tree and check
> > if you see this problem in there too?
> 
> To be more precise, I wonder why you get ACPI_STATE_UNKNOWN in
> device->power.state if the device is power-manageable.  In theory
> acpi_bus_init_power() should change that to something meaningful and if it
> doesn't, then there's a bug either in acpi_bus_init_power() or in the BIOS.
> 
> We may just need to try to force D0 in acpi_bus_init_power() if _PSC is missing,
> for example.

So, I'm pondering the appended patch (on top of linux-pm/linux-next).

Thanks,
Rafael

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / PM: Do not power manage devices in unknown initial states

In general, for ACPI device power management to work, the initial
power states of devices must be known (otherwise, we wouldn't be able
to keep track of power resources, for example).  Hence, if it is
impossible to determine the initial ACPI power states of some
devices, they can't be regarded as power-manageable using ACPI.

For this reason, modify acpi_bus_get_power_flags() to clear the
power_manageable flag if acpi_bus_init_power() fails and add some
extra fallback code to acpi_bus_init_power() to cover broken
BIOSes that provide _PS0/_PS3 without _PSC for some devices.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c |    6 ++++++
 drivers/acpi/scan.c      |    5 ++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -330,6 +330,12 @@ int acpi_bus_init_power(struct acpi_devi
 		result = acpi_dev_pm_explicit_set(device, state);
 		if (result)
 			return result;
+	} else if (state == ACPI_STATE_UNKNOWN) {
+		/* No power resources and missing _PSC? Try to force D0. */
+		state = ACPI_STATE_D0;
+		result = acpi_dev_pm_explicit_set(device, state);
+		if (result)
+			return result;
 	}
 	device->power.state = state;
 	return 0;
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1175,7 +1175,10 @@ static void acpi_bus_get_power_flags(str
 			device->power.flags.power_resources)
 		device->power.states[ACPI_STATE_D3_COLD].flags.os_accessible = 1;
 
-	acpi_bus_init_power(device);
+	if (acpi_bus_init_power(device)) {
+		acpi_free_power_resources_lists(device);
+		device->flags.power_manageable = 0;
+	}
 }
 
 static void acpi_bus_get_flags(struct acpi_device *device)

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.