[PATCH 0/2] soundwire: Fix bandwidth leak in multi-link aggregation failure

[PATCH 0/2] soundwire: Fix bandwidth leak in multi-link aggregation failure

[Bard Liao]

The existing code only copies the params from the first link to the
bus params, but it doesn't copy the bandwidth from the other links.
This can lead to a bandwidth leak when multi-link aggregation fails.
Fix this by saving a snapshot of each bus's parameters before making
any modifications.

Bard Liao (1):
  soundwire: move lane bandwidth into bus params

David Lin (1):
  soundwire: stream: Fix bandwidth leak in multi-link aggregation
    failure

 drivers/soundwire/bus.h                       |  3 +++
 .../soundwire/generic_bandwidth_allocation.c  |  8 +++----
 drivers/soundwire/stream.c                    | 21 +++++++++++++------
 include/linux/soundwire/sdw.h                 |  4 ++--
 4 files changed, 24 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH 1/2] soundwire: move lane bandwidth into bus params

[Bard Liao]

lane_used_bandwidth is used to store the used bandwidth for each lane.
When we restore the bus->params, the lane_used_bandwidth value will not
be restored in current code. Move it to sdw_bus_params to fix the issue.

Fixes: 366fd59fb894 ("soundwire: generic_bandwidth_allocation: add lane in sdw_group_params")
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: David Lin <david.lin@intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 drivers/soundwire/generic_bandwidth_allocation.c | 8 ++++----
 drivers/soundwire/stream.c                       | 4 ++--
 include/linux/soundwire/sdw.h                    | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
