linux-kernel - Re: [PATCH 2/2] drivers: soc: atmel: use automatic cleanup for device_node in atmel_soc_device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ac01fa3e-1d68-419f-8afc-f3ecf3f25b68@kernel.org>
Date: Thu, 31 Oct 2024 12:17:44 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Nicolas Ferre <nicolas.ferre@...rochip.com>,
 Alexandre Belloni <alexandre.belloni@...tlin.com>,
 Claudiu Beznea <claudiu.beznea@...on.dev>,
 Sudeep Holla <sudeep.holla@....com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] drivers: soc: atmel: use automatic cleanup for
 device_node in atmel_soc_device_init()

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@...il.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