Re: [PATCH v2 3/3] ALSA: hda: Use scoped_guard() for open_mutex in azx_pcm_open()

[Takashi Iwai]

On Fri, 10 Jul 2026 10:40:09 +0200,
wangdich9700@xxxxxxx wrote:
> 
> From: wangdicheng <wangdicheng@xxxxxxxxxx>
> 
> The companion function azx_pcm_close() already uses
> scoped_guard(mutex, &chip->open_mutex). Replace the manual
> mutex_lock/mutex_unlock pair in azx_pcm_open() with the same
> pattern for consistency.
> 
> All goto targets are within the scoped_guard() block, avoiding
> the goto-jumping-out-of-scope pattern that is not recommended
> when using cleanup guards.
> 
> No functional changes.
> 
> Signed-off-by: wangdicheng <wangdicheng@xxxxxxxxxx>

This one has also a combination of guard() and goto, which isn't a
good mix.


thanks,

Takashi

> ---
>  sound/hda/common/controller.c | 158 +++++++++++++++++-----------------
>  1 file changed, 81 insertions(+), 77 deletions(-)
> 
> diff --git a/sound/hda/common/controller.c b/sound/hda/common/controller.c
> index afec5c5546ec..04ae21cc5e5e 100644
> --- a/sound/hda/common/controller.c
> +++ b/sound/hda/common/controller.c
> @@ -584,86 +584,90 @@ static int azx_pcm_open(struct snd_pcm_substream *substream)
>  	int buff_step;
>  
>  	snd_hda_codec_pcm_get(apcm->info);
> -	mutex_lock(&chip->open_mutex);
> -	azx_dev = azx_assign_device(chip, substream);
> -	trace_azx_pcm_open(chip, azx_dev);
> -	if (azx_dev == NULL) {
> -		err = -EBUSY;
> -		goto unlock;
> -	}
> -	runtime->private_data = azx_dev;
> -
> -	runtime->hw = azx_pcm_hw;
> -	if (chip->gts_present)
> -		runtime->hw.info |= SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME;
> -	runtime->hw.channels_min = hinfo->channels_min;
> -	runtime->hw.channels_max = hinfo->channels_max;
> -	runtime->hw.formats = hinfo->formats;
> -	runtime->hw.rates = hinfo->rates;
> -	snd_pcm_limit_hw_rates(runtime);
> -	snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
> -
> -	/* avoid wrap-around with wall-clock */
> -	snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_TIME,
> -				     20,
> -				     178000000);
> -
> -	if (chip->align_buffer_size)
> -		/* constrain buffer sizes to be multiple of 128
> -		   bytes. This is more efficient in terms of memory
> -		   access but isn't required by the HDA spec and
> -		   prevents users from specifying exact period/buffer
> -		   sizes. For example for 44.1kHz, a period size set
> -		   to 20ms will be rounded to 19.59ms. */
> -		buff_step = 128;
> -	else
> -		/* Don't enforce steps on buffer sizes, still need to
> -		   be multiple of 4 bytes (HDA spec). Tested on Intel
> -		   HDA controllers, may not work on all devices where
> -		   option needs to be disabled */
> -		buff_step = 4;
> -
> -	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
> -				   buff_step);
> -	snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> -				   buff_step);
> -	snd_hda_power_up(apcm->codec);
> -	if (hinfo->ops.open)
> -		err = hinfo->ops.open(hinfo, apcm->codec, substream);
> -	else
> -		err = -ENODEV;
> -	if (err < 0) {
> -		azx_release_device(azx_dev);
> -		goto powerdown;
> -	}
> -	snd_pcm_limit_hw_rates(runtime);
> -	/* sanity check */
> -	if (snd_BUG_ON(!runtime->hw.channels_min) ||
> -	    snd_BUG_ON(!runtime->hw.channels_max) ||
> -	    snd_BUG_ON(!runtime->hw.formats) ||
> -	    snd_BUG_ON(!runtime->hw.rates)) {
> -		azx_release_device(azx_dev);
> -		if (hinfo->ops.close)
> -			hinfo->ops.close(hinfo, apcm->codec, substream);
> -		err = -EINVAL;
> -		goto powerdown;
> -	}
> +	scoped_guard(mutex, &chip->open_mutex) {
> +		azx_dev = azx_assign_device(chip, substream);
> +		trace_azx_pcm_open(chip, azx_dev);
> +		if (azx_dev == NULL) {
> +			err = -EBUSY;
> +			break;
> +		}
> +		runtime->private_data = azx_dev;
> +
> +		runtime->hw = azx_pcm_hw;
> +		if (chip->gts_present)
> +			runtime->hw.info |= SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME;
> +		runtime->hw.channels_min = hinfo->channels_min;
> +		runtime->hw.channels_max = hinfo->channels_max;
> +		runtime->hw.formats = hinfo->formats;
> +		runtime->hw.rates = hinfo->rates;
> +		snd_pcm_limit_hw_rates(runtime);
> +		snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS);
> +
> +		/* avoid wrap-around with wall-clock */
> +		snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_TIME,
> +					     20,
> +					     178000000);
> +
> +		if (chip->align_buffer_size)
> +			/*
> +			 * Constrain buffer sizes to be multiple of 128
> +			 * bytes. This is more efficient in terms of memory
> +			 * access but isn't required by the HDA spec and
> +			 * prevents users from specifying exact period/buffer
> +			 * sizes. For example for 44.1kHz, a period size set
> +			 * to 20ms will be rounded to 19.59ms.
> +			 */
> +			buff_step = 128;
> +		else
> +			/*
> +			 * Don't enforce steps on buffer sizes, still need to
> +			 * be multiple of 4 bytes (HDA spec). Tested on Intel
> +			 * HDA controllers, may not work on all devices where
> +			 * option needs to be disabled
> +			 */
> +			buff_step = 4;
> +
> +		snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
> +					   buff_step);
> +		snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
> +					   buff_step);
> +		snd_hda_power_up(apcm->codec);
> +		if (hinfo->ops.open)
> +			err = hinfo->ops.open(hinfo, apcm->codec, substream);
> +		else
> +			err = -ENODEV;
> +		if (err < 0) {
> +			azx_release_device(azx_dev);
> +			goto powerdown;
> +		}
> +		snd_pcm_limit_hw_rates(runtime);
> +		/* sanity check */
> +		if (snd_BUG_ON(!runtime->hw.channels_min) ||
> +		    snd_BUG_ON(!runtime->hw.channels_max) ||
> +		    snd_BUG_ON(!runtime->hw.formats) ||
> +		    snd_BUG_ON(!runtime->hw.rates)) {
> +			azx_release_device(azx_dev);
> +			if (hinfo->ops.close)
> +				hinfo->ops.close(hinfo, apcm->codec, substream);
> +			err = -EINVAL;
> +			goto powerdown;
> +		}
>  
> -	/* disable LINK_ATIME timestamps for capture streams
> -	   until we figure out how to handle digital inputs */
> -	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> -		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_WALL_CLOCK; /* legacy */
> -		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_LINK_ATIME;
> -	}
> +		/*
> +		 * Disable LINK_ATIME timestamps for capture streams
> +		 * until we figure out how to handle digital inputs
> +		 */
> +		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +			runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_WALL_CLOCK; /* legacy */
> +			runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_LINK_ATIME;
> +		}
>  
> -	snd_pcm_set_sync(substream);
> -	mutex_unlock(&chip->open_mutex);
> -	return 0;
> +		snd_pcm_set_sync(substream);
> +		return 0;
>  
> - powerdown:
> -	snd_hda_power_down(apcm->codec);
> - unlock:
> -	mutex_unlock(&chip->open_mutex);
> +powerdown:
> +		snd_hda_power_down(apcm->codec);
> +	}
>  	snd_hda_codec_pcm_put(apcm->info);
>  	return err;
>  }
> -- 
> 2.25.1
>