Skip to contentSkip to conversation

soundwire: Fix Critical and High issues reported by Sashiko

PATCH In review Lee Jones 3 participants 20 messages Nov 3 – Nov 10

Subsystems (4)
Sound Arm Qualcomm mailing list Qualcomm audio (ASoC) drivers SoundWire
lore
Share & tools
lore

Lee Jones

author
#0 lore raw

v2 cover soundwire: Fix Critical and High issues reported by Sashiko

These issues were reported by Sashiko.

v1 => v2:
- Drop "ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streams" (applied by Mark Brown)
- Drop AMD SoundWire patches to be handled separately by the AMD team
- Drop Fixes and Stable tags per Vinod
- Only increment m_rt_count in sdw_stream_add_master() when master ports are allocated
- Replace cleanup.h guards with explicit mutex_lock()/mutex_unlock() in intel_ace2x_bpt_{open,close}_stream()
- Check nports >= maxport and unwind port_mask on overflow in qcom_swrm_stream_alloc_ports()

Cc Bard Liao <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Pierre-Louis Bossart <[email protected]>
Cc Srinivas Kandagatla <[email protected]>
Cc Vinod Koul <[email protected]>

Lee Jones (10):
soundwire: bus: Hold bus_lock when looking up slave devices
soundwire: sysfs: Use sysfs_emit_at() in array show callbacks
soundwire: stream: Prevent m_rt_count underflow in
sdw_stream_remove_master()
soundwire: stream: Clean up allocated runtime resources on config
failure
soundwire: intel_ace2x: Protect bus.bpt_stream with bus_lock
soundwire: qcom: Prevent pconfig buffer overflow in
qcom_swrm_stream_alloc_ports()
soundwire: qcom: Disable runtime PM and avoid clock underflow in
qcom_swrm_remove()
soundwire: qcom: Check clk_prepare_enable() return value
soundwire: qcom: Serialise ctrl->intr_mask updates
soundwire: qcom: Prevent stale broadcast IRQ from completing
subsequent command

 drivers/soundwire/bus.c             |  6 +-
 drivers/soundwire/intel_ace2x.c     | 20 ++++--
 drivers/soundwire/qcom.c            | 96 ++++++++++++++++++++++++-----
 drivers/soundwire/stream.c          | 44 +++++++++----
 drivers/soundwire/sysfs_slave.c     |  6 +-
 drivers/soundwire/sysfs_slave_dpn.c |  8 +--
 6 files changed, 137 insertions(+), 43 deletions(-)
Signature · 1 line
2.56.0.385.gac671c819a-goog
author
#1 lore raw

PATCH v2 1/10 soundwire: bus: Hold bus_lock when looking up slave devices

sdw_get_slave() iterates over bus->slaves and requires bus->bus_lock to be held by the caller. However, sdw_handle_slave_status() and sdw_clear_slave_status() drop bus->bus_lock immediately before calling sdw_get_slave(). If a concurrent slave teardown unlinks the node via list_del_init() and frees the slave while bus->bus_lock is dropped, sdw_get_slave() can spin forever on the self-referential list entry or dereference freed memory.

Keep bus->bus_lock held across the sdw_get_slave() lookup in sdw_handle_slave_status() and sdw_clear_slave_status().

Closes https://sashiko.dev/#/bug/linux-b0e65a93-dfd4-4e3d-a96c-8f13ba0aafd2
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/bus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/soundwire/bus.c +3 -3
index aeaae5a57c89..15c66effbf4f 100644
@@ -1907,9 +1907,9 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 			mutex_unlock(&bus->bus_lock);
 			continue;
 		}
-		mutex_unlock(&bus->bus_lock);
 
 		slave = sdw_get_slave(bus, i);
+		mutex_unlock(&bus->bus_lock);
 		if (!slave)
 			continue;
 

@@ -1954,9 +1954,9 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 			mutex_unlock(&bus->bus_lock);
 			continue;
 		}
-		mutex_unlock(&bus->bus_lock);
 
 		slave = sdw_get_slave(bus, i);
+		mutex_unlock(&bus->bus_lock);
 		if (!slave)
 			continue;
 

@@ -2052,9 +2052,9 @@ void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
 			mutex_unlock(&bus->bus_lock);
 			continue;
 		}
-		mutex_unlock(&bus->bus_lock);
 
 		slave = sdw_get_slave(bus, i);
+		mutex_unlock(&bus->bus_lock);
 		if (!slave)
 			continue;
Signature · 1 line
2.56.0.385.gac671c819a-goog
Tested-by on v2 1/10
#2 lore raw

Re: [PATCH v2 01/10] soundwire: bus: Hold bus_lock when looking up slave devices

Tested on a Dell Latitude 9420 (Tiger Lake, sof_sdw) on top of soundwire next at commit afd88e7ad781. The details are in my reply to the cover letter.

Tested-by HyeongJun An <[email protected]>
author
#3 lore raw

PATCH v2 2/10 soundwire: sysfs: Use sysfs_emit_at() in array show callbacks

In reply to Lee Jones