index 30a495abe19e..3d6b9c895e46 100644
--- a/drivers/soundwire/generic_bandwidth_allocation.c
+++ b/drivers/soundwire/generic_bandwidth_allocation.c
@@ -196,7 +196,7 @@ static void _sdw_compute_port_params(struct sdw_bus *bus,
 
 	/* Run loop for all groups to compute transport parameters */
 	for (l = 0; l < SDW_MAX_LANES; l++) {
-		if (l > 0 && !bus->lane_used_bandwidth[l])
+		if (l > 0 && !bus->params.lane_used_bandwidth[l])
 			continue;
 		/* reset hstop for each lane */
 		hstop = bus->params.col - 1;
@@ -267,7 +267,7 @@ static int sdw_compute_group_params(struct sdw_bus *bus,
 	}
 
 	for (l = 0; l < SDW_MAX_LANES; l++) {
-		if (l > 0 && !bus->lane_used_bandwidth[l])
+		if (l > 0 && !bus->params.lane_used_bandwidth[l])
 			continue;
 		/* reset column_needed for each lane */
 		column_needed = 0;
@@ -507,7 +507,7 @@ static int get_manager_lane(struct sdw_bus *bus, struct sdw_master_runtime *m_rt
 					      m_rt->stream->params.bps;
 		}
 		if (required_bandwidth <=
-		    curr_dr_freq - bus->lane_used_bandwidth[l]) {
+		    curr_dr_freq - bus->params.lane_used_bandwidth[l]) {
 			/* Check if m_lane is connected to all Peripherals */
 			if (!is_lane_connected_to_all_peripherals(m_rt,
 				slave_prop->lane_maps[l])) {
@@ -518,7 +518,7 @@ static int get_manager_lane(struct sdw_bus *bus, struct sdw_master_runtime *m_rt
 			}
 			m_lane = slave_prop->lane_maps[l];
 			dev_dbg(&s_rt->slave->dev, "M lane %d is used\n", m_lane);
-			bus->lane_used_bandwidth[l] += required_bandwidth;
+			bus->params.lane_used_bandwidth[l] += required_bandwidth;
 			/*
 			 * Use non-zero manager lane, subtract the lane 0
 			 * bandwidth that is already calculated
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 0b00fcdcab2d..96613dd0677c 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1792,8 +1792,8 @@ static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 			bandwidth = m_rt->stream->params.rate * hweight32(p_rt->ch_mask) *
 				    m_rt->stream->params.bps;
 			multi_lane_bandwidth += bandwidth;
-			bus->lane_used_bandwidth[p_rt->lane] -= bandwidth;
-			if (!bus->lane_used_bandwidth[p_rt->lane])
+			bus->params.lane_used_bandwidth[p_rt->lane] -= bandwidth;
+			if (!bus->params.lane_used_bandwidth[p_rt->lane])
 				p_rt->lane = 0;
 		}
 		/* TODO: Update this during Device-Device support */
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index f710e5932b4b..fff30bb2d9ed 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -592,6 +592,7 @@ enum sdw_port_prep_ops {
  * @bandwidth: Current bandwidth
  * @col: Active columns
  * @row: Active rows
+ * @lane_used_bandwidth: how much bandwidth in bits per second is used by each lane
  * @s_data_mode: NORMAL, STATIC or PRBS mode for all Slave ports
  * @m_data_mode: NORMAL, STATIC or PRBS mode for all Master ports. The value
  * should be the same to detect transmission issues, but can be different to
@@ -605,6 +606,7 @@ struct sdw_bus_params {
 	unsigned int bandwidth;
 	unsigned int col;
 	unsigned int row;
+	unsigned int lane_used_bandwidth[SDW_MAX_LANES];
 	int s_data_mode;
 	int m_data_mode;
 };
@@ -1028,7 +1030,6 @@ struct sdw_stream_runtime {
  * @multi_link: Store bus property that indicates if multi links
  * are supported. This flag is populated by drivers after reading
  * appropriate firmware (ACPI/DT).
- * @lane_used_bandwidth: how much bandwidth in bits per second is used by each lane
  */
 struct sdw_bus {
 	struct device *dev;
@@ -1063,7 +1064,6 @@ struct sdw_bus {
 	struct dentry *debugfs;
 #endif
 	bool multi_link;
-	unsigned int lane_used_bandwidth[SDW_MAX_LANES];
 };
 
 struct sdw_stream_runtime *sdw_alloc_stream(const char *stream_name, enum sdw_stream_type type);
-- 
2.43.0

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

[Bard Liao]

From: David Lin <david.lin@intel.com>

When preparing a multi-link aggregation stream, a failure on a subsequent
link during _sdw_prepare_stream() causes the stream preparation to fail.
However, the bandwidth allocated on the preceding successful links is never
released, causing a cumulative bandwidth leak.

Fix this by saving a snapshot of each bus's parameters before making any
modifications. In case of failure, we iterate through the stream's master
list and restore each bus to its original parameter state, ensuring an
atomic rollback and preventing any bandwidth leaks.

Signed-off-by: David Lin <david.lin@intel.com>
Co-developed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 drivers/soundwire/bus.h    |  3 +++
 drivers/soundwire/stream.c | 17 +++++++++++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 44e4f5193917..476387b70472 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -155,6 +155,8 @@ struct sdw_slave_runtime {
  *
  * @bus: Bus handle
  * @stream: Stream runtime handle
+ * @params_backup: Snapshot of original bus parameters saved before stream
+ * preparation, used for rollback recovery on failure.
  * @direction: Data direction for Master
  * @ch_count: Number of channels handled by the Master for
  * this stream, can be zero.
@@ -166,6 +168,7 @@ struct sdw_slave_runtime {
 struct sdw_master_runtime {
 	struct sdw_bus *bus;
 	struct sdw_stream_runtime *stream;
+	struct sdw_bus_params params_backup;
 	enum sdw_data_direction direction;
 	unsigned int ch_count;
 	struct list_head slave_rt_list;
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 96613dd0677c..86d951cc92f7 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1477,19 +1477,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) {
@@ -1540,7 +1543,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;
 }
 
-- 
2.43.0

Re: [PATCH 1/2] soundwire: move lane bandwidth into bus params

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: unrestored port lanes on error in _sdw_prepare_stream()
--

commit bc8dc54bdb852b57939246f45a1d5ab8025ee456
Author: Bard Liao <yung-chuan.liao@linux.intel.com>

soundwire: move lane bandwidth into bus params

This commit moves the lane_used_bandwidth array from struct sdw_bus into
struct sdw_bus_params so that used lane bandwidth is restored when
bus->params is rolled back on failure.

> diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
> index 30a495abe19ea..3d6b9c895e46f 100644
> --- a/drivers/soundwire/generic_bandwidth_allocation.c
> +++ b/drivers/soundwire/generic_bandwidth_allocation.c

[ ... ]

> @@ -507,7 +507,7 @@ static int get_manager_lane(struct sdw_bus *bus, struct sdw_master_runtime *m_rt
>  					      m_rt->stream->params.bps;
>  		}
>  		if (required_bandwidth <=
> -		    curr_dr_freq - bus->lane_used_bandwidth[l]) {
> +		    curr_dr_freq - bus->params.lane_used_bandwidth[l]) {
>  			/* Check if m_lane is connected to all Peripherals */
>  			if (!is_lane_connected_to_all_peripherals(m_rt,
>  				slave_prop->lane_maps[l])) {
> @@ -518,7 +518,7 @@ static int get_manager_lane(struct sdw_bus *bus, struct sdw_master_runtime *m_rt
>  			}
>  			m_lane = slave_prop->lane_maps[l];
>  			dev_dbg(&s_rt->slave->dev, "M lane %d is used\n", m_lane);
> -			bus->lane_used_bandwidth[l] += required_bandwidth;
> +			bus->params.lane_used_bandwidth[l] += required_bandwidth;
>  			/*
>  			 * Use non-zero manager lane, subtract the lane 0
>  			 * bandwidth that is already calculated

[Severity: High]
When _sdw_prepare_stream() fails after sdw_compute_bus_params() has assigned
a non-zero lane via get_manager_lane(), restoring bus->params rolls back
bus->params.lane_used_bandwidth[l] and bus->params.bandwidth, but leaves
the port runtime lane assignments modified:

drivers/soundwire/generic_bandwidth_allocation.c:sdw_compute_bus_params() {
    ...
out:
	/* multilane can be used */
	if (m_lane > 0) {
		/* Set Peripheral lanes */
		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
			slave_prop = &s_rt->slave->prop;
			for (l = 1; l < SDW_MAX_LANES; l++) {
				if (slave_prop->lane_maps[l] == m_lane) {
					list_for_each_entry(s_p_rt, &s_rt->port_list, port_node) {
						s_p_rt->lane = l;
    ...
		list_for_each_entry(m_p_rt, &m_rt->port_list, port_node) {
			m_p_rt->lane = m_lane;
		}
	}
    ...
}

If a subsequent step fails in sdw_compute_bus_params() (such as
sdw_select_row_col()), sdw_compute_port_params() (such as
sdw_compute_group_params() returning -EINVAL when column_needed > sel_col),
or _sdw_prepare_stream() (sdw_program_params(), do_bank_switch(), or
sdw_prep_deprep_ports()), _sdw_prepare_stream() jumps to restore_params:

drivers/soundwire/stream.c:_sdw_prepare_stream() {
    ...
restore_params:
	memcpy(&bus->params, &params, sizeof(params));
	return ret;
}

With m_p_rt->lane and s_p_rt->lane still set to non-zero values while
bus->params.lane_used_bandwidth[l] is restored to its previous value (such
as 0), won't subsequent calls to sdw_compute_group_params() and
_sdw_compute_port_params() skip computing parameters for those ports when
bus->params.lane_used_bandwidth[l] is 0, and won't _sdw_deprepare_stream()
later underflow bus->params.lane_used_bandwidth[p_rt->lane] and leak lane 0
bandwidth in bus->params.bandwidth?

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

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

[sashiko-bot]

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

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

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

This commit snapshots struct sdw_bus_params for each master in a stream
before preparing it and restores all masters' parameters on failure in
_sdw_prepare_stream() to prevent bandwidth leaks on earlier links.

> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 96613dd0677c4..86d951cc92f72 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1477,19 +1477,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) {
> @@ -1540,7 +1543,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;

[Severity: High]
Can this rollback desynchronize bus->params.curr_bank and
bus->params.next_bank from the active hardware register bank when failure
occurs during or after do_bank_switch()?

At the start of _sdw_prepare_stream(), m_rt->params_backup copies the entire
struct sdw_bus_params, including the pre-switch curr_bank and next_bank
values. Later in _sdw_prepare_stream(), do_bank_switch() switches the
hardware bank and flips curr_bank and next_bank (via sdw_bank_switch() or
sdw_ml_sync_bank_switch()), followed by the loop calling
sdw_prep_deprep_ports():

	ret = do_bank_switch(stream);
	if (ret < 0) {
		pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);
		goto restore_params;
	}

	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
		bus = m_rt->bus;

		/* Prepare port(s) on the new clock configuration */
		ret = sdw_prep_deprep_ports(m_rt, true);
		if (ret < 0) {
			dev_err(bus->dev, "Prepare port(s) failed ret = %d\n",
				ret);
			goto restore_params;
		}
	}

If sdw_prep_deprep_ports() fails on any link (or if do_bank_switch() fails on
a later link after an earlier link has already switched banks), jumping to
restore_params overwrites m_rt->bus->params with m_rt->params_backup across
every master in stream->master_list.

Wouldn't that revert curr_bank and next_bank to their pre-switch values while
the hardware remains on the newly switched bank (and leave any ports prepared
on earlier links in the loop prepared), causing subsequent stream operations
to program banked registers into the currently active hardware bank?

[Severity: High]
Also, what happens to the per-port lane assignments and transport parameters
modified by bus->compute_params() on preceding links when restore_params
rolls back m_rt->bus->params?

During _sdw_prepare_stream(), bus->compute_params() calls
sdw_compute_params() -> sdw_compute_bus_params(), which allocates multi-lane
bandwidth in bus->params.lane_used_bandwidth[] and sets non-zero lane numbers
on the slave and master port runtimes:

	if (m_lane > 0) {
		/* Set Peripheral lanes */
		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
			slave_prop = &s_rt->slave->prop;
			for (l = 1; l < SDW_MAX_LANES; l++) {
				if (slave_prop->lane_maps[l] == m_lane) {
					list_for_each_entry(s_p_rt, &s_rt->port_list, port_node) {
						s_p_rt->lane = l;
						...
					}
					break;
				}
			}
		}
		...
		list_for_each_entry(m_p_rt, &m_rt->port_list, port_node) {
			m_p_rt->lane = m_lane;
		}
	}

It then calls _sdw_compute_port_params(), which updates p_rt->transport_params
across the ports on that bus.

When a subsequent step or link in _sdw_prepare_stream() fails, restoring
m_rt->bus->params = m_rt->params_backup reverts
bus->params.lane_used_bandwidth[] and bus->params.curr_dr_freq, but leaves
s_p_rt->lane, m_p_rt->lane, and p_rt->transport_params modified.

If the stream is later deprepared in _sdw_deprepare_stream():

		list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
			if (!p_rt->lane)
				continue;

			bandwidth = m_rt->stream->params.rate * hweight32(p_rt->ch_mask) *
				    m_rt->stream->params.bps;
			multi_lane_bandwidth += bandwidth;
			bus->params.lane_used_bandwidth[p_rt->lane] -= bandwidth;
			if (!bus->params.lane_used_bandwidth[p_rt->lane])
				p_rt->lane = 0;
		}

won't seeing a non-zero p_rt->lane after bus->params.lane_used_bandwidth[]
was rolled back underflow bus->params.lane_used_bandwidth[p_rt->lane] and
skip releasing multi_lane_bandwidth from bus->params.bandwidth, or cause
sdw_compute_group_params() and _sdw_compute_port_params() to skip the ports
when bus->params.lane_used_bandwidth[p_rt->lane] is zero?

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