Re: [PATCH v3 1/5] ASoC: intel: sst-mfld-platform-pcm: Use __free(kfree) for stream pointer

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 6/23/2026 12:57 PM, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Declare 'stream' with __free(kfree) so it is automatically freed when
> leaving scope. This allows direct returns from error paths and removes
> the explicit kfree(stream) call.
> Set 'stream' to NULL after ownership has been transferred to
> runtime->private_data to prevent it from being freed on the success path.

Hi Phuc,

Thank you for separating the change.  The design of the patchset is off 
though.  __free() is by no means a requirement for the locking update - 
it should be located at the back.  Also, grouping changes targeting the 
same driver makes the entire patchset easier to follow.  Right now we 
have 2 avs patches surrounded by atom ones from either side.

Last but not least, please drop the filename in the commit title.  We do 
not do that here - some filenames are long and the approach leaves 
little space for answering the _what_ question when building a proper 
commit title.  "ASoC: Intel: atom:" is what you want here.

>   sound/soc/intel/atom/sst-mfld-platform-pcm.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)

...


> @@ -347,12 +347,19 @@ static int sst_media_open(struct snd_pcm_substream *substream,
>   	snd_pcm_hw_constraint_step(substream->runtime, 0,
>   			   SNDRV_PCM_HW_PARAM_PERIODS, 2);
>   
> -	return snd_pcm_hw_constraint_integer(runtime,
> -			 SNDRV_PCM_HW_PARAM_PERIODS);
> +	ret_val = snd_pcm_hw_constraint_integer(runtime,
> +						SNDRV_PCM_HW_PARAM_PERIODS);
> +
> +	if (ret_val < 0)
> +		return ret_val;
> +
> +	stream = NULL;
> +
> +	return ret_val;
> +

The entire construct looks bad, unfortunately.  Guess the reason comes 
from its follow up - the locking update.  The delta (+13, -6) isn't 
exactly a good advertisement for the cleanup either.  Again, I see this 
patch as last in the set.  Should lower the delta too.

>   out_ops:
>   	mutex_unlock(&sst_lock);
> -out_power_up:
> -	kfree(stream);
> +
>   	return ret_val;
>   }
>