[PATCH 0/2] drivers: soc: atmel: fix device_node release in atmel_soc_device_init()

[PATCH 0/2] drivers: soc: atmel: fix device_node release in atmel_soc_device_init()

[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):
      drivers: soc: atmel: fix device_node release in atmel_soc_device_init()
      drivers: soc: atmel: use automatic cleanup for device_node in atmel_soc_device_init()

 drivers/soc/atmel/soc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
---
base-commit: 86e3904dcdc7e70e3257fc1de294a1b75f3d8d04
change-id: 20241030-soc-atmel-soc-cleanup-8fcf3029bb28

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

[PATCH 1/2] drivers: soc: atmel: fix device_node release in atmel_soc_device_init()

[Javier Carrasco]

A device_node acquired via of_find_node_by_path() requires explicit
calls to of_node_put() when it is no longer needed to avoid leaking the
resource.

Add the missing of_node_put() in the different execution paths.

Cc: stable@vger.kernel.org
Fixes: 960ddf70cc11 ("drivers: soc: atmel: Avoid calling at91_soc_init on non AT91 SoCs")
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/soc/atmel/soc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
index 2a42b28931c9..64b1ad063073 100644
--- a/drivers/soc/atmel/soc.c
+++ b/drivers/soc/atmel/soc.c
@@ -401,10 +401,13 @@ static int __init atmel_soc_device_init(void)
 {
 	struct device_node *np = of_find_node_by_path("/");
 
-	if (!of_match_node(at91_soc_allowed_list, np))
+	if (!of_match_node(at91_soc_allowed_list, np)) {
+		of_node_put(np);
 		return 0;
+	}
 
 	at91_soc_init(socs);
+	of_node_put(np);
 
 	return 0;
 }

-- 
2.43.0

[PATCH 2/2] drivers: soc: atmel: use automatic cleanup for device_node in atmel_soc_device_init()

[Javier Carrasco]

