[PATCH 0/3] ASoC: amd/sdw: Fix confusing cleanup.h

[PATCH 0/3] ASoC: amd/sdw: Fix confusing cleanup.h

[Krzysztof Kozlowski]

cleanup.h coding style asks to avoid having constructors with redundant
values (= NULL). On purpose, because it is just not making the code
simpler. The constructor should be meaningful not just NULL.

If you do not agree in declaration-in-place-of-use (fair!), then do not
use cleanup.h. If you want to use cleanup.h, then please read cleanup.h
before.

Best regards,
Krzysztof

---
Krzysztof Kozlowski (3):
      ASoC: amd: acp-sdw-legacy: Fix confusing cleanup.h syntax
      ASoC: amd: acp-sdw-sof: Fix confusing cleanup.h syntax
      ASoC: SDCA: functions: Fix confusing cleanup.h syntax

 sound/soc/amd/acp/acp-sdw-legacy-mach.c |  8 ++++----
 sound/soc/amd/acp/acp-sdw-sof-mach.c    |  8 ++++----
 sound/soc/sdca/sdca_functions.c         | 28 ++++++++++++++--------------
 3 files changed, 22 insertions(+), 22 deletions(-)
---
base-commit: 32ed136b4e871f08398f342aa92feffed020839b
change-id: 20251203-asoc-wrong-cleanup-h-continued-6e3d27d097df

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

[PATCH 1/3] ASoC: amd: acp-sdw-legacy: Fix confusing cleanup.h syntax

[Krzysztof Kozlowski]

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@oss.qualcomm.com>
---
 sound/soc/amd/acp/acp-sdw-legacy-mach.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/amd/acp/acp-sdw-legacy-mach.c b/sound/soc/amd/acp/acp-sdw-legacy-mach.c
index fae94b9edd5a..9cb55d592c3c 100644
--- a/sound/soc/amd/acp/acp-sdw-legacy-mach.c
+++ b/sound/soc/amd/acp/acp-sdw-legacy-mach.c
@@ -358,8 +358,6 @@ static int soc_card_dai_links_create(struct snd_soc_card *card)
 	int sdw_be_num = 0, dmic_num = 0;
 	struct asoc_sdw_mc_private *ctx = snd_soc_card_get_drvdata(card);
 	struct snd_soc_acpi_mach_params *mach_params = &mach->mach_params;
-	struct asoc_sdw_endpoint *soc_ends __free(kfree) = NULL;
-	struct asoc_sdw_dailink *soc_dais __free(kfree) = NULL;
 	struct snd_soc_aux_dev *soc_aux;
 	struct snd_soc_codec_conf *codec_conf;
 	struct snd_soc_dai_link *dai_links;
@@ -380,12 +378,14 @@ static int soc_card_dai_links_create(struct snd_soc_card *card)
 	num_confs = num_ends;
 
 	/* One per DAI link, worst case is a DAI link for every endpoint */
-	soc_dais = kcalloc(num_ends, sizeof(*soc_dais), GFP_KERNEL);
+	struct asoc_sdw_dailink *soc_dais __free(kfree) =
+		kcalloc(num_ends, sizeof(*soc_dais), GFP_KERNEL);
 	if (!soc_dais)
 		return -ENOMEM;
 
 	/* One per endpoint, ie. each DAI on each codec/amp */
-	soc_ends = kcalloc(num_ends, sizeof(*soc_ends), GFP_KERNEL);
+	struct asoc_sdw_endpoint *soc_ends __free(kfree) =
+		kcalloc(num_ends, sizeof(*soc_ends), GFP_KERNEL);
 	if (!soc_ends)
 		return -ENOMEM;
 

-- 
2.51.0

[PATCH 2/3] ASoC: amd: acp-sdw-sof: Fix confusing cleanup.h syntax

[Krzysztof Kozlowski]

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@oss.qualcomm.com>
---
 sound/soc/amd/acp/acp-sdw-sof-mach.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/amd/acp/acp-sdw-sof-mach.c b/sound/soc/amd/acp/acp-sdw-sof-mach.c
index 5677ae63fca9..da815b3f6389 100644
--- a/sound/soc/amd/acp/acp-sdw-sof-mach.c
+++ b/sound/soc/amd/acp/acp-sdw-sof-mach.c
@@ -270,8 +270,6 @@ static int sof_card_dai_links_create(struct snd_soc_card *card)
 	int sdw_be_num = 0, dmic_num = 0;
 	struct asoc_sdw_mc_private *ctx = snd_soc_card_get_drvdata(card);
 	struct snd_soc_acpi_mach_params *mach_params = &mach->mach_params;
