Re: [PATCH] PM / OPP: Don't check not plugged in CPUs to avoid error

Re: [PATCH] PM / OPP: Don't check not plugged in CPUs to avoid error

[Stephen Boyd]

On 07/13, Waldemar Rymarkiewicz wrote:
> On systems where not all possible CPUs are available for Linux call to
> dev_pm_opp_of_get_sharing_cpus() will fail as cannot find a cpu device
> for not present (not available) CPUs.
> 
> Example (real use case):
> 2 physical MIPS cores, 4 VPE, cpu0/2 run Linux and cpu1/3 are not available
> for linux. cpufreq-dt driver + opp v2 fail to register opp_table due to the
> fact there is no struct device for cpu1 (cpu1 not available for Linux).
> 
> Solve the problem by iterating over cpu_present_mask instead of
> cpu_possible_mask.
> 
> Signed-off-by: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

> ---
>  drivers/base/power/opp/of.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> index 57eec1c..389c924 100644
> --- a/drivers/base/power/opp/of.c
> +++ b/drivers/base/power/opp/of.c
> @@ -589,7 +589,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
>  	if (!of_property_read_bool(np, "opp-shared"))
>  		goto put_cpu_node;
>  
> -	for_each_possible_cpu(cpu) {
> +	for_each_present_cpu(cpu) {
>  		if (cpu == cpu_dev->id)
>  			continue;
>  

Is there some way to iterate through all cpu nodes in DT without
using devices? I suppose some sort of loop on top of
of_get_cpu_node().

I worry that we may have some problem where we want to know about
all the CPUs that are sharing a particular OPP table but they
haven't been physically hotplugged yet. This also raises the
question about what the DT looks like before and after a CPU is
inserted into a system. If the non-present CPU nodes are in DT
all the time, then it may make sense for cpufreq-dt to intersect
the OPP sharing mask with the cpu_present_mask.

Probably nobody cares about knowing this difference though,
because we pretty much forward the result to cpufreq, so moving
to the present mask is good enough.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] PM / OPP: Don't check not plugged in CPUs to avoid error

[Viresh Kumar]

On 13-07-17, 16:42, Stephen Boyd wrote:
> On 07/13, Waldemar Rymarkiewicz wrote:
> > On systems where not all possible CPUs are available for Linux call to
> > dev_pm_opp_of_get_sharing_cpus() will fail as cannot find a cpu device
> > for not present (not available) CPUs.
> > 
> > Example (real use case):
> > 2 physical MIPS cores, 4 VPE, cpu0/2 run Linux and cpu1/3 are not available
> > for linux. cpufreq-dt driver + opp v2 fail to register opp_table due to the
> > fact there is no struct device for cpu1 (cpu1 not available for Linux).
> > 
> > Solve the problem by iterating over cpu_present_mask instead of
> > cpu_possible_mask.
> > 
> > Signed-off-by: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> 
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

> > ---
> >  drivers/base/power/opp/of.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> > index 57eec1c..389c924 100644
> > --- a/drivers/base/power/opp/of.c
> > +++ b/drivers/base/power/opp/of.c
> > @@ -589,7 +589,7 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
> >  	if (!of_property_read_bool(np, "opp-shared"))
> >  		goto put_cpu_node;
> >  
> > -	for_each_possible_cpu(cpu) {
> > +	for_each_present_cpu(cpu) {
> >  		if (cpu == cpu_dev->id)
> >  			continue;
> >  
> 
> Is there some way to iterate through all cpu nodes in DT without
> using devices? I suppose some sort of loop on top of
> of_get_cpu_node().

I am looking for the same thing :)

> I worry that we may have some problem where we want to know about
> all the CPUs that are sharing a particular OPP table but they
> haven't been physically hotplugged yet.

We will have a problem if we are going to use OPP framework on
platforms where CPUs are hotpluggable. The cpumask returned by this
function is used by the cpufreq drivers to get the related_cpus mask
and that will be used when a cpu is inserted in later on.

Do we have such users of OPP framework right now ? ARM doesn't have
such platforms AFAIK. Does MIPS have it ?

> This also raises the
> question about what the DT looks like before and after a CPU is
> inserted into a system. If the non-present CPU nodes are in DT
> all the time, then it may make sense for cpufreq-dt to intersect
> the OPP sharing mask with the cpu_present_mask.

We need the related-cpus mask in cpufreq to include non-present CPUs
as well.

-- 
viresh

Re: [PATCH] PM / OPP: Don't check not plugged in CPUs to avoid error

[Waldemar Rymarkiewicz]

On 17 July 2017 at 09:12, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 13-07-17, 16:42, Stephen Boyd wrote:
>> On 07/13, Waldemar Rymarkiewicz wrote:

>> Is there some way to iterate through all cpu nodes in DT without
>> using devices? I suppose some sort of loop on top of
>> of_get_cpu_node().
>
> I am looking for the same thing :)
>
>> I worry that we may have some problem where we want to know about
>> all the CPUs that are sharing a particular OPP table but they
>> haven't been physically hotplugged yet.

Isn't that cpufreq registers only online CPUs at boot up and will add
hot-added CPUs later on calling cpufreq_online() ~>
cpufreq_driver->init()?

See   cpufreq_online()

   http://elixir.free-electrons.com/linux/v4.13-rc1/source/drivers/cpufreq/cpufreq.c#L1143

this calls cpufreq_driver->init() which check for sharing CPUs. If
that's called for online_cpus only so I guess it's OK to check only
within online cpus for shared freq_table. If a cpu is plugged in later
 this is called again, so we can eventually add this cpu to the mask.
Is this right?



> We will have a problem if we are going to use OPP framework on
> platforms where CPUs are hotpluggable. The cpumask returned by this
> function is used by the cpufreq drivers to get the related_cpus mask
> and that will be used when a cpu is inserted in later on.

related_cpus bitmask is set in cpufreq_online() too,

   http://elixir.free-electrons.com/linux/v4.13-rc1/source/drivers/cpufreq/cpufreq.c#L1188

Policy->cpus is for online CPUs, so where are the offline CPUs added
to related_cpus mask?


> Do we have such users of OPP framework right now ? ARM doesn't have
> such platforms AFAIK. Does MIPS have it ?

I don't think so there is an up streamed code that uses opp framework
in such use case. I use MIPS platform which starts with maxcpu=4 but
it puts 2 cpus offline at boot up for some reason. Will try to check
how cpufreq behave if I hot-add a CPU in runtime.

>> This also raises the
>> question about what the DT looks like before and after a CPU is
>> inserted into a system. If the non-present CPU nodes are in DT
>> all the time, then it may make sense for cpufreq-dt to intersect
>> the OPP sharing mask with the cpu_present_mask.
>
> We need the related-cpus mask in cpufreq to include non-present CPUs
> as well.

Why? can you refer to the code? I'd like to see it and understand better.

/Waldek

>
> --
> viresh

Re: [PATCH] PM / OPP: Don't check not plugged in CPUs to avoid error

[Viresh Kumar]

On 17-07-17, 15:03, Waldemar Rymarkiewicz wrote:
> Isn't that cpufreq registers only online CPUs at boot up and will add
> hot-added CPUs later on calling cpufreq_online() ~>

Yes, but its a bit complicated than that.

> cpufreq_driver->init()?

init() is called only once for a cpufreq policy and it should fill
policy->cpus with all the *possible* CPUs that share their
clock/voltage rails. i.e. it includes the CPUs which aren't hotplugged
in yet.

Later on when the CPU gets hotplugged in, we check it in existing
policies ->related_cpus field. If its their, then very minimal stuff
is done, else a new policy is created.

> this calls cpufreq_driver->init() which check for sharing CPUs. If
> that's called for online_cpus only so I guess it's OK to check only
> within online cpus for shared freq_table. If a cpu is plugged in later
>  this is called again, so we can eventually add this cpu to the mask.
> Is this right?
>
> > We will have a problem if we are going to use OPP framework on
> > platforms where CPUs are hotpluggable. The cpumask returned by this
> > function is used by the cpufreq drivers to get the related_cpus mask
> > and that will be used when a cpu is inserted in later on.

And that's why I said this.

> related_cpus bitmask is set in cpufreq_online() too,
> 
>    http://elixir.free-electrons.com/linux/v4.13-rc1/source/drivers/cpufreq/cpufreq.c#L1188
> 
> Policy->cpus is for online CPUs, so where are the offline CPUs added
> to related_cpus mask?

So policy->init() sets policy->cpus to all online+offline CPUs. That
is copied to related_cpus by core and then policy->cpus is capped to
only have online CPUs.
 
> I don't think so there is an up streamed code that uses opp framework
> in such use case. I use MIPS platform which starts with maxcpu=4 but
> it puts 2 cpus offline at boot up for some reason. Will try to check
> how cpufreq behave if I hot-add a CPU in runtime.

Ok.

-- 
viresh

Re: [PATCH] PM / OPP: Don't check not plugged in CPUs to avoid error

[Waldemar Rymarkiewicz]

On 18 July 2017 at 06:35, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 17-07-17, 15:03, Waldemar Rymarkiewicz wrote:
> init() is called only once for a cpufreq policy and it should fill
> policy->cpus with all the *possible* CPUs that share their
> clock/voltage rails. i.e. it includes the CPUs which aren't hotplugged
> in yet.
> Later on when the CPU gets hotplugged in, we check it in existing
> policies ->related_cpus field. If its their, then very minimal stuff
> is done, else a new policy is created.

I see. That is not obvious from the code what policy->cpus should
initially include. Now I understand.

policy->related_cpu contains online+offline cpus that share this policy
policy->cpus initially includes online+offline, but end the end it
keeps online cpus mask only that share this policy.

> So policy->init() sets policy->cpus to all online+offline CPUs. That
> is copied to related_cpus by core and then policy->cpus is capped to
> only have online CPUs.

>> I don't think so there is an up streamed code that uses opp framework
>> in such use case. I use MIPS platform which starts with maxcpu=4 but
>> it puts 2 cpus offline at boot up for some reason. Will try to check
>> how cpufreq behave if I hot-add a CPU in runtime.
>
> Ok.
Unfortunately, I failed to test this on my platform, seems like
hotplug is not fully supported yet.

Anyway, below a proposal how we can solve the problem of getting
shared CPUs when hotplug is used.


@@ -555,8 +576,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_cpumask_add_table);
 int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
                                   struct cpumask *cpumask)
 {
-       struct device_node *np, *tmp_np;
-       struct device *tcpu_dev;
+       struct device_node *np, *tmp_np, *cpu_np;
        int cpu, ret = 0;

        /* Get OPP descriptor node */
@@ -572,22 +592,16 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
        if (!of_property_read_bool(np, "opp-shared"))
                goto put_cpu_node;

-       for_each_possible_cpu(cpu) {
+       for_each_node_by_type(cpu_np, "cpu") {
+               cpu = of_device_node_get_cpu(cpu_np);
+
                if (cpu == cpu_dev->id)
                        continue;

-               tcpu_dev = get_cpu_device(cpu);
-               if (!tcpu_dev) {
-                       dev_err(cpu_dev, "%s: failed to get cpu%d device\n",
-                               __func__, cpu);
-                       ret = -ENODEV;
-                       goto put_cpu_node;
-               }
-
                /* Get OPP descriptor node */
-               tmp_np = _of_get_opp_desc_node(tcpu_dev);
+               tmp_np = of_parse_phandle(cpu_np, "operating-points-v2", 0);
                if (!tmp_np) {
-                       dev_err(tcpu_dev, "%s: Couldn't find tcpu_dev node.\n",
+                       dev_err(cpu_dev, "%s: Couldn't find tcpu_dev node.\n",
                                __func__);
                        ret = -ENOENT;
                        goto put_cpu_node;


the helper function
   of_device_node_get_cpu()
comes from ML  https://patchwork.kernel.org/patch/9830203/

What are your thoughts?

/Waldek

Re: [PATCH] PM / OPP: Don't check not plugged in CPUs to avoid error

[Viresh Kumar]

On 19-07-17, 16:10, Waldemar Rymarkiewicz wrote:
> Anyway, below a proposal how we can solve the problem of getting
> shared CPUs when hotplug is used.
> 
> 
> @@ -555,8 +576,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_of_cpumask_add_table);
>  int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
>                                    struct cpumask *cpumask)
>  {
> -       struct device_node *np, *tmp_np;
> -       struct device *tcpu_dev;
> +       struct device_node *np, *tmp_np, *cpu_np;
>         int cpu, ret = 0;
> 
>         /* Get OPP descriptor node */
> @@ -572,22 +592,16 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
>         if (!of_property_read_bool(np, "opp-shared"))
>                 goto put_cpu_node;
> 
> -       for_each_possible_cpu(cpu) {
> +       for_each_node_by_type(cpu_np, "cpu") {
> +               cpu = of_device_node_get_cpu(cpu_np);

Actually you can keep the original loop as it is and use
of_get_cpu_node() here. We better open code of_device_node_get_cpu()
here.

> +
>                 if (cpu == cpu_dev->id)
>                         continue;
> 
> -               tcpu_dev = get_cpu_device(cpu);
> -               if (!tcpu_dev) {
> -                       dev_err(cpu_dev, "%s: failed to get cpu%d device\n",
> -                               __func__, cpu);
> -                       ret = -ENODEV;
> -                       goto put_cpu_node;
> -               }
> -
>                 /* Get OPP descriptor node */
> -               tmp_np = _of_get_opp_desc_node(tcpu_dev);

I see dev_pm_opp_of_get_opp_desc_node() here, not sure how you got
above change..

> +               tmp_np = of_parse_phandle(cpu_np, "operating-points-v2", 0);

I would create _opp_of_get_opp_desc_node(np) and use it everywhere,
i.e. from dev_pm_opp_of_get_opp_desc_node() as well.

>                 if (!tmp_np) {
> -                       dev_err(tcpu_dev, "%s: Couldn't find tcpu_dev node.\n",
> +                       dev_err(cpu_dev, "%s: Couldn't find tcpu_dev node.\n",
>                                 __func__);
>                         ret = -ENOENT;
>                         goto put_cpu_node;

Thanks.

-- 
viresh

Re: [PATCH] PM / OPP: Don't check not plugged in CPUs to avoid error

[Waldemar Rymarkiewicz]

On 20 July 2017 at 05:31, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 19-07-17, 16:10, Waldemar Rymarkiewicz wrote:
>> Anyway, below a proposal how we can solve the problem of getting
>> shared CPUs w
>>         /* Get OPP descriptor node */
>> @@ -572,22 +592,16 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
>>         if (!of_property_read_bool(np, "opp-shared"))
>>                 goto put_cpu_node;
>>
>> -       for_each_possible_cpu(cpu) {
>> +       for_each_node_by_type(cpu_np, "cpu") {
>> +               cpu = of_device_node_get_cpu(cpu_np);
>
> Actually you can keep the original loop as it is and use
> of_get_cpu_node() here. We better open code of_device_node_get_cpu()
> here.

Indeed, I can do so.

>> +
>>                 if (cpu == cpu_dev->id)
>>                         continue;
>>
>> -               tcpu_dev = get_cpu_device(cpu);
>> -               if (!tcpu_dev) {
>> -                       dev_err(cpu_dev, "%s: failed to get cpu%d device\n",
>> -                               __func__, cpu);
>> -                       ret = -ENODEV;
>> -                       goto put_cpu_node;
>> -               }
>> -
>>                 /* Get OPP descriptor node */
>> -               tmp_np = _of_get_opp_desc_node(tcpu_dev);
>
> I see dev_pm_opp_of_get_opp_desc_node() here, not sure how you got
> above change..

Yeah, I test on 4.9 kernel. I will include your comments and build
patch on top of linux-next/master anyway.

>> +               tmp_np = of_parse_phandle(cpu_np, "operating-points-v2", 0);
>
> I would create _opp_of_get_opp_desc_node(np) and use it everywhere,
> i.e. from dev_pm_opp_of_get_opp_desc_node() as well.

Makes sense.

/Waldek