words_show() and the show callbacks generated by sdw_dpn_array_attr() format firmware-supplied property arrays into the sysfs buffer using unbounded sprintf(buf + size, ...) loops. If firmware provides a large array count, sprintf() can write past PAGE_SIZE and corrupt kernel memory.

Replace the sprintf(buf + size, ...) calls with bounds-checked sysfs_emit_at() calls.

Closes https://sashiko.dev/#/bug/linux-543ff498-3bed-412d-8972-4855c5aa6008
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/sysfs_slave.c     | 6 +++---
 drivers/soundwire/sysfs_slave_dpn.c | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)
drivers/soundwire/sysfs_slave.c +3 -3
index a2ee064147e0..35272cfe9fc8 100644
@@ -161,9 +161,9 @@ static ssize_t words_show(struct device *dev,
 	int i;
 
 	for (i = 0; i < slave->prop.dp0_prop->num_words; i++)
-		size += sprintf(buf + size, "%d ",
-				slave->prop.dp0_prop->words[i]);
-	size += sprintf(buf + size, "\n");
+		size += sysfs_emit_at(buf, size, "%d ",
+				      slave->prop.dp0_prop->words[i]);
+	size += sysfs_emit_at(buf, size, "\n");
 
 	return size;
 }
drivers/soundwire/sysfs_slave_dpn.c +4 -4
index d2561400da77..16872ea64b1f 100644
@@ -134,10 +134,10 @@ static ssize_t field##_dpn_show(struct sdw_slave *slave,		\
 	for_each_set_bit(bit, &mask, 32) {				\
 		if (bit == N) {						\
 			for (j = 0; j < dpn[i].num_##field; j++)	\
-				size += sprintf(buf + size,		\
-						format_string,		\
-						dpn[i].field[j]);	\
-			size += sprintf(buf + size, "\n");		\
+				size += sysfs_emit_at(buf, size,	\
+						      format_string,	\
+						      dpn[i].field[j]);	\
+			size += sysfs_emit_at(buf, size, "\n");		\
 			return size;					\
 		}							\
 		i++;							\
Signature · 1 line
2.56.0.385.gac671c819a-goog
Tested-by on v2 2/10
#4 lore raw

Re: [PATCH v2 02/10] soundwire: sysfs: Use sysfs_emit_at() in array show callbacks

Tested on a Dell Latitude 9420 (Tiger Lake, sof_sdw) on top of soundwire next at commit afd88e7ad781. The details are in my reply to the cover letter.

Tested-by HyeongJun An <[email protected]>
author
#5 lore raw

PATCH v2 3/10 soundwire: stream: Prevent m_rt_count underflow in sdw_stream_remove_master()

In reply to Lee Jones

stream->m_rt_count is incremented in sdw_stream_add_master() after sdw_master_port_alloc() succeeds. However, sdw_stream_add_slave() can allocate a master runtime without allocating master ports, and sdw_master_port_alloc() can fail after partially populating m_rt->port_list. When sdw_stream_remove_master() subsequently tears down the stream, it unconditionally decrements stream->m_rt_count, underflowing the counter when master ports were never allocated.

Free any partially allocated master ports on allocation failure in sdw_master_port_alloc(), and only increment and decrement stream->m_rt_count when master ports are allocated.

Closes https://sashiko.dev/#/bug/linux-bce288e6-c97f-46dd-9fc3-023c33d56f36
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2:
- Only increment stream->m_rt_count in sdw_stream_add_master() when sdw_master_port_allocated(m_rt) is true
- Drop Fixes and Stable tags

 drivers/soundwire/stream.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/soundwire/stream.c +7 -3
index 0b00fcdcab2d..db62a68c09f8 100644
@@ -1113,8 +1113,10 @@ static int sdw_master_port_alloc(struct sdw_master_runtime *m_rt,
 	/* Iterate for number of ports to perform initialization */
 	for (i = 0; i < num_ports; i++) {
 		p_rt = sdw_port_alloc(&m_rt->port_list);
-		if (!p_rt)
+		if (!p_rt) {
+			sdw_master_port_free(m_rt);
 			return -ENOMEM;
+		}
 	}
 
 	return 0;

@@ -2061,7 +2063,8 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 		if (ret)
 			goto alloc_error;
 
-		stream->m_rt_count++;
+		if (sdw_master_port_allocated(m_rt))
+			stream->m_rt_count++;
 	}
 
 	ret = sdw_master_rt_config(m_rt, stream_config);

@@ -2108,9 +2111,10 @@ int sdw_stream_remove_master(struct sdw_bus *bus,
 		if (m_rt->bus != bus)
 			continue;
 
+		if (sdw_master_port_allocated(m_rt))
+			stream->m_rt_count--;
 		sdw_master_port_free(m_rt);
 		sdw_master_rt_free(m_rt, stream);
-		stream->m_rt_count--;
 	}
 
 	if (list_empty(&stream->master_list))
Signature · 1 line
2.56.0.385.gac671c819a-goog
Tested-by on v2 3/10
#6 lore raw

Re: [PATCH v2 03/10] soundwire: stream: Prevent m_rt_count underflow in sdw_stream_remove_master()

Tested on a Dell Latitude 9420 (Tiger Lake, sof_sdw) on top of soundwire next at commit afd88e7ad781. The details are in my reply to the cover letter.

Tested-by HyeongJun An <[email protected]>
author
#7 lore raw

PATCH v2 4/10 soundwire: stream: Clean up allocated runtime resources on config failure

In reply to Lee Jones

In sdw_stream_add_master() and sdw_stream_add_slave(), failures during runtime or port configuration jump directly to the unlock label, bypassing alloc_error and leaving newly allocated master/slave runtime and port structures attached to the stream and bus lists.

Track whether ports were newly allocated in the current invocation and route configuration failures through alloc_error so all resources allocated by the failing call are released.

Closes https://sashiko.dev/#/bug/linux-546e137d-6e4d-4f22-99c3-8ccc8beb5350
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2:
- Adapt alloc_master_port tracking in sdw_stream_add_master() to the sdw_master_port_allocated(m_rt) check
- Drop Fixes and Stable tags

 drivers/soundwire/stream.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)
drivers/soundwire/stream.c +27 -9
index db62a68c09f8..e3cdd36be1a1 100644
@@ -1026,14 +1026,18 @@ static int sdw_slave_port_alloc(struct sdw_slave *slave,
 				struct sdw_slave_runtime *s_rt,
 				unsigned int num_config)
 {
-	struct sdw_port_runtime *p_rt;
+	struct sdw_port_runtime *p_rt, *_p_rt;
 	int i;
 
 	/* Iterate for number of ports to perform initialization */
 	for (i = 0; i < num_config; i++) {
 		p_rt = sdw_port_alloc(&s_rt->port_list);
-		if (!p_rt)
+		if (!p_rt) {
+			list_for_each_entry_safe(p_rt, _p_rt,
+						 &s_rt->port_list, port_node)
+				sdw_port_free(p_rt);
 			return -ENOMEM;
+		}
 	}
 
 	return 0;

@@ -2018,6 +2022,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 			  struct sdw_stream_runtime *stream)
 {
 	struct sdw_master_runtime *m_rt;
+	bool alloc_master_port = false;
 	bool alloc_master_rt = false;
 	int ret;
 

@@ -2063,19 +2068,23 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 		if (ret)
 			goto alloc_error;
 
-		if (sdw_master_port_allocated(m_rt))
+		if (sdw_master_port_allocated(m_rt)) {
+			alloc_master_port = true;
 			stream->m_rt_count++;
+		}
 	}
 
 	ret = sdw_master_rt_config(m_rt, stream_config);
 	if (ret < 0)
-		goto unlock;
+		goto alloc_error;
 
 	ret = sdw_config_stream(bus->dev, stream, stream_config, false);
 	if (ret)
-		goto unlock;
+		goto alloc_error;
 
 	ret = sdw_master_port_config(m_rt, port_config);
+	if (ret)
+		goto alloc_error;
 
 	goto unlock;
 

@@ -2083,6 +2092,10 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 	/*
 	 * we only cleanup what was allocated in this routine
 	 */
+	if (alloc_master_port) {
+		sdw_master_port_free(m_rt);
+		stream->m_rt_count--;
+	}
 	if (alloc_master_rt)
 		sdw_master_rt_free(m_rt, stream);
 unlock:

@@ -2148,6 +2161,7 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
 	struct sdw_slave_runtime *s_rt;
 	struct sdw_master_runtime *m_rt;
 	bool alloc_master_rt = false;
+	bool alloc_slave_port = false;
 	bool alloc_slave_rt = false;
 
 	int ret;

@@ -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)
Signature · 1 line
2.56.0.385.gac671c819a-goog
AI review
#8 lore raw

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

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

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 <[email protected]>

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.

