Re: [PATCH 03/11] ASoC: fsl_easrc: Use guard() for spin locks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The success path of `fsl_easrc_runtime_resume` falls through to the error cleanup label, incorrectly disabling the device clock on success.
- [Medium] Mixing goto-based error handling with scope-based cleanup helpers (`scoped_guard`) in the same function violates subsystem rules.

Pre-existing issues:
- [High] The `firmware_loaded` flag is prematurely set to 1 before firmware loading succeeds, leading to a NULL pointer dereference on subsequent resumes if loading fails.
--

commit 4528b201cb9e89015d267dc100f8b7c044c41e30
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: fsl_easrc: Use guard() for spin locks

Clean up the code using guard() for spin locks.
Merely code refactoring, and no behavior change.

> diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
> index 114a6c0b6b730..edfd943197a00 100644
> --- a/sound/soc/fsl/fsl_easrc.c
> +++ b/sound/soc/fsl/fsl_easrc.c
[ ... ]
> @@ -2323,13 +2312,11 @@ static int fsl_easrc_runtime_resume(struct device *dev)
>  	regcache_mark_dirty(easrc->regmap);
>  	regcache_sync(easrc->regmap);
>  
> -	spin_lock_irqsave(&easrc->lock, lock_flags);
> -	if (easrc_priv->firmware_loaded) {
> -		spin_unlock_irqrestore(&easrc->lock, lock_flags);
> -		goto skip_load;
> +	scoped_guard(spinlock_irqsave, &easrc->lock) {
> +		if (easrc_priv->firmware_loaded)
> +			return 0;
> +		easrc_priv->firmware_loaded = 1;

[Severity: High]
This is a pre-existing issue, but does setting firmware_loaded to 1 here cause
a NULL pointer dereference on subsequent resumes if fsl_easrc_get_firmware()
fails?

If the firmware load fails, it returns an error but leaves firmware_loaded
set to 1.

On the next resume attempt, this function will see firmware_loaded is 1, return
success immediately, and bypass initialization. This causes
fsl_easrc_prefilter_config() to dereference a NULL
easrc_priv->firmware_hdr->prefil_scen pointer.

>  	}
> -	easrc_priv->firmware_loaded = 1;
> -	spin_unlock_irqrestore(&easrc->lock, lock_flags);
>  
>  	ret = fsl_easrc_get_firmware(easrc);
>  	if (ret) {
>  		dev_err(dev, "failed to get firmware\n");
>  		goto disable_mem_clk;

[Severity: Medium]
Does mixing goto-based error handling with scope-based cleanup helpers like
scoped_guard() in the same function violate the cleanup subsystem rules?

The expectation in include/linux/cleanup.h is that functions convert all
resources to scope-based cleanup or none of them.

[ ... ]
> @@ -2377,9 +2364,6 @@ static int fsl_easrc_runtime_resume(struct device *dev)
>  			goto disable_mem_clk;
>  	}
>  
> -skip_load:
> -	return 0;
> -
>  disable_mem_clk:
>  	clk_disable_unprepare(easrc->mem_clk);
>  	return ret;

[Severity: High]
Since the return 0 on the success path was removed, does the success path in
fsl_easrc_runtime_resume() now fall through into the disable_mem_clk label?

It appears this will incorrectly disable the device clock when the firmware
initialization successfully completes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612132639.78086-1-phucduc.bui@gmail.com?part=3