[PATCH 0/2] clk: renesas: cpg-mssr: fix 'soc' node handling and automate cleanup

[PATCH 0/2] clk: renesas: cpg-mssr: fix 'soc' node handling and automate cleanup

[Javier Carrasco]

This series releases the 'soc' device_node when it is no longer required
by adding the missing calls to of_node_put() to make the fix compatible
with all affected stable kernels. Then, the more robust approach via
cleanup attribute is used to simplify the handling and prevent issues if
the loop gets new execution paths.

These issues were found while analyzing the code, and the patches have
been successfully compiled, but not tested on real hardware as I don't
have access to it. Any volunteering for testing is always more than
welcome.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (2):
      clk: renesas: cpg-mssr: fix 'soc' node handling in cpg_mssr_reserved_init()
      clk: renesas: cpg-mssr: automate 'soc' node release in cpg_mssr_reserved_init()

 drivers/clk/renesas/renesas-cpg-mssr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
---
base-commit: 86e3904dcdc7e70e3257fc1de294a1b75f3d8d04
change-id: 20241031-clk-renesas-cpg-mssr-cleanup-1933df63bc9c

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

[PATCH 1/2] clk: renesas: cpg-mssr: fix 'soc' node handling in cpg_mssr_reserved_init()

[Javier Carrasco]

A device_node reference obtained via of_find_node_by_path() requires
explicit calls to of_node_put() after it is no longer required to avoid
leaking the resource.

Add the missing calls to of_node_put(soc) in all execution paths when
'soc' is no longer required (one error path, and the success path).

Cc: stable@vger.kernel.org
Fixes: 6aa175476490 ("clk: renesas: cpg-mssr: Ignore all clocks assigned to non-Linux system")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/clk/renesas/renesas-cpg-mssr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index 79e7a90c3b1b..5dc89b1009fe 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -1022,6 +1022,7 @@ static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
 
 			ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
 			if (!ids) {
+				of_node_put(soc);
 				of_node_put(it.node);
 				return -ENOMEM;
 			}
@@ -1036,6 +1037,7 @@ static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
 			num++;
 		}
 	}
+	of_node_put(soc);
 
 	priv->num_reserved_ids	= num;
 	priv->reserved_ids	= ids;

-- 
2.43.0

[PATCH 2/2] clk: renesas: cpg-mssr: automate 'soc' node release in cpg_mssr_reserved_init()

[Javier Carrasco]

Switch to a more robust approach by means of the cleanup attribute,
which automates the calls to of_node_put() when 'soc' goes out of scope.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/clk/renesas/renesas-cpg-mssr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index 5dc89b1009fe..bf85501709f0 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -979,7 +979,7 @@ static void __init cpg_mssr_reserved_exit(struct cpg_mssr_priv *priv)
 static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
 					 const struct cpg_mssr_info *info)
 {
-	struct device_node *soc = of_find_node_by_path("/soc");
+	struct device_node *soc __free(device_node) = of_find_node_by_path("/soc");
 	struct device_node *node;
 	uint32_t args[MAX_PHANDLE_ARGS];
 	unsigned int *ids = NULL;
@@ -1022,7 +1022,6 @@ static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
 
 			ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
 			if (!ids) {
-				of_node_put(soc);
 				of_node_put(it.node);
 				return -ENOMEM;
 			}
@@ -1037,7 +1036,6 @@ static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
 			num++;
 		}
 	}
-	of_node_put(soc);
 
 	priv->num_reserved_ids	= num;
 	priv->reserved_ids	= ids;

-- 
2.43.0

RE: [PATCH 2/2] clk: renesas: cpg-mssr: automate 'soc' node release in cpg_mssr_reserved_init()

[Biju Das]

Hi Javier Carrasco,

Thanks for the patch

> -----Original Message-----
> From: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Sent: 30 October 2024 23:26
> Subject: [PATCH 2/2] clk: renesas: cpg-mssr: automate 'soc' node release in cpg_mssr_reserved_init()
> 
> Switch to a more robust approach by means of the cleanup attribute, which automates the calls to
> of_node_put() when 'soc' goes out of scope.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/clk/renesas/renesas-cpg-mssr.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 5dc89b1009fe..bf85501709f0 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -979,7 +979,7 @@ static void __init cpg_mssr_reserved_exit(struct cpg_mssr_priv *priv)  static int
> __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
>  					 const struct cpg_mssr_info *info)  {
> -	struct device_node *soc = of_find_node_by_path("/soc");
> +	struct device_node *soc __free(device_node) =
> +of_find_node_by_path("/soc");

I guess. by looking at [1] and [2], the cleanup function should be of_node_put(), which translates to __free_of_node_put(),
that extracts of_node_put() and execute it for cleanup??

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/irqchip/irq-renesas-rzg2l.c?h=next-20241030#n535

[2] https://elixir.bootlin.com/linux/v6.12-rc5/source/include/linux/cleanup.h#L198

Cheers,
Biju

>  	struct device_node *node;
>  	uint32_t args[MAX_PHANDLE_ARGS];
>  	unsigned int *ids = NULL;
> @@ -1022,7 +1022,6 @@ static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
> 
>  			ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
>  			if (!ids) {
> -				of_node_put(soc);
>  				of_node_put(it.node);
>  				return -ENOMEM;
>  			}
> @@ -1037,7 +1036,6 @@ static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
>  			num++;
>  		}
>  	}
> -	of_node_put(soc);
> 
>  	priv->num_reserved_ids	= num;
>  	priv->reserved_ids	= ids;
> 
> --
> 2.43.0
> 