Quoted 4 lines
drivers/soundwire/stream.c +0 -0
index db62a68c09f84..e3cdd36be1a1f 100644

[ ... ]

Quoted 32 lines
drivers/soundwire/stream.c +6 -4
@@ -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.

drivers/soundwire/stream.c int sdw_stream_add_slave(struct sdw_slave *slave, · line 2255

Quoted 8 lines
drivers/soundwire/stream.c +2 -0
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
author
#9 lore raw

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

Quoted 2 lines · On Tue, 03 Nov 2026, [email protected] 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.

Quoted 92 lines

--

commit 7c1be891cc3e3787d68318b2ed777a711e4d3fa4
Author: Lee Jones <[email protected]>

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.

Quoted 4 lines
drivers/soundwire/stream.c +0 -0
index db62a68c09f84..e3cdd36be1a1f 100644

[ ... ]

Quoted 32 lines
drivers/soundwire/stream.c +6 -4
@@ -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.

drivers/soundwire/stream.c int sdw_stream_add_slave(struct sdw_slave *slave, · line 2255

Quoted 8 lines
drivers/soundwire/stream.c +2 -0
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
Signature · 1 line
Lee Jones
Tested-by on v2 4/10
#10 lore raw

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

In reply to Lee Jones

Tested on a Dell Latitude 9420 (Tiger Lake, sof_sdw) on top of soundwire next at commit afd88e7ad781. The details are in my reply to the cover letter.

Tested-by HyeongJun An <[email protected]>
author
#11 lore raw

PATCH v2 5/10 soundwire: intel_ace2x: Protect bus.bpt_stream with bus_lock

In reply to Lee Jones

intel_ace2x_bpt_open_stream() checks and assigns cdns->bus.bpt_stream without holding cdns->bus.bus_lock. Concurrent BPT open requests can both observe a NULL cdns->bus.bpt_stream pointer, allocate separate streams, and overwrite cdns->bus.bpt_stream, leaking the first stream. In addition, failures after sdw_slave_bpt_stream_add() jump past sdw_stream_remove_master(), leaving the master runtime allocated by sdw_stream_add_slave() on the bus list.

Hold cdns->bus.bus_lock when checking, assigning, and clearing cdns->bus.bpt_stream, and remove both slave and master stream runtimes on open failure.

Closes https://sashiko.dev/#/bug/linux-88535ed9-b7e9-4ede-93c7-59950c524c0b
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2:
- Replace cleanup.h guards with explicit mutex_lock() and mutex_unlock() in intel_ace2x_bpt_{open,close}_stream()
- Drop Fixes and Stable tags

 drivers/soundwire/intel_ace2x.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
drivers/soundwire/intel_ace2x.c +14 -6
index 642b33ab5526..c964dd4a7c22 100644
@@ -83,16 +83,21 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	int len;
 	int i;
 
+	mutex_lock(&cdns->bus.bus_lock);
 	if (cdns->bus.bpt_stream) {
+		mutex_unlock(&cdns->bus.bus_lock);
 		dev_err(cdns->dev, "%s: BPT stream already exists\n", __func__);
 		return -EAGAIN;
 	}
 
 	stream = sdw_alloc_stream("BPT", SDW_STREAM_BPT);
-	if (!stream)
+	if (!stream) {
+		mutex_unlock(&cdns->bus.bus_lock);
 		return -ENOMEM;
+	}
 
 	cdns->bus.bpt_stream = stream;
+	mutex_unlock(&cdns->bus.bus_lock);
 
 	ret = sdw_slave_bpt_stream_add(slave, stream);
 	if (ret < 0)

@@ -105,7 +110,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	if (!pdi0) {
 		dev_err(cdns->dev, "%s: sdw_cdns_alloc_pdi0 failed\n", __func__);
 		ret = -EINVAL;
-		goto remove_slave;
+		goto remove_master;
 	}
 
 	sdw_cdns_config_stream(cdns, 1, dir, pdi0);

@@ -117,7 +122,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	if (!pdi1) {
 		dev_err(cdns->dev, "%s: sdw_cdns_alloc_pdi1 failed\n", __func__);
 		ret = -EINVAL;
-		goto remove_slave;
+		goto remove_master;
 	}
 
 	sdw_cdns_config_stream(cdns, 1, dir, pdi1);

@@ -136,7 +141,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 	pconfig = kzalloc_objs(*pconfig, 2);
 	if (!pconfig) {
 		ret =  -ENOMEM;
-		goto remove_slave;
+		goto remove_master;
 	}
 
 	for (i = 0; i < 2 /* num_pdi */; i++) {

@@ -149,7 +154,7 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *
 
 	if (ret < 0) {
 		dev_err(cdns->dev, "add master to stream failed:%d\n", ret);
-		goto remove_slave;
+		goto remove_master;
 	}
 
 	ret = sdw_prepare_stream(cdns->bus.bpt_stream);

@@ -293,15 +298,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:
+	mutex_lock(&cdns->bus.bus_lock);
 	sdw_release_stream(cdns->bus.bpt_stream);
 	cdns->bus.bpt_stream = NULL;
+	mutex_unlock(&cdns->bus.bus_lock);
 
 	return ret;
 }

@@ -335,8 +341,10 @@ static void intel_ace2x_bpt_close_stream(struct sdw_intel *sdw, struct sdw_slave
 		dev_err(cdns->dev, "%s: remove slave failed: %d\n",
 			__func__, ret);
 
+	mutex_lock(&cdns->bus.bus_lock);
 	sdw_release_stream(cdns->bus.bpt_stream);
 	cdns->bus.bpt_stream = NULL;
+	mutex_unlock(&cdns->bus.bus_lock);
 }
 
 #define INTEL_BPT_MSG_BYTE_MIN 16
Signature · 1 line
2.56.0.385.gac671c819a-goog
author
#12 lore raw

PATCH v2 6/10 soundwire: qcom: Prevent pconfig buffer overflow in qcom_swrm_stream_alloc_ports()

In reply to Lee Jones

qcom_swrm_stream_alloc_ports() allocates the pconfig array with ctrl->nports elements, but only checks that the assigned port number pn is below maxport. When multiple slave ports map to the same master port via m_port_map or the stream requests more slave ports than ctrl->nports, nports increments past ctrl->nports and writes out of bounds of pconfig.

Verify that nports is less than maxport before writing to pconfig[nports], and clear any port_mask bits allocated during the loop before returning -EBUSY.

Closes https://sashiko.dev/#/bug/linux-543ea0c0-a6e5-4b03-a041-ea0a760802c1
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2:
- Also check nports >= maxport and clear allocated port_mask bits on error
- Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/soundwire/qcom.c +3 -1
index 6e0c5fb10efa..29130745c809 100644
@@ -1350,8 +1350,10 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
 				else
 					pn = find_first_zero_bit(port_mask, maxport);
 
-				if (pn >= maxport) {
+				if (pn >= maxport || nports >= maxport) {
 					dev_err(ctrl->dev, "All ports busy\n");
+					while (nports--)
+						clear_bit(pconfig[nports].num, port_mask);
 					return -EBUSY;
 				}
 				set_bit(pn, port_mask);
Signature · 1 line
2.56.0.385.gac671c819a-goog
AI review
#13 lore raw

Re: [PATCH v2 06/10] soundwire: qcom: Prevent pconfig buffer overflow in qcom_swrm_stream_alloc_ports()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] soundwire: qcom: qcom_swrm_stream_alloc_ports() rollback clears active ports --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] soundwire: qcom: qcom_swrm_stream_alloc_ports() rollback clears active ports --

commit ff22f63970718cc2818a7a2729f4f9674b452688
Author: Lee Jones <[email protected]>

soundwire: qcom: Prevent pconfig buffer overflow in qcom_swrm_stream_alloc_ports()

This commit bounds-checks nports against maxport before writing to pconfig[nports] in qcom_swrm_stream_alloc_ports(), and clears pconfig[].num bits from port_mask on error.

Quoted 5 lines
drivers/soundwire/qcom.c +0 -0
index 6e0c5fb10efad..29130745c8097 100644

drivers/soundwire/qcom.c static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl, · line 1358

Quoted 10 lines
drivers/soundwire/qcom.c +3 -1
@@ -1350,8 +1350,10 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
 				else
 					pn = find_first_zero_bit(port_mask, maxport);
 
-				if (pn >= maxport) {
+				if (pn >= maxport || nports >= maxport) {
 					dev_err(ctrl->dev, "All ports busy\n");
+					while (nports--)
+						clear_bit(pconfig[nports].num, port_mask);
 					return -EBUSY;
 				}

[Severity: Medium]
Can this rollback loop clear bits in ctrl->port_mask that were already in use before qcom_swrm_stream_alloc_ports() was called?

When slave->m_port_map[p_rt->num] is non-zero, qcom_swrm_stream_alloc_ports() assigns pn = m_port directly without checking whether bit pn was already set in ctrl->port_mask:

drivers/soundwire/qcom.c:qcom_swrm_stream_alloc_ports() {
	...
	m_port = slave->m_port_map[p_rt->num];
	/* Port numbers start from 1 - 14*/
	if (m_port)
		pn = m_port;
	else
		pn = find_first_zero_bit(port_mask, maxport);
	...
	set_bit(pn, port_mask);
	pconfig[nports].num = pn;
}

Because ctrl->port_mask is shared across all DAIs on the controller (accessed via qcom_swrm_hw_params() -> qcom_swrm_stream_alloc_ports()), bit m_port may already be set by another active DAI stream or by an earlier slave port sharing the same master port.

If a subsequent port in the loop hits the error condition (pn >= maxport || nports >= maxport), the new while (nports--) loop unconditionally calls clear_bit(pconfig[nports].num, port_mask), clearing that pre-existing bit in ctrl->port_mask and allowing future find_first_zero_bit() calls to hand out an in-use master port.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
author
#14 lore raw

PATCH v2 7/10 soundwire: qcom: Disable runtime PM and avoid clock underflow in qcom_swrm_remove()

In reply to Lee Jones

When the controller is runtime-suspended, swrm_runtime_suspend() has already disabled ctrl->hclk. If the device is unbound in that state, qcom_swrm_remove() omits pm_runtime_disable() and unconditionally calls clk_disable_unprepare(), triggering a clock enable count underflow and leaving runtime PM enabled.

Resume the device before removing the bus master, disable runtime PM, and only disable ctrl->hclk if the device is not runtime-suspended.

Closes https://sashiko.dev/#/bug/linux-e26e04cd-3dec-4a3d-94e6-f45403c4bb57
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/soundwire/qcom.c +6 -1
index 29130745c809..a50efdfc3a6b 100644
@@ -1788,8 +1788,13 @@ static void qcom_swrm_remove(struct platform_device *pdev)
 {
 	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
 
+	pm_runtime_get_sync(&pdev->dev);
 	sdw_bus_master_delete(&ctrl->bus);
-	clk_disable_unprepare(ctrl->hclk);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		clk_disable_unprepare(ctrl->hclk);
+	pm_runtime_set_suspended(&pdev->dev);
 }
 
 static int __maybe_unused swrm_runtime_resume(struct device *dev)
Signature · 1 line
2.56.0.385.gac671c819a-goog
author
#15 lore raw

PATCH v2 8/10 soundwire: qcom: Check clk_prepare_enable() return value

In reply to Lee Jones

qcom_swrm_probe(), qcom_swrm_irq_handler(), and swrm_runtime_resume() ignore the return value of clk_prepare_enable(ctrl->hclk) and proceed to access controller registers even when enabling the interface clock fails. Also jump to err_clk when requesting the wake IRQ fails in qcom_swrm_probe() so ctrl->hclk is disabled on error.

Check the return value of clk_prepare_enable() in all three callers and abort register access on failure.

Closes https://sashiko.dev/#/bug/linux-f65cddc9-08ff-4531-9acf-76981d74d914
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/soundwire/qcom.c +11 -5
index a50efdfc3a6b..785096b232f2 100644
@@ -778,7 +778,9 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 	u32 i;
 	int devnum;
 	int ret = IRQ_HANDLED;
-	clk_prepare_enable(ctrl->hclk);
+
+	if (clk_prepare_enable(ctrl->hclk))
+		return IRQ_NONE;
 
 	ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_STATUS],
 		       &intr_sts);

