[PATCH 0/2] cpuidle: riscv-sbi: fix device node release in early exit of for_each_possible_cpu

[PATCH 0/2] cpuidle: riscv-sbi: fix device node release in early exit of for_each_possible_cpu

[Javier Carrasco]

This series releases the np 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):
      cpuidle: riscv-sbi: fix device node release in early exit of for_each_possible_cpu
      cpuidle: riscv-sbi: use cleanup attribute for np in for_each_possible_cpu

 drivers/cpuidle/cpuidle-riscv-sbi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
---
base-commit: 6fb2fa9805c501d9ade047fc511961f3273cdcb5
change-id: 20241029-cpuidle-riscv-sbi-cleanup-e9b3cb96e16d

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

[PATCH 1/2] cpuidle: riscv-sbi: fix device node release in early exit of for_each_possible_cpu

[Javier Carrasco]

The 'np' device_node is initialized via of_cpu_device_node_get(), which
requires explicit calls to of_node_put() when it is no longer required
to avoid leaking the resource.

Add the missing calls to of_node_put(np) in all execution paths.

Cc: stable@vger.kernel.org
Fixes: 6abf32f1d9c5 ("cpuidle: Add RISC-V SBI CPU idle driver")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/cpuidle/cpuidle-riscv-sbi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
index 14462c092039..2b3aec09b895 100644
--- a/drivers/cpuidle/cpuidle-riscv-sbi.c
+++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
@@ -513,11 +513,14 @@ static int sbi_cpuidle_probe(struct platform_device *pdev)
 		if (np &&
 		    of_property_present(np, "power-domains") &&
 		    of_property_present(np, "power-domain-names")) {
+			of_node_put(np);
 			continue;
 		} else {
 			sbi_cpuidle_use_osi = false;
+			of_node_put(np);
 			break;
 		}
+		of_node_put(np);
 	}
 
 	/* Populate generic power domains from DT nodes */

-- 
2.43.0

[PATCH 2/2] cpuidle: riscv-sbi: use cleanup attribute for np in for_each_possible_cpu

[Javier Carrasco]

Simplify the code and make it more robust against new execution paths in
the loop by means of the cleanup attribute.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/cpuidle/cpuidle-riscv-sbi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
index 2b3aec09b895..3a78d6b7598b 100644
--- a/drivers/cpuidle/cpuidle-riscv-sbi.c
+++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
@@ -504,23 +504,21 @@ static int sbi_cpuidle_probe(struct platform_device *pdev)
 	int cpu, ret;
 	struct cpuidle_driver *drv;
 	struct cpuidle_device *dev;
-	struct device_node *np, *pds_node;
+	struct device_node *pds_node;
 
 	/* Detect OSI support based on CPU DT nodes */
 	sbi_cpuidle_use_osi = true;
 	for_each_possible_cpu(cpu) {
-		np = of_cpu_device_node_get(cpu);
+		struct device_node *np __free(device_node) =
+			of_cpu_device_node_get(cpu);
 		if (np &&
 		    of_property_present(np, "power-domains") &&
 		    of_property_present(np, "power-domain-names")) {
-			of_node_put(np);
 			continue;
 		} else {
 			sbi_cpuidle_use_osi = false;
-			of_node_put(np);
 			break;
 		}
-		of_node_put(np);
 	}
 
 	/* Populate generic power domains from DT nodes */

-- 
2.43.0

Re: [PATCH 2/2] cpuidle: riscv-sbi: use cleanup attribute for np in for_each_possible_cpu

[Krzysztof Kozlowski]

On 30/10/2024 07:44, Javier Carrasco wrote:
> Simplify the code and make it more robust against new execution paths in
> the loop by means of the cleanup attribute.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/cpuidle/cpuidle-riscv-sbi.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
> index 2b3aec09b895..3a78d6b7598b 100644
> --- a/drivers/cpuidle/cpuidle-riscv-sbi.c
> +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
> @@ -504,23 +504,21 @@ static int sbi_cpuidle_probe(struct platform_device *pdev)
>  	int cpu, ret;
>  	struct cpuidle_driver *drv;
>  	struct cpuidle_device *dev;
> -	struct device_node *np, *pds_node;
> +	struct device_node *pds_node;
>  
>  	/* Detect OSI support based on CPU DT nodes */
>  	sbi_cpuidle_use_osi = true;
>  	for_each_possible_cpu(cpu) {
> -		np = of_cpu_device_node_get(cpu);
> +		struct device_node *np __free(device_node) =
> +			of_cpu_device_node_get(cpu);
>  		if (np &&
>  		    of_property_present(np, "power-domains") &&
>  		    of_property_present(np, "power-domain-names")) {
> -			of_node_put(np);

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

If you want to backport something: send a backport. We work here on
mainline and in mainline this is one logical change: fixing issue.
Whether you fix issue with of_node_put or cleanup or by removing this
code entirely, it does not matter. All of these are fixing the same, one
issue. This is inflating mainline history with unnecessary commits.



Best regards,
Krzysztof