Re: [PATCH 31/31] ASoC: SOF: Use auto-cleanup for firmware loading

On 05/08/2026 16:52, Takashi Iwai wrote:
> Simplify the code to manage the firmware loading with __free(firmware)
> and __free(kfree) auto-cleanups for the firmware data and the temporary
> string or array.
> 
> Only the code refactoring, no functional changes.
> 
> Cc: Liam Girdwood <[email protected]>
> Cc: Peter Ujfalusi <[email protected]>
> Cc: Bard Liao <[email protected]>
> Cc: Daniel Baluta <[email protected]>
> Cc: Pierre-Louis Bossart <[email protected]>
> Cc: Vijendar Mukunda <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
>  sound/soc/sof/fw-file-profile.c | 19 +++++++------------
>  sound/soc/sof/topology.c        | 20 ++++++--------------
>  2 files changed, 13 insertions(+), 26 deletions(-)
> 
> diff --git a/sound/soc/sof/fw-file-profile.c b/sound/soc/sof/fw-file-profile.c
> index 76bde2e0be1d..fcd57f04ca36 100644
> --- a/sound/soc/sof/fw-file-profile.c
> +++ b/sound/soc/sof/fw-file-profile.c
> @@ -16,20 +16,19 @@ static int sof_test_firmware_file(struct device *dev,
>  				  enum sof_ipc_type *ipc_type_to_adjust)
>  {
>  	enum sof_ipc_type fw_ipc_type;
> -	const struct firmware *fw;
> -	const char *fw_filename;
>  	const u32 *magic;
>  	int ret;
>  
> -	fw_filename = kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
> -				profile->fw_name);
> +	const char *fw_filename __free(kfree) =
> +		kasprintf(GFP_KERNEL, "%s/%s", profile->fw_path,
> +			  profile->fw_name);
>  	if (!fw_filename)
>  		return -ENOMEM;
>  
> +	const struct firmware *fw __free(firmware) = NULL;

can we keep this at the head of the function?

>  	ret = firmware_request_nowarn(&fw, fw_filename, dev);
>  	if (ret < 0) {
>  		dev_dbg(dev, "Failed to open firmware file: %s\n", fw_filename);
> -		kfree(fw_filename);
>  		return ret;
>  	}
>  
> @@ -44,8 +43,7 @@ static int sof_test_firmware_file(struct device *dev,
>  		break;
>  	default:
>  		dev_err(dev, "Invalid firmware magic: %#x\n", *magic);
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>  	}
>  
>  	if (ipc_type_to_adjust) {
> @@ -54,13 +52,10 @@ static int sof_test_firmware_file(struct device *dev,
>  		dev_err(dev,
>  			"ipc type mismatch between %s and expected: %d vs %d\n",
>  			fw_filename, fw_ipc_type, profile->ipc_type);
> -		ret = -EINVAL;
> +		return -EINVAL;
>  	}
> -out:
> -	release_firmware(fw);
> -	kfree(fw_filename);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int sof_test_topology_file(struct device *dev,
> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
> index 6fd69ba11c41..820513bb2577 100644
> --- a/sound/soc/sof/topology.c
> +++ b/sound/soc/sof/topology.c
> @@ -2506,13 +2506,12 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
>  	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp);
>  	struct snd_sof_pdata *sof_pdata = sdev->pdata;
>  	const char *tplg_filename_prefix = sof_pdata->tplg_filename_prefix;
> -	const struct firmware *fw;
> -	const char **tplg_files;
>  	int tplg_cnt = 0;
>  	int ret;
>  	int i;
>  
> -	tplg_files = kcalloc(scomp->card->num_links, sizeof(char *), GFP_KERNEL);
> +	const char **tplg_files __free(kfree) =
> +		kcalloc(scomp->card->num_links, sizeof(char *), GFP_KERNEL);
>  	if (!tplg_files)
>  		return -ENOMEM;
>  
> @@ -2538,10 +2537,8 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
>  								       tplg_filename_prefix,
>  								       &tplg_files,
>  								       no_fallback);
> -		if (tplg_cnt < 0) {
> -			kfree(tplg_files);
> +		if (tplg_cnt < 0)
>  			return tplg_cnt;
> -		}
>  	}
>  
>  	/*
> @@ -2552,8 +2549,6 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
>  		if (strstr(file, "dummy")) {
>  			dev_err(scomp->dev,
>  				"Function topology is required, please upgrade sof-firmware\n");
> -
> -			kfree(tplg_files);
>  			return -EINVAL;
>  		}
>  		tplg_files[0] = file;
> @@ -2568,6 +2563,7 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
>  		if (tplg_files[0] != file)
>  			dev_info(scomp->dev, "loading topology %d: %s\n", i, tplg_files[i]);
>  
> +		const struct firmware *fw __free(firmware) = NULL;

can we move this at the start of the for {} loop?
for (i = 0; i < tplg_cnt; i++) {
	const struct firmware *fw __free(firmware) = NULL;
...


>  		ret = request_firmware(&fw, tplg_files[i], scomp->dev);
>  		if (ret < 0) {
>  			/*
> @@ -2586,8 +2582,6 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
>  		else
>  			ret = snd_soc_tplg_component_load(scomp, &sof_tplg_ops, fw);
>  
> -		release_firmware(fw);
> -
>  		if (ret < 0) {
>  			dev_err(scomp->dev, "tplg %s component load failed %d\n",
>  				tplg_files[i], ret);
> @@ -2606,6 +2600,8 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
>  			goto out;
>  		}
>  		dev_info(scomp->dev, "loading feature topology %d: %s\n", i, feature_topology);
> +
> +		const struct firmware *fw __free(firmware) = NULL;

can we move this at the start of the for {} loop?
	for (i = 0; i < feature_tplg_cnt; i++) {
	const struct firmware *fw __free(firmware) = NULL;
...

But I'm not sure if we buy clarity with the tplg loading.

>  		ret = request_firmware(&fw, feature_topology, scomp->dev);
>  		if (ret < 0) {
>  			/*
> @@ -2630,8 +2626,6 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
>  		else
>  			ret = snd_soc_tplg_component_load(scomp, &sof_tplg_ops, fw);
>  
> -		release_firmware(fw);
> -
>  		if (ret < 0) {
>  			dev_err(scomp->dev, "feature tplg %s component load failed %d\n",
>  				feature_topologies[i], ret);
> @@ -2650,8 +2644,6 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
>  	if (ret >= 0 && sdev->led_present)
>  		ret = snd_ctl_led_request();
>  
> -	kfree(tplg_files);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL(snd_sof_load_topology);

with that,
Acked-by: Peter Ujfalusi <[email protected]>
Tested-by: Peter Ujfalusi <[email protected]>

-- 
Péter