[PATCH v2 0/3] ALSA: hda: Use scoped_guard() for remaining manual mutex handling

[PATCH v2 0/3] ALSA: hda: Use scoped_guard() for remaining manual mutex handling

[wangdich9700]

From: wangdicheng <wangdicheng@kylinos.cn>

This is a cleanup that lets the compiler guarantee the mutex is
released on all exit paths, reducing the risk of accidental lock
leaks in future changes.  The same pattern is already used in
these files: azx_pcm_close() uses scoped_guard() for open_mutex,
and the rest of cs35l41 and cs35l56 drivers already use
guard()/scoped_guard() for their respective mutexes.

Most of the diff is indentation change from wrapping the code
inside scoped_guard() {}; the logical changes are minimal and
can be reviewed with git diff/show -w.

Verified with clang-17 W=1 build, no warnings.

In v1, some goto statements jumped out of the
scoped_guard() block, which is problematic because scoped_guard()
is implemented as a for-loop and such gotos change the control
flow in non-obvious ways.  This has caused real bugs before; see
commit a5c627d90809 ("gpio: adnp: fix flow control regression caused by scoped_guard()").
In v2, all goto targets are within the scoped_guard() block,
or replaced with break where the original label only served
to reach mutex_unlock.

No functional changes in any patch.

Changes in v2:
  - Patch 2: replaced 'goto err' (jumped out of scoped_guard to
    mutex_unlock) with 'goto err_powered_up' (stays within block),
    removed the 'err' label.
  - Patch 3: replaced 'goto unlock' (jumped out of scoped_guard)
    with 'break' (exits the scoped_guard for-loop naturally), and
    removed the now-unused 'unlock' label.

wangdicheng (3):
  ALSA: hda/cs35l41: Use scoped_guard() for fw_mutex in
    cs35l41_hda_bind()
  ALSA: hda/cs35l56: Use scoped_guard() for irq_lock in
    cs35l56_hda_fw_load()
  ALSA: hda: Use scoped_guard() for open_mutex in azx_pcm_open()

 sound/hda/codecs/side-codecs/cs35l41_hda.c |  57 ++++----
 sound/hda/codecs/side-codecs/cs35l56_hda.c | 104 +++++++-------
 sound/hda/common/controller.c              | 158 +++++++++++----------
 3 files changed, 160 insertions(+), 159 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] ALSA: hda/cs35l41: Use scoped_guard() for fw_mutex in cs35l41_hda_bind()

[wangdich9700]

From: wangdicheng <wangdicheng@kylinos.cn>

The rest of the driver already uses guard()/scoped_guard() for
fw_mutex. Replace the manual mutex_lock/mutex_unlock pair in
cs35l41_hda_bind() with scoped_guard() for consistency.

No functional changes.

Signed-off-by: wangdicheng <wangdicheng@kylinos.cn>
---
 sound/hda/codecs/side-codecs/cs35l41_hda.c | 57 +++++++++++-----------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/sound/hda/codecs/side-codecs/cs35l41_hda.c b/sound/hda/codecs/side-codecs/cs35l41_hda.c
index 64a5bd895fd1..818ea8e3f8d6 100644
--- a/sound/hda/codecs/side-codecs/cs35l41_hda.c
+++ b/sound/hda/codecs/side-codecs/cs35l41_hda.c
@@ -1482,42 +1482,41 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas
 
 	guard(pm_runtime_active_auto)(dev);
 
