Re: [PATCH v2 5/5] soundwire: stream: Fix bandwidth leak in multi-link aggregation failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] soundwire: incomplete port lane and transport rollback in
  _sdw_prepare_stream()
- [High] soundwire: stream: bank state desync in _sdw_prepare_stream()
  rollback
--

commit f77415deadf5285d82da231d58b3652a2ff280ce
Author: David Lin <david.lin@intel.com>

soundwire: stream: Fix bandwidth leak in multi-link aggregation failure

This commit snapshots bus->params into m_rt->params_backup for each master
before stream preparation and restores them on failure, while clearing
port lane assignments in sdw_compute_bus_params() when preparing on lane 0.

> diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
> index 9bacde2c67a11..214a1be6c33e0 100644
> --- a/drivers/soundwire/generic_bandwidth_allocation.c
> +++ b/drivers/soundwire/generic_bandwidth_allocation.c
> @@ -656,6 +656,20 @@ static int sdw_compute_bus_params(struct sdw_bus *bus,
>  		list_for_each_entry(m_p_rt, &m_rt->port_list, port_node) {
>  			m_p_rt->lane = m_lane;
>  		}
> +	} else if (prepare) {
> +		/* Clear stale lane assignments when preparing on lane 0. */
> +		list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
> +			if (m_rt->stream != stream)
> +				continue;
> +
> +			list_for_each_entry(m_p_rt, &m_rt->port_list, port_node)
> +				m_p_rt->lane = 0;
> +
> +			list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node)
> +				list_for_each_entry(s_p_rt, &s_rt->port_list, port_node)
> +					s_p_rt->lane = 0;
> +			break;
> +		}
>  	}

[Severity: High]
Does clearing m_p_rt->lane and s_p_rt->lane only for the stream currently
being prepared leave stale non-zero lanes on a rolled-back stream when
another stream on the same bus is later prepared or deprepared?

When _sdw_prepare_stream() fails on a later link after bus->compute_params()
has selected m_lane > 0 and recomputed port transport parameters on an
earlier link, restore_params reverts bus->params (including
lane_used_bandwidth[] and curr_dr_freq) while the failed stream remains in
SDW_STREAM_CONFIGURED with its non-zero lane assignments.  If a second
stream on that bus is then prepared or deprepared before the failed stream
is re-prepared on lane 0, sdw_compute_group_params() excludes the
SDW_STREAM_CONFIGURED stream from payload_bw and hwidth, but
_sdw_compute_port_params() still includes it:

drivers/soundwire/generic_bandwidth_allocation.c:_sdw_compute_port_params() {
    ...
				if (!(prepare && m_rt->stream == stream) &&
				    (m_rt->stream->state > SDW_STREAM_DISABLED ||
				     m_rt->stream->state < SDW_STREAM_CONFIGURED))
					continue;
				sdw_compute_master_ports(m_rt, &params[i], &port_bo, hstop);
    ...
}

When an active stream uses that same non-zero lane and sample rate,
sdw_compute_master_ports() matches the failed stream's stale p_rt->lane and
advances port_bo without allocated hwidth, shifting active streams' transport
block offsets out of bounds.

In addition, bus->compute_params() on the preceding link has already updated
mstr_prop->default_col and recomputed p_rt->transport_params and
p_rt->port_params across existing streams on that bus; if an already
PREPARED or DISABLED stream on that link is later enabled or disabled,
sdw_program_params(bus, false) programs those un-reverted transport
parameters into hardware without recomputing them for the restored
bus->params.

> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 06f051f03a218..7c748ba847672 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1479,19 +1479,22 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
>  	struct sdw_master_runtime *m_rt;
>  	struct sdw_bus *bus;
>  	struct sdw_master_prop *prop;
> -	struct sdw_bus_params params;
>  	int ret;
>  
> +	/* Pre-pass: snapshot original bus parameters for all masters */
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> +		m_rt->params_backup = m_rt->bus->params;
> +
>  	/* Prepare  Master(s) and Slave(s) port(s) associated with stream */
>  	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>  		bus = m_rt->bus;
>  		prop = &bus->prop;
> -		memcpy(&params, &bus->params, sizeof(params));
>  
>  		/* TODO: Support Asynchronous mode */
>  		if ((prop->max_clk_freq % stream->params.rate) != 0) {
>  			dev_err(bus->dev, "Async mode not supported\n");
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			goto restore_params;
>  		}
>  
>  		if (update_params) {
> @@ -1542,7 +1545,13 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
>  	return ret;
>  
>  restore_params:
> -	memcpy(&bus->params, &params, sizeof(params));
> +	/*
> +	 * Revert all masters to their snapshotted original parameters.
> +	 * Any master that was not modified will safely revert to its own original state.
> +	 */
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> +		m_rt->bus->params = m_rt->params_backup;
> +
>  	return ret;
>  }

[Severity: High]
Can restoring m_rt->params_backup across all masters desynchronize
bus->params.curr_bank and bus->params.next_bank from hardware when
_sdw_prepare_stream() fails during or after do_bank_switch()?

m_rt->params_backup snapshots curr_bank and next_bank before
do_bank_switch() switches the active register bank in hardware and inverts
those fields.  If sdw_prep_deprep_ports(m_rt, true) subsequently fails on
any master (or if do_bank_switch() fails after an earlier master in
stream->master_list has already switched banks), jumping to restore_params
overwrites curr_bank and next_bank on every master with their pre-switch
values and leaves ports prepared on earlier masters in the loop without
depreparing them.  Later stream operations on those buses will then program
banked registers into the currently active hardware bank and attempt to
switch to the bank already in use.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112072105.1296515-1-yung-chuan.liao@linux.intel.com?part=5