PM / OPP: use of_cpu_device_node_get() instead of of_get_cpu_node()

Message ID 1507632519-19648-1-git-send-email-sudeep.holla@arm.com
State New
Headers show
Series
  • PM / OPP: use of_cpu_device_node_get() instead of of_get_cpu_node()
Related show

Commit Message

Sudeep Holla Oct. 10, 2017, 10:48 a.m.
Commit 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")
moved away from using cpu_dev->of_node because of some limitations.
However commit 7467c9d95989 ("of: return of_get_cpu_node from
of_cpu_device_node_get if CPUs are not registered") added support to
falls back to of_get_cpu_node if called if CPUs are not registered yet.

This patch moves back to use of_cpu_device_node_get in
dev_pm_opp_of_get_sharing_cpus to avoid scanning the device tree again.
It also adds the missing of_node_put for the CPU device nodes.

Cc: Viresh Kumar <vireshk@kernel.org>
Cc: Nishanth Menon <nm@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Fixes: 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

---
 drivers/base/power/opp/of.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--
2.7.4

Comments

Viresh Kumar Oct. 10, 2017, 11:45 a.m. | #1
On 10-10-17, 11:48, Sudeep Holla wrote:
> Commit 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

> moved away from using cpu_dev->of_node because of some limitations.

> However commit 7467c9d95989 ("of: return of_get_cpu_node from

> of_cpu_device_node_get if CPUs are not registered") added support to

> falls back to of_get_cpu_node if called if CPUs are not registered yet.

> 

> This patch moves back to use of_cpu_device_node_get in

> dev_pm_opp_of_get_sharing_cpus to avoid scanning the device tree again.

> It also adds the missing of_node_put for the CPU device nodes.

> 

> Cc: Viresh Kumar <vireshk@kernel.org>

> Cc: Nishanth Menon <nm@ti.com>

> Cc: Stephen Boyd <sboyd@codeaurora.org>

> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

> Fixes: 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---

>  drivers/base/power/opp/of.c | 3 ++-

>  1 file changed, 2 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c

> index 0b718886479b..3505193043fe 100644

> --- a/drivers/base/power/opp/of.c

> +++ b/drivers/base/power/opp/of.c

> @@ -603,13 +603,14 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,

>  		if (cpu == cpu_dev->id)

>  			continue;

> 

> -		cpu_np = of_get_cpu_node(cpu, NULL);

> +		cpu_np = of_cpu_device_node_get(cpu);

>  		if (!cpu_np) {

>  			dev_err(cpu_dev, "%s: failed to get cpu%d node\n",

>  				__func__, cpu);

>  			ret = -ENOENT;

>  			goto put_cpu_node;

>  		}

> +		of_node_put(cpu_np);

> 

>  		/* Get OPP descriptor node */

>  		tmp_np = _opp_of_get_opp_desc_node(cpu_np);


This line is going to use cpu_np, how can we put cpu_np before this ?

And you need to rebase this on pm/linux-next.

-- 
viresh
Sudeep Holla Oct. 10, 2017, 12:39 p.m. | #2
On 10/10/17 12:45, Viresh Kumar wrote:
> On 10-10-17, 11:48, Sudeep Holla wrote:

>> Commit 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>> moved away from using cpu_dev->of_node because of some limitations.

>> However commit 7467c9d95989 ("of: return of_get_cpu_node from

>> of_cpu_device_node_get if CPUs are not registered") added support to

>> falls back to of_get_cpu_node if called if CPUs are not registered yet.

>>

>> This patch moves back to use of_cpu_device_node_get in

>> dev_pm_opp_of_get_sharing_cpus to avoid scanning the device tree again.

>> It also adds the missing of_node_put for the CPU device nodes.

>>

>> Cc: Viresh Kumar <vireshk@kernel.org>

>> Cc: Nishanth Menon <nm@ti.com>

>> Cc: Stephen Boyd <sboyd@codeaurora.org>

>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

>> Fixes: 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>> ---

>>  drivers/base/power/opp/of.c | 3 ++-

>>  1 file changed, 2 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c

>> index 0b718886479b..3505193043fe 100644

>> --- a/drivers/base/power/opp/of.c

>> +++ b/drivers/base/power/opp/of.c

>> @@ -603,13 +603,14 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,

>>  		if (cpu == cpu_dev->id)

>>  			continue;

>>

>> -		cpu_np = of_get_cpu_node(cpu, NULL);

>> +		cpu_np = of_cpu_device_node_get(cpu);

>>  		if (!cpu_np) {

>>  			dev_err(cpu_dev, "%s: failed to get cpu%d node\n",

>>  				__func__, cpu);

>>  			ret = -ENOENT;

>>  			goto put_cpu_node;

>>  		}

>> +		of_node_put(cpu_np);

>>

>>  		/* Get OPP descriptor node */

>>  		tmp_np = _opp_of_get_opp_desc_node(cpu_np);

> 

> This line is going to use cpu_np, how can we put cpu_np before this ?

> 


We didn't take the reference before commit 762792913f8c as we were using
cpu_dev->of_node directly. The above mentioned commit used
of_get_cpu_node() which introduces the reference. So I assumed it
shouldn't matter much and in order to keep the change simple, I did it
before _opp_of_get_opp_desc_node. I can move just after
_opp_of_get_opp_desc_node and before if check to avoid having both
before failure goto and after the block.

Let me know what is your preference ?

> And you need to rebase this on pm/linux-next.

> 


OK
-- 
Regards,
Sudeep
Sudeep Holla Oct. 10, 2017, 3:54 p.m. | #3
On 10/10/17 13:39, Sudeep Holla wrote:
> 

> 

> On 10/10/17 12:45, Viresh Kumar wrote:

>> On 10-10-17, 11:48, Sudeep Holla wrote:

>>> Commit 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>>> moved away from using cpu_dev->of_node because of some limitations.

>>> However commit 7467c9d95989 ("of: return of_get_cpu_node from

>>> of_cpu_device_node_get if CPUs are not registered") added support to

>>> falls back to of_get_cpu_node if called if CPUs are not registered yet.

>>>

>>> This patch moves back to use of_cpu_device_node_get in

>>> dev_pm_opp_of_get_sharing_cpus to avoid scanning the device tree again.

>>> It also adds the missing of_node_put for the CPU device nodes.

>>>

>>> Cc: Viresh Kumar <vireshk@kernel.org>

>>> Cc: Nishanth Menon <nm@ti.com>

>>> Cc: Stephen Boyd <sboyd@codeaurora.org>

>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

>>> Fixes: 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>> ---

>>>  drivers/base/power/opp/of.c | 3 ++-

>>>  1 file changed, 2 insertions(+), 1 deletion(-)

>>>

>>> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c

>>> index 0b718886479b..3505193043fe 100644

>>> --- a/drivers/base/power/opp/of.c

>>> +++ b/drivers/base/power/opp/of.c

>>> @@ -603,13 +603,14 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,

>>>  		if (cpu == cpu_dev->id)

>>>  			continue;

>>>

>>> -		cpu_np = of_get_cpu_node(cpu, NULL);

>>> +		cpu_np = of_cpu_device_node_get(cpu);

>>>  		if (!cpu_np) {

>>>  			dev_err(cpu_dev, "%s: failed to get cpu%d node\n",

>>>  				__func__, cpu);

>>>  			ret = -ENOENT;

>>>  			goto put_cpu_node;

>>>  		}

>>> +		of_node_put(cpu_np);

>>>

>>>  		/* Get OPP descriptor node */

>>>  		tmp_np = _opp_of_get_opp_desc_node(cpu_np);

>>

>> This line is going to use cpu_np, how can we put cpu_np before this ?

>>

> 

> We didn't take the reference before commit 762792913f8c as we were using

> cpu_dev->of_node directly. The above mentioned commit used

> of_get_cpu_node() which introduces the reference. So I assumed it

> shouldn't matter much and in order to keep the change simple, I did it

> before _opp_of_get_opp_desc_node. I can move just after

> _opp_of_get_opp_desc_node and before if check to avoid having both

> before failure goto and after the block.

> 

> Let me know what is your preference ?

> 

>> And you need to rebase this on pm/linux-next.

>>

> 

> OK

> 

I see the path has changed. I am not sure how you would consider this
patch ? as fix for v4.14(the refcount issue is introduced in v4.14)
or for v4.15

I can split the patch if use of of_cpu_device_node_get is not a fix
material. Let me know.

-- 
Regards,
Sudeep
Rafael J. Wysocki Oct. 10, 2017, 5:12 p.m. | #4
On Tue, Oct 10, 2017 at 5:54 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>

>

> On 10/10/17 13:39, Sudeep Holla wrote:

>>

>>

>> On 10/10/17 12:45, Viresh Kumar wrote:

>>> On 10-10-17, 11:48, Sudeep Holla wrote:

>>>> Commit 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>>>> moved away from using cpu_dev->of_node because of some limitations.

>>>> However commit 7467c9d95989 ("of: return of_get_cpu_node from

>>>> of_cpu_device_node_get if CPUs are not registered") added support to

>>>> falls back to of_get_cpu_node if called if CPUs are not registered yet.

>>>>

>>>> This patch moves back to use of_cpu_device_node_get in

>>>> dev_pm_opp_of_get_sharing_cpus to avoid scanning the device tree again.

>>>> It also adds the missing of_node_put for the CPU device nodes.

>>>>

>>>> Cc: Viresh Kumar <vireshk@kernel.org>

>>>> Cc: Nishanth Menon <nm@ti.com>

>>>> Cc: Stephen Boyd <sboyd@codeaurora.org>

>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

>>>> Fixes: 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>>> ---

>>>>  drivers/base/power/opp/of.c | 3 ++-

>>>>  1 file changed, 2 insertions(+), 1 deletion(-)

>>>>

>>>> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c

>>>> index 0b718886479b..3505193043fe 100644

>>>> --- a/drivers/base/power/opp/of.c

>>>> +++ b/drivers/base/power/opp/of.c

>>>> @@ -603,13 +603,14 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,

>>>>             if (cpu == cpu_dev->id)

>>>>                     continue;

>>>>

>>>> -           cpu_np = of_get_cpu_node(cpu, NULL);

>>>> +           cpu_np = of_cpu_device_node_get(cpu);

>>>>             if (!cpu_np) {

>>>>                     dev_err(cpu_dev, "%s: failed to get cpu%d node\n",

>>>>                             __func__, cpu);

>>>>                     ret = -ENOENT;

>>>>                     goto put_cpu_node;

>>>>             }

>>>> +           of_node_put(cpu_np);

>>>>

>>>>             /* Get OPP descriptor node */

>>>>             tmp_np = _opp_of_get_opp_desc_node(cpu_np);

>>>

>>> This line is going to use cpu_np, how can we put cpu_np before this ?

>>>

>>

>> We didn't take the reference before commit 762792913f8c as we were using

>> cpu_dev->of_node directly. The above mentioned commit used

>> of_get_cpu_node() which introduces the reference. So I assumed it

>> shouldn't matter much and in order to keep the change simple, I did it

>> before _opp_of_get_opp_desc_node. I can move just after

>> _opp_of_get_opp_desc_node and before if check to avoid having both

>> before failure goto and after the block.

>>

>> Let me know what is your preference ?

>>

>>> And you need to rebase this on pm/linux-next.

>>>

>>

>> OK

>>

> I see the path has changed. I am not sure how you would consider this

> patch ? as fix for v4.14(the refcount issue is introduced in v4.14)

> or for v4.15

>

> I can split the patch if use of of_cpu_device_node_get is not a fix

> material. Let me know.


It will go into 4.15, no need to resend.
Rafael J. Wysocki Oct. 10, 2017, 5:13 p.m. | #5
On Tue, Oct 10, 2017 at 7:12 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Oct 10, 2017 at 5:54 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

>>

>>

>> On 10/10/17 13:39, Sudeep Holla wrote:

>>>

>>>

>>> On 10/10/17 12:45, Viresh Kumar wrote:

>>>> On 10-10-17, 11:48, Sudeep Holla wrote:

>>>>> Commit 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>>>>> moved away from using cpu_dev->of_node because of some limitations.

>>>>> However commit 7467c9d95989 ("of: return of_get_cpu_node from

>>>>> of_cpu_device_node_get if CPUs are not registered") added support to

>>>>> falls back to of_get_cpu_node if called if CPUs are not registered yet.

>>>>>

>>>>> This patch moves back to use of_cpu_device_node_get in

>>>>> dev_pm_opp_of_get_sharing_cpus to avoid scanning the device tree again.

>>>>> It also adds the missing of_node_put for the CPU device nodes.

>>>>>

>>>>> Cc: Viresh Kumar <vireshk@kernel.org>

>>>>> Cc: Nishanth Menon <nm@ti.com>

>>>>> Cc: Stephen Boyd <sboyd@codeaurora.org>

>>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

>>>>> Fixes: 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>>>> ---

>>>>>  drivers/base/power/opp/of.c | 3 ++-

>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)

>>>>>

>>>>> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c

>>>>> index 0b718886479b..3505193043fe 100644

>>>>> --- a/drivers/base/power/opp/of.c

>>>>> +++ b/drivers/base/power/opp/of.c

>>>>> @@ -603,13 +603,14 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,

>>>>>             if (cpu == cpu_dev->id)

>>>>>                     continue;

>>>>>

>>>>> -           cpu_np = of_get_cpu_node(cpu, NULL);

>>>>> +           cpu_np = of_cpu_device_node_get(cpu);

>>>>>             if (!cpu_np) {

>>>>>                     dev_err(cpu_dev, "%s: failed to get cpu%d node\n",

>>>>>                             __func__, cpu);

>>>>>                     ret = -ENOENT;

>>>>>                     goto put_cpu_node;

>>>>>             }

>>>>> +           of_node_put(cpu_np);

>>>>>

>>>>>             /* Get OPP descriptor node */

>>>>>             tmp_np = _opp_of_get_opp_desc_node(cpu_np);

>>>>

>>>> This line is going to use cpu_np, how can we put cpu_np before this ?

>>>>

>>>

>>> We didn't take the reference before commit 762792913f8c as we were using

>>> cpu_dev->of_node directly. The above mentioned commit used

>>> of_get_cpu_node() which introduces the reference. So I assumed it

>>> shouldn't matter much and in order to keep the change simple, I did it

>>> before _opp_of_get_opp_desc_node. I can move just after

>>> _opp_of_get_opp_desc_node and before if check to avoid having both

>>> before failure goto and after the block.

>>>

>>> Let me know what is your preference ?

>>>

>>>> And you need to rebase this on pm/linux-next.

>>>>

>>>

>>> OK

>>>

>> I see the path has changed. I am not sure how you would consider this

>> patch ? as fix for v4.14(the refcount issue is introduced in v4.14)

>> or for v4.15

>>

>> I can split the patch if use of of_cpu_device_node_get is not a fix

>> material. Let me know.

>

> It will go into 4.15, no need to resend.


I mean no need to resend just because of the path change.

It looks like the patch itself needs to be modified, though.

Anyway, 4.15 material.
Sudeep Holla Oct. 10, 2017, 5:20 p.m. | #6
On 10/10/17 18:13, Rafael J. Wysocki wrote:
> On Tue, Oct 10, 2017 at 7:12 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:

>> On Tue, Oct 10, 2017 at 5:54 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

>>>

>>>

>>> On 10/10/17 13:39, Sudeep Holla wrote:

>>>>

>>>>

>>>> On 10/10/17 12:45, Viresh Kumar wrote:

>>>>> On 10-10-17, 11:48, Sudeep Holla wrote:

>>>>>> Commit 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>>>>>> moved away from using cpu_dev->of_node because of some limitations.

>>>>>> However commit 7467c9d95989 ("of: return of_get_cpu_node from

>>>>>> of_cpu_device_node_get if CPUs are not registered") added support to

>>>>>> falls back to of_get_cpu_node if called if CPUs are not registered yet.

>>>>>>

>>>>>> This patch moves back to use of_cpu_device_node_get in

>>>>>> dev_pm_opp_of_get_sharing_cpus to avoid scanning the device tree again.

>>>>>> It also adds the missing of_node_put for the CPU device nodes.

>>>>>>

>>>>>> Cc: Viresh Kumar <vireshk@kernel.org>

>>>>>> Cc: Nishanth Menon <nm@ti.com>

>>>>>> Cc: Stephen Boyd <sboyd@codeaurora.org>

>>>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>

>>>>>> Fixes: 762792913f8c ("PM / OPP: Fix get sharing CPUs when hotplug is used")

>>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

>>>>>> ---

>>>>>>  drivers/base/power/opp/of.c | 3 ++-

>>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)

>>>>>>

>>>>>> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c

>>>>>> index 0b718886479b..3505193043fe 100644

>>>>>> --- a/drivers/base/power/opp/of.c

>>>>>> +++ b/drivers/base/power/opp/of.c

>>>>>> @@ -603,13 +603,14 @@ int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,

>>>>>>             if (cpu == cpu_dev->id)

>>>>>>                     continue;

>>>>>>

>>>>>> -           cpu_np = of_get_cpu_node(cpu, NULL);

>>>>>> +           cpu_np = of_cpu_device_node_get(cpu);

>>>>>>             if (!cpu_np) {

>>>>>>                     dev_err(cpu_dev, "%s: failed to get cpu%d node\n",

>>>>>>                             __func__, cpu);

>>>>>>                     ret = -ENOENT;

>>>>>>                     goto put_cpu_node;

>>>>>>             }

>>>>>> +           of_node_put(cpu_np);

>>>>>>

>>>>>>             /* Get OPP descriptor node */

>>>>>>             tmp_np = _opp_of_get_opp_desc_node(cpu_np);

>>>>>

>>>>> This line is going to use cpu_np, how can we put cpu_np before this ?

>>>>>

>>>>

>>>> We didn't take the reference before commit 762792913f8c as we were using

>>>> cpu_dev->of_node directly. The above mentioned commit used

>>>> of_get_cpu_node() which introduces the reference. So I assumed it

>>>> shouldn't matter much and in order to keep the change simple, I did it

>>>> before _opp_of_get_opp_desc_node. I can move just after

>>>> _opp_of_get_opp_desc_node and before if check to avoid having both

>>>> before failure goto and after the block.

>>>>

>>>> Let me know what is your preference ?

>>>>

>>>>> And you need to rebase this on pm/linux-next.

>>>>>

>>>>

>>>> OK

>>>>

>>> I see the path has changed. I am not sure how you would consider this

>>> patch ? as fix for v4.14(the refcount issue is introduced in v4.14)

>>> or for v4.15

>>>

>>> I can split the patch if use of of_cpu_device_node_get is not a fix

>>> material. Let me know.

>>

>> It will go into 4.15, no need to resend.

> 

> I mean no need to resend just because of the path change.

> 

> It looks like the patch itself needs to be modified, though.

> 

> Anyway, 4.15 material.

> 


Got it. I will base it on pm/linux-next then when I repost.

-- 
Regards,
Sudeep
Viresh Kumar Oct. 11, 2017, 4:23 a.m. | #7
On 10-10-17, 13:39, Sudeep Holla wrote:
> We didn't take the reference before commit 762792913f8c as we were using

> cpu_dev->of_node directly.


Yeah, and so the refcount was never screwed for us.

> The above mentioned commit used

> of_get_cpu_node() which introduces the reference.


And it missing putting it down and we missed catching that in reviews.

> So I assumed it

> shouldn't matter much and in order to keep the change simple, I did it

> before _opp_of_get_opp_desc_node. I can move just after

> _opp_of_get_opp_desc_node and before if check to avoid having both

> before failure goto and after the block.


So the right thing to do based on current code is to put the reference only
after we are done using the pointer.

-- 
viresh

Patch

diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
index 0b718886479b..3505193043fe 100644
--- a/drivers/base/power/opp/of.c
+++ b/drivers/base/power/opp/of.c
@@ -603,13 +603,14 @@  int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev,
 		if (cpu == cpu_dev->id)
 			continue;

-		cpu_np = of_get_cpu_node(cpu, NULL);
+		cpu_np = of_cpu_device_node_get(cpu);
 		if (!cpu_np) {
 			dev_err(cpu_dev, "%s: failed to get cpu%d node\n",
 				__func__, cpu);
 			ret = -ENOENT;
 			goto put_cpu_node;
 		}
+		of_node_put(cpu_np);

 		/* Get OPP descriptor node */
 		tmp_np = _opp_of_get_opp_desc_node(cpu_np);