-	mutex_lock(&cs35l41->fw_mutex);
-
-	comp->dev = dev;
-	cs35l41->codec = parent->codec;
-	if (!cs35l41->acpi_subsystem_id)
-		cs35l41->acpi_subsystem_id = kasprintf(GFP_KERNEL, "%.8x",
-						       cs35l41->codec->core.subsystem_id);
-
-	strscpy(comp->name, dev_name(dev), sizeof(comp->name));
+	scoped_guard(mutex, &cs35l41->fw_mutex) {
+		comp->dev = dev;
+		cs35l41->codec = parent->codec;
+		if (!cs35l41->acpi_subsystem_id)
+			cs35l41->acpi_subsystem_id = kasprintf(GFP_KERNEL, "%.8x",
+							       cs35l41->codec->core.subsystem_id);
 
-	cs35l41->firmware_type = CS35L41_HDA_FW_SPK_PROT;
+		strscpy(comp->name, dev_name(dev), sizeof(comp->name));
 
-	if (firmware_autostart) {
-		dev_dbg(cs35l41->dev, "Firmware Autostart.\n");
-		cs35l41->request_fw_load = true;
-		if (cs35l41_smart_amp(cs35l41) < 0)
-			dev_warn(cs35l41->dev, "Cannot Run Firmware, reverting to dsp bypass...\n");
-	} else {
-		dev_dbg(cs35l41->dev, "Firmware Autostart is disabled.\n");
-	}
+		cs35l41->firmware_type = CS35L41_HDA_FW_SPK_PROT;
 
-	ret = cs35l41_create_controls(cs35l41);
+		if (firmware_autostart) {
+			dev_dbg(cs35l41->dev, "Firmware Autostart.\n");
+			cs35l41->request_fw_load = true;
+			if (cs35l41_smart_amp(cs35l41) < 0)
+				dev_warn(cs35l41->dev, "Cannot Run Firmware, reverting to dsp bypass...\n");
+		} else {
+			dev_dbg(cs35l41->dev, "Firmware Autostart is disabled.\n");
+		}
 
-	comp->playback_hook = cs35l41_hda_playback_hook;
-	comp->pre_playback_hook = cs35l41_hda_pre_playback_hook;
-	comp->post_playback_hook = cs35l41_hda_post_playback_hook;
-	comp->acpi_notify = cs35l41_acpi_device_notify;
-	comp->adev = cs35l41->dacpi;
+		ret = cs35l41_create_controls(cs35l41);
 
-	comp->acpi_notifications_supported = cs35l41_dsm_supported(acpi_device_handle(comp->adev),
-		CS35L41_DSM_GET_MUTE);
+		comp->playback_hook = cs35l41_hda_playback_hook;
+		comp->pre_playback_hook = cs35l41_hda_pre_playback_hook;
+		comp->post_playback_hook = cs35l41_hda_post_playback_hook;
+		comp->acpi_notify = cs35l41_acpi_device_notify;
+		comp->adev = cs35l41->dacpi;
 
-	cs35l41->mute_override = cs35l41_get_acpi_mute_state(cs35l41,
-						acpi_device_handle(cs35l41->dacpi)) > 0;
+		comp->acpi_notifications_supported =
+			cs35l41_dsm_supported(acpi_device_handle(comp->adev),
+					      CS35L41_DSM_GET_MUTE);
 
-	mutex_unlock(&cs35l41->fw_mutex);
+		cs35l41->mute_override = cs35l41_get_acpi_mute_state(cs35l41,
+							acpi_device_handle(cs35l41->dacpi)) > 0;
+	}
 
 	sleep_flags = lock_system_sleep();
 	if (!device_link_add(&cs35l41->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS))
-- 
2.25.1

[PATCH v2 2/3] ALSA: hda/cs35l56: Use scoped_guard() for irq_lock in cs35l56_hda_fw_load()

[wangdich9700]

From: wangdicheng <wangdicheng@kylinos.cn>

Replace the manual mutex_lock/mutex_unlock of irq_lock with
scoped_guard(). This lets the compiler guarantee the lock is
released on all exit paths, eliminating the err label that
previously existed only to reach the unlock call.

No functional changes.

Signed-off-by: wangdicheng <wangdicheng@kylinos.cn>
---
 sound/hda/codecs/side-codecs/cs35l56_hda.c | 104 ++++++++++-----------
 1 file changed, 51 insertions(+), 53 deletions(-)

diff --git a/sound/hda/codecs/side-codecs/cs35l56_hda.c b/sound/hda/codecs/side-codecs/cs35l56_hda.c
index a0ea08eb96a9..4f74397ea06f 100644
--- a/sound/hda/codecs/side-codecs/cs35l56_hda.c
+++ b/sound/hda/codecs/side-codecs/cs35l56_hda.c
@@ -625,74 +625,72 @@ static void cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
 		goto err_fw_release;
 	}
 
