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

[Lee Jones <lee@kernel.org>]

On Tue, 03 Nov 2026, sashiko-bot@kernel.org wrote:

> 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()

Well that is frustrating!  This passes local Sashiko review.

> --
> 
> 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

-- 
Lee Jones