Re: [RFD][PATCH 5/7] PM / QoS: Make it possible to expose PM QoS device flags to user space

Re: [RFD][PATCH 5/7] PM / QoS: Make it possible to expose PM QoS device flags to user space

[Aaron Lu]

On 09/29/2012 05:55 AM, Rafael J. Wysocki wrote:
> Define two device PM QoS flags, PM_QOS_FLAG_NO_POWER_OFF
> and PM_QOS_FLAG_REMOTE_WAKEUP, and introduce routines
> dev_pm_qos_expose_flags() and dev_pm_qos_hide_flags() allowing the
> caller to expose those two flags to user space or to hide them
> from it, respectively.
> 
> After the flags have been exposed, user space will see two
> additional sysfs attributes, pm_qos_no_power_off and
> pm_qos_remote_wakeup, under the device's /sys/devices/.../power/
> directory.  Then, writing 1 to one of them will update the
> PM QoS flags request owned by user space so that the corresponding
> flag is requested to be set.  In turn, writing 0 to one of them
> will cause the corresponding flag in the user space's request to
> be cleared (however, the owners of the other PM QoS flags requests
> for the same device may still request the flag to be set and it
> may be effectively set even if user space doesn't request that).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  Documentation/ABI/testing/sysfs-devices-power |   31 ++++
>  drivers/base/power/power.h                    |    6 
>  drivers/base/power/qos.c                      |  169 ++++++++++++++++++++------
>  drivers/base/power/sysfs.c                    |   95 +++++++++++++-
>  include/linux/pm.h                            |    1 
>  include/linux/pm_qos.h                        |   26 ++++
>  6 files changed, 280 insertions(+), 48 deletions(-)
> 
> Index: linux/drivers/base/power/qos.c
> ===================================================================
> --- linux.orig/drivers/base/power/qos.c
> +++ linux/drivers/base/power/qos.c
> @@ -40,6 +40,7 @@
>  #include <linux/device.h>
>  #include <linux/mutex.h>
>  #include <linux/export.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "power.h"
>  
> @@ -322,6 +323,38 @@ int dev_pm_qos_add_request(struct device
>  EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
>  
>  /**
> + * __dev_pm_qos_update_request - Modify an existing device PM QoS request.
> + * @req : PM QoS request to modify.
> + * @new_value: New value to request.
> + */
> +int __dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value)
> +{
> +	s32 curr_value;
> +	int ret = 0;
> +
> +	if (!req->dev->power.qos) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	switch(req->type) {
> +	case DEV_PM_QOS_LATENCY:
> +		curr_value = req->data.pnode.prio;
> +		break;
> +	case DEV_PM_QOS_FLAGS:
> +		curr_value = req->data.flr.flags;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (curr_value != new_value)
> +		ret = apply_constraint(req, PM_QOS_UPDATE_REQ, new_value);
> +
> +	return ret;
> +}
> +
> +/**
>   * dev_pm_qos_update_request - modifies an existing qos request
>   * @req : handle to list element holding a dev_pm_qos request to use
>   * @new_value: defines the qos request
> @@ -336,11 +369,9 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_request
>   * -EINVAL in case of wrong parameters, -ENODEV if the device has been
>   * removed from the system
>   */
> -int dev_pm_qos_update_request(struct dev_pm_qos_request *req,
> -			      s32 new_value)
> +int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value)
>  {
> -	s32 curr_value;
> -	int ret = 0;
> +	int ret;
>  
>  	if (!req) /*guard against callers passing in null */
>  		return -EINVAL;
> @@ -350,29 +381,9 @@ int dev_pm_qos_update_request(struct dev
>  		return -EINVAL;
>  
>  	mutex_lock(&dev_pm_qos_mtx);
> -
> -	if (!req->dev->power.qos) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> -
> -	switch(req->type) {
> -	case DEV_PM_QOS_LATENCY:
> -		curr_value = req->data.pnode.prio;
> -		break;
> -	case DEV_PM_QOS_FLAGS:
> -		curr_value = req->data.flr.flags;
> -		break;
> -	default:
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	if (curr_value != new_value)
> -		ret = apply_constraint(req, PM_QOS_UPDATE_REQ, new_value);
> -
> - out:
> +	__dev_pm_qos_update_request(req, new_value);

You forgot to assign the return value here.

Thanks,
Aaron

>  	mutex_unlock(&dev_pm_qos_mtx);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_qos_update_request);
> @@ -533,10 +544,19 @@ int dev_pm_qos_add_ancestor_request(stru
>  EXPORT_SYMBOL_GPL(dev_pm_qos_add_ancestor_request);

Re: [RFD][PATCH 5/7] PM / QoS: Make it possible to expose PM QoS device flags to user space

[Rafael J. Wysocki]

On Tuesday, October 30, 2012 04:53:28 PM Aaron Lu wrote:
> On 09/29/2012 05:55 AM, Rafael J. Wysocki wrote:
> > Define two device PM QoS flags, PM_QOS_FLAG_NO_POWER_OFF
> > and PM_QOS_FLAG_REMOTE_WAKEUP, and introduce routines
> > dev_pm_qos_expose_flags() and dev_pm_qos_hide_flags() allowing the
> > caller to expose those two flags to user space or to hide them
> > from it, respectively.
> > 
> > After the flags have been exposed, user space will see two
> > additional sysfs attributes, pm_qos_no_power_off and
> > pm_qos_remote_wakeup, under the device's /sys/devices/.../power/
> > directory.  Then, writing 1 to one of them will update the
> > PM QoS flags request owned by user space so that the corresponding
> > flag is requested to be set.  In turn, writing 0 to one of them
> > will cause the corresponding flag in the user space's request to
> > be cleared (however, the owners of the other PM QoS flags requests
> > for the same device may still request the flag to be set and it
> > may be effectively set even if user space doesn't request that).
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  Documentation/ABI/testing/sysfs-devices-power |   31 ++++
> >  drivers/base/power/power.h                    |    6 
> >  drivers/base/power/qos.c                      |  169 ++++++++++++++++++++------
> >  drivers/base/power/sysfs.c                    |   95 +++++++++++++-
> >  include/linux/pm.h                            |    1 
> >  include/linux/pm_qos.h                        |   26 ++++
> >  6 files changed, 280 insertions(+), 48 deletions(-)
> > 
> > Index: linux/drivers/base/power/qos.c
> > ===================================================================
> > --- linux.orig/drivers/base/power/qos.c
> > +++ linux/drivers/base/power/qos.c
> > @@ -40,6 +40,7 @@
> >  #include <linux/device.h>
> >  #include <linux/mutex.h>
> >  #include <linux/export.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #include "power.h"
> >  
> > @@ -322,6 +323,38 @@ int dev_pm_qos_add_request(struct device
> >  EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
> >  
> >  /**
> > + * __dev_pm_qos_update_request - Modify an existing device PM QoS request.
> > + * @req : PM QoS request to modify.
> > + * @new_value: New value to request.
> > + */
> > +int __dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value)
> > +{
> > +	s32 curr_value;
> > +	int ret = 0;
> > +
> > +	if (!req->dev->power.qos) {
> > +		ret = -ENODEV;
> > +		goto out;
> > +	}
> > +
> > +	switch(req->type) {
> > +	case DEV_PM_QOS_LATENCY:
> > +		curr_value = req->data.pnode.prio;
> > +		break;
> > +	case DEV_PM_QOS_FLAGS:
> > +		curr_value = req->data.flr.flags;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (curr_value != new_value)
> > +		ret = apply_constraint(req, PM_QOS_UPDATE_REQ, new_value);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> >   * dev_pm_qos_update_request - modifies an existing qos request
> >   * @req : handle to list element holding a dev_pm_qos request to use
> >   * @new_value: defines the qos request
> > @@ -336,11 +369,9 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_request
> >   * -EINVAL in case of wrong parameters, -ENODEV if the device has been
> >   * removed from the system
> >   */
> > -int dev_pm_qos_update_request(struct dev_pm_qos_request *req,
> > -			      s32 new_value)
> > +int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value)
> >  {
> > -	s32 curr_value;
> > -	int ret = 0;
> > +	int ret;
> >  
> >  	if (!req) /*guard against callers passing in null */
> >  		return -EINVAL;
> > @@ -350,29 +381,9 @@ int dev_pm_qos_update_request(struct dev
> >  		return -EINVAL;
> >  
> >  	mutex_lock(&dev_pm_qos_mtx);
> > -
> > -	if (!req->dev->power.qos) {
> > -		ret = -ENODEV;
> > -		goto out;
> > -	}
> > -
> > -	switch(req->type) {
> > -	case DEV_PM_QOS_LATENCY:
> > -		curr_value = req->data.pnode.prio;
> > -		break;
> > -	case DEV_PM_QOS_FLAGS:
> > -		curr_value = req->data.flr.flags;
> > -		break;
> > -	default:
> > -		ret = -EINVAL;
> > -		goto out;
> > -	}
> > -
> > -	if (curr_value != new_value)
> > -		ret = apply_constraint(req, PM_QOS_UPDATE_REQ, new_value);
> > -
> > - out:
> > +	__dev_pm_qos_update_request(req, new_value);
> 
> You forgot to assign the return value here.

Quite obviously.  I'm going to apply the appended patch for that.

Thanks for spotting it!

Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: PM / QoS: Fix the return value of dev_pm_qos_update_request()

Commit e39473d (PM / QoS: Make it possible to expose PM QoS device)
flags to user space introduced __dev_pm_qos_update_request() to be
called internally by dev_pm_qos_update_request(), but forgot to make
the latter actually use the return value of the former.  Fix this
mistake.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/qos.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/base/power/qos.c
===================================================================
--- linux.orig/drivers/base/power/qos.c
+++ linux/drivers/base/power/qos.c
@@ -380,7 +380,7 @@ int dev_pm_qos_update_request(struct dev
 		return -EINVAL;
 
 	mutex_lock(&dev_pm_qos_mtx);
-	__dev_pm_qos_update_request(req, new_value);
+	ret = __dev_pm_qos_update_request(req, new_value);
 	mutex_unlock(&dev_pm_qos_mtx);
 
 	return ret;

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

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

[Bjorn Helgaas]

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

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

[Rafael J. Wysocki]

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


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