-	mutex_lock(&cs35l56->base.irq_lock);
+	scoped_guard(mutex, &cs35l56->base.irq_lock) {
+		/*
+		 * If the firmware hasn't been patched it must be shutdown before
+		 * doing a full patch and reset afterwards. If it is already
+		 * running a patched version the firmware files only contain
+		 * tunings and we can use the lower cost reinit sequence instead.
+		 */
+		if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
+			ret = cs35l56_firmware_shutdown(&cs35l56->base);
+			if (ret)
+				goto err_powered_up;
+		}
 
-	/*
-	 * If the firmware hasn't been patched it must be shutdown before
-	 * doing a full patch and reset afterwards. If it is already
-	 * running a patched version the firmware files only contain
-	 * tunings and we can use the lower cost reinit sequence instead.
-	 */
-	if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
-		ret = cs35l56_firmware_shutdown(&cs35l56->base);
-		if (ret)
-			goto err;
-	}
+		ret = cs_dsp_power_up(&cs35l56->cs_dsp, wmfw_firmware, wmfw_filename,
+				      coeff_firmware, coeff_filename, "misc");
+		if (ret) {
+			dev_dbg(cs35l56->base.dev, "%s: cs_dsp_power_up ret %d\n", __func__, ret);
+			goto err_powered_up;
+		}
 
-	ret = cs_dsp_power_up(&cs35l56->cs_dsp, wmfw_firmware, wmfw_filename,
-			      coeff_firmware, coeff_filename, "misc");
-	if (ret) {
-		dev_dbg(cs35l56->base.dev, "%s: cs_dsp_power_up ret %d\n", __func__, ret);
-		goto err;
-	}
+		if (wmfw_filename)
+			dev_dbg(cs35l56->base.dev, "Loaded WMFW Firmware: %s\n", wmfw_filename);
+
+		if (coeff_filename)
+			dev_dbg(cs35l56->base.dev, "Loaded Coefficients: %s\n", coeff_filename);
 
-	if (wmfw_filename)
-		dev_dbg(cs35l56->base.dev, "Loaded WMFW Firmware: %s\n", wmfw_filename);
+		/* If we downloaded firmware, reset the device and wait for it to boot */
+		if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
+			cs35l56_system_reset(&cs35l56->base, false);
+			regcache_mark_dirty(cs35l56->base.regmap);
+			ret = cs35l56_wait_for_firmware_boot(&cs35l56->base);
+			if (ret)
+				goto err_powered_up;
 
-	if (coeff_filename)
-		dev_dbg(cs35l56->base.dev, "Loaded Coefficients: %s\n", coeff_filename);
+			regcache_cache_only(cs35l56->base.regmap, false);
+		}
 
-	/* If we downloaded firmware, reset the device and wait for it to boot */
-	if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
-		cs35l56_system_reset(&cs35l56->base, false);
-		regcache_mark_dirty(cs35l56->base.regmap);
-		ret = cs35l56_wait_for_firmware_boot(&cs35l56->base);
+		/* Disable auto-hibernate so that runtime_pm has control */
+		ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE);
 		if (ret)
 			goto err_powered_up;
 
-		regcache_cache_only(cs35l56->base.regmap, false);
-	}
+		regcache_sync(cs35l56->base.regmap);
 
-	/* Disable auto-hibernate so that runtime_pm has control */
-	ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE);
-	if (ret)
-		goto err_powered_up;
-
-	regcache_sync(cs35l56->base.regmap);
-
-	regmap_clear_bits(cs35l56->base.regmap,
-			  cs35l56->base.fw_reg->prot_sts,
-			  CS35L56_FIRMWARE_MISSING);
-	cs35l56->base.fw_patched = true;
+		regmap_clear_bits(cs35l56->base.regmap,
+				  cs35l56->base.fw_reg->prot_sts,
+				  CS35L56_FIRMWARE_MISSING);
+		cs35l56->base.fw_patched = true;
 
-	ret = cs_dsp_run(&cs35l56->cs_dsp);
-	if (ret)
-		dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
+		ret = cs_dsp_run(&cs35l56->cs_dsp);
+		if (ret)
+			dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
 
-	/* Don't need to check return code, it's not fatal if this fails */
-	cs35l56_hda_apply_calibration(cs35l56);
+		/* Don't need to check return code, it's not fatal if this fails */
+		cs35l56_hda_apply_calibration(cs35l56);
 
