cpufreq: sun50i: fix memory leak and remove of_node_put()

[PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler

Posted by Javier Carrasco 1 year, 7 months ago

Make use of the __free() cleanup handler to automatically free nodes
when they get out of scope.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index ef83e4bf2639..eb47c193269c 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -131,14 +131,14 @@ 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;
 	struct device *cpu_dev;
 
 	cpu_dev = get_cpu_device(0);
 	if (!cpu_dev)
 		return false;
 
-	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
+	struct device_node *np __free(device_node) =
+		dev_pm_opp_of_get_opp_desc_node(cpu_dev);
 	if (!np)
 		return false;
 
@@ -149,8 +149,6 @@ static bool dt_has_supported_hw(void)
 		}
 	}
 
-	of_node_put(np);
-
 	return has_opp_supported_hw;
 }
 
@@ -165,7 +163,6 @@ static int sun50i_cpufreq_get_efuse(void)
 	const struct sunxi_cpufreq_data *opp_data;
 	struct nvmem_cell *speedbin_nvmem;
 	const struct of_device_id *match;
-	struct device_node *np;
 	struct device *cpu_dev;
 	u32 *speedbin;
 	int ret;
@@ -174,19 +171,18 @@ static int sun50i_cpufreq_get_efuse(void)
 	if (!cpu_dev)
 		return -ENODEV;
 
-	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
+	struct device_node *np __free(device_node) =
+		dev_pm_opp_of_get_opp_desc_node(cpu_dev);
 	if (!np)
 		return -ENOENT;
 
 	match = of_match_node(cpu_opp_match_list, np);
-	if (!match) {
-		of_node_put(np);
+	if (!match)
 		return -ENOENT;
-	}
+
 	opp_data = match->data;
 
 	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
-	of_node_put(np);
 	if (IS_ERR(speedbin_nvmem))
 		return dev_err_probe(cpu_dev, PTR_ERR(speedbin_nvmem),
 				     "Could not get nvmem cell\n");
