Re: [PATCH 01/10] soc: qcom: aoss: Use __cleanup() for device_node pointers

[Krzysztof Kozlowski <krzk@kernel.org>]

On 18/11/2025 14:16, Konrad Dybcio wrote:
> On 11/18/25 1:52 PM, Dmitry Baryshkov wrote:
>> On Tue, Nov 18, 2025 at 01:32:51PM +0100, Krzysztof Kozlowski wrote:
>>> On 18/11/2025 13:25, Dmitry Baryshkov wrote:
>>>> On Tue, Nov 18, 2025 at 12:39:51PM +0100, Krzysztof Kozlowski wrote:
>>>>> On 17/11/2025 12:35, Konrad Dybcio wrote:
>>>>>> On 11/17/25 5:51 AM, Kathiravan Thirumoorthy wrote:
>>>>>>> Make use of the __cleanup() attribute for device_node pointers to simplify
>>>>>>> resource management and remove explicit of_node_put() calls.
>>>>>>>
>>>>>>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>>>>>>> ---
>>>>>>
>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>
>>>>> This is obviously wrong and not helpful patch.
>>>>
>>>> Describing why it is wrong would be helpful (or having a pointer to an
>>>> explanation). Bear in mind people who read email archives and find this
>>>> very brief note.
>>>
>>> I gave some rationale in other patches, but summarizing:
>>> 1. It is against cleanup.h - author did not bother to read it - which
>>> clearly asks for constructor with declaration. This was discussed many
>>> times in the list, including many bugs and explicit checkpatch warning
>>> (on LKML) because people don't bother to read cleanup.h.
> 
> Looks like I didn't read it either.. now that I did, I see that
> _free(x) = NULL is somewhat of an anti-pattern, but none of these

True

> patches seem to introduce any bugs related to it

True

> 
>>> 2. It makes simple get+put code complicated, not simpler.
> 
> Here I tend to disagree.. 

of_get and immediate of_put is really obvious and easy to read. It is
really a common pattern.

OTOH code like:

	struct device_node *np = of_parse_whatever_which_is_get()
	...
	bunch of code here where np is actually used.
	...
	... more code
	return;

is not simple. The lifecycle of this variable becomes very long and
where it is acquired - at beginning - causes question - why do you need
to acquire it there?

That's why cleanup.h has VERY specific syntax different than rest of kernel.

If contributor does not know this syntax, don't send the patches. They
are wrong.



> 
>>> 3. It grows the scope of OF reference without benefits.
> 
> This makes sense
> 
> Ultimately as you've noticed, this is mostly a cosmetic change and
> I don't mind it going either way

It is not cosmetic. It is anti-pattern.

Anti-patterns, even without introducing bugs, make the code WORSE to
maintain. Worse code is not cosmetic.


Best regards,
Krzysztof