On 08/09/16 13:30, Ulf Hansson wrote:
> 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?
Ok. I was not sure if it would seem odd to add pm_genpd_remove() and
genpd_remove() in the same patch because pm_genpd_remove() is the only
user of genpd_remove(). However, it would simplify the diff and so I am
fine with that.
>>
>> #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?
I think that I would prefer either of_genpd_remove_last() or
of_genpd_remove_one(). Although _first is accurate from the list
perspective it seems odd from the user perspective. I think that _last
is more meaningful as we are removing the last that was added regardless
or how things appear on the list. Alternatively, _one could be a good
compromise.
>> + *
>> + * 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;".
Ok.
Cheers
Jon
--
nvpublic
--
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
