On Fri, Sep 2, 2011 at 12:07 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> Hi,
>
> On Thursday, September 01, 2011, Jean Pihet wrote:
>> 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?
>
> I don't think it's useful for the caller. If the device is gone, the
> constraing simply doesn't matter, so there's no error to handle.
>
>> > */
>> > 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?
>
> Pretty much it is. Is that a problem?
I think the caller needs to know if the constraint has been applied
correctly or not. That is why I changed the API functions to int.
Note: I still need to come with an API documentation patch after the
code is settled down.
>
> Rafael
>
Thanks,
Jean
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
