linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put()
@ 2024-05-03 17:52 Javier Carrasco
  2024-05-03 17:52 ` [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw() Javier Carrasco
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Javier Carrasco @ 2024-05-03 17:52 UTC (permalink / raw)
  To: Yangtao Li, Rafael J. Wysocki, Viresh Kumar, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Andre Przywara, Jonathan Cameron
  Cc: linux-pm, linux-arm-kernel, linux-sunxi, linux-kernel, Javier Carrasco

This series fixes a memory leak by means of the _scoped version of the
for_each_child_of_node() loop, which was recently introduced with
34af4554fb0c ("of: Introduce for_each_*_child_of_node_scoped() to
automate of_node_put() handling").

The new approach is still not widely used, but this might be a good
occasion to use it in a driver because it actually fixes a bug, and
the loop is rather simple.

The creator of the new macro was added to the discussion in case the
new approach is still not mature enough, even for such simple case.

Additionally, the existing uses of of_node_put() have been removed to
favour the _free() cleanup handler, which reduces the chances of having
any other memory leak because some of_node_put() is missing as well as
simplifies the current code.

I don't have the real hardware to test the series, so I "faked" the node
in a device tree for an arm64 device (Rockchip) and hacked the driver
to get to run dt_has_supported_hw(). The new implementation works as
expected, but if someone wants to test it with the proper SoC,
additional tests are always welcome. The same applies for the removals
of of_node_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (2):
      cpufreq: sun50i: fix memory leak in dt_has_supported_hw()
      cpufreq: sun50i: replace of_node_put() with automatic cleanup handler

 drivers/cpufreq/sun50i-cpufreq-nvmem.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)
---
base-commit: 9221b2819b8a4196eecf5476d66201be60fbcf29
change-id: 20240503-sun50i-cpufreq-nvmem-cleanup-40a4cf8fa56d

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw()
  2024-05-03 17:52 [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put() Javier Carrasco
@ 2024-05-03 17:52 ` Javier Carrasco
  2024-05-10 16:49   ` Andre Przywara
  2024-05-03 17:52 ` [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler Javier Carrasco
  2024-05-20  9:32 ` [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put() Viresh Kumar
  2 siblings, 1 reply; 12+ messages in thread
From: Javier Carrasco @ 2024-05-03 17:52 UTC (permalink / raw)
  To: Yangtao Li, Rafael J. Wysocki, Viresh Kumar, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Andre Przywara, Jonathan Cameron
  Cc: linux-pm, linux-arm-kernel, linux-sunxi, linux-kernel, Javier Carrasco

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(-)

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;

-- 
2.40.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler
  2024-05-03 17:52 [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put() Javier Carrasco
  2024-05-03 17:52 ` [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw() Javier Carrasco
@ 2024-05-03 17:52 ` Javier Carrasco
  2024-05-10 17:42   ` Andre Przywara
  2024-05-20  9:32 ` [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put() Viresh Kumar
  2 siblings, 1 reply; 12+ messages in thread
From: Javier Carrasco @ 2024-05-03 17:52 UTC (permalink / raw)
  To: Yangtao Li, Rafael J. Wysocki, Viresh Kumar, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Andre Przywara, Jonathan Cameron
  Cc: linux-pm, linux-arm-kernel, linux-sunxi, linux-kernel, Javier Carrasco

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw()
  2024-05-03 17:52 ` [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw() Javier Carrasco
@ 2024-05-10 16:49   ` Andre Przywara
  2024-05-20  7:33     ` Viresh Kumar
  0 siblings, 1 reply; 12+ messages in thread
From: Andre Przywara @ 2024-05-10 16:49 UTC (permalink / raw)
  To: Javier Carrasco
  Cc: Yangtao Li, Rafael J. Wysocki, Viresh Kumar, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Jonathan Cameron, linux-pm,
	linux-arm-kernel, linux-sunxi, linux-kernel

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;
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler
  2024-05-03 17:52 ` [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler Javier Carrasco
@ 2024-05-10 17:42   ` Andre Przywara
  2024-05-20  8:35     ` Viresh Kumar
  0 siblings, 1 reply; 12+ messages in thread
From: Andre Przywara @ 2024-05-10 17:42 UTC (permalink / raw)
  To: Javier Carrasco
  Cc: Yangtao Li, Rafael J. Wysocki, Viresh Kumar, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Jonathan Cameron, linux-pm,
	linux-arm-kernel, linux-sunxi, linux-kernel

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);
>  }
>  
>  /*
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw()
  2024-05-10 16:49   ` Andre Przywara
@ 2024-05-20  7:33     ` Viresh Kumar
  2024-05-20  8:26       ` Andre Przywara
  0 siblings, 1 reply; 12+ messages in thread
From: Viresh Kumar @ 2024-05-20  7:33 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Javier Carrasco, Yangtao Li, Rafael J. Wysocki, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Jonathan Cameron, linux-pm,
	linux-arm-kernel, linux-sunxi, linux-kernel

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;
> > 

-- 
viresh

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw()
  2024-05-20  7:33     ` Viresh Kumar
@ 2024-05-20  8:26       ` Andre Przywara
  2024-05-20  8:34         ` Viresh Kumar
  0 siblings, 1 reply; 12+ messages in thread
From: Andre Przywara @ 2024-05-20  8:26 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Javier Carrasco, Yangtao Li, Rafael J. Wysocki, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Jonathan Cameron, linux-pm,
	linux-arm-kernel, linux-sunxi, linux-kernel

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;
> > >   
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw()
  2024-05-20  8:26       ` Andre Przywara
@ 2024-05-20  8:34         ` Viresh Kumar
  0 siblings, 0 replies; 12+ messages in thread
From: Viresh Kumar @ 2024-05-20  8:34 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Javier Carrasco, Yangtao Li, Rafael J. Wysocki, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Jonathan Cameron, linux-pm,
	linux-arm-kernel, linux-sunxi, linux-kernel

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.

-- 
viresh

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler
  2024-05-10 17:42   ` Andre Przywara
@ 2024-05-20  8:35     ` Viresh Kumar
  2024-05-20  9:28       ` Markus Elfring
  0 siblings, 1 reply; 12+ messages in thread
From: Viresh Kumar @ 2024-05-20  8:35 UTC (permalink / raw)
  To: Andre Przywara
  Cc: Javier Carrasco, Yangtao Li, Rafael J. Wysocki, Chen-Yu Tsai,
	Jernej Skrabec, Samuel Holland, Jonathan Cameron, linux-pm,
	linux-arm-kernel, linux-sunxi, linux-kernel

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler
  2024-05-20  8:35     ` Viresh Kumar
@ 2024-05-20  9:28       ` Markus Elfring
  2024-05-20  9:31         ` Viresh Kumar
  0 siblings, 1 reply; 12+ messages in thread
From: Markus Elfring @ 2024-05-20  9:28 UTC (permalink / raw)
  To: Viresh Kumar, Javier Carrasco, linux-sunxi, linux-arm-kernel,
	linux-pm, kernel-janitors
  Cc: LKML, Andre Przywara, Chen-Yu Tsai, Jernej Skrabec,
	Jonathan Cameron, Peter Zijlstra, Rafael J. Wysocki,
	Samuel Holland, Yangtao Li

…
> > > +++ 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler
  2024-05-20  9:28       ` Markus Elfring
@ 2024-05-20  9:31         ` Viresh Kumar
  0 siblings, 0 replies; 12+ messages in thread
From: Viresh Kumar @ 2024-05-20  9:31 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Javier Carrasco, linux-sunxi, linux-arm-kernel, linux-pm,
	kernel-janitors, LKML, Andre Przywara, Chen-Yu Tsai,
	Jernej Skrabec, Jonathan Cameron, Peter Zijlstra,
	Rafael J. Wysocki, Samuel Holland, Yangtao Li

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put()
  2024-05-03 17:52 [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put() Javier Carrasco
  2024-05-03 17:52 ` [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw() Javier Carrasco
  2024-05-03 17:52 ` [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler Javier Carrasco
@ 2024-05-20  9:32 ` Viresh Kumar
  2 siblings, 0 replies; 12+ messages in thread
From: Viresh Kumar @ 2024-05-20  9:32 UTC (permalink / raw)
  To: Javier Carrasco
  Cc: Yangtao Li, Rafael J. Wysocki, Chen-Yu Tsai, Jernej Skrabec,
	Samuel Holland, Andre Przywara, Jonathan Cameron, linux-pm,
	linux-arm-kernel, linux-sunxi, linux-kernel

On 03-05-24, 19:52, Javier Carrasco wrote:
> This series fixes a memory leak by means of the _scoped version of the
> for_each_child_of_node() loop, which was recently introduced with
> 34af4554fb0c ("of: Introduce for_each_*_child_of_node_scoped() to
> automate of_node_put() handling").
> 
> The new approach is still not widely used, but this might be a good
> occasion to use it in a driver because it actually fixes a bug, and
> the loop is rather simple.
> 
> The creator of the new macro was added to the discussion in case the
> new approach is still not mature enough, even for such simple case.
> 
> Additionally, the existing uses of of_node_put() have been removed to
> favour the _free() cleanup handler, which reduces the chances of having
> any other memory leak because some of_node_put() is missing as well as
> simplifies the current code.
> 
> I don't have the real hardware to test the series, so I "faked" the node
> in a device tree for an arm64 device (Rockchip) and hacked the driver
> to get to run dt_has_supported_hw(). The new implementation works as
> expected, but if someone wants to test it with the proper SoC,
> additional tests are always welcome. The same applies for the removals
> of of_node_put().
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> Javier Carrasco (2):
>       cpufreq: sun50i: fix memory leak in dt_has_supported_hw()
>       cpufreq: sun50i: replace of_node_put() with automatic cleanup handler

Applied. Thanks.

-- 
viresh

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-05-20  9:32 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-03 17:52 [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put() Javier Carrasco
2024-05-03 17:52 ` [PATCH 1/2] cpufreq: sun50i: fix memory leak in dt_has_supported_hw() Javier Carrasco
2024-05-10 16:49   ` Andre Przywara
2024-05-20  7:33     ` Viresh Kumar
2024-05-20  8:26       ` Andre Przywara
2024-05-20  8:34         ` Viresh Kumar
2024-05-03 17:52 ` [PATCH 2/2] cpufreq: sun50i: replace of_node_put() with automatic cleanup handler Javier Carrasco
2024-05-10 17:42   ` Andre Przywara
2024-05-20  8:35     ` Viresh Kumar
2024-05-20  9:28       ` Markus Elfring
2024-05-20  9:31         ` Viresh Kumar
2024-05-20  9:32 ` [PATCH 0/2] cpufreq: sun50i: fix memory leak and remove of_node_put() Viresh Kumar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).