linux-kernel - Re: [PATCH 3/3] ASoC: SDCA: functions: Fix confusing cleanup.h syntax

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: <67ead532-6e38-4504-bfda-1cf7e998c885@oss.qualcomm.com>
Date: Thu, 4 Dec 2025 12:59:45 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@....qualcomm.com>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
        Maciej Strozek <mstrozek@...nsource.cirrus.com>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>,
        linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH 3/3] ASoC: SDCA: functions: Fix confusing cleanup.h syntax

On 04/12/2025 12:26, Charles Keepax wrote:
> On Wed, Dec 03, 2025 at 05:12:40PM +0100, Krzysztof Kozlowski wrote:
>> Initializing automatic __free variables to NULL without need (e.g.
>> branches with different allocations), followed by actual allocation is
>> in contrary to explicit coding rules guiding cleanup.h:
>>
>> "Given that the "__free(...) = NULL" pattern for variables defined at
>> the top of the function poses this potential interdependency problem the
>> recommendation is to always define and assign variables in one statement
>> and not group variable definitions at the top of the function when
>> __free() is used."
>>
>> Code does not have a bug, but is less readable and uses discouraged
>> coding practice, so fix that by moving declaration to the place of
>> assignment.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@....qualcomm.com>
>> ---
> 
> Hmm... yeah a fair point. Can't say I love the inline
> declarations but I guess I will get used to it.
> 
> I wonder if for consistency we should do something about
> the __free in find_sdca_init_table as well. One could move
> the alloc to before the error checks, does risk doing an
> unecessary alloc but its on the error path so I don't feel like
> performance matters. Or rather than sizeof(*raw) we could just
> do sizeof(struct raw_init_write)?

If there is a reason to deviate from the declaration+allocation rule,
then sure go for it. This is style, so it describes only the preference.
But if you do not have a reason, then coding style described in
cleanup.h should be used (or don't use cleanup.h).

> 
> But if you would rather leave as is I don't mind either so:

Yep. Few sdca-related files use that exception. The
find_sdca_init_table() at least has a reason - it uses sizeof(*raw) as
you mentioned, so that's why I decided not to change it. Just too much
churn.

But I want to fix all the cases where '= NULL' is used without any need.


> 
> Reviewed-by: Charles Keepax <ckeepax@...nsource.cirrus.com>
> 
> Thanks,
> Charles


Best regards,
Krzysztof