Switch to a more robust approach to automatically release the node when
it goes out of scope, dropping the need for explicit calls to
of_node_put().

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/soc/atmel/soc.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
index 64b1ad063073..298b542dd1c0 100644
--- a/drivers/soc/atmel/soc.c
+++ b/drivers/soc/atmel/soc.c
@@ -399,15 +399,12 @@ static const struct of_device_id at91_soc_allowed_list[] __initconst = {
 
 static int __init atmel_soc_device_init(void)
 {
-	struct device_node *np = of_find_node_by_path("/");
+	struct device_node *np __free(device_node) = of_find_node_by_path("/");
 
-	if (!of_match_node(at91_soc_allowed_list, np)) {
-		of_node_put(np);
+	if (!of_match_node(at91_soc_allowed_list, np))
 		return 0;
-	}
 
 	at91_soc_init(socs);
-	of_node_put(np);
 
 	return 0;
 }

-- 
2.43.0

Re: [PATCH 2/2] drivers: soc: atmel: use automatic cleanup for device_node in atmel_soc_device_init()

[Krzysztof Kozlowski]

On 30/10/2024 18:10, Javier Carrasco wrote:
> Switch to a more robust approach to automatically release the node when
> it goes out of scope, dropping the need for explicit calls to
> of_node_put().

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

There is never a "drivers" prefix. Especially not first (because as
middle appears for FEW subsystems, not for SoC though).

> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
>  drivers/soc/atmel/soc.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
> index 64b1ad063073..298b542dd1c0 100644
> --- a/drivers/soc/atmel/soc.c
> +++ b/drivers/soc/atmel/soc.c
> @@ -399,15 +399,12 @@ static const struct of_device_id at91_soc_allowed_list[] __initconst = {
>  
>  static int __init atmel_soc_device_init(void)
>  {
> -	struct device_node *np = of_find_node_by_path("/");
> +	struct device_node *np __free(device_node) = of_find_node_by_path("/");
>  
> -	if (!of_match_node(at91_soc_allowed_list, np)) {
> -		of_node_put(np);

You just added this code. Don't add code which immediately you remove.
Squash two patches.

Best regards,
Krzysztof

Re: [PATCH 2/2] drivers: soc: atmel: use automatic cleanup for device_node in atmel_soc_device_init()

[Javier Carrasco]

On 31/10/2024 12:07, Krzysztof Kozlowski wrote:
> On 30/10/2024 18:10, Javier Carrasco wrote:
>> Switch to a more robust approach to automatically release the node when
>> it goes out of scope, dropping the need for explicit calls to
>> of_node_put().
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
> There is never a "drivers" prefix. Especially not first (because as
> middle appears for FEW subsystems, not for SoC though).
> 

Thanks, I added that by mistake. I will fix that for v2.

>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>>  drivers/soc/atmel/soc.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
>> index 64b1ad063073..298b542dd1c0 100644
>> --- a/drivers/soc/atmel/soc.c
>> +++ b/drivers/soc/atmel/soc.c
>> @@ -399,15 +399,12 @@ static const struct of_device_id at91_soc_allowed_list[] __initconst = {
>>  
>>  static int __init atmel_soc_device_init(void)
>>  {
>> -	struct device_node *np = of_find_node_by_path("/");
>> +	struct device_node *np __free(device_node) = of_find_node_by_path("/");
>>  
>> -	if (!of_match_node(at91_soc_allowed_list, np)) {
>> -		of_node_put(np);
> 
> You just added this code. Don't add code which immediately you remove.
> Squash two patches.
> 
> Best regards,
> Krzysztof
> 


As I said in another thread, I split the solution into a first one to be
applied to stable kernels, and a second one that uses a more robust
approach that is not supported by all stable kernels.

Best regards,
Javier Carrasco

Re: [PATCH 2/2] drivers: soc: atmel: use automatic cleanup for device_node in atmel_soc_device_init()

[Krzysztof Kozlowski]

On 31/10/2024 12:14, Javier Carrasco wrote:
> On 31/10/2024 12:07, Krzysztof Kozlowski wrote:
>> On 30/10/2024 18:10, Javier Carrasco wrote:
>>> Switch to a more robust approach to automatically release the node when
>>> it goes out of scope, dropping the need for explicit calls to
>>> of_node_put().
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching. For bindings, the preferred subjects are
>> explained here:
>> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>>
>> There is never a "drivers" prefix. Especially not first (because as
>> middle appears for FEW subsystems, not for SoC though).
>>
> 
> Thanks, I added that by mistake. I will fix that for v2.
> 
>>>
>>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>>> ---
>>>  drivers/soc/atmel/soc.c | 7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
>>> index 64b1ad063073..298b542dd1c0 100644
>>> --- a/drivers/soc/atmel/soc.c
>>> +++ b/drivers/soc/atmel/soc.c
>>> @@ -399,15 +399,12 @@ static const struct of_device_id at91_soc_allowed_list[] __initconst = {
>>>  
>>>  static int __init atmel_soc_device_init(void)
>>>  {
>>> -	struct device_node *np = of_find_node_by_path("/");
>>> +	struct device_node *np __free(device_node) = of_find_node_by_path("/");
>>>  
>>> -	if (!of_match_node(at91_soc_allowed_list, np)) {
>>> -		of_node_put(np);
>>
>> You just added this code. Don't add code which immediately you remove.
>> Squash two patches.
>>
>> Best regards,
>> Krzysztof
>>
> 
> 
> As I said in another thread, I split the solution into a first one to be
> applied to stable kernels, and a second one that uses a more robust
> approach that is not supported by all stable kernels.
> 

Same response as in other thread:

1. Then send backport for stable, if you care about stable. You inflate
history with irrational commits just instead of doing proper stable
backport.

2. Creating some commits purely for stable in the mainline kernel is not
a correct approach. 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 of_find_node_by_path() call, it does not matter.
All of these are fixing the same, one issue.

If you think about stable kernels, then work on backports, not inflate
mainline kernel with multiple commits doing the same, creating
artificial history.

Best regards,
Krzysztof

Re: [PATCH 2/2] drivers: soc: atmel: use automatic cleanup for device_node in atmel_soc_device_init()

[Javier Carrasco]

On 31/10/2024 12:07, Krzysztof Kozlowski wrote:
> On 30/10/2024 18:10, Javier Carrasco wrote:
>> Switch to a more robust approach to automatically release the node when
>> it goes out of scope, dropping the need for explicit calls to
>> of_node_put().
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
> There is never a "drivers" prefix. Especially not first (because as
> middle appears for FEW subsystems, not for SoC though).
> 

Interestingly, 10 out of 20 patches for this file use the "drivers"
prefix first, so I guess it was an error that propagated for some time.
I will stick to soc: atmel: for v2.

> 
> Best regards,
> Krzysztof
>