@@ -1677,7 +1679,9 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 		goto err_init;
 	}
 
-	clk_prepare_enable(ctrl->hclk);
+	ret = clk_prepare_enable(ctrl->hclk);
+	if (ret)
+		goto err_init;
 
 	ctrl->dev = dev;
 	dev_set_drvdata(&pdev->dev, ctrl);

@@ -1733,7 +1737,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 						"swr_wake_irq", ctrl);
 		if (ret) {
 			dev_err(dev, "Failed to request soundwire wake irq\n");
-			goto err_init;
+			goto err_clk;
 		}
 	}
 

@@ -1802,13 +1806,15 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
 	struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev);
 	int ret;
 
+	ret = clk_prepare_enable(ctrl->hclk);
+	if (ret)
+		return ret;
+
 	if (ctrl->wake_irq > 0) {
 		if (!irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq)))
 			disable_irq_nosync(ctrl->wake_irq);
 	}
 
-	clk_prepare_enable(ctrl->hclk);
-
 	if (ctrl->clock_stop_not_supported) {
 		reinit_completion(&ctrl->enumeration);
 		ctrl->reg_write(ctrl, SWRM_COMP_SW_RESET, 0x01);
Signature · 1 line
2.56.0.385.gac671c819a-goog
author
#16 lore raw

PATCH v2 9/10 soundwire: qcom: Serialise ctrl->intr_mask updates

In reply to Lee Jones

qcom_swrm_irq_handler(), swrm_runtime_suspend(), and swrm_runtime_resume() concurrently read, modify, and write ctrl->intr_mask and program the interrupt enable registers without synchronisation. Overlapping read-modify-write updates can clobber interrupt mask bits and leave the hardware interrupt mask out of sync.

Add ctrl->intr_lock to serialise accesses to ctrl->intr_mask and its associated interrupt mask register updates.

Closes https://sashiko.dev/#/bug/linux-ea523727-6467-42aa-850f-8f838e4b84a8
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/soundwire/qcom.c +23 -2
index 785096b232f2..cc1b07e2406d 100644
@@ -202,6 +202,8 @@ struct qcom_swrm_ctrl {
 	struct completion enumeration;
 	/* Port alloc/free lock */
 	struct mutex port_lock;
+	/* Interrupt mask and broadcast completion lock */
+	struct mutex intr_lock;
 	struct clk *hclk;
 	int irq;
 	u32 ee;

@@ -784,7 +786,9 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 
 	ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_STATUS],
 		       &intr_sts);
