Hi Rafael,
On Wed, Aug 31, 2011 at 12:21 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> From: Rafael J. Wysocki <rjw@xxxxxxx>
>
> To read the current PM QoS value for a given device we need to
> make sure that the device's power.constraints object won't be
> removed while we're doing that. For this reason, put the
> operation under dev->power.lock and acquire the lock
> around the initialization and removal of power.constraints.
Ok.
> Moreover, since we're using the value of power.constraints to
> determine whether or not the object is present, the
> power.constraints_state field isn't necessary any more and may be
> removed. However, dev_pm_qos_add_request() needs to check if the
> device is being removed from the system before allocating a new
> PM QoS constraints object for it, so it has to use device_pm_lock()
> and the device PM QoS initialization and destruction should be done
> under device_pm_lock() as well.
Ok that makes sense.
The constraints_state field can be replaced by a combination of
dev->power.constraints and list_empty(&dev->power.entry), which makes
the code more compact and less redundant.
>
> Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx>
> ---
> drivers/base/power/main.c | 4 -
> drivers/base/power/qos.c | 167 ++++++++++++++++++++++++++--------------------
> include/linux/pm.h | 8 --
> include/linux/pm_qos.h | 3
> 4 files changed, 101 insertions(+), 81 deletions(-)
>
> Index: linux/drivers/base/power/qos.c
> ===================================================================
> --- linux.orig/drivers/base/power/qos.c
> +++ linux/drivers/base/power/qos.c
> @@ -30,15 +30,6 @@
...
>
> @@ -178,8 +202,8 @@ void dev_pm_qos_constraints_destroy(stru
> *
> * Returns 1 if the aggregated constraint value has changed,
> * 0 if the aggregated constraint value has not changed,
> - * -EINVAL in case of wrong parameters, -ENODEV if the device has been
> - * removed from the system
> + * -EINVAL in case of wrong parameters, -ENOMEM if there's not enough memory
> + * to allocate for data structures.
Why not use -ENODEV in case there is no device?
> */
> int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
> s32 value)
> @@ -195,28 +219,35 @@ int dev_pm_qos_add_request(struct device
> return -EINVAL;
> }
>
> - mutex_lock(&dev_pm_qos_mtx);
> req->dev = dev;
>
> - /* Return if the device has been removed */
> - if (req->dev->power.constraints_state == DEV_PM_QOS_NO_DEVICE) {
> - ret = -ENODEV;
> - goto out;
> - }
> + device_pm_lock();
> + mutex_lock(&dev_pm_qos_mtx);
>
> - /*
> - * Allocate the constraints data on the first call to add_request,
> - * i.e. only if the data is not already allocated and if the device has
> - * not been removed
> - */
> - if (dev->power.constraints_state == DEV_PM_QOS_DEVICE_PRESENT)
> - ret = dev_pm_qos_constraints_allocate(dev);
> + if (dev->power.constraints) {
> + device_pm_unlock();
> + } else {
> + if (list_empty(&dev->power.entry)) {
> + /* The device has been removed from the system. */
> + device_pm_unlock();
> + goto out;
0 is silently returned in case the device has been removed. Is that
the intention?
> + } else {
> + device_pm_unlock();
> + /*
> + * Allocate the constraints data on the first call to
> + * add_request, i.e. only if the data is not already
> + * allocated and if the device has not been removed.
> + */
> + ret = dev_pm_qos_constraints_allocate(dev);
> + }
> + }
>
> if (!ret)
> ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
>
> -out:
> + out:
> mutex_unlock(&dev_pm_qos_mtx);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(dev_pm_qos_add_request);
...
Thanks,
Jean
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
