| Message ID | 20240503-sun50i-cpufreq-nvmem-cleanup-v1-1-0a2352cac46b@gmail.com |
|---|---|
| State | Accepted |
| Commit | 6282fba6abd7c3c8896c239cc8aa9ec45edcb97b |
| Headers | show |
| Series | cpufreq: sun50i: fix memory leak and remove of_node_put() | expand |
On Fri, 03 May 2024 19:52:32 +0200 Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: Hi Javier, > The for_each_child_of_node() loop does not decrement the child node > refcount before the break instruction, even though the node is no > longer required. Ah, thanks for spotting this, there is indeed a leak. Sorry for the blunder! > This can be avoided with the new for_each_child_of_node_scoped() macro > that removes the need for any of_node_put(). Wow, that's the typical convoluted Linux macro, but it looks correct to me. It would call the put even if the loop ends naturally, but there is a NULL test in there, so that's fine. > Fixes: fa5aec9561cf ("cpufreq: sun50i: Add support for opp_supported_hw") > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> Reviewed-by: Andre Przywara <andre.przywara@arm.com> Thanks! Andre > --- > drivers/cpufreq/sun50i-cpufreq-nvmem.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > index 0b882765cd66..ef83e4bf2639 100644 > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > @@ -131,7 +131,7 @@ static const struct of_device_id cpu_opp_match_list[] = { > static bool dt_has_supported_hw(void) > { > bool has_opp_supported_hw = false; > - struct device_node *np, *opp; > + struct device_node *np; > struct device *cpu_dev; > > cpu_dev = get_cpu_device(0); > @@ -142,7 +142,7 @@ static bool dt_has_supported_hw(void) > if (!np) > return false; > > - for_each_child_of_node(np, opp) { > + for_each_child_of_node_scoped(np, opp) { > if (of_find_property(opp, "opp-supported-hw", NULL)) { > has_opp_supported_hw = true; > break; >
On 10-05-24, 17:49, Andre Przywara wrote: > On Fri, 03 May 2024 19:52:32 +0200 > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > index 0b882765cd66..ef83e4bf2639 100644 > > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > @@ -131,7 +131,7 @@ static const struct of_device_id cpu_opp_match_list[] = { > > static bool dt_has_supported_hw(void) > > { > > bool has_opp_supported_hw = false; > > - struct device_node *np, *opp; > > + struct device_node *np; Why is the opp pointer removed ? > > struct device *cpu_dev; > > > > cpu_dev = get_cpu_device(0); > > @@ -142,7 +142,7 @@ static bool dt_has_supported_hw(void) > > if (!np) > > return false; > > > > - for_each_child_of_node(np, opp) { > > + for_each_child_of_node_scoped(np, opp) { > > if (of_find_property(opp, "opp-supported-hw", NULL)) { > > has_opp_supported_hw = true; > > break; > >
On Mon, 20 May 2024 13:03:39 +0530 Viresh Kumar <viresh.kumar@linaro.org> wrote: Hi, > On 10-05-24, 17:49, Andre Przywara wrote: > > On Fri, 03 May 2024 19:52:32 +0200 > > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > > index 0b882765cd66..ef83e4bf2639 100644 > > > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > > @@ -131,7 +131,7 @@ static const struct of_device_id cpu_opp_match_list[] = { > > > static bool dt_has_supported_hw(void) > > > { > > > bool has_opp_supported_hw = false; > > > - struct device_node *np, *opp; > > > + struct device_node *np; > > Why is the opp pointer removed ? Because it's now declared *inside* the for_each_child_of_node_scoped loop below, courtesy of this new macro. The idea is that by doing so, any "break;" will exit the scope, triggering the cleanup routine. The loop running till "the end" will also make "opp" exit its scope, triggering the same routine. Cheers, Andre > > > > struct device *cpu_dev; > > > > > > cpu_dev = get_cpu_device(0); > > > @@ -142,7 +142,7 @@ static bool dt_has_supported_hw(void) > > > if (!np) > > > return false; > > > > > > - for_each_child_of_node(np, opp) { > > > + for_each_child_of_node_scoped(np, opp) { > > > if (of_find_property(opp, "opp-supported-hw", NULL)) { > > > has_opp_supported_hw = true; > > > break; > > > >
On 20-05-24, 09:26, Andre Przywara wrote: > On Mon, 20 May 2024 13:03:39 +0530 > Viresh Kumar <viresh.kumar@linaro.org> wrote: > > Hi, > > > On 10-05-24, 17:49, Andre Przywara wrote: > > > On Fri, 03 May 2024 19:52:32 +0200 > > > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote: > > > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > > > index 0b882765cd66..ef83e4bf2639 100644 > > > > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > > > > @@ -131,7 +131,7 @@ static const struct of_device_id cpu_opp_match_list[] = { > > > > static bool dt_has_supported_hw(void) > > > > { > > > > bool has_opp_supported_hw = false; > > > > - struct device_node *np, *opp; > > > > + struct device_node *np; > > > > Why is the opp pointer removed ? > > Because it's now declared *inside* the for_each_child_of_node_scoped loop > below, courtesy of this new macro. The idea is that by doing so, any > "break;" will exit the scope, triggering the cleanup routine. The loop > running till "the end" will also make "opp" exit its scope, triggering the > same routine. Applied. Thanks.
diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c index 0b882765cd66..ef83e4bf2639 100644 --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c @@ -131,7 +131,7 @@ static const struct of_device_id cpu_opp_match_list[] = { static bool dt_has_supported_hw(void) { bool has_opp_supported_hw = false; - struct device_node *np, *opp; + struct device_node *np; struct device *cpu_dev; cpu_dev = get_cpu_device(0); @@ -142,7 +142,7 @@ static bool dt_has_supported_hw(void) if (!np) return false; - for_each_child_of_node(np, opp) { + for_each_child_of_node_scoped(np, opp) { if (of_find_property(opp, "opp-supported-hw", NULL)) { has_opp_supported_hw = true; break;
The for_each_child_of_node() loop does not decrement the child node refcount before the break instruction, even though the node is no longer required. This can be avoided with the new for_each_child_of_node_scoped() macro that removes the need for any of_node_put(). Fixes: fa5aec9561cf ("cpufreq: sun50i: Add support for opp_supported_hw") Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/cpufreq/sun50i-cpufreq-nvmem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)