diff mbox

[RFD,7/7] PM / ACPI: Take device PM QoS flags into account

Message ID 201209282356.52558.rjw@sisk.pl (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Rafael Wysocki Sept. 28, 2012, 9:56 p.m. UTC 
Make ACPI power management routines and PCI power management
routines depending on ACPI take device PM QoS flags into account
when deciding what power state to put the device into.

In particular, after this change acpi_pm_device_sleep_state() will
not return ACPI_STATE_D3_COLD as the deepest available low-power
state if PM_QOS_FLAG_NO_POWER_OFF is requested for the device and it
will not require remote wakeup to work for the device in the returned
low-power state if there is at least one PM QoS flags request for the
device, but PM_QOS_FLAG_REMOTE_WAKEUP is not requested for it.

Accordingly, acpi_pci_set_power_state() will refuse to put the
device into D3cold if PM_QOS_FLAG_NO_POWER_OFF is requested for it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep.c   |   16 ++++++++++++----
 drivers/pci/pci-acpi.c |    8 +++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Huang, Ying Sept. 29, 2012, 3:16 a.m. UTC | #1 
On Fri, 2012-09-28 at 23:56 +0200, Rafael J. Wysocki wrote:
> Make ACPI power management routines and PCI power management
> routines depending on ACPI take device PM QoS flags into account
> when deciding what power state to put the device into.
> 
> In particular, after this change acpi_pm_device_sleep_state() will
> not return ACPI_STATE_D3_COLD as the deepest available low-power
> state if PM_QOS_FLAG_NO_POWER_OFF is requested for the device and it
> will not require remote wakeup to work for the device in the returned
> low-power state if there is at least one PM QoS flags request for the
> device, but PM_QOS_FLAG_REMOTE_WAKEUP is not requested for it.
> 
> Accordingly, acpi_pci_set_power_state() will refuse to put the
> device into D3cold if PM_QOS_FLAG_NO_POWER_OFF is requested for it.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/sleep.c   |   16 ++++++++++++----
>  drivers/pci/pci-acpi.c |    8 +++++++-
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> Index: linux/drivers/pci/pci-acpi.c
> ===================================================================
> --- linux.orig/drivers/pci/pci-acpi.c
> +++ linux/drivers/pci/pci-acpi.c
> @@ -17,6 +17,7 @@
>  
>  #include <linux/pci-acpi.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_qos.h>
>  #include "pci.h"
>  
>  static DEFINE_MUTEX(pci_acpi_pm_notify_mtx);
> @@ -257,11 +258,16 @@ static int acpi_pci_set_power_state(stru
>  		return -ENODEV;
>  
>  	switch (state) {
> +	case PCI_D3cold:
> +		if (dev_pm_qos_flags(&dev->dev, PM_QOS_FLAG_NO_POWER_OFF) ==
> +				PM_QOS_FLAGS_ALL) {
> +			error = -EBUSY;
> +			break;
> +		}
>  	case PCI_D0:
>  	case PCI_D1:
>  	case PCI_D2:
>  	case PCI_D3hot:
> -	case PCI_D3cold:
>  		error = acpi_bus_set_power(handle, state_conv[state]);
>  	}
>  
> Index: linux/drivers/acpi/sleep.c
> ===================================================================
> --- linux.orig/drivers/acpi/sleep.c
> +++ linux/drivers/acpi/sleep.c
> @@ -19,6 +19,7 @@
>  #include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_qos.h>
>  
>  #include <asm/io.h>
>  
> @@ -711,6 +712,7 @@ int acpi_pm_device_sleep_state(struct de
>  	struct acpi_device *adev;
>  	char acpi_method[] = "_SxD";
>  	unsigned long long d_min, d_max;
> +	bool wakeup = false;
>  
>  	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3)
>  		return -EINVAL;
> @@ -718,6 +720,8 @@ int acpi_pm_device_sleep_state(struct de
>  		printk(KERN_DEBUG "ACPI handle has no context!\n");
>  		return -ENODEV;
>  	}
> +	if (dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF) == PM_QOS_FLAGS_ALL)
> +		d_max_in = ACPI_STATE_D3_HOT;

In theory, d_max_in may < ACPI_STATE_D3_HOT.  So better to be

d_max_in = min_t(int, d_max_in, ACPI_STATE_D3_HOT);

But in fact, d_max_in is introduced to support something similar to
PM_QOS_FLAG_NO_POWER_OFF, so I think we can remove d_max_in in the
future.

Best Regards,
Huang Ying

>  
>  	acpi_method[2] = '0' + acpi_target_sleep_state;
>  	/*
> @@ -737,8 +741,14 @@ int acpi_pm_device_sleep_state(struct de
>  	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
>  	 * provided -- that's our fault recovery, we ignore retval.
>  	 */
> -	if (acpi_target_sleep_state > ACPI_STATE_S0)
> +	if (acpi_target_sleep_state > ACPI_STATE_S0) {
>  		acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
> +		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
> +			&& adev->wakeup.sleep_state >= acpi_target_sleep_state;
> +	} else if (dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) !=
> +			PM_QOS_FLAGS_NONE) {
> +		wakeup = adev->wakeup.flags.valid;
> +	}
>  
>  	/*
>  	 * If _PRW says we can wake up the system from the target sleep state,
> @@ -747,9 +757,7 @@ int acpi_pm_device_sleep_state(struct de
>  	 * (ACPI 3.x), it should return the maximum (lowest power) D-state that
>  	 * can wake the system.  _S0W may be valid, too.
>  	 */
> -	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
> -	    (device_may_wakeup(dev) && adev->wakeup.flags.valid &&
> -	     adev->wakeup.sleep_state >= acpi_target_sleep_state)) {
> +	if (wakeup) {
>  		acpi_status status;
>  
>  		acpi_method[3] = 'W';
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Sept. 29, 2012, 9:39 p.m. UTC | #2 
On Saturday, September 29, 2012, Huang Ying wrote:
> On Fri, 2012-09-28 at 23:56 +0200, Rafael J. Wysocki wrote:
> > Make ACPI power management routines and PCI power management
> > routines depending on ACPI take device PM QoS flags into account
> > when deciding what power state to put the device into.
> > 
> > In particular, after this change acpi_pm_device_sleep_state() will
> > not return ACPI_STATE_D3_COLD as the deepest available low-power
> > state if PM_QOS_FLAG_NO_POWER_OFF is requested for the device and it
> > will not require remote wakeup to work for the device in the returned
> > low-power state if there is at least one PM QoS flags request for the
> > device, but PM_QOS_FLAG_REMOTE_WAKEUP is not requested for it.
> > 
> > Accordingly, acpi_pci_set_power_state() will refuse to put the
> > device into D3cold if PM_QOS_FLAG_NO_POWER_OFF is requested for it.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/acpi/sleep.c   |   16 ++++++++++++----
> >  drivers/pci/pci-acpi.c |    8 +++++++-
> >  2 files changed, 19 insertions(+), 5 deletions(-)
> > 
> > Index: linux/drivers/pci/pci-acpi.c
> > ===================================================================
> > --- linux.orig/drivers/pci/pci-acpi.c
> > +++ linux/drivers/pci/pci-acpi.c
> > @@ -17,6 +17,7 @@
> >  
> >  #include <linux/pci-acpi.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/pm_qos.h>
> >  #include "pci.h"
> >  
> >  static DEFINE_MUTEX(pci_acpi_pm_notify_mtx);
> > @@ -257,11 +258,16 @@ static int acpi_pci_set_power_state(stru
> >  		return -ENODEV;
> >  
> >  	switch (state) {
> > +	case PCI_D3cold:
> > +		if (dev_pm_qos_flags(&dev->dev, PM_QOS_FLAG_NO_POWER_OFF) ==
> > +				PM_QOS_FLAGS_ALL) {
> > +			error = -EBUSY;
> > +			break;
> > +		}
> >  	case PCI_D0:
> >  	case PCI_D1:
> >  	case PCI_D2:
> >  	case PCI_D3hot:
> > -	case PCI_D3cold:
> >  		error = acpi_bus_set_power(handle, state_conv[state]);
> >  	}
> >  
> > Index: linux/drivers/acpi/sleep.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/sleep.c
> > +++ linux/drivers/acpi/sleep.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/module.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/pm_qos.h>
> >  
> >  #include <asm/io.h>
> >  
> > @@ -711,6 +712,7 @@ int acpi_pm_device_sleep_state(struct de
> >  	struct acpi_device *adev;
> >  	char acpi_method[] = "_SxD";
> >  	unsigned long long d_min, d_max;
> > +	bool wakeup = false;
> >  
> >  	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3)
> >  		return -EINVAL;
> > @@ -718,6 +720,8 @@ int acpi_pm_device_sleep_state(struct de
> >  		printk(KERN_DEBUG "ACPI handle has no context!\n");
> >  		return -ENODEV;
> >  	}
> > +	if (dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF) == PM_QOS_FLAGS_ALL)
> > +		d_max_in = ACPI_STATE_D3_HOT;
> 
> In theory, d_max_in may < ACPI_STATE_D3_HOT.

Good catch!

> So better to be
> 
> d_max_in = min_t(int, d_max_in, ACPI_STATE_D3_HOT);

In fact, we don't even need to check the PM  QoS flags if d_max_in is not D3_COLD.

> But in fact, d_max_in is introduced to support something similar to
> PM_QOS_FLAG_NO_POWER_OFF, so I think we can remove d_max_in in the
> future.

We probably can, but for now I'll fix the patch.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Nov. 12, 2012, 9:07 p.m. UTC | #3 
On Fri, Sep 28, 2012 at 3:56 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Make ACPI power management routines and PCI power management
> routines depending on ACPI take device PM QoS flags into account
> when deciding what power state to put the device into.
>
> In particular, after this change acpi_pm_device_sleep_state() will
> not return ACPI_STATE_D3_COLD as the deepest available low-power
> state if PM_QOS_FLAG_NO_POWER_OFF is requested for the device and it
> will not require remote wakeup to work for the device in the returned
> low-power state if there is at least one PM QoS flags request for the
> device, but PM_QOS_FLAG_REMOTE_WAKEUP is not requested for it.
>
> Accordingly, acpi_pci_set_power_state() will refuse to put the
> device into D3cold if PM_QOS_FLAG_NO_POWER_OFF is requested for it.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/sleep.c   |   16 ++++++++++++----
>  drivers/pci/pci-acpi.c |    8 +++++++-

This touches a file in drivers/pci, but I expect it depends on all the
previous patches in the series, so if you haven't merged it already,
Rafael, it seems like it makes sense for you to merge this instead of
me.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Nov. 12, 2012, 11:42 p.m. UTC | #4 
On Monday, November 12, 2012 02:07:03 PM Bjorn Helgaas wrote:
> On Fri, Sep 28, 2012 at 3:56 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > Make ACPI power management routines and PCI power management
> > routines depending on ACPI take device PM QoS flags into account
> > when deciding what power state to put the device into.
> >
> > In particular, after this change acpi_pm_device_sleep_state() will
> > not return ACPI_STATE_D3_COLD as the deepest available low-power
> > state if PM_QOS_FLAG_NO_POWER_OFF is requested for the device and it
> > will not require remote wakeup to work for the device in the returned
> > low-power state if there is at least one PM QoS flags request for the
> > device, but PM_QOS_FLAG_REMOTE_WAKEUP is not requested for it.
> >
> > Accordingly, acpi_pci_set_power_state() will refuse to put the
> > device into D3cold if PM_QOS_FLAG_NO_POWER_OFF is requested for it.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/acpi/sleep.c   |   16 ++++++++++++----
> >  drivers/pci/pci-acpi.c |    8 +++++++-
> 
> This touches a file in drivers/pci, but I expect it depends on all the
> previous patches in the series, so if you haven't merged it already,
> Rafael, it seems like it makes sense for you to merge this instead of
> me.

I will, thanks a lot!

Rafael
diff mbox

Patch

Index: linux/drivers/pci/pci-acpi.c
===================================================================
--- linux.orig/drivers/pci/pci-acpi.c
+++ linux/drivers/pci/pci-acpi.c
@@ -17,6 +17,7 @@ 
 
 #include <linux/pci-acpi.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_qos.h>
 #include "pci.h"
 
 static DEFINE_MUTEX(pci_acpi_pm_notify_mtx);
@@ -257,11 +258,16 @@  static int acpi_pci_set_power_state(stru
 		return -ENODEV;
 
 	switch (state) {
+	case PCI_D3cold:
+		if (dev_pm_qos_flags(&dev->dev, PM_QOS_FLAG_NO_POWER_OFF) ==
+				PM_QOS_FLAGS_ALL) {
+			error = -EBUSY;
+			break;
+		}
 	case PCI_D0:
 	case PCI_D1:
 	case PCI_D2:
 	case PCI_D3hot:
-	case PCI_D3cold:
 		error = acpi_bus_set_power(handle, state_conv[state]);
 	}
 
Index: linux/drivers/acpi/sleep.c
===================================================================
--- linux.orig/drivers/acpi/sleep.c
+++ linux/drivers/acpi/sleep.c
@@ -19,6 +19,7 @@ 
 #include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_qos.h>
 
 #include <asm/io.h>
 
@@ -711,6 +712,7 @@  int acpi_pm_device_sleep_state(struct de
 	struct acpi_device *adev;
 	char acpi_method[] = "_SxD";
 	unsigned long long d_min, d_max;
+	bool wakeup = false;
 
 	if (d_max_in < ACPI_STATE_D0 || d_max_in > ACPI_STATE_D3)
 		return -EINVAL;
@@ -718,6 +720,8 @@  int acpi_pm_device_sleep_state(struct de
 		printk(KERN_DEBUG "ACPI handle has no context!\n");
 		return -ENODEV;
 	}
+	if (dev_pm_qos_flags(dev, PM_QOS_FLAG_NO_POWER_OFF) == PM_QOS_FLAGS_ALL)
+		d_max_in = ACPI_STATE_D3_HOT;
 
 	acpi_method[2] = '0' + acpi_target_sleep_state;
 	/*
@@ -737,8 +741,14 @@  int acpi_pm_device_sleep_state(struct de
 	 * NOTE: We rely on acpi_evaluate_integer() not clobbering the integer
 	 * provided -- that's our fault recovery, we ignore retval.
 	 */
-	if (acpi_target_sleep_state > ACPI_STATE_S0)
+	if (acpi_target_sleep_state > ACPI_STATE_S0) {
 		acpi_evaluate_integer(handle, acpi_method, NULL, &d_min);
+		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
+			&& adev->wakeup.sleep_state >= acpi_target_sleep_state;
+	} else if (dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP) !=
+			PM_QOS_FLAGS_NONE) {
+		wakeup = adev->wakeup.flags.valid;
+	}
 
 	/*
 	 * If _PRW says we can wake up the system from the target sleep state,
@@ -747,9 +757,7 @@  int acpi_pm_device_sleep_state(struct de
 	 * (ACPI 3.x), it should return the maximum (lowest power) D-state that
 	 * can wake the system.  _S0W may be valid, too.
 	 */
-	if (acpi_target_sleep_state == ACPI_STATE_S0 ||
-	    (device_may_wakeup(dev) && adev->wakeup.flags.valid &&
-	     adev->wakeup.sleep_state >= acpi_target_sleep_state)) {
+	if (wakeup) {
 		acpi_status status;
 
 		acpi_method[3] = 'W';