| Message ID | 1507632519-19648-1-git-send-email-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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.
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
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.
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.
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.
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.
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.
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);
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