RE: [PATCH 2/2] clk: renesas: cpg-mssr: automate 'soc' node release in cpg_mssr_reserved_init()

[Biju Das]

Hi,

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: 31 October 2024 06:59
> Subject: RE: [PATCH 2/2] clk: renesas: cpg-mssr: automate 'soc' node release in
> cpg_mssr_reserved_init()
> 
> Hi Javier Carrasco,
> 
> Thanks for the patch
> 
> > -----Original Message-----
> > From: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > Sent: 30 October 2024 23:26
> > Subject: [PATCH 2/2] clk: renesas: cpg-mssr: automate 'soc' node
> > release in cpg_mssr_reserved_init()
> >
> > Switch to a more robust approach by means of the cleanup attribute,
> > which automates the calls to
> > of_node_put() when 'soc' goes out of scope.
> >
> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> > ---
> >  drivers/clk/renesas/renesas-cpg-mssr.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c
> > b/drivers/clk/renesas/renesas-cpg-mssr.c
> > index 5dc89b1009fe..bf85501709f0 100644
> > --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> > +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> > @@ -979,7 +979,7 @@ static void __init cpg_mssr_reserved_exit(struct
> > cpg_mssr_priv *priv)  static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
> >  					 const struct cpg_mssr_info *info)  {
> > -	struct device_node *soc = of_find_node_by_path("/soc");
> > +	struct device_node *soc __free(device_node) =
> > +of_find_node_by_path("/soc");
> 
> I guess. by looking at [1] and [2], the cleanup function should be of_node_put(), which translates to
> __free_of_node_put(), that extracts of_node_put() and execute it for cleanup??
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/irqchip/irq-
> renesas-rzg2l.c?h=next-20241030#n535
> 
> [2] https://elixir.bootlin.com/linux/v6.12-rc5/source/include/linux/cleanup.h#L198

Please ignore me. I am wrong.

https://elixir.bootlin.com/linux/v6.12-rc5/source/include/linux/of.h#L138

Cheers,
Biju

> 
> Cheers,
> Biju
> 
> >  	struct device_node *node;
> >  	uint32_t args[MAX_PHANDLE_ARGS];
> >  	unsigned int *ids = NULL;
> > @@ -1022,7 +1022,6 @@ static int __init cpg_mssr_reserved_init(struct
> > cpg_mssr_priv *priv,
> >
> >  			ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
> >  			if (!ids) {
> > -				of_node_put(soc);
> >  				of_node_put(it.node);
> >  				return -ENOMEM;
> >  			}
> > @@ -1037,7 +1036,6 @@ static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
> >  			num++;
> >  		}
> >  	}
> > -	of_node_put(soc);
> >
> >  	priv->num_reserved_ids	= num;
> >  	priv->reserved_ids	= ids;
> >
> > --
> > 2.43.0
> >

Re: [PATCH 2/2] clk: renesas: cpg-mssr: automate 'soc' node release in cpg_mssr_reserved_init()

[Krzysztof Kozlowski]

On 31/10/2024 00:25, Javier Carrasco wrote:
> Switch to a more robust approach by means of the cleanup attribute,
> which automates the calls to of_node_put() when 'soc' goes out of scope.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/clk/renesas/renesas-cpg-mssr.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 5dc89b1009fe..bf85501709f0 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -979,7 +979,7 @@ static void __init cpg_mssr_reserved_exit(struct cpg_mssr_priv *priv)
>  static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
>  					 const struct cpg_mssr_info *info)
>  {
> -	struct device_node *soc = of_find_node_by_path("/soc");
> +	struct device_node *soc __free(device_node) = of_find_node_by_path("/soc");
>  	struct device_node *node;
>  	uint32_t args[MAX_PHANDLE_ARGS];
>  	unsigned int *ids = NULL;
> @@ -1022,7 +1022,6 @@ static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
>  
>  			ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
>  			if (!ids) {
> -				of_node_put(soc);

You just added this. Don't add code which is immediately removed. It's a
noop or wrong code.

Best regards,
Krzysztof