Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1662308

Re: [PATCH V7 1/2] PM / Domains: Add support to select performance-state of domains

Show all headers | View raw

On 8 June 2017 at 11:42, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 08-06-17, 09:48, Ulf Hansson wrote:
>> It's not a nightmare, just a tricky thing to solve. :-)
>
> I may have just solved it actually :)

That was quick. :-)

>
> So the real locking problem was the case where a subdomain have multiple parent
> domains and how do we access its performance_state field from all the paths that
> originate from the parent's update and from the subdomains own path. And a
> single performance_state field isn't going to help in that as multiple locks are
> involved here. I have added another per parent-domain field and that will help
> get the locking quite simple. Here is the new patch (compile tested):

Obviously you didn't think about this long enough. Please spare me
from having to review something of this complexity just being compile
tested. I don't have unlimited bandwidth. :-)

I recommend at least some functional tests run together with lockdep tests.

>
> ---
>  drivers/base/power/domain.c | 193 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h   |  22 +++++
>  2 files changed, 215 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 71c95ad808d5..40815974287f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -466,6 +466,187 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>         return NOTIFY_DONE;
>  }
>
> +/*
> + * Returns true if anyone in genpd's parent hierarchy has
> + * set_performance_state() set.
> + */
> +static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
> +{
> +       struct gpd_link *link;
> +
> +       if (genpd->set_performance_state)
> +               return true;
> +
> +       list_for_each_entry(link, &genpd->slave_links, slave_node) {
> +               if (genpd_has_set_performance_state(link->master))
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
> +/**
> + * pm_genpd_has_performance_state - Checks if power domain does performance
> + * state management.
> + *
> + * @dev: Device whose power domain is getting inquired.
> + */
> +bool pm_genpd_has_performance_state(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = dev_to_genpd(dev);
> +
> +       /* The parent domain must have set get_performance_state() */
> +       if (!IS_ERR(genpd) && genpd->get_performance_state)
> +               return true;
> +
> +       return false;
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_has_performance_state);

I think you didn't read my earlier comments well enough.

Who is going to call this? What prevents the genpd object from being
removed in the middle of when you are operating on the genpd pointer?
How can you even be sure the pm_domain pointer is a valid genpd
object?

[...]

Kind regards
Uffe

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH V7 1/2] PM / Domains: Add support to select  performance-state of domains Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-08 09:10 +0200
  Re: [PATCH V7 1/2] PM / Domains: Add support to select  performance-state of domains Ulf Hansson <ulf.hansson@linaro.org> - 2017-06-08 09:50 +0200
    Re: [PATCH V7 1/2] PM / Domains: Add support to select  performance-state of domains Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-08 11:50 +0200
      Re: [PATCH V7 1/2] PM / Domains: Add support to select  performance-state of domains Ulf Hansson <ulf.hansson@linaro.org> - 2017-06-09 13:10 +0200
        Re: [PATCH V7 1/2] PM / Domains: Add support to select  performance-state of domains Viresh Kumar <viresh.kumar@linaro.org> - 2017-06-13 13:00 +0200

csiph-web