-	ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT);
-	if (ret)
-		cs_dsp_stop(&cs35l56->cs_dsp);
+		ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT);
+		if (ret)
+			cs_dsp_stop(&cs35l56->cs_dsp);
 
-	cs35l56_log_tuning(&cs35l56->base, &cs35l56->cs_dsp);
+		cs35l56_log_tuning(&cs35l56->base, &cs35l56->cs_dsp);
 
 err_powered_up:
-	if (!cs35l56->base.fw_patched)
-		cs_dsp_power_down(&cs35l56->cs_dsp);
-err:
-	mutex_unlock(&cs35l56->base.irq_lock);
+		if (!cs35l56->base.fw_patched)
+			cs_dsp_power_down(&cs35l56->cs_dsp);
+	}
 err_fw_release:
 	cs35l56_hda_release_firmware_files(wmfw_firmware, wmfw_filename,
 					   coeff_firmware, coeff_filename);
-- 
2.25.1

Re: [PATCH v2 2/3] ALSA: hda/cs35l56: Use scoped_guard() for irq_lock in cs35l56_hda_fw_load()

[Takashi Iwai]

On Fri, 10 Jul 2026 10:40:08 +0200,
wangdich9700@163.com wrote:
> 
> From: wangdicheng <wangdicheng@kylinos.cn>
> 
> Replace the manual mutex_lock/mutex_unlock of irq_lock with
> scoped_guard(). This lets the compiler guarantee the lock is
> released on all exit paths, eliminating the err label that
> previously existed only to reach the unlock call.

The combination of gotos in different scopes have risks, so I'm not
thrilled by this change.  As you noticed, the locking scheme there is
complex, hence just replacing with guard() doesn't improve the
readability well.  If any, better to re-design the code and locking.


thanks,

Takashi