+	mutex_lock(&ctrl->intr_lock);
 	intr_sts_masked = intr_sts & ctrl->intr_mask;
+	mutex_unlock(&ctrl->intr_lock);
 
 	do {
 		for (i = 0; i < SWRM_INTERRUPT_MAX; i++) {

@@ -820,10 +824,12 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 				dev_err_ratelimited(ctrl->dev,
 						"%s: SWR bus clsh detected\n",
 						__func__);
+				mutex_lock(&ctrl->intr_lock);
 				ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
 				ctrl->reg_write(ctrl,
 						ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
 						ctrl->intr_mask);
+				mutex_unlock(&ctrl->intr_lock);
 				break;
 			case SWRM_INTERRUPT_STATUS_RD_FIFO_OVERFLOW:
 				ctrl->reg_read(ctrl,

@@ -863,20 +869,24 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 				dev_err_ratelimited(ctrl->dev,
 						"%s: SWR Port collision detected\n",
 						__func__);
+				mutex_lock(&ctrl->intr_lock);
 				ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_DOUT_PORT_COLLISION;
 				ctrl->reg_write(ctrl,
 						ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
 						ctrl->intr_mask);
+				mutex_unlock(&ctrl->intr_lock);
 				break;
 			case SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH:
 				dev_err_ratelimited(ctrl->dev,
 					"%s: SWR read enable valid mismatch\n",
 					__func__);
+				mutex_lock(&ctrl->intr_lock);
 				ctrl->intr_mask &=
 					~SWRM_INTERRUPT_STATUS_READ_EN_RD_VALID_MISMATCH;
 				ctrl->reg_write(ctrl,
 						ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
 						ctrl->intr_mask);
+				mutex_unlock(&ctrl->intr_lock);
 				break;
 			case SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED:
 				complete(&ctrl->broadcast);

@@ -910,7 +920,9 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 				intr_sts);
 		ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_STATUS],
 			       &intr_sts);
