linux-kernel - Re: [PATCH 3/5] PM / QoS: Add function dev_pm_qos_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAORVsuX=vKiGxHrR4hZjeT4DV0vhUh+MXLwNjoOwgBDu1HRR5A@mail.gmail.com>
Date:	Thu, 1 Sep 2011 17:13:44 +0200
From:	Jean Pihet <jean.pihet@...oldbits.com>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	Linux PM mailing list <linux-pm@...ts.linux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux-sh list <linux-sh@...r.kernel.org>,
	Magnus Damm <magnus.damm@...il.com>,
	Kevin Hilman <khilman@...com>
Subject: Re: [PATCH 3/5] PM / QoS: Add function dev_pm_qos_read_value()

Hi Rafael,

On Wed, Aug 31, 2011 at 12:21 AM, Rafael J. Wysocki <rjw@...k.pl> wrote:
> From: Rafael J. Wysocki <rjw@...k.pl>
>
> 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@...k.pl>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/