[RFC PATCH 5/8] PM / Domains: Verify the PM domain is present when adding a provider
Ulf Hansson
ulf.hansson at linaro.org
Fri Aug 5 04:57:35 PDT 2016
On 4 March 2016 at 12:23, Jon Hunter <jonathanh at nvidia.com> wrote:
> When a PM domain provider is added, there is currently no way to tell if
> the PM domain is actually present in the system. Naturally, the PM domain
> provider should not be registered if the PM domain has not been added.
> Nonetheless, to verify that the PM domain associated with a provider is
> present, store the 'provider_data' in the PM domain structure when adding
> the provider and make sure that the PM domain is found the list of PM
> domains registered.
>
> The of_genpd_add_provider_simple() and of_genpd_add_provider_onecell()
> functions have been updated to store the 'provider_data' by default to
> avoid having to modify all the current PM domain provider
> implementations.
>
> By doing this, we can also verify that the provider has been removed
> from the list of providers before removing a PM domain.
>
> Signed-off-by: Jon Hunter <jonathanh at nvidia.com>
> ---
> drivers/base/power/domain.c | 74 +++++++++++++++++++++++++++++++++++++++++++--
> include/linux/pm_domain.h | 13 +++++---
> 2 files changed, 79 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index c2ba1d6dbad3..72055fef6256 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1570,6 +1570,24 @@ static LIST_HEAD(of_genpd_providers);
> static DEFINE_MUTEX(of_genpd_mutex);
>
> /**
> + * pm_genpd_provider_present - Check if the provider's PM domain is present.
> + * @data: Provider data associated with the PM domain.
> + */
> +static bool pm_genpd_provider_present(void *data)
Please rename to genpd_provider_present().
> +{
> + struct generic_pm_domain *gpd;
> +
> + if (!data)
> + return false;
> +
> + list_for_each_entry(gpd, &gpd_list, gpd_list_node)
> + if (gpd->provider_data == data)
> + return true;
> +
> + return false;
> +}
> +
> +/**
> * __of_genpd_xlate_simple() - Xlate function for direct node-domain mapping
> * @genpdspec: OF phandle args to map into a PM domain
> * @data: xlate function private data - pointer to struct generic_pm_domain
> @@ -1625,9 +1643,15 @@ EXPORT_SYMBOL_GPL(__of_genpd_xlate_onecell);
> * @np: Device node pointer associated with the PM domain provider.
> * @xlate: Callback for decoding PM domain from phandle arguments.
> * @data: Context pointer for @xlate callback.
> + *
> + * The PM domain assocaited with the provider must have the
> + * 'provider_data' member of the PM domain structure populated with the
> + * same data pointer passed to this function. This is used to verify
> + * that the PM domain associated with the provider is present in the
> + * list of registered PM domains.
> */
> -int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> - void *data)
> +static int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> + void *data)
Please rename to genpd_add_provider().
> {
> struct of_genpd_provider *cp;
>
> @@ -1635,6 +1659,13 @@ int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> if (!cp)
> return -ENOMEM;
>
> + mutex_lock(&gpd_list_lock);
> +
> + if (!pm_genpd_provider_present(data)) {
> + mutex_unlock(&gpd_list_lock);
> + return -EINVAL;
> + }
> +
> cp->node = of_node_get(np);
> cp->data = data;
> cp->xlate = xlate;
> @@ -1642,11 +1673,48 @@ int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> mutex_lock(&of_genpd_mutex);
> list_add(&cp->link, &of_genpd_providers);
> mutex_unlock(&of_genpd_mutex);
> + mutex_unlock(&gpd_list_lock);
> pr_debug("Added domain provider from %s\n", np->full_name);
>
> return 0;
> }
> -EXPORT_SYMBOL_GPL(__of_genpd_add_provider);
I agree that it makes sense to remove this API and to turn the
function into becoming static. Although perhaps you should do that as
a clean up patch that comes prior $subject patch instead!?
The clean up also includes the exporting of the
of_genpd_add_provider_onecell() and the of_genpd_add_provider_simple()
APIs.
... and while you do these cleanups, you could also remove the
exported functions __of_genpd_xlate_simple() and
__of_genpd_xlate_onecell() as those should be internal to genpd.
> +
> +/**
> + * of_genpd_add_provider_simple() - Register a simple PM domain provider
> + * @np: Device node pointer associated with the PM domain provider.
> + * @genpd: Pointer to PM domain associated with the PM domain provider.
> + */
> +int of_genpd_add_provider_simple(struct device_node *np,
> + struct generic_pm_domain *genpd)
> +{
> + if (!np || !genpd)
> + return -EINVAL;
> +
> + genpd->provider_data = genpd;
> +
> + return __of_genpd_add_provider(np, __of_genpd_xlate_simple, genpd);
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_add_provider_simple);
> +
> +/**
> + * of_genpd_add_provider_onecell() - Register a onecell PM domain provider
> + * @np: Device node pointer associated with the PM domain provider.
> + * @data: Pointer to the data associated with the PM domain provider.
> + */
> +int of_genpd_add_provider_onecell(struct device_node *np,
> + struct genpd_onecell_data *data)
> +{
> + unsigned int i;
> +
> + if (!np || !data)
> + return -EINVAL;
> +
> + for (i = 0; i < data->num_domains; i++)
> + data->domains[i]->provider_data = data;
> +
> + return __of_genpd_add_provider(np, __of_genpd_xlate_onecell, data);
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_add_provider_onecell);
>
> /**
> * of_genpd_del_provider() - Remove a previously registered PM domain provider
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 510512d5390e..bed84413546f 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -53,6 +53,7 @@ struct generic_pm_domain {
> struct mutex lock;
> struct dev_power_governor *gov;
> struct work_struct power_off_work;
> + void *provider_data;
As per comment on patch 6/8. Couldn't we store the provider's device
node here instead?
> const char *name;
> atomic_t sd_count; /* Number of subdomains with power "on" */
> enum gpd_status status; /* Current state of the domain */
> @@ -193,8 +194,10 @@ typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
> void *data);
>
> #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> -int __of_genpd_add_provider(struct device_node *np, genpd_xlate_t xlate,
> - void *data);
> +int of_genpd_add_provider_simple(struct device_node *np,
> + struct generic_pm_domain *genpd);
> +int of_genpd_add_provider_onecell(struct device_node *np,
> + struct genpd_onecell_data *data);
> void of_genpd_del_provider(struct device_node *np);
> struct generic_pm_domain *__of_genpd_xlate_simple(
> struct of_phandle_args *genpdspec,
> @@ -235,18 +238,18 @@ static inline int genpd_dev_pm_attach(struct device *dev)
> {
> return -ENODEV;
> }
> -#endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
> static inline int of_genpd_add_provider_simple(struct device_node *np,
> struct generic_pm_domain *genpd)
> {
> - return __of_genpd_add_provider(np, __of_genpd_xlate_simple, genpd);
> + return -ENOTSUPP;
> }
> static inline int of_genpd_add_provider_onecell(struct device_node *np,
> struct genpd_onecell_data *data)
> {
> - return __of_genpd_add_provider(np, __of_genpd_xlate_onecell, data);
> + return -ENOTSUPP;
> }
> +#endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
> #ifdef CONFIG_PM
> extern int dev_pm_domain_attach(struct device *dev, bool power_on);
> --
> 2.1.4
>
Kind regards
Uffe
More information about the linux-arm-kernel
mailing list