Re: [PATCH 06/21] soundwire: intel_ace2x: Protect bus.bpt_stream with bus_lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] soundwire: mix goto and scoped_guard() in intel_ace2x_bpt_open_stream()
--

commit c2ba69ef3a3456c129416a89de7531b53eb520ca
Author: Lee Jones <lee@kernel.org>

soundwire: intel_ace2x: Protect bus.bpt_stream with bus_lock

This commit protects checks, assignments, and clearing of cdns->bus.bpt_stream
with cdns->bus.bus_lock in intel_ace2x_bpt_open_stream() and
intel_ace2x_bpt_close_stream(), and ensures both master and slave stream
runtimes are removed on open failure.

> diff --git a/drivers/soundwire/intel_ace2x.c b/drivers/soundwire/intel_ace2x.c
> index 642b33ab55260..3623fc9f4451f 100644
> --- a/drivers/soundwire/intel_ace2x.c
> +++ b/drivers/soundwire/intel_ace2x.c
> @@ -83,16 +83,18 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
>  	int len;
>  	int i;
>  
> -	if (cdns->bus.bpt_stream) {
> -		dev_err(cdns->dev, "%s: BPT stream already exists\n", __func__);
> -		return -EAGAIN;
> -	}
> +	scoped_guard(mutex, &cdns->bus.bus_lock) {
> +		if (cdns->bus.bpt_stream) {
> +			dev_err(cdns->dev, "%s: BPT stream already exists\n", __func__);
> +			return -EAGAIN;
> +		}
>  
> -	stream = sdw_alloc_stream("BPT", SDW_STREAM_BPT);
> -	if (!stream)
> -		return -ENOMEM;
> +		stream = sdw_alloc_stream("BPT", SDW_STREAM_BPT);
> +		if (!stream)
> +			return -ENOMEM;
>  
> -	cdns->bus.bpt_stream = stream;
> +		cdns->bus.bpt_stream = stream;
> +	}
>  
>  	ret = sdw_slave_bpt_stream_add(slave, stream);
>  	if (ret < 0)

[ ... ]

> @@ -293,15 +295,16 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
>  		dev_err(cdns->dev, "%s: remove master failed: %d\n",
>  			__func__, ret1);
>  
> -remove_slave:
>  	ret1 = sdw_stream_remove_slave(slave, cdns->bus.bpt_stream);
>  	if (ret1 < 0)
>  		dev_err(cdns->dev, "%s: remove slave failed: %d\n",
>  			__func__, ret1);
>  
>  release_stream:
> -	sdw_release_stream(cdns->bus.bpt_stream);
> -	cdns->bus.bpt_stream = NULL;
> +	scoped_guard(mutex, &cdns->bus.bus_lock) {
> +		sdw_release_stream(cdns->bus.bpt_stream);
> +		cdns->bus.bpt_stream = NULL;
> +	}

[Severity: Low]
This isn't a bug, but intel_ace2x_bpt_open_stream() relies on multi-label goto
error unwinding (goto release_stream, goto remove_master, and goto
deprepare_stream) while this patch introduces scoped_guard(mutex,
&cdns->bus.bus_lock) blocks at the start of the function and under the
release_stream label.

Since kernel cleanup API conventions documented in include/linux/cleanup.h
discourage mixing goto-based error handling with scope-based cleanup helpers
in the same function, would it be better to use explicit mutex_lock() and
mutex_unlock() on cdns->bus.bus_lock here instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030104840.3361217-1-lee@kernel.org?part=6