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@xxxxxxx wrote:
> 
> From: wangdicheng <wangdicheng@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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
>