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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch erroneously removes `return 0;` from the success path in `fsl_easrc_runtime_resume()`, causing it to unconditionally fall through to the error handler and disable the memory clock.
- [Medium] The function `fsl_easrc_runtime_resume` improperly mixes `goto`-based error handling with scope-based cleanup helpers (`scoped_guard`).
--

commit d28002047081c21222edc1134f1efc00e25fe426
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: Medium]
Does this code improperly mix goto-based error handling with scope-based
cleanup helpers?

The function fsl_easrc_runtime_resume() now uses scoped_guard() but still
contains goto disable_mem_clk error handling further down.

According to the cleanup subsystem architecture rules, for a given routine,
all resources needing cleanup should be converted to scope-based cleanup, or
none of them should be. Mixing goto and scoped guards can lead to confusing
ownership semantics.

> -	easrc_priv->firmware_loaded = 1;
> -	spin_unlock_irqrestore(&easrc->lock, lock_flags);
>  
>  	ret = fsl_easrc_get_firmware(easrc);
>  	if (ret) {
[ ... ]
> @@ -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]
Does removing this return statement cause a normal success path to fall
through to the error handler?

By removing the skip_load label and its return 0 statement, the normal
success path of fsl_easrc_runtime_resume() directly falls through to the
disable_mem_clk block.

This incorrectly disables the memory clock while returning 0 to indicate
success, potentially causing bus faults on subsequent register accesses.

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