On 16 August 2016 at 11:49, Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
> If a device supports PM domains that are subdomains of another PM
> domain, then the PM domains should be removed in reverse order to
> ensure that the subdomains are removed first. Furthermore, if there is
> more than one provider, then there needs to be a way to remove the
> domains in reverse order for a specific provider.
>
> Add the function of_genpd_remove_tail() to remove the last PM domain
> added by a given PM domain provider and return the generic_pm_domain
> structure for the PM domain that was removed.
>
> A PM domain should only be removed once the associated PM domain
> provider has been removed from the list of providers. Otherwise, it
> could be possible for a client to be associated with a PM domain that
> could have been removed. Add a helper function to verify if the PM
> domain provider is present and only allow a PM domain to be removed if
> the provider has been removed.
>
> The function of_genpd_remove_tail() must hold the gpd_list_lock while
> finding and removing a PM domain. It is natural for
> of_genpd_remove_tail() to call pm_genpd_remove() once the appropriate
> PM domain is found to remove it. However, pm_genpd_remove(), also
> acquires the gpd_list_lock. Therefore, move the core of the function
> pm_genpd_remove() to a new function genpd_remove() which does not
> acquire the gpd_list_lock so this can be used by both pm_genpd_remove()
> and of_genpd_remove_tail().
>
> Signed-off-by: Jon Hunter <jonathanh@xxxxxxxxxx>
> ---
> drivers/base/power/domain.c | 87 +++++++++++++++++++++++++++++++++++++++++----
> include/linux/pm_domain.h | 7 ++++
> 2 files changed, 88 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0bc145e8e902..b6d1d0441a2d 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1357,7 +1357,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> EXPORT_SYMBOL_GPL(pm_genpd_init);
>
> /**
> - * pm_genpd_remove - Remove a generic I/O PM domain
> + * genpd_remove - Remove a generic I/O PM domain
> * @genpd: Pointer to PM domain that is to be removed.
> *
> * To remove the PM domain, this function:
> @@ -1366,9 +1366,10 @@ EXPORT_SYMBOL_GPL(pm_genpd_init);
> * - Removes the PM domain from the list of registered PM domains.
> *
> * The PM domain will only be removed, if it is not a parent to any
> - * other PM domain and has no devices associated with it.
> + * other PM domain and has no devices associated with it. Must be called
> + * with the gpd_list_lock held.
> */
> -int pm_genpd_remove(struct generic_pm_domain *genpd)
> +static int genpd_remove(struct generic_pm_domain *genpd)
> {
> struct gpd_link *l, *link;
> int ret = 0;
> @@ -1376,12 +1377,10 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)
> if (IS_ERR_OR_NULL(genpd))
> return -EINVAL;
>
> - mutex_lock(&gpd_list_lock);
> mutex_lock(&genpd->lock);
>
> if (!list_empty(&genpd->master_links) || genpd->device_count) {
> mutex_unlock(&genpd->lock);
> - mutex_unlock(&gpd_list_lock);
> pr_err("%s: unable to remove %s\n", __func__, genpd->name);
> return -EBUSY;
> }
> @@ -1395,11 +1394,25 @@ int pm_genpd_remove(struct generic_pm_domain *genpd)
> list_del(&genpd->gpd_list_node);
> mutex_unlock(&genpd->lock);
> cancel_work_sync(&genpd->power_off_work);
> - mutex_unlock(&gpd_list_lock);
> pr_debug("%s: removed %s\n", __func__, genpd->name);
>
> return ret;
> }
> +
> +/**
> + * pm_genpd_remove - Remove a generic I/O PM domain
> + * @genpd: Pointer to PM domain that is to be removed.
> + */
> +int pm_genpd_remove(struct generic_pm_domain *genpd)
> +{
> + int ret;
> +
> + mutex_lock(&gpd_list_lock);
> + ret = genpd_remove(genpd);
> + mutex_unlock(&gpd_list_lock);
> +
> + return ret;
> +}
> EXPORT_SYMBOL_GPL(pm_genpd_remove);
All above changes could have been made already in the patch when
adding the pm_genpd_remove() API. Could you please fold these changes
into that patch instead?
>
> #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> @@ -1610,6 +1623,26 @@ void of_genpd_del_provider(struct device_node *np)
> EXPORT_SYMBOL_GPL(of_genpd_del_provider);
>
> /**
> + * genpd_provider_present() - Verify if a PM domain provider is present
> + * @np: Device node pointer associated with the PM domain provider
> + */
> +static bool genpd_provider_present(struct device_node *np)
> +{
> + struct of_genpd_provider *cp;
> +
> + mutex_lock(&of_genpd_mutex);
> + list_for_each_entry(cp, &of_genpd_providers, link) {
> + if (cp->node == np) {
> + mutex_unlock(&of_genpd_mutex);
> + return true;
> + }
> + }
> + mutex_unlock(&of_genpd_mutex);
> +
> + return false;
> +}
> +
> +/**
> * genpd_get_from_provider() - Look-up PM domain
> * @genpdspec: OF phandle args to use for look-up
> *
> @@ -1713,6 +1746,48 @@ out:
> EXPORT_SYMBOL_GPL(of_genpd_add_subdomain);
>
> /**
> + * of_genpd_remove_tail - Remove the last PM domain registered for a provider
> + * @provider: Pointer to device structure associated with provider
The naming of this function would be okay, if we only have added
genpds in the gpd_list by using list_add_tail(), although we don't.
Instead we use list_add() and put them first in the list.
So, unless we change to use list_add_tail() when adding genpds (I
assume we can do that!), I would rather change the name of this
function to of_genpd_remove_first().
What option do you prefer?
> + *
> + * Find the last PM domain that was added by a particular provider and
> + * remove this PM domain from the list of PM domains. The provider is
> + * identified by the 'provider' device structure that is passed. The PM
> + * domain will only be removed, if the provider associated with domain
> + * has been removed.
> + *
> + * Returns a valid pointer to struct generic_pm_domain on success or
> + * ERR_PTR() on failure.
> + */
> +struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
> +{
> + struct generic_pm_domain *gpd, *genpd = ERR_PTR(-ENOENT);
> + int ret;
> +
> + if (IS_ERR_OR_NULL(np))
> + return ERR_PTR(-EINVAL);
> +
> + mutex_lock(&gpd_list_lock);
> + list_for_each_entry(gpd, &gpd_list, gpd_list_node) {
> + if (gpd->provider == &np->fwnode) {
> + if (!genpd_provider_present(np)) {
> + ret = genpd_remove(gpd);
> + genpd = ret ? ERR_PTR(ret) : gpd;
> + break;
> + }
Maybe use an "else" here instead to avoid the double "break;".
> + pr_warn("Provider maybe present, unable to remove %s\n",
> + genpd->name);
> + genpd = ERR_PTR(-EBUSY);
> + break;
> + }
> + }
> + mutex_unlock(&gpd_list_lock);
> +
> + return genpd;
> +}
> +EXPORT_SYMBOL_GPL(of_genpd_remove_tail);
> +
> +/**
> * genpd_dev_pm_detach - Detach a device from its PM domain.
> * @dev: Device to detach.
> * @power_off: Currently not used
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 4aa285e44eb0..253f348b0ee9 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -203,6 +203,7 @@ extern int of_genpd_add_device(struct of_phandle_args *args,
> struct device *dev);
> extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
> struct of_phandle_args *new_subdomain);
> +extern struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np);
>
> int genpd_dev_pm_attach(struct device *dev);
> #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
> @@ -236,6 +237,12 @@ static inline int genpd_dev_pm_attach(struct device *dev)
> {
> return -ENODEV;
> }
> +
> +static inline
> +struct generic_pm_domain *of_genpd_remove_tail(struct device_node *np)
> +{
> + return ERR_PTR(-ENOTSUPP);
> +}
> #endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
>
> #ifdef CONFIG_PM
> --
> 2.1.4
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
