Re: [PATCH v2 04/10] soundwire: stream: Clean up allocated runtime resources on config failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] soundwire: stream: unrolled stream params on error in sdw_stream_add_slave()
--

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

soundwire: stream: Clean up allocated runtime resources on config failure

This commit updates sdw_stream_add_master(), sdw_stream_add_slave(), and
sdw_slave_port_alloc() in drivers/soundwire/stream.c to release newly
allocated port and runtime structures when stream or port configuration
fails.

> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index db62a68c09f84..e3cdd36be1a1f 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c

[ ... ]

> @@ -2198,24 +2212,26 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
>  		ret = sdw_slave_port_alloc(slave, s_rt, num_ports);
>  		if (ret)
>  			goto alloc_error;
> +
> +		alloc_slave_port = true;
>  	}
>  
>  	ret =  sdw_master_rt_config(m_rt, stream_config);
>  	if (ret)
> -		goto unlock;
> +		goto alloc_error;
>  
>  	ret = sdw_slave_rt_config(s_rt, stream_config);
>  	if (ret)
> -		goto unlock;
> +		goto alloc_error;
>  
>  	ret = sdw_config_stream(&slave->dev, stream, stream_config, true);
>  	if (ret)
> -		goto unlock;
> +		goto alloc_error;
>  
>  	ret = sdw_slave_port_config(slave, s_rt, port_config,
>  				    stream->type == SDW_STREAM_BPT);
>  	if (ret)
> -		goto unlock;
> +		goto alloc_error;
>  
>  	/*
>  	 * Change stream state to CONFIGURED on first Slave add.
> @@ -2232,6 +2248,8 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
>  	 * is intentional, the 'master_rt_free' will call sdw_slave_rt_free()
>  	 * internally.
>  	 */
> +	if (alloc_slave_port)
> +		sdw_slave_port_free(slave, stream);
>  	if (alloc_master_rt)
>  		sdw_master_rt_free(m_rt, stream);
>  	else if (alloc_slave_rt)

[Severity: Medium]
If sdw_slave_port_config() fails in sdw_stream_add_slave() (for example,
when sdw_slave_port_is_valid_range() or sdw_get_slave_dpn_prop() returns
-EINVAL, or when port_config[i].num is non-zero on a BPT stream), does
jumping to alloc_error leave stream->params and any pre-existing m_rt or
s_rt modified?

Before sdw_slave_port_config() runs, sdw_config_stream() in
drivers/soundwire/stream.c has already updated the stream parameters:

	stream->type = stream_config->type;
	stream->params.rate = stream_config->frame_rate;
	stream->params.bps = stream_config->bps;

	/* TODO: Update this check during Device-device support */
	if (is_slave)
		stream->params.ch_count += stream_config->ch_count;

And sdw_master_rt_config() and sdw_slave_rt_config() have already updated
ch_count and direction on m_rt and s_rt.

When sdw_slave_port_config() fails and jumps to alloc_error, the newly
allocated ports and runtime structures are freed so the slave is no longer
attached to the stream, but stream->params.ch_count remains incremented by
stream_config->ch_count and stream->params.rate, stream->params.bps, and
stream->type remain set to the failed configuration. Could
sdw_slave_port_config() (or its validation checks) be performed before
mutating stream->params, m_rt, and s_rt, or should those state changes be
rolled back under alloc_error?

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