> 
> No functional changes.
> 
> Signed-off-by: wangdicheng <wangdicheng@kylinos.cn>
> ---
>  sound/hda/codecs/side-codecs/cs35l56_hda.c | 104 ++++++++++-----------
>  1 file changed, 51 insertions(+), 53 deletions(-)
> 
> diff --git a/sound/hda/codecs/side-codecs/cs35l56_hda.c b/sound/hda/codecs/side-codecs/cs35l56_hda.c
> index a0ea08eb96a9..4f74397ea06f 100644
> --- a/sound/hda/codecs/side-codecs/cs35l56_hda.c
> +++ b/sound/hda/codecs/side-codecs/cs35l56_hda.c
> @@ -625,74 +625,72 @@ static void cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
>  		goto err_fw_release;
>  	}
>  
> -	mutex_lock(&cs35l56->base.irq_lock);
> +	scoped_guard(mutex, &cs35l56->base.irq_lock) {
> +		/*
> +		 * If the firmware hasn't been patched it must be shutdown before
> +		 * doing a full patch and reset afterwards. If it is already
> +		 * running a patched version the firmware files only contain
> +		 * tunings and we can use the lower cost reinit sequence instead.
> +		 */
> +		if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
> +			ret = cs35l56_firmware_shutdown(&cs35l56->base);
> +			if (ret)
> +				goto err_powered_up;
> +		}
>  
> -	/*
> -	 * If the firmware hasn't been patched it must be shutdown before
> -	 * doing a full patch and reset afterwards. If it is already
> -	 * running a patched version the firmware files only contain
> -	 * tunings and we can use the lower cost reinit sequence instead.
> -	 */
> -	if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
> -		ret = cs35l56_firmware_shutdown(&cs35l56->base);
> -		if (ret)
> -			goto err;
> -	}
> +		ret = cs_dsp_power_up(&cs35l56->cs_dsp, wmfw_firmware, wmfw_filename,
> +				      coeff_firmware, coeff_filename, "misc");
> +		if (ret) {
> +			dev_dbg(cs35l56->base.dev, "%s: cs_dsp_power_up ret %d\n", __func__, ret);
> +			goto err_powered_up;
> +		}
>  
> -	ret = cs_dsp_power_up(&cs35l56->cs_dsp, wmfw_firmware, wmfw_filename,
> -			      coeff_firmware, coeff_filename, "misc");
> -	if (ret) {
> -		dev_dbg(cs35l56->base.dev, "%s: cs_dsp_power_up ret %d\n", __func__, ret);
> -		goto err;
> -	}
> +		if (wmfw_filename)
> +			dev_dbg(cs35l56->base.dev, "Loaded WMFW Firmware: %s\n", wmfw_filename);
> +
> +		if (coeff_filename)
> +			dev_dbg(cs35l56->base.dev, "Loaded Coefficients: %s\n", coeff_filename);
>  
> -	if (wmfw_filename)
> -		dev_dbg(cs35l56->base.dev, "Loaded WMFW Firmware: %s\n", wmfw_filename);
> +		/* If we downloaded firmware, reset the device and wait for it to boot */
> +		if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
> +			cs35l56_system_reset(&cs35l56->base, false);
> +			regcache_mark_dirty(cs35l56->base.regmap);
> +			ret = cs35l56_wait_for_firmware_boot(&cs35l56->base);
> +			if (ret)
> +				goto err_powered_up;
>  
> -	if (coeff_filename)
> -		dev_dbg(cs35l56->base.dev, "Loaded Coefficients: %s\n", coeff_filename);
> +			regcache_cache_only(cs35l56->base.regmap, false);
> +		}
>  
> -	/* If we downloaded firmware, reset the device and wait for it to boot */
> -	if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
> -		cs35l56_system_reset(&cs35l56->base, false);
> -		regcache_mark_dirty(cs35l56->base.regmap);
> -		ret = cs35l56_wait_for_firmware_boot(&cs35l56->base);
> +		/* Disable auto-hibernate so that runtime_pm has control */
> +		ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE);
>  		if (ret)
>  			goto err_powered_up;
>  
> -		regcache_cache_only(cs35l56->base.regmap, false);
> -	}
> +		regcache_sync(cs35l56->base.regmap);
>  
> -	/* Disable auto-hibernate so that runtime_pm has control */
> -	ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE);
> -	if (ret)
> -		goto err_powered_up;
> -
> -	regcache_sync(cs35l56->base.regmap);
> -
> -	regmap_clear_bits(cs35l56->base.regmap,
> -			  cs35l56->base.fw_reg->prot_sts,
> -			  CS35L56_FIRMWARE_MISSING);
> -	cs35l56->base.fw_patched = true;
> +		regmap_clear_bits(cs35l56->base.regmap,
> +				  cs35l56->base.fw_reg->prot_sts,
> +				  CS35L56_FIRMWARE_MISSING);
> +		cs35l56->base.fw_patched = true;
>  
> -	ret = cs_dsp_run(&cs35l56->cs_dsp);
> -	if (ret)
> -		dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
> +		ret = cs_dsp_run(&cs35l56->cs_dsp);
> +		if (ret)
> +			dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
>  
> -	/* Don't need to check return code, it's not fatal if this fails */
> -	cs35l56_hda_apply_calibration(cs35l56);
> +		/* Don't need to check return code, it's not fatal if this fails */
> +		cs35l56_hda_apply_calibration(cs35l56);
>  
> -	ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT);
> -	if (ret)
> -		cs_dsp_stop(&cs35l56->cs_dsp);
> +		ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT);
> +		if (ret)
> +			cs_dsp_stop(&cs35l56->cs_dsp);
>  
> -	cs35l56_log_tuning(&cs35l56->base, &cs35l56->cs_dsp);
> +		cs35l56_log_tuning(&cs35l56->base, &cs35l56->cs_dsp);
>  
>  err_powered_up:
> -	if (!cs35l56->base.fw_patched)
> -		cs_dsp_power_down(&cs35l56->cs_dsp);
> -err:
> -	mutex_unlock(&cs35l56->base.irq_lock);
> +		if (!cs35l56->base.fw_patched)
> +			cs_dsp_power_down(&cs35l56->cs_dsp);
> +	}
>  err_fw_release:
>  	cs35l56_hda_release_firmware_files(wmfw_firmware, wmfw_filename,
>  					   coeff_firmware, coeff_filename);
> -- 
> 2.25.1
> 

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

[wangdich9700]

From: wangdicheng <wangdicheng@kylinos.cn>

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@kylinos.cn>
---
 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

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@163.com wrote:
> 
> From: wangdicheng <wangdicheng@kylinos.cn>
> 
> 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@kylinos.cn>

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
>