@@ -301,14 +297,9 @@ MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);
 
 static const struct of_device_id *sun50i_cpufreq_match_node(void)
 {
-	const struct of_device_id *match;
-	struct device_node *np;
-
-	np = of_find_node_by_path("/");
-	match = of_match_node(sun50i_cpufreq_match_list, np);
-	of_node_put(np);
+	struct device_node *np __free(device_node) = of_find_node_by_path("/");
 
-	return match;
+	return of_match_node(sun50i_cpufreq_match_list, np);
 }
 
 /*

-- 
2.40.1

Re: [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler

Posted by Andre Przywara 1 year, 7 months ago

On Fri, 03 May 2024 19:52:33 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

Hi,

> Make use of the __free() cleanup handler to automatically free nodes
> when they get out of scope.

Looks alright, the last function is now particularly neat.

> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

I haven't tested the error paths yet, but it certainly boots fine on an
OrangePi Zero3.

Cheers,
Andre

> ---
>  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> index ef83e4bf2639..eb47c193269c 100644
> --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> @@ -131,14 +131,14 @@ 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;
>  	struct device *cpu_dev;
>  
>  	cpu_dev = get_cpu_device(0);
>  	if (!cpu_dev)
>  		return false;
>  
> -	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> +	struct device_node *np __free(device_node) =
> +		dev_pm_opp_of_get_opp_desc_node(cpu_dev);
>  	if (!np)
>  		return false;
>  
> @@ -149,8 +149,6 @@ static bool dt_has_supported_hw(void)
>  		}
>  	}
>  
> -	of_node_put(np);
> -
>  	return has_opp_supported_hw;
>  }
>  
> @@ -165,7 +163,6 @@ static int sun50i_cpufreq_get_efuse(void)
>  	const struct sunxi_cpufreq_data *opp_data;
>  	struct nvmem_cell *speedbin_nvmem;
>  	const struct of_device_id *match;
> -	struct device_node *np;
>  	struct device *cpu_dev;
>  	u32 *speedbin;
>  	int ret;
> @@ -174,19 +171,18 @@ static int sun50i_cpufreq_get_efuse(void)
>  	if (!cpu_dev)
>  		return -ENODEV;
>  
> -	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> +	struct device_node *np __free(device_node) =
> +		dev_pm_opp_of_get_opp_desc_node(cpu_dev);
>  	if (!np)
>  		return -ENOENT;
>  
>  	match = of_match_node(cpu_opp_match_list, np);
> -	if (!match) {
> -		of_node_put(np);
> +	if (!match)
>  		return -ENOENT;
> -	}
> +
>  	opp_data = match->data;
>  
>  	speedbin_nvmem = of_nvmem_cell_get(np, NULL);
> -	of_node_put(np);
>  	if (IS_ERR(speedbin_nvmem))
>  		return dev_err_probe(cpu_dev, PTR_ERR(speedbin_nvmem),
>  				     "Could not get nvmem cell\n");
> @@ -301,14 +297,9 @@ MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);
>  
>  static const struct of_device_id *sun50i_cpufreq_match_node(void)
>  {
> -	const struct of_device_id *match;
> -	struct device_node *np;
> -
> -	np = of_find_node_by_path("/");
> -	match = of_match_node(sun50i_cpufreq_match_list, np);
> -	of_node_put(np);
> +	struct device_node *np __free(device_node) = of_find_node_by_path("/");
>  
> -	return match;
> +	return of_match_node(sun50i_cpufreq_match_list, np);
>  }
>  
>  /*
>

Re: [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler

Posted by Viresh Kumar 1 year, 7 months ago

On 10-05-24, 18:42, Andre Przywara wrote:
> On Fri, 03 May 2024 19:52:33 +0200
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> I haven't tested the error paths yet, but it certainly boots fine on an
> OrangePi Zero3.
> 
> > ---
> >  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 25 ++++++++-----------------
> >  1 file changed, 8 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > index ef83e4bf2639..eb47c193269c 100644
> > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > @@ -131,14 +131,14 @@ 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;
> >  	struct device *cpu_dev;
> >  
> >  	cpu_dev = get_cpu_device(0);
> >  	if (!cpu_dev)
> >  		return false;
> >  
> > -	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> > +	struct device_node *np __free(device_node) =
> > +		dev_pm_opp_of_get_opp_desc_node(cpu_dev);

Won't that result in build warning, mixed code and definitions now ?


-- 
viresh

Re: [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler

Posted by Markus Elfring 1 year, 7 months ago

…
> > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > @@ -131,14 +131,14 @@ 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;
> > >  	struct device *cpu_dev;
> > >
> > >  	cpu_dev = get_cpu_device(0);
> > >  	if (!cpu_dev)
> > >  		return false;
> > >
> > > -	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> > > +	struct device_node *np __free(device_node) =
> > > +		dev_pm_opp_of_get_opp_desc_node(cpu_dev);
>
> Won't that result in build warning, mixed code and definitions now ?

I suggest to take another look at a corresponding information source.

[PATCH v3 04/57] kbuild: Drop -Wdeclaration-after-statement
https://lore.kernel.org/all/20230612093537.693926033@infradead.org/

See also:
https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Warning-Options.html#index-Wdeclaration-after-statement


Would you like to stress a scope reduction for the affected local variable
by adding any curly brackets?

Regards,
Markus

Re: [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler

Posted by Viresh Kumar 1 year, 7 months ago

On 20-05-24, 11:28, Markus Elfring wrote:
> …
> > > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > > @@ -131,14 +131,14 @@ 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;
> > > >  	struct device *cpu_dev;
> > > >
> > > >  	cpu_dev = get_cpu_device(0);
> > > >  	if (!cpu_dev)
> > > >  		return false;
> > > >
> > > > -	np = dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> > > > +	struct device_node *np __free(device_node) =
> > > > +		dev_pm_opp_of_get_opp_desc_node(cpu_dev);
> >
> > Won't that result in build warning, mixed code and definitions now ?
> 
> I suggest to take another look at a corresponding information source.
> 
> [PATCH v3 04/57] kbuild: Drop -Wdeclaration-after-statement
> https://lore.kernel.org/all/20230612093537.693926033@infradead.org/

Ah, I wasn't aware of this one.

> See also:
> https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc/Warning-Options.html#index-Wdeclaration-after-statement
> 
> 
> Would you like to stress a scope reduction for the affected local variable
> by adding any curly brackets?

No, it looks fine.

-- 
viresh