linux-kernel - Re: [PATCH 06/10] OPP: Add dev_pm_opp_{set|put}_required

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] [day] [month] [year] [list]

Date:   Mon, 10 Sep 2018 16:18:53 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Rafael Wysocki <rjw@...ysocki.net>,
        Kevin Hilman <khilman@...nel.org>, Pavel Machek <pavel@....cz>,
        Len Brown <len.brown@...el.com>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 06/10] OPP: Add dev_pm_opp_{set|put}_required_device() helper

On 29 June 2018 at 08:19, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> Multiple generic power domains for a device are supported with the help
> of virtual devices, which are created for each device-genpd pair. These

What "device-genpd" pair are you referring to?

> are the device structures which are attached to the power domain and are
> required by the OPP core to set the performance state of the genpd.
>
> The helpers added by this commit are required to be called once for each
> genpd of a device. These are required only if multiple domains are
> present for a device, otherwise the actual device structure will be used
> instead by the OPP core.
>
> This commit also updates the genpd core to automatically call the new
> helper to set the required devices with the OPP layer, whenever the
> virtual devices are created for multiple genpd. The prototype of
> __genpd_dev_pm_attach() is slightly updated for this.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
>  drivers/base/power/domain.c | 31 ++++++++++++++---
>  drivers/opp/core.c          | 69 +++++++++++++++++++++++++++++++++++++
>  drivers/opp/of.c            | 12 +++++++
>  drivers/opp/opp.h           |  2 ++
>  include/linux/pm_opp.h      |  8 +++++
>  5 files changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index c298de8a8308..e9c85c96580c 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2219,8 +2219,14 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>         genpd_queue_power_off_work(pd);
>
>         /* Unregister the device if it was created by genpd. */
> -       if (dev->bus == &genpd_bus_type)
> +       if (dev->bus == &genpd_bus_type) {
> +               struct opp_table *opp_table = dev_get_drvdata(dev);

This feels wrong, to me. Genpd is not a driver, but rather a
subsystem, hence I think it's a bad idea to "abuse" the driver data
pointer like this.

Instead, in genpd we store device specific data, through the
dev->power.subsys_data.

Seems like a better option is to extend the subsys_data, to also
include data needed to be shared for opp/performance states.

In that way, we might not even need to call into the opp library
(dev_pm_opp_set_required_device()) API, but rather the opp library can
itself check what is in the subsys_data for the device that is being
targeted. Would that work?

[...]

Kind regards
Uffe