-	struct asoc_sdw_endpoint *sof_ends __free(kfree) = NULL;
-	struct asoc_sdw_dailink *sof_dais __free(kfree) = NULL;
 	struct snd_soc_aux_dev *sof_aux;
 	struct snd_soc_codec_conf *codec_conf;
 	struct snd_soc_dai_link *dai_links;
@@ -289,12 +287,14 @@ static int sof_card_dai_links_create(struct snd_soc_card *card)
 	}
 
 	/* One per DAI link, worst case is a DAI link for every endpoint */
-	sof_dais = kcalloc(num_ends, sizeof(*sof_dais), GFP_KERNEL);
+	struct asoc_sdw_dailink *sof_dais __free(kfree) =
+		kcalloc(num_ends, sizeof(*sof_dais), GFP_KERNEL);
 	if (!sof_dais)
 		return -ENOMEM;
 
 	/* One per endpoint, ie. each DAI on each codec/amp */
-	sof_ends = kcalloc(num_ends, sizeof(*sof_ends), GFP_KERNEL);
+	struct asoc_sdw_endpoint *sof_ends __free(kfree) =
+		kcalloc(num_ends, sizeof(*sof_ends), GFP_KERNEL);
 	if (!sof_ends)
 		return -ENOMEM;
 

-- 
2.51.0

[PATCH 3/3] ASoC: SDCA: functions: Fix confusing cleanup.h syntax

[Krzysztof Kozlowski]

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@oss.qualcomm.com>
---
 sound/soc/sdca/sdca_functions.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/sound/soc/sdca/sdca_functions.c b/sound/soc/sdca/sdca_functions.c
