[PATCH v2 00/10] soundwire: Fix Critical and High issues reported by Sashiko

[PATCH v2 00/10] soundwire: Fix Critical and High issues reported by Sashiko

[Lee Jones]

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 <yung-chuan.liao@linux.intel.com>
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-sound@vger.kernel.org
Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
Cc: Srinivas Kandagatla <srini@kernel.org>
Cc: Vinod Koul <vkoul@kernel.org>

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

-- 
2.56.0.385.gac671c819a-goog

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

[Lee Jones]

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 <lee@kernel.org>
---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/bus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index aeaae5a57c89..15c66effbf4f 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -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;
 
-- 
2.56.0.385.gac671c819a-goog

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

[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 <lee@kernel.org>
---

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

diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
index a2ee064147e0..35272cfe9fc8 100644
--- a/drivers/soundwire/sysfs_slave.c
+++ b/drivers/soundwire/sysfs_slave.c
@@ -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;
 }
diff --git a/drivers/soundwire/sysfs_slave_dpn.c b/drivers/soundwire/sysfs_slave_dpn.c
index d2561400da77..16872ea64b1f 100644
--- a/drivers/soundwire/sysfs_slave_dpn.c
+++ b/drivers/soundwire/sysfs_slave_dpn.c
@@ -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++;							\
-- 
2.56.0.385.gac671c819a-goog

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

[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 <lee@kernel.org>
---

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

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 0b00fcdcab2d..db62a68c09f8 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -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))
-- 
2.56.0.385.gac671c819a-goog

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

[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 <lee@kernel.org>
---

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

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index db62a68c09f8..e3cdd36be1a1 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -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)
-- 
2.56.0.385.gac671c819a-goog

[PATCH v2 05/10] soundwire: intel_ace2x: Protect bus.bpt_stream with bus_lock

[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 <lee@kernel.org>
---

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

diff --git a/drivers/soundwire/intel_ace2x.c b/drivers/soundwire/intel_ace2x.c
index 642b33ab5526..c964dd4a7c22 100644
--- a/drivers/soundwire/intel_ace2x.c
+++ b/drivers/soundwire/intel_ace2x.c
@@ -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
-- 
2.56.0.385.gac671c819a-goog

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

[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 <lee@kernel.org>
---

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

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 6e0c5fb10efa..29130745c809 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -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);
-- 
2.56.0.385.gac671c819a-goog

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

[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 <lee@kernel.org>
---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 29130745c809..a50efdfc3a6b 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -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)
-- 
2.56.0.385.gac671c819a-goog

[PATCH v2 08/10] soundwire: qcom: Check clk_prepare_enable() return value

[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 <lee@kernel.org>
---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index a50efdfc3a6b..785096b232f2 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -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);
-- 
2.56.0.385.gac671c819a-goog

[PATCH v2 09/10] soundwire: qcom: Serialise ctrl->intr_mask updates

[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 <lee@kernel.org>
---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index 785096b232f2..cc1b07e2406d 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -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) {
-- 
2.56.0.385.gac671c819a-goog

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

[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 <lee@kernel.org>
---

v1 => v2: Drop Fixes and Stable tags

 drivers/soundwire/qcom.c | 44 +++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index cc1b07e2406d..8d1346158ecd 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -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;
-- 
2.56.0.385.gac671c819a-goog

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

[sashiko-bot]

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

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

[sashiko-bot]

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 <lee@kernel.org>

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.

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 6e0c5fb10efad..29130745c8097 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -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.

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

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

[Lee Jones]

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