+		mutex_lock(&ctrl->intr_lock);
 		intr_sts_masked = intr_sts & ctrl->intr_mask;
+		mutex_unlock(&ctrl->intr_lock);
 	} while (intr_sts_masked);
 
 	clk_disable_unprepare(ctrl->hclk);

@@ -952,11 +964,13 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 	/* Enable Auto enumeration */
 	ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 1);
 
+	mutex_lock(&ctrl->intr_lock);
 	ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK;
 	/* Mask soundwire interrupts */
 	if (ctrl->version < SWRM_VERSION_2_0_0)
 		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_MASK_ADDR],
-				SWRM_INTERRUPT_STATUS_RMSK);
+				ctrl->intr_mask);
+	mutex_unlock(&ctrl->intr_lock);
 
 	/* Configure No pings */
 	ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR, &val);

@@ -997,8 +1011,10 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl *ctrl)
 			0xFFFFFFFF);
 
 	/* enable CPU IRQs for the selected EE window */
+	mutex_lock(&ctrl->intr_lock);
 	ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
-			SWRM_INTERRUPT_STATUS_RMSK);
+			ctrl->intr_mask);
+	mutex_unlock(&ctrl->intr_lock);
 
 	/* Set IRQ to PULSE */
 	ctrl->reg_write(ctrl, SWRM_COMP_CFG_ADDR,

@@ -1686,6 +1702,7 @@ static int qcom_swrm_probe(struct platform_device *pdev)
 	ctrl->dev = dev;
 	dev_set_drvdata(&pdev->dev, ctrl);
 	mutex_init(&ctrl->port_lock);
+	mutex_init(&ctrl->intr_lock);
 	init_completion(&ctrl->broadcast);
 	init_completion(&ctrl->enumeration);
 

@@ -1851,6 +1868,7 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
 		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
 			SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET);
 
+		mutex_lock(&ctrl->intr_lock);
 		ctrl->intr_mask |= SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
 		if (ctrl->version < SWRM_VERSION_2_0_0)
 			ctrl->reg_write(ctrl,

@@ -1858,6 +1876,7 @@ static int __maybe_unused swrm_runtime_resume(struct device *dev)
 					ctrl->intr_mask);
 		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
 				ctrl->intr_mask);
+		mutex_unlock(&ctrl->intr_lock);
 
 		usleep_range(100, 105);
 		if (!swrm_wait_for_frame_gen_enabled(ctrl))

@@ -1879,6 +1898,7 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev)
 	swrm_wait_for_wr_fifo_done(ctrl);
 	if (!ctrl->clock_stop_not_supported) {
 		/* Mask bus clash interrupt */
+		mutex_lock(&ctrl->intr_lock);
 		ctrl->intr_mask &= ~SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET;
 		if (ctrl->version < SWRM_VERSION_2_0_0)
 			ctrl->reg_write(ctrl,

@@ -1886,6 +1906,7 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev)
 					ctrl->intr_mask);
 		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_CPU_EN],
 				ctrl->intr_mask);
+		mutex_unlock(&ctrl->intr_lock);
 		/* Prepare slaves for clock stop */
 		ret = sdw_bus_prep_clk_stop(&ctrl->bus);
 		if (ret < 0 && ret != -ENODATA) {
Signature · 1 line
2.56.0.385.gac671c819a-goog
author
#17 lore raw

PATCH v2 10/10 soundwire: qcom: Prevent stale broadcast IRQ from completing subsequent command

In reply to Lee Jones

When wait_for_completion_timeout() expires for a broadcast command in qcom_swrm_cmd_fifo_wr_cmd(), any late SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED interrupt from the timed-out command can fire concurrently with a subsequent broadcast command after reinit_completion(&ctrl->broadcast), prematurely completing the new command.

Clear any pending special command completion interrupt and reinitialise ctrl->broadcast before issuing a broadcast command under ctrl->intr_lock, clear the interrupt on timeout, and verify that the interrupt status bit is still asserted under ctrl->intr_lock before signalling ctrl->broadcast.

Closes https://sashiko.dev/#/bug/linux-fa1aa3a1-f9c7-4e8a-bdfc-49a559353546
Signed-off-by Lee Jones <[email protected]>

---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 44 +++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)
drivers/soundwire/qcom.c +37 -7
index cc1b07e2406d..8d1346158ecd 100644
@@ -554,11 +554,18 @@ static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
 	if (swrm_wait_for_wr_fifo_avail(ctrl))
 		return SDW_CMD_FAIL_OTHER;
 
-	if (cmd_id == SWR_BROADCAST_CMD_ID)
+	if (cmd_id == SWR_BROADCAST_CMD_ID) {
+		mutex_lock(&ctrl->intr_lock);
+		ctrl->reg_write(ctrl,
+				ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
+				SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED);
 		reinit_completion(&ctrl->broadcast);
-
-	/* Its assumed that write is okay as we do not get any status back */
-	ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_CMD_FIFO_WR_CMD], val);
+		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_CMD_FIFO_WR_CMD], val);
+		mutex_unlock(&ctrl->intr_lock);
+	} else {
+		/* Its assumed that write is okay as we do not get any status back */
+		ctrl->reg_write(ctrl, ctrl->reg_layout[SWRM_REG_CMD_FIFO_WR_CMD], val);
+	}
 
 	if (ctrl->version <= SWRM_VERSION_1_3_0)
 		usleep_range(150, 155);