index 5a1f120487ef..acac066f1d8d 100644
--- a/sound/soc/sdca/sdca_functions.c
+++ b/sound/soc/sdca/sdca_functions.c
@@ -1184,7 +1184,6 @@ static int find_sdca_entity_pde(struct device *dev,
 {
 	static const int mult_delay = 3;
 	struct sdca_entity_pde *power = &entity->pde;
-	u32 *delay_list __free(kfree) = NULL;
 	struct sdca_pde_delay *delays;
 	int num_delays;
 	int i, j;
@@ -1205,7 +1204,8 @@ static int find_sdca_entity_pde(struct device *dev,
 		return -EINVAL;
 	}
 
-	delay_list = kcalloc(num_delays, sizeof(*delay_list), GFP_KERNEL);
+	u32 *delay_list __free(kfree) = kcalloc(num_delays, sizeof(*delay_list),
+						GFP_KERNEL);
 	if (!delay_list)
 		return -ENOMEM;
 
@@ -1250,7 +1250,6 @@ static int find_sdca_entity_ge(struct device *dev,
 			       struct sdca_entity *entity)
 {
 	struct sdca_entity_ge *group = &entity->ge;
-	u8 *affected_list __free(kfree) = NULL;
 	u8 *affected_iter;
 	int num_affected;
 	int i, j;
@@ -1269,7 +1268,8 @@ static int find_sdca_entity_ge(struct device *dev,
 		return -EINVAL;
 	}
 
-	affected_list = kcalloc(num_affected, sizeof(*affected_list), GFP_KERNEL);
+	u8 *affected_list __free(kfree) = kcalloc(num_affected, sizeof(*affected_list),
+						  GFP_KERNEL);
 	if (!affected_list)
 		return -ENOMEM;
 
@@ -1495,7 +1495,6 @@ static int find_sdca_entities(struct device *dev, struct sdw_slave *sdw,
 			      struct fwnode_handle *function_node,
 			      struct sdca_function_data *function)
 {
-	u32 *entity_list __free(kfree) = NULL;
 	struct sdca_entity *entities;
 	int num_entities;
 	int i, ret;
@@ -1517,7 +1516,8 @@ static int find_sdca_entities(struct device *dev, struct sdw_slave *sdw,
 	if (!entities)
 		return -ENOMEM;
 
-	entity_list = kcalloc(num_entities, sizeof(*entity_list), GFP_KERNEL);
+	u32 *entity_list __free(kfree) = kcalloc(num_entities, sizeof(*entity_list),
+						 GFP_KERNEL);
 	if (!entity_list)
 		return -ENOMEM;
 
@@ -1642,7 +1642,6 @@ static int find_sdca_entity_connection_pde(struct device *dev,
 					   struct sdca_entity *entity)
 {
 	struct sdca_entity_pde *power = &entity->pde;
-	u32 *managed_list __free(kfree) = NULL;
 	struct sdca_entity **managed;
 	int num_managed;
 	int i;
@@ -1664,7 +1663,8 @@ static int find_sdca_entity_connection_pde(struct device *dev,
 	if (!managed)
 		return -ENOMEM;
 
-	managed_list = kcalloc(num_managed, sizeof(*managed_list), GFP_KERNEL);
+	u32 *managed_list __free(kfree) = kcalloc(num_managed, sizeof(*managed_list),
+						  GFP_KERNEL);
 	if (!managed_list)
 		return -ENOMEM;
 
@@ -1961,7 +1961,6 @@ static int find_sdca_clusters(struct device *dev,
 			      struct fwnode_handle *function_node,
 			      struct sdca_function_data *function)
 {
-	u32 *cluster_list __free(kfree) = NULL;
 	struct sdca_cluster *clusters;
 	int num_clusters;
 	int i, ret;
@@ -1982,7 +1981,8 @@ static int find_sdca_clusters(struct device *dev,
 	if (!clusters)
 		return -ENOMEM;
 
-	cluster_list = kcalloc(num_clusters, sizeof(*cluster_list), GFP_KERNEL);
+	u32 *cluster_list __free(kfree) = kcalloc(num_clusters, sizeof(*cluster_list),
+						  GFP_KERNEL);
 	if (!cluster_list)
 		return -ENOMEM;
 
@@ -2026,7 +2026,6 @@ static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
 {
 	static const int mult_fileset = 3;
 	char fileset_name[SDCA_PROPERTY_LENGTH];
-	u32 *filesets_list __free(kfree) = NULL;
 	struct sdca_fdl_set *sets;
 	int num_sets;
 	int i, j;
@@ -2041,7 +2040,8 @@ static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
 		return num_sets;
 	}
 
-	filesets_list = kcalloc(num_sets, sizeof(u32), GFP_KERNEL);
+	u32 *filesets_list __free(kfree) = kcalloc(num_sets, sizeof(u32),
+						   GFP_KERNEL);
 	if (!filesets_list)
 		return -ENOMEM;
 
@@ -2053,7 +2053,6 @@ static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
 		return -ENOMEM;
 
 	for (i = 0; i < num_sets; i++) {
-		u32 *fileset_entries __free(kfree) = NULL;
 		struct sdca_fdl_set *set = &sets[i];
 		struct sdca_fdl_file *files;
 		int num_files, num_entries;
@@ -2079,7 +2078,8 @@ static int find_sdca_filesets(struct device *dev, struct sdw_slave *sdw,
 		if (!files)
 			return -ENOMEM;
 
-		fileset_entries = kcalloc(num_entries, sizeof(u32), GFP_KERNEL);
+		u32 *fileset_entries __free(kfree) = kcalloc(num_entries, sizeof(u32),
+							     GFP_KERNEL);
 		if (!fileset_entries)
 			return -ENOMEM;
 

-- 
2.51.0

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

[Charles Keepax]

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@oss.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)?

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

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

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

[Krzysztof Kozlowski]

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@oss.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@opensource.cirrus.com>
> 
> Thanks,
> Charles


Best regards,
Krzysztof

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

[Charles Keepax]

On Thu, Dec 04, 2025 at 12:59:45PM +0100, Krzysztof Kozlowski wrote:
> On 04/12/2025 12:26, Charles Keepax wrote:
> > On Wed, Dec 03, 2025 at 05:12:40PM +0100, Krzysztof Kozlowski wrote:
> 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.

Groovy yeah, I am happy with that if you are.

Thanks,
Charles

> > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Re: [PATCH 0/3] ASoC: amd/sdw: Fix confusing cleanup.h

[Mark Brown]

On Wed, 03 Dec 2025 17:12:37 +0100, Krzysztof Kozlowski wrote:
> cleanup.h coding style asks to avoid having constructors with redundant
> values (= NULL). On purpose, because it is just not making the code
> simpler. The constructor should be meaningful not just NULL.
> 
> If you do not agree in declaration-in-place-of-use (fair!), then do not
> use cleanup.h. If you want to use cleanup.h, then please read cleanup.h
> before.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: amd: acp-sdw-legacy: Fix confusing cleanup.h syntax
      commit: 5a7e236925b417332a674a8488ef19da233a5764
[2/3] ASoC: amd: acp-sdw-sof: Fix confusing cleanup.h syntax
      commit: bafd5cf04b2822bf3c865add7262d86e22b9588e
[3/3] ASoC: SDCA: functions: Fix confusing cleanup.h syntax
      commit: e39011184f23de3d04ca8e80b4df76c9047b4026

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark