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

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

[wangdich9700]

From: wangdicheng <wangdicheng@kylinos.cn>

Replace the remaining manual mutex_lock/mutex_unlock pairs in the
HD-Audio subsystem with scoped_guard(), which provides automatic
lock release through RAII-style scope management.

No functional changes in any patch.

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              | 159 +++++++++++----------
 3 files changed, 161 insertions(+), 159 deletions(-)

-- 
2.25.1

[PATCH 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 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..2d9d75053293 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_fw_release;
+		}
 
-	/*
-	 * 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_fw_release;
+		}
 
-	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);
-	}
-
-	/* 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);
+		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 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.

No functional changes.

Signed-off-by: wangdicheng <wangdicheng@kylinos.cn>
---
 sound/hda/common/controller.c | 159 ++++++++++++++++++----------------
 1 file changed, 82 insertions(+), 77 deletions(-)

diff --git a/sound/hda/common/controller.c b/sound/hda/common/controller.c
index afec5c5546ec..2c9aaf74ce7f 100644
--- a/sound/hda/common/controller.c
+++ b/sound/hda/common/controller.c
@@ -584,86 +584,91 @@ 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;
+			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;
+		}
 
-	/* 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);
+	}
+unlock:
 	snd_hda_codec_pcm_put(apcm->info);
 	return err;
 }
-- 
2.25.1

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

[Richard Fitzgerald]

On 01/07/2026 7:51 am, 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.
> 
> No functional changes.
> 
> Signed-off-by: wangdicheng <wangdicheng@kylinos.cn>
> ---

Is this fixing any bugs?

This is a lot of code churn to fix nothing.

>   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..2d9d75053293 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_fw_release;
> +		}
>   
> -	/*
> -	 * 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_fw_release;
> +		}
>   
> -	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);
> -	}
> -
> -	/* 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);
> +		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);

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

[Takashi Iwai]

On Wed, 01 Jul 2026 11:46:18 +0200,
Richard Fitzgerald wrote:
> 
> On 01/07/2026 7:51 am, 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.
> > 
> > No functional changes.
> > 
> > Signed-off-by: wangdicheng <wangdicheng@kylinos.cn>
> > ---
> 
> Is this fixing any bugs?
> 
> This is a lot of code churn to fix nothing.

IMO, as a cleanup, such a change *may* be acceptable.  Dropping the
manual locking calls can reduce the mistakes and makes the code better
maintained.

Most of changes are due to the indentation, and you can ignore them
via -w option of git-diff or git-show.  (It's difficult to do it in
the review of manual patches, though.)

But, one thing still needs to be clarified: such changes must be
verified with clang W=1 build, at least, too; the combination of
guard() and goto aren't always good.


thanks,

Takashi

> 
> >   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..2d9d75053293 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_fw_release;
> > +		}
> >   -	/*
> > -	 * 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_fw_release;
> > +		}
> >   -	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);
> > -	}
> > -
> > -	/* 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);
> > +		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);
> 

[PATCH 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..2d9d75053293 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_fw_release;
+		}
 
-	/*
-	 * 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_fw_release;
+		}
 
-	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);
-	}
-
-	/* 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);
+		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