@@ -571,10 +578,16 @@ static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
 		 */
 		ret = wait_for_completion_timeout(&ctrl->broadcast,
 						  msecs_to_jiffies(TIMEOUT_MS));
-		if (!ret)
+		if (!ret) {
+			mutex_lock(&ctrl->intr_lock);
+			ctrl->reg_write(ctrl,
+					ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
+					SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED);
+			mutex_unlock(&ctrl->intr_lock);
 			ret = SDW_CMD_IGNORED;
-		else
+		} else {
 			ret = SDW_CMD_OK;
+		}
 
 	} else {
 		ret = SDW_CMD_OK;

@@ -773,6 +786,22 @@ static irqreturn_t qcom_swrm_wake_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void qcom_swrm_complete_broadcast(struct qcom_swrm_ctrl *ctrl)
+{
+	u32 status;
+
+	guard(mutex)(&ctrl->intr_lock);
+
+	ctrl->reg_read(ctrl, ctrl->reg_layout[SWRM_REG_INTERRUPT_STATUS],
+		       &status);
+	if (status & SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED) {
+		ctrl->reg_write(ctrl,
+				ctrl->reg_layout[SWRM_REG_INTERRUPT_CLEAR],
+				SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED);
+		complete(&ctrl->broadcast);
+	}
+}
+
 static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 {
 	struct qcom_swrm_ctrl *ctrl = dev_id;

@@ -889,7 +918,8 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 				mutex_unlock(&ctrl->intr_lock);
 				break;
 			case SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED:
-				complete(&ctrl->broadcast);
+				qcom_swrm_complete_broadcast(ctrl);
+				intr_sts &= ~SWRM_INTERRUPT_STATUS_SPECIAL_CMD_ID_FINISHED;
 				break;
 			case SWRM_INTERRUPT_STATUS_BUS_RESET_FINISHED_V2:
 				break;
Signature · 1 line
2.56.0.385.gac671c819a-goog
#18 lore raw
In reply to Lee Jones

On a Dell Latitude 9420 (Tiger Lake, SOF IPC3, sof_sdw with rt711-sdca on link 0, two rt1316 on links 1 and 2 that are aggregated into one Speaker stream, and rt714 on link 3), I tested this series on top of soundwire next at commit afd88e7ad781. Only patches 1-4 run on this machine. The intel_ace2x and qcom patches are not exercised here.

With and without the series:
- all four peripherals attach at boot
- playback and capture work on every endpoint
- 40 start/stop cycles of the multi-link Speaker stream (10 of them after the links had runtime suspended), all links streaming at the same time, and Jack Out start/stop while Speaker was playing all passed
- ALSA pcm-test: 24 pass, 0 fail
- the dev-properties, dp0 and dpN sysfs attributes of all four peripherals read back identical (the words arrays are empty on these codecs, so only the trailing newline goes through the changed code)

With the series, s2idle with Speaker playback and Microphone capture
running: all four peripherals were attached again after resume, both streams continued after aplay/arecord re-prepared them, and playback and capture on every endpoint still worked. There were no SoundWire or SOF kernel messages. I did not run the same s2idle test without the series.

These runs only take the normal paths, so the failure cases that patches 3 and 4 guard against were not hit.

I ran these with Claude Code (Claude Opus 5.5) on the laptop.

I am sending Tested-by on patches 1-4 only, in replies to those patches.

author
#19 lore raw
Quoted 24 lines · HyeongJun An

On a Dell Latitude 9420 (Tiger Lake, SOF IPC3, sof_sdw with rt711-sdca on link 0, two rt1316 on links 1 and 2 that are aggregated into one Speaker stream, and rt714 on link 3), I tested this series on top of soundwire next at commit afd88e7ad781. Only patches 1-4 run on this machine. The intel_ace2x and qcom patches are not exercised here.

With and without the series:
- all four peripherals attach at boot
- playback and capture work on every endpoint
- 40 start/stop cycles of the multi-link Speaker stream (10 of them after the links had runtime suspended), all links streaming at the same time, and Jack Out start/stop while Speaker was playing all passed
- ALSA pcm-test: 24 pass, 0 fail
- the dev-properties, dp0 and dpN sysfs attributes of all four peripherals read back identical (the words arrays are empty on these codecs, so only the trailing newline goes through the changed code)

With the series, s2idle with Speaker playback and Microphone capture
running: all four peripherals were attached again after resume, both streams continued after aplay/arecord re-prepared them, and playback and capture on every endpoint still worked. There were no SoundWire or SOF kernel messages. I did not run the same s2idle test without the series.

Quoted 7 lines

These runs only take the normal paths, so the failure cases that patches 3 and 4 guard against were not hit.

I ran these with Claude Code (Claude Opus 5.5) on the laptop.

I am sending Tested-by on patches 1-4 only, in replies to those patches.

I really appreciate you taking the time to test these. Thank you.

Signature · 1 line
Lee Jones