Re: [PATCH v2 1/2] ALSA: hda: cs35l56: Free ACPI _SUB string on errors

[Richard Fitzgerald <rf@opensource.cirrus.com>]

On 20/05/2026 3:50 pm, Cássio Gabriel wrote:
> cs35l56_hda_read_acpi() gets an allocated ACPI _SUB string from
> acpi_get_subsystem_id(). On success, that string is either used directly
> as the firmware system name or used to construct a speaker-ID-specific
> firmware system name.
> 
> Several error paths after the _SUB lookup can return without releasing
> the allocated string. This includes speaker ID lookup errors other than
> -ENOENT, and errors after a firmware system name has been derived but
> before ownership is transferred to cs35l56->system_name.
> 
> Use scoped cleanup for the temporary _SUB string and the derived system
> name, and transfer ownership to cs35l56->system_name only after the
> remaining failure paths in cs35l56_hda_read_acpi() have completed.
> 
> Fixes: 6f03b446cbae ("ALSA: hda: cs35l56: Add support for speaker id")
> Fixes: 40b1c2f9b299 ("ALSA: hda/cs35l56: Workaround bad dev-index on Lenovo Yoga Book 9i GenX")
> Signed-off-by: Cássio Gabriel <cassiogabrielcontato@gmail.com>
> ---
>   sound/hda/codecs/side-codecs/cs35l56_hda.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/hda/codecs/side-codecs/cs35l56_hda.c b/sound/hda/codecs/side-codecs/cs35l56_hda.c
> index cdbc576569ef..7c9c98436757 100644
> --- a/sound/hda/codecs/side-codecs/cs35l56_hda.c
> +++ b/sound/hda/codecs/side-codecs/cs35l56_hda.c
> @@ -1025,7 +1025,7 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int hid, int id)
>   	u32 values[HDA_MAX_COMPONENTS];
>   	char hid_string[8];
>   	struct acpi_device *adev;
> -	const char *property, *sub;
> +	const char *property;
>   	int i, ret;
>   
>   	/*
> @@ -1047,7 +1047,9 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int hid, int id)
>   	/* Initialize things that could be overwritten by a fixup */
>   	cs35l56->index = -1;
>   
> -	sub = acpi_get_subsystem_id(ACPI_HANDLE(cs35l56->base.dev));
> +	const char *sub __free(kfree) = acpi_get_subsystem_id(ACPI_HANDLE(cs35l56->base.dev));
> +	const char *system_name __free(kfree) = NULL;
> +

Initializing a __free(kfree) to NULL is controversial. The kerneldoc
warns against it. Though I think your code is totally fine it might be
best to avoid.

I can't see any reason why we didn't use devm cleanup for
cs35l56->system_name.

So

    		if (ret == -ENOENT) {
			cs35l56->system_name = devm_kstrdup(dev, sub,
GFP_KERNEL);
   		} else if (ret >= 0) {
			cs35l56->system_name = devm_kasprintf(dev, ...);
			...


and delete the manual kfree(cs35l56->system_name) in
cs35l56_hda_remove().

>   	ret = cs35l56_hda_apply_platform_fixups(cs35l56, sub, &id);
>   	if (ret)
>   		return ret;
> @@ -1095,11 +1097,10 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int hid, int id)
>   		ret = cirrus_scodec_get_speaker_id(cs35l56->base.dev, cs35l56->index,
>   						   cs35l56->num_amps, -1);
>   		if (ret == -ENOENT) {
> -			cs35l56->system_name = sub;
> +			system_name = no_free_ptr(sub);
>   		} else if (ret >= 0) {
> -			cs35l56->system_name = kasprintf(GFP_KERNEL, "%s-spkid%d", sub, ret);
> -			kfree(sub);
> -			if (!cs35l56->system_name)
> +			system_name = kasprintf(GFP_KERNEL, "%s-spkid%d", sub, ret);
> +			if (!system_name)
>   				return -ENOMEM;
>   		} else {
>   			return ret;
> @@ -1118,12 +1119,14 @@ static int cs35l56_hda_read_acpi(struct cs35l56_hda *cs35l56, int hid, int id)
>   		 * line and reset all the amps
>   		 */
>   		if (ret != -EBUSY)
> -			return dev_err_probe(cs35l56->base.dev, ret, "Failed to get reset GPIO\n");
> +			return dev_err_probe(cs35l56->base.dev, ret,
> +					     "Failed to get reset GPIO\n");
>   
>   		dev_info(cs35l56->base.dev, "Reset GPIO busy, assume shared reset\n");
>   		cs35l56->base.reset_gpio = NULL;
>   	}
>   
> +	cs35l56->system_name = no_free_ptr(system_name);
>   	return 0;
>   
>   err:
>