Re: [PATCH v3 1/1] PM / OPP: Fix get sharing cpus when hotplug is used

Re: [PATCH v3 1/1] PM / OPP: Fix get sharing cpus when hotplug is used

[Stephen Boyd]

On 07/24/2017 09:03 AM, Waldemar Rymarkiewicz wrote:
> We fail dev_pm_opp_of_get_sharing_cpus() when possible cpu device does not
> exist. This can happen on platforms where not all possible CPUs are
> available at start up ie. hotplugged out. Cpu device is not registered in
> the system so we are not able to check struct device to set the sharing
> CPUs bitmask properly.
>
> Example (real use case):
> 2 physical MIPS cores, 4 VPE, cpu0/2 run Linux and cpu1/3 are not available
> for Linux at boot up. cpufreq-dt driver + opp v2 fail to register opp_table
> due to the fact there is no struct device for cpu1 (remains offline at
> bootup).
>
> To solve the bug, stop using device struct to check device_node. Instead
> get cpu device_node directly from device tree with of_get_cpu_node().
>
> Signed-off-by: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---

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

One minor nit, but it's fine to change later as well.

> @@ -593,18 +599,18 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
>  		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",
> +		cpu_np = of_get_cpu_node(cpu, NULL);
> +		if (!cpu_np) {
> +			dev_err(cpu_dev, "%s: failed to get cpu%d node\n",
>  				__func__, cpu);
> -			ret = -ENODEV;
> +			ret = -ENOENT;
>  			goto put_cpu_node;
>  		}
>  
>  		/* Get OPP descriptor node */
> -		tmp_np = dev_pm_opp_of_get_opp_desc_node(tcpu_dev);
> +		tmp_np = _opp_of_get_opp_desc_node(cpu_np);
>  		if (!tmp_np) {
> -			dev_err(tcpu_dev, "%s: Couldn't find opp node.\n",
> +			dev_err(cpu_dev, "%s: Couldn't find opp node.\n",

I would remove the full-stop. And also use %pOF with cpu_np and pr_err()
instead of dev_err() here so that we print out the node that's missing
the OPP node pointer.

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

Re: [PATCH v3 1/1] PM / OPP: Fix get sharing cpus when hotplug is used

[Viresh Kumar]

+Rafael,

On 24-07-17, 18:03, Waldemar Rymarkiewicz wrote:
> We fail dev_pm_opp_of_get_sharing_cpus() when possible cpu device does not
> exist. This can happen on platforms where not all possible CPUs are
> available at start up ie. hotplugged out. Cpu device is not registered in
> the system so we are not able to check struct device to set the sharing
> CPUs bitmask properly.
> 
> Example (real use case):
> 2 physical MIPS cores, 4 VPE, cpu0/2 run Linux and cpu1/3 are not available
> for Linux at boot up. cpufreq-dt driver + opp v2 fail to register opp_table
> due to the fact there is no struct device for cpu1 (remains offline at
> bootup).
> 
> To solve the bug, stop using device struct to check device_node. Instead
> get cpu device_node directly from device tree with of_get_cpu_node().
> 
> Signed-off-by: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/opp/of.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)

@Waldemar: I don't see this series on patchwork [1]. Are you
subscribed for that list ?

-- 
viresh

[1] https://patchwork.kernel.org/project/linux-pm/list/

Re: [PATCH v3 1/1] PM / OPP: Fix get sharing cpus when hotplug is used

[Waldemar Rymarkiewicz]

On 26 July 2017 at 02:54, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 07/24/2017 09:03 AM, Waldemar Rymarkiewicz wrote:
>
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
>
> One minor nit, but it's fine to change later as well.
>
>> @@ -593,18 +599,18 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
>>               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",
>> +             cpu_np = of_get_cpu_node(cpu, NULL);
>> +             if (!cpu_np) {
>> +                     dev_err(cpu_dev, "%s: failed to get cpu%d node\n",
>>                               __func__, cpu);
>> -                     ret = -ENODEV;
>> +                     ret = -ENOENT;
>>                       goto put_cpu_node;
>>               }
>>
>>               /* Get OPP descriptor node */
>> -             tmp_np = dev_pm_opp_of_get_opp_desc_node(tcpu_dev);
>> +             tmp_np = _opp_of_get_opp_desc_node(cpu_np);
>>               if (!tmp_np) {
>> -                     dev_err(tcpu_dev, "%s: Couldn't find opp node.\n",
>> +                     dev_err(cpu_dev, "%s: Couldn't find opp node.\n",
>
> I would remove the full-stop. And also use %pOF with cpu_np and pr_err()
> instead of dev_err() here so that we print out the node that's missing
> the OPP node pointer.

Makes sense. I  will send v4 anyway so can include this too.

/Waldek

Re: [PATCH v3 1/1] PM / OPP: Fix get sharing cpus when hotplug is used

[Waldemar Rymarkiewicz]

On 26 July 2017 at 08:16, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> +Rafael,
>
> On 24-07-17, 18:03, Waldemar Rymarkiewicz wrote:
>> We fail dev_pm_opp_of_get_sharing_cpus() when possible cpu device does not
>> exist. This can happen on platforms where not all possible CPUs are
>> available at start up ie. hotplugged out. Cpu device is not registered in
>> the system so we are not able to check struct device to set the sharing
>> CPUs bitmask properly.
>>
>> Example (real use case):
>> 2 physical MIPS cores, 4 VPE, cpu0/2 run Linux and cpu1/3 are not available
>> for Linux at boot up. cpufreq-dt driver + opp v2 fail to register opp_table
>> due to the fact there is no struct device for cpu1 (remains offline at
>> bootup).
>>
>> To solve the bug, stop using device struct to check device_node. Instead
>> get cpu device_node directly from device tree with of_get_cpu_node().
>>
>> Signed-off-by: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  drivers/base/power/opp/of.c | 28 +++++++++++++++++-----------
>>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> @Waldemar: I don't see this series on patchwork [1]. Are you
> subscribed for that list ?

Well, I am indeed but with my gmail. I have all my mailboxes attached
to my patchwork account but didn't know that patchwork checks for the
subscription before it picks up a patch.  Will subscribe with my
@intel.com and send v4  including Stephen comment and Reviewed-by.

/Waldek