| 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
| ||
|
Message-ID: <CAPDyKFrviW6BkjL3-2=587xrdrNaR=VNf_2tOSzzqcRzMc+u=A@mail.gmail.com>
Date: Thu, 22 Mar 2018 10:32:02 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Kevin Hilman <khilman@...nel.org>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
Linux PM <linux-pm@...r.kernel.org>,
Stephen Boyd <sboyd@...eaurora.org>,
Nishanth Menon <nm@...com>,
Vincent Guittot <vincent.guittot@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Rajendra Nayak <rnayak@...eaurora.org>,
Sudeep Holla <sudeep.holla@....com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/7] PM / Domain: Implement of_dev_pm_genpd_get_performance_state()
On 22 December 2017 at 08:26, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> This implements of_dev_pm_genpd_get_performance_state() which can be
> used from the device drivers or the OPP core to find the performance
> state encoded in the "required-opp" property of a node. Different
Please add a two newlines after "node".
> platforms may encode the performance state differently using the OPP
> table (they may simply return value of opp-hz or opp-microvolt, or apply
> some algorithm on top of those values) and so a new callback is
> implemented to allow platform specific drivers to convert the power
> domain OPP to a performance state.
Could you perhaps clarify at what typical point(s) the drivers or the
OPP core should call this new API? That is useful information in the
changelog.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
> ---
> drivers/base/power/domain.c | 48 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 11 +++++++++++
> 2 files changed, 59 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 1ad4ad0b0de0..ef43b75982fa 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2415,6 +2415,54 @@ int of_genpd_parse_idle_states(struct device_node *dn,
> }
> EXPORT_SYMBOL_GPL(of_genpd_parse_idle_states);
>
> +/**
> + * of_dev_pm_genpd_get_performance_state- Gets performance state of device's
> + * power domain corresponding to a DT node's "required-opp" property.
> + *
> + * @dev: Device for which the performance-state needs to be found.
> + * @np: DT node where the "required-opp" property is present. This can be
> + * the device node itself (if it doesn't have an OPP table) or a node
> + * within the OPP table of a device (if device has an OPP table).
> + * @state: Pointer to return performance state.
> + *
> + * Returns performance state corresponding to the "required-opp" property of
> + * a DT node. This calls platform specific genpd->get_performance_state()
> + * callback to translate power domain OPP to performance state.
> + *
> + * Returns performance state on success and 0 on failure.
> + */
> +unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
> + struct device_node *np)
First: To clarify the API, perhaps change to "...struct device_node *opp_node)"
Second, the function name is confusing. To me, it sounds like an API
that will get the dynamically requested performance state for the
device and that isn't the case.
So, I would rather name it something along the lines of,
"of_genpd_opp_to_performance_state()". Can you please consider
renaming it?
> +{
> + struct generic_pm_domain *genpd;
> + struct dev_pm_opp *opp;
> + int state = 0;
> +
> + genpd = dev_to_genpd(dev);
> + if (IS_ERR(genpd))
> + return 0;
> +
> + if (unlikely(!genpd->set_performance_state))
> + return 0;
> +
> + genpd_lock(genpd);
> +
> + opp = of_dev_pm_opp_find_required_opp(&genpd->dev, np);
> + if (IS_ERR(opp)) {
> + state = PTR_ERR(opp);
> + goto unlock;
> + }
> +
> + state = genpd->get_performance_state(genpd, opp);
> + dev_pm_opp_put(opp);
> +
> +unlock:
> + genpd_unlock(genpd);
> +
> + return state;
> +}
> +EXPORT_SYMBOL_GPL(of_dev_pm_genpd_get_performance_state);
> +
> #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index aaacaa35005d..4edbdaa54cec 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -47,6 +47,7 @@ struct genpd_power_state {
> };
>
> struct genpd_lock_ops;
> +struct dev_pm_opp;
>
> struct generic_pm_domain {
> struct device dev;
> @@ -68,6 +69,8 @@ struct generic_pm_domain {
> unsigned int performance_state; /* Aggregated max performance state */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> + unsigned int (*get_performance_state)(struct generic_pm_domain *genpd,
> + struct dev_pm_opp *opp);
According to the comments above, please rename the callback to:
"opp_to_performance_state", or whatever better name you can come up with. :-)
> int (*set_performance_state)(struct generic_pm_domain *genpd,
> unsigned int state);
> struct gpd_dev_ops dev_ops;
> @@ -244,6 +247,8 @@ extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
> extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
> extern int of_genpd_parse_idle_states(struct device_node *dn,
> struct genpd_power_state **states, int *n);
> +extern unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
> + struct device_node *np);
>
> int genpd_dev_pm_attach(struct device *dev);
> #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
> @@ -279,6 +284,12 @@ static inline int of_genpd_parse_idle_states(struct device_node *dn,
> return -ENODEV;
> }
>
> +static inline unsigned int of_dev_pm_genpd_get_performance_state(struct device *dev,
> + struct device_node *np)
> +{
> + return -ENODEV;
> +}
> +
> static inline int genpd_dev_pm_attach(struct device *dev)
> {
> return -ENODEV;
> --
> 2.15.0.194.g9af6a3dea062
>
Mostly minor things, so overall this looks good to me!
Kind regards
Uffe
Powered by blists - more mailing lists