soundwire: Fix Critical and High issues reported by Sashiko
Subsystems (7)
cover soundwire: Fix Critical and High issues reported by Sashiko
These issues were reported by Sashiko.
Lee Jones (21):
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
ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT
streams
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
soundwire: amd: Check for NULL manager drvdata in sdw_amd_startup()
soundwire: amd: Check ping command response for timeout
soundwire: amd: Preserve lane control bits in amd_sdw_port_enable()
soundwire: amd: Avoid static port offset leak in
amd_sdw_compute_params()
soundwire: amd: Initialise and cancel IRQ work around manager lifetime
soundwire: amd: Resume manager before hardware teardown in
amd_sdw_manager_remove()
soundwire: amd: Synchronise parent IRQ and disable work on manager
removal
soundwire: amd: Cancel pending work before suspending manager
soundwire: amd: Check clock resume timeout in amd_resume_runtime()
soundwire: amd: Disable interrupts if enabling manager fails
drivers/soundwire/amd_init.c | 13 ++- drivers/soundwire/amd_manager.c | 130 +++++++++++++++++++++++----- drivers/soundwire/bus.c | 6 +- drivers/soundwire/intel_ace2x.c | 36 ++++---- drivers/soundwire/qcom.c | 97 +++++++++++++++++---- drivers/soundwire/stream.c | 40 ++++++--- drivers/soundwire/sysfs_slave.c | 6 +- drivers/soundwire/sysfs_slave_dpn.c | 8 +- sound/soc/sof/intel/hda-sdw-bpt.c | 6 ++ 9 files changed, 267 insertions(+), 75 deletions(-)
Signature · 1 line
2.56.0.317.g454f8302e1-goog
PATCH 1/21 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().
--- drivers/soundwire/bus.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/soundwire/bus.c +3 -3
@@ -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.317.g454f8302e1-goog
PATCH 2/21 soundwire: sysfs: Use sysfs_emit_at() in array show callbacks
In reply to Lee Joneswords_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.
--- 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
@@ -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
@@ -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.317.g454f8302e1-goog
PATCH 3/21 soundwire: stream: Prevent m_rt_count underflow in sdw_stream_remove_master()
In reply to Lee Jonesstream->m_rt_count is only 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 decrement stream->m_rt_count in sdw_stream_remove_master() when master ports were allocated.
--- drivers/soundwire/stream.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/soundwire/stream.c +5 -2
@@ -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 ;
@@ -2108,9 +2110,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.317.g454f8302e1-goog
[email protected]
AI reviewRe: [PATCH 03/21] soundwire: stream: Prevent m_rt_count underflow in sdw_stream_remove_master()
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] soundwire: stream: m_rt_count leak in sdw_stream_remove_master() --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] soundwire: stream: m_rt_count leak in sdw_stream_remove_master() --
commit 029ba42fadc93c4fc5794332114bf86183041b53
Author: Lee Jones <[email protected]>
soundwire: stream: Prevent m_rt_count underflow in sdw_stream_remove_master()
This commit frees partially allocated master ports on allocation failure in sdw_master_port_alloc() and gates the stream->m_rt_count decrement in sdw_stream_remove_master() on sdw_master_port_allocated(m_rt).
Quoted 4 lines
drivers/soundwire/stream.c +0
-0
[ ... ]
drivers/soundwire/stream.c int sdw_stream_remove_master(struct sdw_bus *bus, · line 2113
Quoted 9 lines
drivers/soundwire/stream.c +2
-1@@ -2108,9 +2110,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--;
[Severity: Medium]
Can this leave stream->m_rt_count elevated when sdw_stream_add_master() is called with num_ports == 0?
In sdw_stream_add_master(), when !sdw_master_port_allocated(m_rt) is true, sdw_master_port_alloc(m_rt, num_ports) is called:
drivers/soundwire/stream.c:sdw_stream_add_master() {
...
if (!sdw_master_port_allocated(m_rt)) {
ret = sdw_master_port_alloc(m_rt, num_ports);
if (ret)
goto alloc_error;
stream->m_rt_count++;
}
...
}When num_ports == 0 (for example via qcom_swrm_hw_params() -> qcom_swrm_stream_alloc_ports() -> sdw_stream_add_master() when a codec DAI has no active ports enabled), sdw_master_port_alloc() executes zero loop iterations and returns 0 while leaving m_rt->port_list empty, and sdw_stream_add_master() increments stream->m_rt_count.
When sdw_stream_remove_master() later tears down the master runtime, sdw_master_port_allocated(m_rt) checks !list_empty(&m_rt->port_list), which is false because m_rt->port_list is empty. As a result, stream->m_rt_count is not decremented before m_rt is freed, causing subsequent sdw_stream_add_master() calls on single-link buses (!bus->multi_link) to fail with -EINVAL or miscounting active master runtimes in do_bank_switch().
Could sdw_master_runtime track whether sdw_stream_add_master() incremented m_rt_count, or should the num_ports == 0 case be handled consistently between sdw_stream_add_master() and sdw_stream_remove_master()?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH 4/21 soundwire: stream: Clean up allocated runtime resources on config failure
In reply to Lee JonesIn 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.
--- drivers/soundwire/stream.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)
drivers/soundwire/stream.c +25 -8
@@ -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,18 +2068,21 @@ int sdw_stream_add_master(struct sdw_bus *bus,if (ret) goto alloc_error; + 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;
@@ -2082,6 +2090,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:
@@ -2147,6 +2159,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;
@@ -2197,24 +2210,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.
@@ -2231,6 +2246,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.317.g454f8302e1-goog
PATCH 5/21 ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streams
In reply to Lee JonesWhen sdw_enable_stream() fails in intel_ace2x_bpt_send_async() after hda_sdw_bpt_send_async() has already started the TX and RX DMA streams, intel_ace2x_bpt_close_stream() invokes hda_sdw_bpt_close() directly without calling hda_sdw_bpt_wait(). As a result, hda_sdw_bpt_dma_deprepare() tears down the DMA buffers and BDL while the DMA hardware is still running.
Disable any running TX or RX BPT DMA stream at the start of hda_sdw_bpt_close() before unmapping channels and depreparing DMA resources.
--- sound/soc/sof/intel/hda-sdw-bpt.c | 6 ++++++ 1 file changed, 6 insertions(+)
sound/soc/sof/intel/hda-sdw-bpt.c +6 -0
@@ -456,6 +456,12 @@ int hda_sdw_bpt_close(struct device *dev, int link_id, struct hdac_ext_stream *bint ret;int ret1; +if (bpt_rx_stream &&hdac_stream (bpt_rx_stream)->running) +hda_sdw_bpt_dma_disable (dev, bpt_rx_stream); + +if (bpt_tx_stream &&hdac_stream (bpt_tx_stream)->running) +hda_sdw_bpt_dma_disable (dev, bpt_tx_stream); +/* * In the case of SoundWire we need to reset the PCMSyCM registers. * Need to continue depreparing the DMA buffers even if this fails.
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mark Brown
maintainerRe: [PATCH 05/21] ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streams
Quoted 6 lines · Lee Jones
When
sdw_enable_stream()fails inintel_ace2x_bpt_send_async()afterhda_sdw_bpt_send_async()has already started the TX and RX DMA streams,intel_ace2x_bpt_close_stream()invokeshda_sdw_bpt_close()directly without callinghda_sdw_bpt_wait(). As a result,hda_sdw_bpt_dma_deprepare()tears down the DMA buffers and BDL while the DMA hardware is still running.
Does this actually need to be in with all the Soundwire stuff? There's no immediately obvious dependency.
Vinod Koul
maintainerRe: [PATCH 05/21] ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streams
Quoted 10 lines · Mark Brown
Quoted 6 lines · Lee Jones
When
sdw_enable_stream()fails inintel_ace2x_bpt_send_async()afterhda_sdw_bpt_send_async()has already started the TX and RX DMA streams,intel_ace2x_bpt_close_stream()invokeshda_sdw_bpt_close()directly without callinghda_sdw_bpt_wait(). As a result,hda_sdw_bpt_dma_deprepare()tears down the DMA buffers and BDL while the DMA hardware is still running.Does this actually need to be in with all the Soundwire stuff? There's no immediately obvious dependency.
Doesnt seem dependent to me
Signature · 1 line
~Vinod
Re: [PATCH 05/21] ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streams
Quoted 9 lines · Vinod Koul
Quoted 8 lines · Mark Brown
Quoted 6 lines · Lee Jones
When
sdw_enable_stream()fails inintel_ace2x_bpt_send_async()afterhda_sdw_bpt_send_async()has already started the TX and RX DMA streams,intel_ace2x_bpt_close_stream()invokeshda_sdw_bpt_close()directly without callinghda_sdw_bpt_wait(). As a result,hda_sdw_bpt_dma_deprepare()tears down the DMA buffers and BDL while the DMA hardware is still running.
Quoted 4 lines
Quoted 2 lines
Does this actually need to be in with all the Soundwire stuff? There's no immediately obvious dependency.
Doesnt seem dependent to me
Sashiko grouped this report under the SoundWire subsystem because the caller (intel_ace2x_bpt_close_stream()) lives in drivers/soundwire/intel_ace2x.c, even though hda_sdw_bpt_close() itself is under sound/soc/sof/intel/.
You're both right, there is no dependency on the rest of the series.
Happy for you to take it directly through ASoC if the patch looks good to you or I can drop it from v2 of the SoundWire set and send it standalone or when I handle ASoC.
Signature · 1 line
Lee Jones
Re: [PATCH 05/21] ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streams
In reply to Lee JonesQuoted 7 lines · Lee Jones
When
sdw_enable_stream()fails inintel_ace2x_bpt_send_async()afterhda_sdw_bpt_send_async()has already started the TX and RX DMA streams,intel_ace2x_bpt_close_stream()invokeshda_sdw_bpt_close()directly without callinghda_sdw_bpt_wait(). As a result,hda_sdw_bpt_dma_deprepare()tears down the DMA buffers and BDL while the DMA hardware is still running.
Quoted 8 lines
Disable any running TX or RX BPT DMA stream at the start of
hda_sdw_bpt_close()before unmapping channels and depreparing DMA resources.Fixes 4c1ce9f37d8a ("soundwire: intel_ace2x: add BPT send_async/wait callbacks")Signed-off-by Lee Jones <[email protected]>
Quoted 21 lines
--- sound/soc/sof/intel/hda-sdw-bpt.c | 6 ++++++ 1 file changed, 6 insertions(+)sound/soc/sof/intel/hda-sdw-bpt.c +6
-0@@ -456,6 +456,12 @@ int hda_sdw_bpt_close(struct device *dev, int link_id, struct hdac_ext_stream *bint ret;int ret1; +if (bpt_rx_stream &&hdac_stream (bpt_rx_stream)->running) +hda_sdw_bpt_dma_disable (dev, bpt_rx_stream); + +if (bpt_tx_stream &&hdac_stream (bpt_tx_stream)->running) +hda_sdw_bpt_dma_disable (dev, bpt_tx_stream); +/* * In the case of SoundWire we need to reset the PCMSyCM registers. * Need to continue depreparing the DMA buffers even if this fails.
Signature · 1 line
Péter
RE: [PATCH 05/21] ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streams
In reply to Lee JonesQuoted 45 lines
-----Original Message-----
From: Lee Jones <[email protected]>
Sent: Friday, October 30, 2026 6:47 PM
To: [email protected]; Liam Girdwood <[email protected]>; Peter Ujfalusi <[email protected]>; Bard Liao <[email protected]>; Daniel Baluta <[email protected]>; Kai Vehmanen <[email protected]>; Pierre-Louis Bossart <pierre- [email protected]>; Vijendar Mukunda <[email protected]>; Mark Brown <[email protected]>; Jaroslav Kysela <[email protected]>; Takashi Iwai <[email protected]>; Vinod Koul <[email protected]>; Ranjani Sridharan <[email protected]>; [email protected]; [email protected]; [email protected]
Cc: [email protected]
Subject: [PATCH 05/21] ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streamsWhen
sdw_enable_stream()fails inintel_ace2x_bpt_send_async()afterhda_sdw_bpt_send_async()has already started the TX and RX DMA streams,intel_ace2x_bpt_close_stream()invokeshda_sdw_bpt_close()directly without callinghda_sdw_bpt_wait(). As a result,hda_sdw_bpt_dma_deprepare()tears down the DMA buffers and BDL while the DMA hardware is still running.Disable any running TX or RX BPT DMA stream at the start of
hda_sdw_bpt_close()before unmapping channels and depreparing DMA resources.Fixes 4c1ce9f37d8a ("soundwire: intel_ace2x: add BPT send_async/waitcallbacks")
Signed-off-by Lee Jones <[email protected]>--- sound/soc/sof/intel/hda-sdw-bpt.c | 6 ++++++ 1 file changed, 6 insertions(+)sound/soc/sof/intel/hda-sdw- renamed from sound/soc/sof/intel/hda-sdw-bpt.c +0
-0
bpt.c
indexba9fbd662b85..9d97d8676251100644sound/soc/sof/intel/hda-sdw-bpt.c +0
-0@@ -456,6 +456,12 @@ int hda_sdw_bpt_close(struct device *dev, int link_id,struct
hdac_ext_stream*b
sound/soc/sof/intel/hda-sdw-bpt.c int hda_sdw_bpt_close(struct device *dev, int link_id, struct hdac_ext_stream *b · line 459
Quoted 4 lines
sound/soc/sof/intel/hda-sdw-bpt.c +1
-0int ret;int ret1; +if (bpt_rx_stream &&hdac_stream (bpt_rx_stream)->running)
bpt_rx_stream and bpt_tx_stream will never be NULL. The current code doesn't test it, too.
Quoted 11 lines
sound/soc/sof/intel/hda-sdw-bpt.c +5
-1+hda_sdw_bpt_dma_disable (dev, bpt_rx_stream); + +if (bpt_tx_stream &&hdac_stream (bpt_tx_stream)->running) +hda_sdw_bpt_dma_disable (dev, bpt_tx_stream); +/* * In the case of SoundWire we need to reset the PCMSyCM registers. * Need to continue depreparing the DMA buffers even if this fails. -- 2.56.0.317.g454f8302e1-goog
PATCH 6/21 soundwire: intel_ace2x: Protect bus.bpt_stream with bus_lock
In reply to Lee Jonesintel_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.
--- drivers/soundwire/intel_ace2x.c | 36 ++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 16 deletions(-)
drivers/soundwire/intel_ace2x.c +20 -16
@@ -83,17 +83,19 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *int len;int i; -if (cdns->bus.bpt_stream) { -dev_err (cdns->dev,"%s: BPT stream already exists\n" , __func__); -return -EAGAIN ; + scoped_guard(mutex, &cdns->bus.bus_lock) { +if (cdns->bus.bpt_stream) { +dev_err (cdns->dev,"%s: BPT stream already exists\n" , __func__); +return -EAGAIN ; + } + + stream =sdw_alloc_stream ("BPT" ,SDW_STREAM_BPT ); +if (!stream) +return -ENOMEM ; + + cdns->bus.bpt_stream = stream; } - stream =sdw_alloc_stream ("BPT" ,SDW_STREAM_BPT ); -if (!stream) -return -ENOMEM ; - - cdns->bus.bpt_stream = stream; - ret =sdw_slave_bpt_stream_add (slave, stream);if (ret <0 ) goto release_stream;
@@ -105,7 +107,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 +119,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 +138,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 +151,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 +295,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: -sdw_release_stream (cdns->bus.bpt_stream); - cdns->bus.bpt_stream =NULL ; + scoped_guard(mutex, &cdns->bus.bus_lock) { +sdw_release_stream (cdns->bus.bpt_stream); + cdns->bus.bpt_stream =NULL ; + }return ret; }
@@ -335,6 +338,7 @@ static void intel_ace2x_bpt_close_stream(struct sdw_intel *sdw, struct sdw_slavedev_err (cdns->dev,"%s: remove slave failed: %d\n" , __func__, ret); +guard (mutex)(&cdns->bus.bus_lock);sdw_release_stream (cdns->bus.bpt_stream); cdns->bus.bpt_stream =NULL ; }
Signature · 1 line
2.56.0.317.g454f8302e1-goog
[email protected]
AI reviewRe: [PATCH 06/21] soundwire: intel_ace2x: Protect bus.bpt_stream with bus_lock
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] soundwire: mix goto and scoped_guard() in intel_ace2x_bpt_open_stream() --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] soundwire: mix goto and scoped_guard() in intel_ace2x_bpt_open_stream() --
commit c2ba69ef3a3456c129416a89de7531b53eb520ca
Author: Lee Jones <[email protected]>
soundwire: intel_ace2x: Protect bus.bpt_stream with bus_lock
This commit protects checks, assignments, and clearing of cdns->bus.bpt_stream with cdns->bus.bus_lock in intel_ace2x_bpt_open_stream() and intel_ace2x_bpt_close_stream(), and ensures both master and slave stream runtimes are removed on open failure.
Quoted 31 lines
drivers/soundwire/intel_ace2x.c +10
-8@@ -83,16 +83,18 @@ static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave *int len;int i; -if (cdns->bus.bpt_stream) { -dev_err (cdns->dev,"%s: BPT stream already exists\n" , __func__); -return -EAGAIN ; - } + scoped_guard(mutex, &cdns->bus.bus_lock) { +if (cdns->bus.bpt_stream) { +dev_err (cdns->dev,"%s: BPT stream already exists\n" , __func__); +return -EAGAIN ; + } - stream =sdw_alloc_stream ("BPT" ,SDW_STREAM_BPT ); -if (!stream) -return -ENOMEM ; + stream =sdw_alloc_stream ("BPT" ,SDW_STREAM_BPT ); +if (!stream) +return -ENOMEM ; - cdns->bus.bpt_stream = stream; + cdns->bus.bpt_stream = stream; + } ret =sdw_slave_bpt_stream_add (slave, stream);if (ret <0 )
[ ... ]
Quoted 7 lines
drivers/soundwire/intel_ace2x.c +0
-1@@ -293,15 +295,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 )
drivers/soundwire/intel_ace2x.c static int intel_ace2x_bpt_open_stream(struct sdw_intel *sdw, struct sdw_slave * · line 307
Quoted 10 lines
drivers/soundwire/intel_ace2x.c +4
-2dev_err (cdns->dev,"%s: remove slave failed: %d\n" , __func__, ret1); release_stream: -sdw_release_stream (cdns->bus.bpt_stream); - cdns->bus.bpt_stream =NULL ; + scoped_guard(mutex, &cdns->bus.bus_lock) { +sdw_release_stream (cdns->bus.bpt_stream); + cdns->bus.bpt_stream =NULL ; + }
[Severity: Low]
This isn't a bug, but intel_ace2x_bpt_open_stream() relies on multi-label goto error unwinding (goto release_stream, goto remove_master, and goto deprepare_stream) while this patch introduces scoped_guard(mutex, &cdns->bus.bus_lock) blocks at the start of the function and under the release_stream label.
Since kernel cleanup API conventions documented in include/linux/cleanup.h discourage mixing goto-based error handling with scope-based cleanup helpers in the same function, would it be better to use explicit mutex_lock() and mutex_unlock() on cdns->bus.bus_lock here instead?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
PATCH 7/21 soundwire: qcom: Prevent pconfig buffer overflow in qcom_swrm_stream_alloc_ports()
In reply to Lee Jonesqcom_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].
--- drivers/soundwire/qcom.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/soundwire/qcom.c +5 -0
@@ -1343,6 +1343,11 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,list_for_each_entry (s_rt, &m_rt->slave_rt_list, m_rt_node) { slave = s_rt->slave;list_for_each_entry (p_rt, &s_rt->port_list, port_node) { +if (nports >= maxport) { +dev_err (ctrl->dev,"All ports busy\n" ); +return -EBUSY ; + } + m_port = slave->m_port_map[p_rt->num];/* Port numbers start from 1 - 14*/ if (m_port)
Signature · 1 line
2.56.0.317.g454f8302e1-goog
[email protected]
AI reviewRe: [PATCH 07/21] 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: - [High] soundwire: qcom: port_mask leak on error in qcom_swrm_stream_alloc_ports() --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: qcom: port_mask leak on error in qcom_swrm_stream_alloc_ports() --
commit 67cffb71aa319b54456dfa761fc2fc2da4b71496
Author: Lee Jones <[email protected]>
soundwire: qcom: Prevent pconfig buffer overflow in qcom_swrm_stream_alloc_ports()
This commit adds a bounds check in qcom_swrm_stream_alloc_ports() to verify that nports is less than maxport before writing to pconfig[nports], preventing an out-of-bounds write when more slave ports are requested than ctrl->nports.
drivers/soundwire/qcom.c static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl, · line 1349
Quoted 12 lines
drivers/soundwire/qcom.c +4
-0@@ -1343,6 +1343,11 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,list_for_each_entry (s_rt, &m_rt->slave_rt_list, m_rt_node) { slave = s_rt->slave;list_for_each_entry (p_rt, &s_rt->port_list, port_node) { +if (nports >= maxport) { +dev_err (ctrl->dev,"All ports busy\n" ); +return -EBUSY ; + }
[Severity: High]
Does returning -EBUSY here leak the port allocations in ctrl->port_mask?
In qcom_swrm_stream_alloc_ports(), each loop iteration over s_rt->port_list sets bit pn in ctrl->port_mask and records pn in the local pconfig array:
set_bit(pn, port_mask); pconfig[nports].num = pn; pconfig[nports].ch_mask = p_rt->ch_mask; nports++;
When nports reaches maxport on a subsequent iteration and returns -EBUSY (or when the pn >= maxport check below triggers after earlier iterations already succeeded), qcom_swrm_stream_alloc_ports() returns before calling sdw_stream_add_master().
Although the caller qcom_swrm_hw_params() invokesqcom_swrm_stream_free_ports() on error:
ret = qcom_swrm_stream_alloc_ports(ctrl, sruntime, params, substream->stream); if (ret) qcom_swrm_stream_free_ports(ctrl, sruntime);
qcom_swrm_stream_free_ports() only clears bits in ctrl->port_mask by iterating over m_rt->port_list:
list_for_each_entry(m_rt, &stream->master_list, stream_node) {
port_mask = &ctrl->port_mask;
list_for_each_entry(p_rt, &m_rt->port_list, port_node)
clear_bit(p_rt->num, port_mask);
}Because m_rt->port_list is only populated inside sdw_stream_add_master() and pconfig is freed on return, m_rt->port_list is still empty when qcom_swrm_stream_alloc_ports() fails, leaving all previously set bits in ctrl->port_mask permanently set.
Should qcom_swrm_stream_alloc_ports() unwind the bits set in port_mask from pconfig[0 .. nports - 1] before returning -EBUSY?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
PATCH 8/21 soundwire: qcom: Disable runtime PM and avoid clock underflow in qcom_swrm_remove()
In reply to Lee JonesWhen 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.
--- drivers/soundwire/qcom.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/soundwire/qcom.c +6 -1
@@ -1791,8 +1791,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.317.g454f8302e1-goog
PATCH 9/21 soundwire: qcom: Check clk_prepare_enable() return value
In reply to Lee Jonesqcom_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.
--- drivers/soundwire/qcom.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/soundwire/qcom.c +11 -5
@@ -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);
@@ -1680,7 +1682,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);
@@ -1736,7 +1740,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; } }
@@ -1805,13 +1809,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.317.g454f8302e1-goog
PATCH 10/21 soundwire: qcom: Serialise ctrl->intr_mask updates
In reply to Lee Jonesqcom_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.
--- drivers/soundwire/qcom.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/soundwire/qcom.c +23 -2
@@ -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,
@@ -1689,6 +1705,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);
@@ -1854,6 +1871,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,
@@ -1861,6 +1879,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))
@@ -1882,6 +1901,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,
@@ -1889,6 +1909,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.317.g454f8302e1-goog
PATCH 11/21 soundwire: qcom: Prevent stale broadcast IRQ from completing subsequent command
In reply to Lee JonesWhen 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.
--- drivers/soundwire/qcom.c | 44 +++++++++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 7 deletions(-)
drivers/soundwire/qcom.c +37 -7
@@ -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.317.g454f8302e1-goog
PATCH 12/21 soundwire: amd: Check for NULL manager drvdata in sdw_amd_startup()
In reply to Lee JonesIf amd_sdw_manager_probe() fails or defers when registering a SoundWire manager platform device, dev_get_drvdata(&ctx->pdev[i]->dev) returns NULL. sdw_amd_startup() passes this pointer directly to amd_sdw_manager_start(), which dereferences it unconditionally.
Return -ENODEV in sdw_amd_startup() if dev_get_drvdata() returns NULL, matching sdw_amd_get_slave_info(), and clean up the allocated context and platform devices via sdw_amd_exit() if sdw_amd_startup() fails in sdw_amd_probe().
--- drivers/soundwire/amd_init.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/soundwire/amd_init.c +12 -1
@@ -156,6 +156,8 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)if (!(ctx->link_mask &BIT (i)))continue ; amd_manager =dev_get_drvdata (&ctx->pdev[i]->dev); +if (!amd_manager) +return -ENODEV ; ret =amd_sdw_manager_start (amd_manager);if (ret)return ret;
@@ -166,11 +168,20 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)int sdw_amd_probe (struct sdw_amd_res *res,struct sdw_amd_ctx **sdw_ctx) { +int ret; + *sdw_ctx =sdw_amd_probe_controller (res);if (!*sdw_ctx)return -ENODEV ; -return sdw_amd_startup (*sdw_ctx); + ret =sdw_amd_startup (*sdw_ctx); +if (ret) { +sdw_amd_exit (*sdw_ctx); + *sdw_ctx =NULL ; +return ret; + } + +return 0 ; }EXPORT_SYMBOL_NS (sdw_amd_probe,"SOUNDWIRE_AMD_INIT" );
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 12/21] soundwire: amd: Check for NULL manager drvdata in sdw_amd_startup()
Quoted 11 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
If
amd_sdw_manager_probe()fails or defers when registering a SoundWire manager platform device,dev_get_drvdata(&ctx->pdev[i]->dev)returns NULL.sdw_amd_startup()passes this pointer directly toamd_sdw_manager_start(), which dereferences it unconditionally.Return
-ENODEVinsdw_amd_startup()ifdev_get_drvdata()returns NULL, matchingsdw_amd_get_slave_info(), and clean up the allocated context and platform devices viasdw_amd_exit()ifsdw_amd_startup()fails insdw_amd_probe().
The sdw_amd_probe() cleanup fix — calling sdw_amd_exit() and nulling *sdw_ctx on sdw_amd_startup() failure — is already addressed in our v3 patch series posted for upstream review:
https://lore.kernel.org/alsa-devel/[email protected]/T/#u
The NULL check in sdw_amd_startup() is a valid observation and will be addressed in our upcoming v4 patch series. Please hold this patch until v4 is on-list to avoid conflicts.
Quoted 46 lines
Fixes ed5e8741b8db ("soundwire: amd: refactor amd soundwire manager device node creation")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_init.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)drivers/soundwire/amd_init.c +12
-2@@ -156,6 +156,8 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)if (!(ctx->link_mask &BIT (i)))continue ; amd_manager =dev_get_drvdata (&ctx->pdev[i]->dev); +if (!amd_manager) +return -ENODEV ; ret =amd_sdw_manager_start (amd_manager);if (ret)return ret;@@ -166,11 +168,20 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)int sdw_amd_probe (struct sdw_amd_res *res,struct sdw_amd_ctx **sdw_ctx) { +int ret; + *sdw_ctx =sdw_amd_probe_controller (res);if (!*sdw_ctx)return -ENODEV ; -return sdw_amd_startup (*sdw_ctx); + ret =sdw_amd_startup (*sdw_ctx); +if (ret) { +sdw_amd_exit (*sdw_ctx); + *sdw_ctx =NULL ; +return ret; + } + +return 0 ; }EXPORT_SYMBOL_NS (sdw_amd_probe,"SOUNDWIRE_AMD_INIT" ); --2.56.0.317.g454f8302e1-goog
PATCH 13/21 soundwire: amd: Check ping command response for timeout
In reply to Lee JonesWhen amd_sdw_send_cmd_get_resp() times out, it returns -ETIMEDOUT as a sign-extended 64-bit value. amd_sdw_read_and_process_ping_status() and amd_sdw_read_ping_status() extract peripheral status bits directly from the returned value without checking for -ETIMEDOUT, interpreting the error code bits as peripheral status values.
Check for -ETIMEDOUT before extracting peripheral status from the ping response, matching amd_program_scp_addr() and amd_sdw_fill_msg_resp().
--- drivers/soundwire/amd_manager.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/soundwire/amd_manager.c +4 -0
@@ -448,6 +448,8 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_manmutex_lock (&amd_manager->bus.msg_lock); response =amd_sdw_send_cmd_get_resp (amd_manager,0 ,0 );mutex_unlock (&amd_manager->bus.msg_lock); +if (response == -ETIMEDOUT ) +return ;amd_sdw_process_ping_status (response, amd_manager); }
@@ -458,6 +460,8 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)u32 slave_stat; response =amd_sdw_send_cmd_get_resp (amd_manager,0 ,0 ); +if (response == -ETIMEDOUT ) +return 0 ;/* slave status from ping response */ slave_stat =FIELD_GET (AMD_SDW_MCP_SLAVE_STAT_0_3 , response); slave_stat |=FIELD_GET (AMD_SDW_MCP_SLAVE_STAT_4_11 , response) <<8 ;
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 13/21] soundwire: amd: Check ping command response for timeout
Quoted 24 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
When
amd_sdw_send_cmd_get_resp()times out, it returns-ETIMEDOUTas a sign-extended 64-bit value.amd_sdw_read_and_process_ping_status()andamd_sdw_read_ping_status()extract peripheral status bits directly from the returned value without checking for-ETIMEDOUT, interpreting the error code bits as peripheral status values.Check for
-ETIMEDOUTbefore extracting peripheral status from the ping response, matchingamd_program_scp_addr()andamd_sdw_fill_msg_resp().Fixes d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/soundwire/amd_manager.c +0
-0
drivers/soundwire/amd_manager.c static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man · line 452
Quoted 5 lines
drivers/soundwire/amd_manager.c +2
-0@@ -448,6 +448,8 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_manmutex_lock (&amd_manager->bus.msg_lock); response =amd_sdw_send_cmd_get_resp (amd_manager,0 ,0 );mutex_unlock (&amd_manager->bus.msg_lock); +if (response == -ETIMEDOUT ) +return ;
++
Response should be u64.
This patch can be dropped.
Please refer below fix.
https://lore.kernel.org/alsa-devel/[email protected]/T/#m786cce22308c97d055566dce17be5a38b5d98e9e
I am going to post V4 patch set.
Quoted 14 lines
drivers/soundwire/amd_manager.c +2
-1amd_sdw_process_ping_status (response, amd_manager); }@@ -458,6 +460,8 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)u32 slave_stat; response =amd_sdw_send_cmd_get_resp (amd_manager,0 ,0 ); +if (response == -ETIMEDOUT ) +return 0 ;/* slave status from ping response */ slave_stat =FIELD_GET (AMD_SDW_MCP_SLAVE_STAT_0_3 , response); slave_stat |=FIELD_GET (AMD_SDW_MCP_SLAVE_STAT_4_11 , response) <<8 ; --2.56.0.317.g454f8302e1-goog
Re: [PATCH 13/21] soundwire: amd: Check ping command response for timeout
Quoted 34 lines · Mukunda,Vijendar
Quoted 24 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
When
amd_sdw_send_cmd_get_resp()times out, it returns-ETIMEDOUTas a sign-extended 64-bit value.amd_sdw_read_and_process_ping_status()andamd_sdw_read_ping_status()extract peripheral status bits directly from the returned value without checking for-ETIMEDOUT, interpreting the error code bits as peripheral status values.Check for
-ETIMEDOUTbefore extracting peripheral status from the ping response, matchingamd_program_scp_addr()andamd_sdw_fill_msg_resp().Fixes d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/soundwire/amd_manager.c +0
-0drivers/soundwire/amd_manager.c static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man · line 452
Quoted 5 lines
drivers/soundwire/amd_manager.c +2
-0@@ -448,6 +448,8 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_manmutex_lock (&amd_manager->bus.msg_lock); response =amd_sdw_send_cmd_get_resp (amd_manager,0 ,0 );mutex_unlock (&amd_manager->bus.msg_lock); +if (response == -ETIMEDOUT ) +return ;++
Quoted 5 lines
Response should be u64.
This patch can be dropped.
Please refer below fix.
https://lore.kernel.org/alsa-devel/[email protected]/T/#m786cce22308c97d055566dce17be5a38b5d98e9e
I am going to post V4 patch set.
Excellent. Will do. Thanks again.
Signature · 1 line
Lee Jones
[email protected]
AI reviewRe: [PATCH 13/21] soundwire: amd: Check ping command response for timeout
In reply to Lee JonesThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] soundwire: amd: stale status[] processed on ping timeout --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] soundwire: amd: stale status[] processed on ping timeout
--
commit 390668deca85bb859cd47cec7a3a11161bb84e4d
Author: Lee Jones <[email protected]>
soundwire: amd: Check ping command response for timeout
This commit adds checks for -ETIMEDOUT after calling amd_sdw_send_cmd_get_resp() in amd_sdw_read_and_process_ping_status() and amd_sdw_read_ping_status() so that a timeout error code is not decoded as peripheral status bits.
drivers/soundwire/amd_manager.c static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man · line 452
Quoted 10 lines
drivers/soundwire/amd_manager.c +2
-0@@ -448,6 +448,8 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_manmutex_lock (&amd_manager->bus.msg_lock); response =amd_sdw_send_cmd_get_resp (amd_manager,0 ,0 );mutex_unlock (&amd_manager->bus.msg_lock); +if (response == -ETIMEDOUT ) +return ;
[Severity: Medium]
When amd_sdw_send_cmd_get_resp() times out here, returning early without propagating an error leaves amd_manager->status[] containing stale status values while callers proceed as if the status array was refreshed.
In drivers/soundwire/amd_manager.c, amd_sdw_update_slave_status_work() relies on amd_sdw_read_and_process_ping_status() to overwrite amd_manager->status[] before jumping back to update_status:
if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
if (retry_count++ < SDW_MAX_DEVICES) {
writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
amd_sdw_read_and_process_ping_status(amd_manager);
goto update_status;
}
...
}Because sdw_handle_slave_status() only reads amd_manager->status[] and does not clear status[0], a ping timeout leaves status[0] set to SDW_SLAVE_ATTACHED (along with any other stale entries in status[]), causing the loop to repeatedly call sdw_handle_slave_status() with stale peripheral status up to SDW_MAX_DEVICES times.
Similarly, in amd_sdw_irq_thread():
if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
amd_sdw_read_and_process_ping_status(amd_manager);
} else {
/* Check for the updated status on peripheral device */
amd_sdw_update_slave_status(status_change_0to7, status_change_8to11, amd_manager);
}
if (status_change_8to11 || status_change_0to7)
schedule_work(&amd_manager->amd_sdw_work);If the ping command times out when handling AMD_SDW_PREQ_INTR_STAT, amd_sdw_work is still scheduled and will process stale status values left in amd_manager->status[] from a previous interrupt.
Should amd_sdw_read_and_process_ping_status() return an error code so that its callers can avoid reprocessing stale amd_manager->status[] entries on timeout?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
PATCH 14/21 soundwire: amd: Preserve lane control bits in amd_sdw_port_enable()
In reply to Lee Joneslane_ctrl_ch_en_reg holds both the channel enable mask (AMD_DPN_CH_EN_CHMASK) and the lane control field (AMD_DPN_CH_EN_LCTRL) programmed by amd_sdw_transport_params(). When enable_ch->enable is false, amd_sdw_port_enable() writes 0 to the entire register instead of clearing only AMD_DPN_CH_EN_CHMASK, wiping out the configured lane control bits.
Clear only the AMD_DPN_CH_EN_CHMASK field when disabling port channels.
--- drivers/soundwire/amd_manager.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/soundwire/amd_manager.c +3 -3
@@ -706,11 +706,11 @@ static int amd_sdw_port_enable(struct sdw_bus *bus,* parameters. */ dpn_ch_enable =readl (amd_manager->mmio + lane_ctrl_ch_en_reg); -u32p_replace_bits (&dpn_ch_enable, enable_ch->ch_mask,AMD_DPN_CH_EN_CHMASK );if (enable_ch->enable) -writel (dpn_ch_enable, amd_manager->mmio + lane_ctrl_ch_en_reg); +u32p_replace_bits (&dpn_ch_enable, enable_ch->ch_mask,AMD_DPN_CH_EN_CHMASK );else -writel (0 , amd_manager->mmio + lane_ctrl_ch_en_reg); +u32p_replace_bits (&dpn_ch_enable,0 ,AMD_DPN_CH_EN_CHMASK ); +writel (dpn_ch_enable, amd_manager->mmio + lane_ctrl_ch_en_reg);return 0 ; }
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 14/21] soundwire: amd: Preserve lane control bits in amd_sdw_port_enable()
Quoted 10 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
lane_ctrl_ch_en_regholds both the channel enable mask (AMD_DPN_CH_EN_CHMASK) and the lane control field (AMD_DPN_CH_EN_LCTRL) programmed byamd_sdw_transport_params(). Whenenable_ch->enableis false,amd_sdw_port_enable()writes 0 to the entire register instead of clearing onlyAMD_DPN_CH_EN_CHMASK, wiping out the configured lane control bits.Clear only the
AMD_DPN_CH_EN_CHMASKfield when disabling port channels.
Multi-lane is not supported on any of the platforms handled by this function (ACP6.3, ACP7.0, ACP7.1, ACP7.2). AMD_DPN_CH_EN_LCTRL (bits [2:0]) is never programmed for these platforms, so it always reads back as 0. Writing 0 to the entire register is therefore functionally identical to clearing only AMD_DPN_CH_EN_CHMASK.
Upcoming AMD platforms use an entirely different implementation and will not use this code path either.
The described bug has no practical impact on any hardware this code runs on, so the Fixes: tag and Cc: [email protected] are not warranted. Please drop this patch.
Quoted 30 lines
Fixes d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/soundwire/amd_manager.c +3
-4@@ -706,11 +706,11 @@ static int amd_sdw_port_enable(struct sdw_bus *bus,* parameters. */ dpn_ch_enable =readl (amd_manager->mmio + lane_ctrl_ch_en_reg); -u32p_replace_bits (&dpn_ch_enable, enable_ch->ch_mask,AMD_DPN_CH_EN_CHMASK );if (enable_ch->enable) -writel (dpn_ch_enable, amd_manager->mmio + lane_ctrl_ch_en_reg); +u32p_replace_bits (&dpn_ch_enable, enable_ch->ch_mask,AMD_DPN_CH_EN_CHMASK );else -writel (0 , amd_manager->mmio + lane_ctrl_ch_en_reg); +u32p_replace_bits (&dpn_ch_enable,0 ,AMD_DPN_CH_EN_CHMASK ); +writel (dpn_ch_enable, amd_manager->mmio + lane_ctrl_ch_en_reg);return 0 ; } --2.56.0.317.g454f8302e1-goog
Re: [PATCH 14/21] soundwire: amd: Preserve lane control bits in amd_sdw_port_enable()
Quoted 20 lines · Mukunda,Vijendar
Quoted 10 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
lane_ctrl_ch_en_regholds both the channel enable mask (AMD_DPN_CH_EN_CHMASK) and the lane control field (AMD_DPN_CH_EN_LCTRL) programmed byamd_sdw_transport_params(). Whenenable_ch->enableis false,amd_sdw_port_enable()writes 0 to the entire register instead of clearing onlyAMD_DPN_CH_EN_CHMASK, wiping out the configured lane control bits.Clear only the
AMD_DPN_CH_EN_CHMASKfield when disabling port channels.Multi-lane is not supported on any of the platforms handled by this function (ACP6.3, ACP7.0, ACP7.1, ACP7.2).
AMD_DPN_CH_EN_LCTRL(bits [2:0]) is never programmed for these platforms, so it always reads back as 0. Writing 0 to the entire register is therefore functionally identical to clearing onlyAMD_DPN_CH_EN_CHMASK.
Quoted 6 lines
Upcoming AMD platforms use an entirely different implementation and will not use this code path either.
The described bug has no practical impact on any hardware this code runs on, so the Fixes: tag and Cc: [email protected] are not warranted. Please drop this patch.
Thanks for the review.
Wilco. I'll go manually turn down the Sashiko bug to this effect as well.
Signature · 1 line
Lee Jones
PATCH 15/21 soundwire: amd: Avoid static port offset leak in amd_sdw_compute_params()
In reply to Lee Jonesamd_sdw_compute_params() tracks the next dynamic port block offset in a static next_offset[] array initialised with {1} (which also leaves instance 1 initialised to 0). Because next_offset[] is static and never reset when streams are freed or when the driver is unbound and rebound, the offset grows monotonically across stream lifecycles until it exceeds max_slots - 1 and permanently fails with -ENOMEM.
Compute next_offset locally in amd_sdw_compute_params() from the active ports on the bus and clear port_offset_map[dai->id] in amd_sdw_hw_free().
--- drivers/soundwire/amd_manager.c | 48 +++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 5 deletions(-)
drivers/soundwire/amd_manager.c +43 -5
@@ -469,6 +469,41 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)return slave_stat; } +static unsigned int amd_sdw_find_free_port_offset (struct sdw_bus *bus, +unsigned int slot_size) +{ +struct amd_sdw_manager *amd_manager =to_amd_sdw (bus); +struct sdw_master_runtime *m_rt; +struct sdw_port_runtime *p_rt; +unsigned int next_offset =1 ; +bool overlap; + +do { + overlap = false; +list_for_each_entry (m_rt, &bus->m_rt_list, bus_node) { +unsigned int other_size = m_rt->stream->params.bps * + m_rt->stream->params.ch_count; + +list_for_each_entry (p_rt, &m_rt->port_list, port_node) { +unsigned int other_bo; + +if (p_rt->num >= amd_manager->max_ports) +continue ; + other_bo = amd_manager->port_offset_map[p_rt->num]; +if (!other_bo) +continue ; +if (next_offset < other_bo + other_size && + other_bo < next_offset + slot_size) { + next_offset = other_bo + other_size; + overlap = true; + } + } + } + }while (overlap); + +return next_offset; +} +static int amd_sdw_compute_params (struct sdw_bus *bus,struct sdw_stream_runtime *stream) {struct amd_sdw_manager *amd_manager =to_amd_sdw (bus);
@@ -479,8 +514,7 @@ static int amd_sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtimeint port_bo, hstart, hstop, sample_int;unsigned int rate, bps, channels;unsigned int stream_slot_size, max_slots; -static unsigned int next_offset[AMD_SDW_MAX_MANAGER_COUNT ] = {1 }; -unsigned int inst_id = amd_manager->instance; +unsigned int next_offset; port_bo =0 ; hstart =1 ;
@@ -511,11 +545,13 @@ static int amd_sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime*/ if (bus->params.curr_dr_freq ==12000000 ) { max_slots = bus->params.row * (bus->params.col -1 ); -if (next_offset[inst_id] + stream_slot_size <= + next_offset = +amd_sdw_find_free_port_offset (bus, + stream_slot_size); +if (next_offset + stream_slot_size <= (max_slots -1 )) { amd_manager->port_offset_map[p_rt->num] = - next_offset[inst_id]; - next_offset[inst_id] += stream_slot_size; + next_offset; }else {dev_err (bus->dev,"No space for port %d\n" , p_rt->num);
@@ -824,6 +860,8 @@ static int amd_sdw_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dif (ret <0 )dev_err (dai->dev,"remove manager from stream %s failed: %d\n" , dai_runtime->stream->name, ret); +if (dai->id >=0 && dai->id < amd_manager->max_ports) + amd_manager->port_offset_map[dai->id] =0 ;return ret; }
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 15/21] soundwire: amd: Avoid static port offset leak in amd_sdw_compute_params()
Quoted 9 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_compute_params()tracks the next dynamic port block offset in a static next_offset[] array initialised with {1} (which also leaves instance 1 initialised to 0). Because next_offset[] is static and never reset when streams are freed or when the driver is unbound and rebound, the offset grows monotonically across stream lifecycles until it exceedsmax_slots- 1 and permanently fails with-ENOMEM.
Quoted 3 lines
Compute
next_offsetlocally inamd_sdw_compute_params()from the active ports on the bus and clear port_offset_map[dai->id] inamd_sdw_hw_free().
This is not a bug for ACP6.3/7.0/7.1/7.2 platforms. These variants do not have banked registers for data port programming. Instead, a fixed port mapping per DAI ID is used by design: each use case (e.g. headset playback, headphone capture) always maps to the same DAI ID and port number across stream lifecycles.
The guard at line 402:
if (!amd_manager->port_offset_map[p_rt->num])
ensures that port_offset_map[] is populated exactly once per port at first use and never reassigned. Since the DAI-to-port mapping is fixed, next_offset[] advancing after initial assignment has no effect on subsequent stream open/close cycles.
Furthermore, the hw_free change introduced by this patch:
amd_manager->port_offset_map[dai->id] = 0;
would be a regression. Clearing the map entry on stream teardown causes the port to be reassigned a new block offset on the next stream open, at which point next_offset[] has already advanced past the original value. This breaks the fixed mapping invariant that the driver relies on.
The number of concurrent streams per instance is known and bounded, so -ENOMEM from slot exhaustion cannot occur in practice. Please drop this patch.
Quoted 93 lines
Fixes 27ab4f1e4909 ("soundwire: amd: refactor bandwidth calculation logic")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 48 +++++++++++++++++++++++++++++---- 1 file changed, 43 insertions(+), 5 deletions(-)drivers/soundwire/amd_manager.c +43
-6@@ -469,6 +469,41 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)return slave_stat; } +static unsigned int amd_sdw_find_free_port_offset (struct sdw_bus *bus, +unsigned int slot_size) +{ +struct amd_sdw_manager *amd_manager =to_amd_sdw (bus); +struct sdw_master_runtime *m_rt; +struct sdw_port_runtime *p_rt; +unsigned int next_offset =1 ; +bool overlap; + +do { + overlap = false; +list_for_each_entry (m_rt, &bus->m_rt_list, bus_node) { +unsigned int other_size = m_rt->stream->params.bps * + m_rt->stream->params.ch_count; + +list_for_each_entry (p_rt, &m_rt->port_list, port_node) { +unsigned int other_bo; + +if (p_rt->num >= amd_manager->max_ports) +continue ; + other_bo = amd_manager->port_offset_map[p_rt->num]; +if (!other_bo) +continue ; +if (next_offset < other_bo + other_size && + other_bo < next_offset + slot_size) { + next_offset = other_bo + other_size; + overlap = true; + } + } + } + }while (overlap); + +return next_offset; +} +static int amd_sdw_compute_params (struct sdw_bus *bus,struct sdw_stream_runtime *stream) {struct amd_sdw_manager *amd_manager =to_amd_sdw (bus);@@ -479,8 +514,7 @@ static int amd_sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtimeint port_bo, hstart, hstop, sample_int;unsigned int rate, bps, channels;unsigned int stream_slot_size, max_slots; -static unsigned int next_offset[AMD_SDW_MAX_MANAGER_COUNT ] = {1 }; -unsigned int inst_id = amd_manager->instance; +unsigned int next_offset; port_bo =0 ; hstart =1 ;@@ -511,11 +545,13 @@ static int amd_sdw_compute_params(struct sdw_bus *bus, struct sdw_stream_runtime*/ if (bus->params.curr_dr_freq ==12000000 ) { max_slots = bus->params.row * (bus->params.col -1 ); -if (next_offset[inst_id] + stream_slot_size <= + next_offset = +amd_sdw_find_free_port_offset (bus, + stream_slot_size); +if (next_offset + stream_slot_size <= (max_slots -1 )) { amd_manager->port_offset_map[p_rt->num] = - next_offset[inst_id]; - next_offset[inst_id] += stream_slot_size; + next_offset; }else {dev_err (bus->dev,"No space for port %d\n" , p_rt->num);@@ -824,6 +860,8 @@ static int amd_sdw_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dif (ret <0 )dev_err (dai->dev,"remove manager from stream %s failed: %d\n" , dai_runtime->stream->name, ret); +if (dai->id >=0 && dai->id < amd_manager->max_ports) + amd_manager->port_offset_map[dai->id] =0 ;return ret; } --2.56.0.317.g454f8302e1-goog
Re: [PATCH 15/21] soundwire: amd: Avoid static port offset leak in amd_sdw_compute_params()
Quoted 39 lines · Mukunda,Vijendar
Quoted 9 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_compute_params()tracks the next dynamic port block offset in a static next_offset[] array initialised with {1} (which also leaves instance 1 initialised to 0). Because next_offset[] is static and never reset when streams are freed or when the driver is unbound and rebound, the offset grows monotonically across stream lifecycles until it exceedsmax_slots- 1 and permanently fails with-ENOMEM.Quoted 3 lines
Compute
next_offsetlocally inamd_sdw_compute_params()from the active ports on the bus and clear port_offset_map[dai->id] inamd_sdw_hw_free().This is not a bug for ACP6.3/7.0/7.1/7.2 platforms. These variants do not have banked registers for data port programming. Instead, a fixed port mapping per DAI ID is used by design: each use case (e.g. headset playback, headphone capture) always maps to the same DAI ID and port number across stream lifecycles.
The guard at line 402:
if (!amd_manager->port_offset_map[p_rt->num])
ensures that port_offset_map[] is populated exactly once per port at first use and never reassigned. Since the DAI-to-port mapping is fixed, next_offset[] advancing after initial assignment has no effect on subsequent stream open/close cycles.
Furthermore, the
hw_freechange introduced by this patch:amd_manager->port_offset_map[dai->id] = 0;
would be a regression. Clearing the map entry on stream teardown causes the port to be reassigned a new block offset on the next stream open, at which point next_offset[] has already advanced past the original value. This breaks the fixed mapping invariant that the driver relies on.
Quoted 3 lines
The number of concurrent streams per instance is known and bounded, so
-ENOMEMfrom slot exhaustion cannot occur in practice. Please drop this patch.
Done.
Signature · 1 line
Lee Jones
PATCH 16/21 soundwire: amd: Initialise and cancel IRQ work around manager lifetime
In reply to Lee Jonesamd_sdw_manager_probe() publishes amd_manager via dev_set_drvdata() before initialising amd_sdw_irq_thread and amd_sdw_work, allowing an early interrupt to schedule uninitialised work. During removal, amd_sdw_manager_remove() only cancels amd_sdw_work without first cancelling amd_sdw_irq_thread, allowing deferred IRQ work to re-queue amd_sdw_work or dereference amd_manager after it is freed.
Initialise both work items before calling dev_set_drvdata() in amd_sdw_manager_probe(), and cancel amd_sdw_irq_thread before amd_sdw_work in amd_sdw_manager_remove().
--- drivers/soundwire/amd_manager.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/soundwire/amd_manager.c +2 -1
@@ -1185,9 +1185,9 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)sdw_bus_master_delete (&amd_manager->bus);return ret; } -dev_set_drvdata (dev, amd_manager);INIT_WORK (&amd_manager->amd_sdw_irq_thread, amd_sdw_irq_thread);INIT_WORK (&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work); +dev_set_drvdata (dev, amd_manager);return 0 ; }
@@ -1197,6 +1197,7 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)int ret;pm_runtime_disable (&pdev->dev); +cancel_work_sync (&amd_manager->amd_sdw_irq_thread);cancel_work_sync (&amd_manager->amd_sdw_work);amd_disable_sdw_interrupts (amd_manager);sdw_bus_master_delete (&amd_manager->bus);
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 16/21] soundwire: amd: Initialise and cancel IRQ work around manager lifetime
Quoted 26 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_manager_probe()publishesamd_managerviadev_set_drvdata()before initialisingamd_sdw_irq_threadandamd_sdw_work, allowing an early interrupt to schedule uninitialised work. During removal,amd_sdw_manager_remove()only cancelsamd_sdw_workwithout first cancellingamd_sdw_irq_thread, allowing deferred IRQ work to re-queueamd_sdw_workor dereferenceamd_managerafter it is freed.Initialise both work items before calling
dev_set_drvdata()inamd_sdw_manager_probe(), and cancelamd_sdw_irq_threadbeforeamd_sdw_workinamd_sdw_manager_remove().Fixes 65f93e4096a0 ("soundwire: amd: add SoundWire manager interrupt handling")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/soundwire/amd_manager.c +0
-0
drivers/soundwire/amd_manager.c static int amd_sdw_manager_probe(struct platform_device *pdev) · line 1190
Quoted 7 lines
drivers/soundwire/amd_manager.c +1
-1@@ -1185,9 +1185,9 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)sdw_bus_master_delete (&amd_manager->bus);return ret; } -dev_set_drvdata (dev, amd_manager);INIT_WORK (&amd_manager->amd_sdw_irq_thread, amd_sdw_irq_thread);INIT_WORK (&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work); +dev_set_drvdata (dev, amd_manager);
The probe-side ordering issue is valid — an interrupt arriving after dev_set_drvdata() but before INIT_WORK() could schedule work on an uninitialised work_struct.
The remove-side fix is already posted as part of our v3 patch series:
https://lore.kernel.org/alsa-devel/[email protected]/T/#m60177687bb84758efdd6f5f09eba8ad32f0e4631
The probe-side fix will be included in our upcoming v4 patch series, which contains major driver restructuring and customer-critical bug fixes. Merging this patch independently would conflict with v4 and complicate the review process.
Could you please drop this patch? Both issues will be covered by our v3/v4 series.
drivers/soundwire/amd_manager.c static void amd_sdw_manager_remove(struct platform_device *pdev) · line 1200
Quoted 8 lines
drivers/soundwire/amd_manager.c +1
-0return 0 ; }@@ -1197,6 +1197,7 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)int ret;pm_runtime_disable (&pdev->dev); +cancel_work_sync (&amd_manager->amd_sdw_irq_thread);
proper fix already posted as part of v3 version.
https://lore.kernel.org/alsa-devel/[email protected]/T/#m60177687bb84758efdd6f5f09eba8ad32f0e4631
Quoted 5 lines
drivers/soundwire/amd_manager.c +0
-1cancel_work_sync (&amd_manager->amd_sdw_work);amd_disable_sdw_interrupts (amd_manager);sdw_bus_master_delete (&amd_manager->bus); --2.56.0.317.g454f8302e1-goog
Re: [PATCH 16/21] soundwire: amd: Initialise and cancel IRQ work around manager lifetime
Quoted 44 lines · Mukunda,Vijendar
Quoted 26 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_manager_probe()publishesamd_managerviadev_set_drvdata()before initialisingamd_sdw_irq_threadandamd_sdw_work, allowing an early interrupt to schedule uninitialised work. During removal,amd_sdw_manager_remove()only cancelsamd_sdw_workwithout first cancellingamd_sdw_irq_thread, allowing deferred IRQ work to re-queueamd_sdw_workor dereferenceamd_managerafter it is freed.Initialise both work items before calling
dev_set_drvdata()inamd_sdw_manager_probe(), and cancelamd_sdw_irq_threadbeforeamd_sdw_workinamd_sdw_manager_remove().Fixes 65f93e4096a0 ("soundwire: amd: add SoundWire manager interrupt handling")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/soundwire/amd_manager.c +0
-0drivers/soundwire/amd_manager.c static int amd_sdw_manager_probe(struct platform_device *pdev) · line 1190
Quoted 7 lines
drivers/soundwire/amd_manager.c +1
-1@@ -1185,9 +1185,9 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)sdw_bus_master_delete (&amd_manager->bus);return ret; } -dev_set_drvdata (dev, amd_manager);INIT_WORK (&amd_manager->amd_sdw_irq_thread, amd_sdw_irq_thread);INIT_WORK (&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work); +dev_set_drvdata (dev, amd_manager);The probe-side ordering issue is valid — an interrupt arriving after
dev_set_drvdata()but beforeINIT_WORK()could schedule work on an uninitialisedwork_struct.The remove-side fix is already posted as part of our v3 patch series:
https://lore.kernel.org/alsa-devel/[email protected]/T/#m60177687bb84758efdd6f5f09eba8ad32f0e4631
Quoted 7 lines
The probe-side fix will be included in our upcoming v4 patch series, which contains major driver restructuring and customer-critical bug fixes. Merging this patch independently would conflict with v4 and complicate the review process.
Could you please drop this patch? Both issues will be covered by our v3/v4 series.
Yes, no problem.
Signature · 1 line
Lee Jones
Re: [PATCH 16/21] soundwire: amd: Initialise and cancel IRQ work around manager lifetime
In reply to Mukunda,VijendarQuoted 45 lines · Mukunda,Vijendar
Quoted 26 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_manager_probe()publishesamd_managerviadev_set_drvdata()before initialisingamd_sdw_irq_threadandamd_sdw_work, allowing an early interrupt to schedule uninitialised work. During removal,amd_sdw_manager_remove()only cancelsamd_sdw_workwithout first cancellingamd_sdw_irq_thread, allowing deferred IRQ work to re-queueamd_sdw_workor dereferenceamd_managerafter it is freed.Initialise both work items before calling
dev_set_drvdata()inamd_sdw_manager_probe(), and cancelamd_sdw_irq_threadbeforeamd_sdw_workinamd_sdw_manager_remove().Fixes 65f93e4096a0 ("soundwire: amd: add SoundWire manager interrupt handling")Closes https://sashiko.dev/#/bug/linux-9890e8dc-4b7d-44d3- bfb0-171dc421e28cSigned-off-by Lee Jones <[email protected]>---
drivers/soundwire/amd_manager.c| 3 ++-1 file changed, 2 insertions(+), 1 deletion(-)drivers/soundwire/amd_manager.c +0
-0drivers/soundwire/amd_manager.c static int amd_sdw_manager_probe(struct · line 1191
Quoted 8 lines
drivers/soundwire/amd_manager.c +1
-1@@ -1185,9 +1185,9 @@ static int amd_sdw_manager_probe(struct platform_device *pdev) sdw_bus_master_delete(&amd_manager->bus); return ret; } - dev_set_drvdata(dev, amd_manager); INIT_WORK(&amd_manager->amd_sdw_irq_thread, amd_sdw_irq_thread); INIT_WORK(&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work); + dev_set_drvdata(dev, amd_manager);The probe-side ordering issue is valid — an interrupt arriving after
dev_set_drvdata()but beforeINIT_WORK()could schedule work on an uninitialisedwork_struct.The remove-side fix is already posted as part of our v3 patch series:
https://lore.kernel.org/alsa-devel/20261013085111.2009061-6- [email protected]/T/#m60177687bb84758efdd6f5f09eba8ad32f0e4631
Quoted 8 lines
The probe-side fix will be included in our upcoming v4 patch series, which contains major driver restructuring and customer-critical bug fixes. Merging this patch independently would conflict with v4 and complicate the review process.
Could you please drop this patch? Both issues will be covered by our v3/v4 series.
Lee,
I think as a general statement since there is still these big pending changes already in flight, could you either wait a few weeks until they merge or rebase your series on the in-flight version to see what still actually makes sense?
Re: [PATCH 16/21] soundwire: amd: Initialise and cancel IRQ work around manager lifetime
Quoted 58 lines · Mario Limonciello
Quoted 49 lines · Mukunda,Vijendar
Quoted 30 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_manager_probe()publishesamd_managerviadev_set_drvdata()before initialisingamd_sdw_irq_threadandamd_sdw_work, allowing an early interrupt to schedule uninitialised work. During removal,amd_sdw_manager_remove()only cancelsamd_sdw_workwithout first cancellingamd_sdw_irq_thread, allowing deferred IRQ work to re-queueamd_sdw_workor dereferenceamd_managerafter it is freed.Initialise both work items before calling
dev_set_drvdata()inamd_sdw_manager_probe(), and cancelamd_sdw_irq_threadbeforeamd_sdw_workinamd_sdw_manager_remove().Fixes 65f93e4096a0 ("soundwire: amd: add SoundWire managerinterrupt handling")
bfb0-171dc421e28c
Signed-off-by Lee Jones <[email protected]>---
drivers/soundwire/amd_manager.c| 3 ++-1 file changed, 2 insertions(+), 1 deletion(-)drivers/soundwire/ renamed from drivers/soundwire/amd_manager.c +0
-0
amd_manager.c
index98b09d81f3b8..2e1038ea0f36100644drivers/soundwire/amd_manager.c static int amd_sdw_manager_probe(struct · line 1192
Quoted 9 lines
drivers/soundwire/amd_manager.c +1
-1@@ -1185,9 +1185,9 @@ static int amd_sdw_manager_probe(struct platform_device *pdev) sdw_bus_master_delete(&amd_manager->bus); return ret; } - dev_set_drvdata(dev, amd_manager); INIT_WORK(&amd_manager->amd_sdw_irq_thread, amd_sdw_irq_thread); INIT_WORK(&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work); + dev_set_drvdata(dev, amd_manager);The probe-side ordering issue is valid — an interrupt arriving after
dev_set_drvdata()but beforeINIT_WORK()could schedule work on an uninitialisedwork_struct.The remove-side fix is already posted as part of our v3 patch series:
https://lore.kernel.org/alsa-devel/20261013085111.2009061-6-
[email protected]/T/#m60177687bb84758efdd6f5f09eba8ad32f0e4631Quoted 8 lines
The probe-side fix will be included in our upcoming v4 patch series, which contains major driver restructuring and customer-critical bug fixes. Merging this patch independently would conflict with v4 and complicate the review process.
Could you please drop this patch? Both issues will be covered by our v3/v4 series.
Quoted 6 lines
Lee,
I think as a general statement since there is still these big pending changes already in flight, could you either wait a few weeks until they merge or rebase your series on the in-flight version to see what still actually makes sense?
I will leave the AMD driver alone.
Please be kind enough to add my Reported-by for any issued that you fix as a result of these reports.
Thanks.
Signature · 1 line
Lee Jones
Mukunda,Vijendar
reviewerRe: [PATCH 16/21] soundwire: amd: Initialise and cancel IRQ work around manager lifetime
Quoted 63 lines · Lee Jones
[Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Quoted 60 lines · Mario Limonciello
Quoted 49 lines · Mukunda,Vijendar
Quoted 30 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_manager_probe()publishesamd_managerviadev_set_drvdata()before initialisingamd_sdw_irq_threadandamd_sdw_work, allowing an early interrupt to schedule uninitialised work. During removal,amd_sdw_manager_remove()only cancelsamd_sdw_workwithout first cancellingamd_sdw_irq_thread, allowing deferred IRQ work to re-queueamd_sdw_workor dereferenceamd_managerafter it is freed.Initialise both work items before calling
dev_set_drvdata()inamd_sdw_manager_probe(), and cancelamd_sdw_irq_threadbeforeamd_sdw_workinamd_sdw_manager_remove().Fixes 65f93e4096a0 ("soundwire: amd: add SoundWire managerinterrupt handling")
bfb0-171dc421e28c
Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/soundwire/ renamed from drivers/soundwire/amd_manager.c +0
-0
amd_manager.c
index98b09d81f3b8..2e1038ea0f36100644drivers/soundwire/amd_manager.c static int amd_sdw_manager_probe(struct · line 1192
Quoted 9 lines
drivers/soundwire/amd_manager.c +1
-1@@ -1185,9 +1185,9 @@ static int amd_sdw_manager_probe(struct platform_device *pdev) sdw_bus_master_delete(&amd_manager->bus);return ret; } -dev_set_drvdata (dev, amd_manager);INIT_WORK (&amd_manager->amd_sdw_irq_thread, amd_sdw_irq_thread);INIT_WORK (&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work); +dev_set_drvdata (dev, amd_manager);The probe-side ordering issue is valid — an interrupt arriving after
dev_set_drvdata()but beforeINIT_WORK()could schedule work on an uninitialisedwork_struct.The remove-side fix is already posted as part of our v3 patch series:
https://lore.kernel.org/alsa-devel/20261013085111.2009061-6-
[email protected]/T/#m60177687bb84758efdd6f5f09eba8ad32f0e4631Quoted 8 lines
The probe-side fix will be included in our upcoming v4 patch series, which contains major driver restructuring and customer-critical bug fixes. Merging this patch independently would conflict with v4 and complicate the review process.
Could you please drop this patch? Both issues will be covered by our v3/v4 series.
Lee,
Quoted 8 lines
Quoted 4 lines
I think as a general statement since there is still these big pending changes already in flight, could you either wait a few weeks until they merge or rebase your series on the in-flight version to see what still actually makes sense?
I will leave the AMD driver alone.
Please be kind enough to add my Reported-by for any issued that you fix as a result of these reports.
Sure, thanks for reporting these issues.
I'll make sure to include your Reported-by: tag for any fixes resulting from these reports.
Quoted 5 lines
Thanks.
--
Lee Jones
PATCH 17/21 soundwire: amd: Resume manager before hardware teardown in amd_sdw_manager_remove()
In reply to Lee Jonesamd_sdw_manager_remove() calls pm_runtime_disable() and immediately accesses MMIO registers in amd_disable_sdw_interrupts() and amd_disable_sdw_manager() without waking the device if it is runtime-suspended.
Call pm_runtime_get_sync() before disabling runtime PM and balance the usage count with pm_runtime_put_noidle() so the controller is awake during teardown.
--- drivers/soundwire/amd_manager.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/soundwire/amd_manager.c +2 -0
@@ -1196,7 +1196,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)struct amd_sdw_manager *amd_manager =dev_get_drvdata (&pdev->dev);int ret; +pm_runtime_get_sync (&pdev->dev);pm_runtime_disable (&pdev->dev); +pm_runtime_put_noidle (&pdev->dev);cancel_work_sync (&amd_manager->amd_sdw_irq_thread);cancel_work_sync (&amd_manager->amd_sdw_work);amd_disable_sdw_interrupts (amd_manager);
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 17/21] soundwire: amd: Resume manager before hardware teardown in amd_sdw_manager_remove()
Quoted 24 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_manager_remove()callspm_runtime_disable()and immediately accesses MMIO registers inamd_disable_sdw_interrupts()andamd_disable_sdw_manager()without waking the device if it is runtime-suspended.Call
pm_runtime_get_sync()before disabling runtime PM and balance the usage count withpm_runtime_put_noidle()so the controller is awake during teardown.Fixes 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 2 ++ 1 file changed, 2 insertions(+)drivers/soundwire/amd_manager.c +0
-0
drivers/soundwire/amd_manager.c static void amd_sdw_manager_remove(struct platform_device *pdev) · line 1201
Quoted 6 lines
drivers/soundwire/amd_manager.c +2
-0@@ -1196,7 +1196,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)struct amd_sdw_manager *amd_manager =dev_get_drvdata (&pdev->dev);int ret; +pm_runtime_get_sync (&pdev->dev);pm_runtime_disable (&pdev->dev); +pm_runtime_put_noidle (&pdev->dev);
The SoundWire manager is a child platform device of the ACP PCI device and does not have a dedicated BAR. Its MMIO is mapped through the parent ACP PCI device's address space, which remains valid and accessible for the lifetime of the parent device.
Runtime-suspending the SoundWire manager child device does not unmap or gate that MMIO region. As long as the parent ACP PCI device is alive — which it is during child device teardown — MMIO access is always safe, regardless of the child's runtime PM state.
The described scenario cannot occur on this hardware. Please drop this patch.
Quoted 5 lines
drivers/soundwire/amd_manager.c +0
-1cancel_work_sync (&amd_manager->amd_sdw_irq_thread);cancel_work_sync (&amd_manager->amd_sdw_work);amd_disable_sdw_interrupts (amd_manager); --2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 17/21] soundwire: amd: Resume manager before hardware teardown in amd_sdw_manager_remove()
In reply to Lee JonesQuoted 24 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
amd_sdw_manager_remove()callspm_runtime_disable()and immediately accesses MMIO registers inamd_disable_sdw_interrupts()andamd_disable_sdw_manager()without waking the device if it is runtime-suspended.Call
pm_runtime_get_sync()before disabling runtime PM and balance the usage count withpm_runtime_put_noidle()so the controller is awake during teardown.Fixes 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 2 ++ 1 file changed, 2 insertions(+)drivers/soundwire/amd_manager.c +0
-0
drivers/soundwire/amd_manager.c static void amd_sdw_manager_remove(struct platform_device *pdev) · line 1201
Quoted 6 lines
drivers/soundwire/amd_manager.c +2
-0@@ -1196,7 +1196,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)struct amd_sdw_manager *amd_manager =dev_get_drvdata (&pdev->dev);int ret; +pm_runtime_get_sync (&pdev->dev);pm_runtime_disable (&pdev->dev); +pm_runtime_put_noidle (&pdev->dev);
The SoundWire manager is a child platform device of the ACP PCI device and does not have a dedicated BAR. Its MMIO is mapped through the parent ACP PCI device's address space, which remains valid and accessible for the lifetime of the parent device.
Runtime-suspending the SoundWire manager child device does not unmap or gate that MMIO region. As long as the parent ACP PCI device is alive — which it is during child device teardown — MMIO access is always safe, regardless of the child's runtime PM state.
The described scenario cannot occur on this hardware. Please drop this patch.
Quoted 5 lines
drivers/soundwire/amd_manager.c +0
-1cancel_work_sync (&amd_manager->amd_sdw_irq_thread);cancel_work_sync (&amd_manager->amd_sdw_work);amd_disable_sdw_interrupts (amd_manager); --2.56.0.317.g454f8302e1-goog
PATCH 18/21 soundwire: amd: Synchronise parent IRQ and disable work on manager removal
In reply to Lee JonesIn the ACP top-half interrupt handlers, amd_manager is retrieved via dev_get_drvdata() and used to schedule amd_sdw_irq_thread when non-NULL. If an interrupt is already in flight on another CPU when amd_sdw_manager_remove() runs, the handler can call schedule_work(&amd_manager->amd_sdw_irq_thread) after cancel_work_sync() returns and after devm frees amd_manager.
Disable SoundWire interrupts, synchronise with the parent PCI device IRQ, disable both manager work items with disable_work_sync(), and mask interrupts again in case amd_sdw_update_slave_status_work() was running.
--- drivers/soundwire/amd_manager.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/soundwire/amd_manager.c +7 -2
@@ -8,10 +8,12 @@#include <linux/completion.h> #include <linux/cleanup.h> #include <linux/device.h> +#include <linux/interrupt.h> #include <linux/io.h> #include <linux/jiffies.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/pci.h> #include <linux/slab.h> #include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_registers.h>
@@ -1199,8 +1201,11 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)pm_runtime_get_sync (&pdev->dev);pm_runtime_disable (&pdev->dev);pm_runtime_put_noidle (&pdev->dev); -cancel_work_sync (&amd_manager->amd_sdw_irq_thread); -cancel_work_sync (&amd_manager->amd_sdw_work); +amd_disable_sdw_interrupts (amd_manager); +if (pdev->dev.parent &&dev_is_pci (pdev->dev.parent)) +synchronize_irq (to_pci_dev (pdev->dev.parent)->irq); +disable_work_sync (&amd_manager->amd_sdw_irq_thread); +disable_work_sync (&amd_manager->amd_sdw_work);amd_disable_sdw_interrupts (amd_manager);sdw_bus_master_delete (&amd_manager->bus); ret =amd_disable_sdw_manager (amd_manager);
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 18/21] soundwire: amd: Synchronise parent IRQ and disable work on manager removal
Quoted 39 lines · Lee Jones
[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
In the ACP top-half interrupt handlers,
amd_manageris retrieved viadev_get_drvdata()and used to scheduleamd_sdw_irq_threadwhen non-NULL. If an interrupt is already in flight on another CPU whenamd_sdw_manager_remove()runs, the handler can callschedule_work(&amd_manager->amd_sdw_irq_thread)aftercancel_work_sync()returns and after devm freesamd_manager.Disable SoundWire interrupts, synchronise with the parent PCI device IRQ, disable both manager work items with
disable_work_sync(), and mask interrupts again in caseamd_sdw_update_slave_status_work()was running.Fixes f93b697ed98e ("soundwire: amd: cancel pending slave status handling workqueue during remove sequence")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)drivers/soundwire/amd_manager.c +2
-0@@ -8,10 +8,12 @@#include <linux/completion.h> #include <linux/cleanup.h> #include <linux/device.h> +#include <linux/interrupt.h> #include <linux/io.h> #include <linux/jiffies.h> #include <linux/kernel.h> #include <linux/module.h> +#include <linux/pci.h> #include <linux/slab.h> #include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_registers.h>
drivers/soundwire/amd_manager.c static void amd_sdw_manager_remove(struct platform_device *pdev) · line 1208
Quoted 10 lines
drivers/soundwire/amd_manager.c +5
-2@@ -1199,8 +1201,11 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)pm_runtime_get_sync (&pdev->dev);pm_runtime_disable (&pdev->dev);pm_runtime_put_noidle (&pdev->dev); -cancel_work_sync (&amd_manager->amd_sdw_irq_thread); -cancel_work_sync (&amd_manager->amd_sdw_work); +amd_disable_sdw_interrupts (amd_manager); +if (pdev->dev.parent &&dev_is_pci (pdev->dev.parent)) +synchronize_irq (to_pci_dev (pdev->dev.parent)->irq); +disable_work_sync (&amd_manager->amd_sdw_irq_thread); +disable_work_sync (&amd_manager->amd_sdw_work);
++
The synchronize_irq() approach is incorrect for this hardware. The ACP IP uses a dedicated line-based interrupt. Using to_pci_dev(pdev->dev.parent)->irq gives the parent PCI device's primary IRQ, which is not the SoundWire interrupt line. Synchronizing the wrong IRQ does not ensure the ACP SoundWire top-half handler has completed on another CPU.
Additionally, disable_work_sync() is not available in older stable kernels, making the Cc: stable tag unworkable without further backport effort.
The double call to amd_disable_sdw_interrupts() is also unjustified — the work handler does not re-enable interrupts.
We have addressed this race in our upcoming v4 patch series using an interrupt_enabled flag checked in the top-half before scheduling work. This is the standard pattern for this type of race and has been validated on the hardware. Please drop this patch.
Quoted 5 lines
drivers/soundwire/amd_manager.c +0
-1amd_disable_sdw_interrupts (amd_manager);sdw_bus_master_delete (&amd_manager->bus); ret =amd_disable_sdw_manager (amd_manager); --2.56.0.317.g454f8302e1-goog
PATCH 19/21 soundwire: amd: Cancel pending work before suspending manager
In reply to Lee Jonesamd_suspend_runtime() stops the SoundWire bus clock and transitions the controller into D3 without cancelling amd_sdw_irq_thread and amd_sdw_work, and amd_suspend() only cancels amd_sdw_work without first cancelling amd_sdw_irq_thread. Queued work executing concurrently with suspend can access MMIO registers after the controller is clock-stopped or powered down.
Mask manager interrupts, synchronise the parent PCI IRQ, and cancel both amd_sdw_irq_thread and amd_sdw_work before stopping the bus clock and in amd_deinit_sdw_manager(), unmask wake interrupts only after clock stop completes, and avoid non-wake register accesses in amd_sdw_irq_thread() while the clock is stopped.
--- drivers/soundwire/amd_manager.c | 40 ++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 6 deletions(-)
drivers/soundwire/amd_manager.c +34 -6
@@ -148,9 +148,24 @@ static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)writel (0x00 , amd_manager->mmio +ACP_SW_ERROR_INTR_MASK ); } +static void amd_sdw_quiesce_work (struct amd_sdw_manager *amd_manager) +{ +struct device *parent = amd_manager->dev->parent; + +disable_work_sync (&amd_manager->amd_sdw_work); +writel (0 , amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7 ); +writel (0 , amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11 ); +writel (0 , amd_manager->mmio +ACP_SW_ERROR_INTR_MASK ); +if (parent &&dev_is_pci (parent)) +synchronize_irq (to_pci_dev (parent)->irq); +cancel_work_sync (&amd_manager->amd_sdw_irq_thread); +enable_work (&amd_manager->amd_sdw_work); +} +static int amd_deinit_sdw_manager (struct amd_sdw_manager *amd_manager) {amd_disable_sdw_interrupts (amd_manager); +amd_sdw_quiesce_work (amd_manager);return amd_disable_sdw_manager (amd_manager); }
@@ -1049,6 +1064,14 @@ static void amd_sdw_irq_thread(struct work_struct *work)u32 status_change_8to11;u32 status_change_0to7; +if (amd_manager->clk_stopped) { + status_change_8to11 =readl (amd_manager->mmio + +ACP_SW_STATE_CHANGE_STATUS_8TO11 ); +if (status_change_8to11 &AMD_SDW_WAKE_STAT_MASK ) +amd_sdw_process_wake_event (amd_manager); +return ; + } + status_change_8to11 =readl (amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_8TO11 ); status_change_0to7 =readl (amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_0TO7 );if (!status_change_0to7 && !status_change_8to11)
@@ -1341,24 +1364,24 @@ static int __maybe_unused amd_suspend(struct device *dev) }if (amd_manager->power_mode_mask &AMD_SDW_CLK_STOP_MODE ) { -cancel_work_sync (&amd_manager->amd_sdw_work); -amd_sdw_wake_enable (amd_manager, false);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, false);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); +amd_sdw_wake_enable (amd_manager, false); ret =amd_sdw_clock_stop (amd_manager);if (ret)return ret; }else if (amd_manager->power_mode_mask &AMD_SDW_POWER_OFF_MODE ) { -cancel_work_sync (&amd_manager->amd_sdw_work); -amd_sdw_wake_enable (amd_manager, false);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, false);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); +amd_sdw_wake_enable (amd_manager, false);/* * As per hardware programming sequence on AMD platforms, * clock stop should be invoked first before powering-off
@@ -1391,25 +1414,29 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)return 0 ; }if (amd_manager->power_mode_mask &AMD_SDW_CLK_STOP_MODE ) { -amd_sdw_wake_enable (amd_manager, true);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, true);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); ret =amd_sdw_clock_stop (amd_manager);if (ret)return ret; +if (amd_manager->clk_stopped) +amd_sdw_wake_enable (amd_manager, true); }else if (amd_manager->power_mode_mask &AMD_SDW_POWER_OFF_MODE ) { -amd_sdw_wake_enable (amd_manager, true);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, true);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); ret =amd_sdw_clock_stop (amd_manager);if (ret)return ret; +if (amd_manager->clk_stopped) +amd_sdw_wake_enable (amd_manager, true); ret =amd_deinit_sdw_manager (amd_manager);if (ret)return ret;
@@ -1452,6 +1479,7 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)if (ret)return ret; } +amd_enable_sdw_interrupts (amd_manager); }else if (amd_manager->power_mode_mask &AMD_SDW_POWER_OFF_MODE ) {writel (0x00 , amd_manager->acp_mmio +ACP_SW_WAKE_EN (amd_manager->instance));if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) {
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 19/21] soundwire: amd: Cancel pending work before suspending manager
Quoted 7 lines · Lee Jones
amd_suspend_runtime()stops the SoundWire bus clock and transitions the controller into D3 without cancellingamd_sdw_irq_threadandamd_sdw_work, andamd_suspend()only cancelsamd_sdw_workwithout first cancellingamd_sdw_irq_thread. Queued work executing concurrently with suspend can access MMIO registers after the controller is clock-stopped or powered down.
Quoted 5 lines
Mask manager interrupts, synchronise the parent PCI IRQ, and cancel both
amd_sdw_irq_threadandamd_sdw_workbefore stopping the bus clock and inamd_deinit_sdw_manager(), unmask wake interrupts only after clock stop completes, and avoid non-wake register accesses inamd_sdw_irq_thread()while the clock is stopped.
Both issues are already addressed in our v3 patch series:
https://lore.kernel.org/alsa-devel/[email protected]/T/#m7741edcedb1264764edc7f8559e39af8b23433fb
In our v3 implementation, both the suspend and suspend_runtime paths call acp_disable_sdw_interrupts() followed by cancel_work_sync() for both amd_sdw_irq_thread and amd_sdw_work, in that order, before clock stop — for both CLK_STOP_MODE and POWER_OFF_MODE.
Additionally, the amd_sdw_quiesce_work() helper introduced by this patch has the same synchronize_irq() issue raised on the previous patch — to_pci_dev(parent)->irq is not the correct IRQ line for the ACP dedicated interrupt. The disable_work_sync()/enable_work() pattern also introduces a new window where a late IRQ can re-queue work after enable_work() but before clock stop completes.
Please drop this patch. The fix is already on-list as part of our v3 series, and v4 with further improvements is being prepared.
Quoted 121 lines
Fixes 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 40 ++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 6 deletions(-)drivers/soundwire/amd_manager.c +34
-6@@ -148,9 +148,24 @@ static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)writel (0x00 , amd_manager->mmio +ACP_SW_ERROR_INTR_MASK ); } +static void amd_sdw_quiesce_work (struct amd_sdw_manager *amd_manager) +{ +struct device *parent = amd_manager->dev->parent; + +disable_work_sync (&amd_manager->amd_sdw_work); +writel (0 , amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7 ); +writel (0 , amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11 ); +writel (0 , amd_manager->mmio +ACP_SW_ERROR_INTR_MASK ); +if (parent &&dev_is_pci (parent)) +synchronize_irq (to_pci_dev (parent)->irq); +cancel_work_sync (&amd_manager->amd_sdw_irq_thread); +enable_work (&amd_manager->amd_sdw_work); +} +static int amd_deinit_sdw_manager (struct amd_sdw_manager *amd_manager) {amd_disable_sdw_interrupts (amd_manager); +amd_sdw_quiesce_work (amd_manager);return amd_disable_sdw_manager (amd_manager); }@@ -1049,6 +1064,14 @@ static void amd_sdw_irq_thread(struct work_struct *work)u32 status_change_8to11;u32 status_change_0to7; +if (amd_manager->clk_stopped) { + status_change_8to11 =readl (amd_manager->mmio + +ACP_SW_STATE_CHANGE_STATUS_8TO11 ); +if (status_change_8to11 &AMD_SDW_WAKE_STAT_MASK ) +amd_sdw_process_wake_event (amd_manager); +return ; + } + status_change_8to11 =readl (amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_8TO11 ); status_change_0to7 =readl (amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_0TO7 );if (!status_change_0to7 && !status_change_8to11)@@ -1341,24 +1364,24 @@ static int __maybe_unused amd_suspend(struct device *dev) }if (amd_manager->power_mode_mask &AMD_SDW_CLK_STOP_MODE ) { -cancel_work_sync (&amd_manager->amd_sdw_work); -amd_sdw_wake_enable (amd_manager, false);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, false);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); +amd_sdw_wake_enable (amd_manager, false); ret =amd_sdw_clock_stop (amd_manager);if (ret)return ret; }else if (amd_manager->power_mode_mask &AMD_SDW_POWER_OFF_MODE ) { -cancel_work_sync (&amd_manager->amd_sdw_work); -amd_sdw_wake_enable (amd_manager, false);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, false);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); +amd_sdw_wake_enable (amd_manager, false);/* * As per hardware programming sequence on AMD platforms, * clock stop should be invoked first before powering-off @@ -1391,25 +1414,29 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)return 0 ; }if (amd_manager->power_mode_mask &AMD_SDW_CLK_STOP_MODE ) { -amd_sdw_wake_enable (amd_manager, true);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, true);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); ret =amd_sdw_clock_stop (amd_manager);if (ret)return ret; +if (amd_manager->clk_stopped) +amd_sdw_wake_enable (amd_manager, true); }else if (amd_manager->power_mode_mask &AMD_SDW_POWER_OFF_MODE ) { -amd_sdw_wake_enable (amd_manager, true);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, true);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); ret =amd_sdw_clock_stop (amd_manager);if (ret)return ret; +if (amd_manager->clk_stopped) +amd_sdw_wake_enable (amd_manager, true); ret =amd_deinit_sdw_manager (amd_manager);if (ret)return ret;@@ -1452,6 +1479,7 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)if (ret)return ret; } +amd_enable_sdw_interrupts (amd_manager); }else if (amd_manager->power_mode_mask &AMD_SDW_POWER_OFF_MODE ) {writel (0x00 , amd_manager->acp_mmio +ACP_SW_WAKE_EN (amd_manager->instance));if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) {
[email protected]
AI reviewRe: [PATCH 19/21] soundwire: amd: Cancel pending work before suspending manager
In reply to Lee JonesThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] soundwire: amd: stale clk_stopped flag in amd_sdw_irq_thread() after power-off - [High] soundwire…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] soundwire: amd: stale clk_stopped flag in amd_sdw_irq_thread() after power-off
- [High] soundwire: amd: runtime PM deadlock in amd_sdw_quiesce_work()
- [Medium] soundwire: amd: wake IRQ unmasked then cancelled in amd_suspend_runtime() --
commit ec00457d1c5224b384fcee6f0242d7bebb6b2292
Author: Lee Jones <[email protected]>
soundwire: amd: Cancel pending work before suspending manager
This commit masks manager interrupts, synchronizes the parent PCI IRQ, and cancels both amd_sdw_irq_thread and amd_sdw_work before stopping the SoundWire bus clock during suspend and manager deinitialization. It also avoids non-wake register accesses in amd_sdw_irq_thread() while the clock is stopped.
drivers/soundwire/amd_manager.c static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager) · line 155
Quoted 13 lines
drivers/soundwire/amd_manager.c +5
-0@@ -148,9 +148,24 @@ static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)writel (0x00 , amd_manager->mmio +ACP_SW_ERROR_INTR_MASK ); } +static void amd_sdw_quiesce_work (struct amd_sdw_manager *amd_manager) +{ +struct device *parent = amd_manager->dev->parent; + +disable_work_sync (&amd_manager->amd_sdw_work);
[Severity: High]
Can calling disable_work_sync(&amd_manager->amd_sdw_work) from amd_suspend_runtime() cause a runtime PM deadlock if a peripheral alert is being processed concurrently?
In the SoundWire device hierarchy (amd_manager->dev -> bus->dev -> slave->dev), neither amd_manager->dev nor bus->dev sets power.ignore_children. When all child slave->dev devices are runtime-suspended, the manager remains active until its autosuspend delay expires.
When rpm_suspend() runs on amd_manager->dev, its runtime status is set to RPM_SUSPENDING before invoking:
amd_suspend_runtime()
-> amd_sdw_quiesce_work()
-> disable_work_sync(&amd_manager->amd_sdw_work)If an alert interrupt scheduled amd_sdw_work just prior to this, the worker executes:
amd_sdw_update_slave_status_work()
-> sdw_handle_slave_status()
-> sdw_handle_slave_alerts()
-> pm_runtime_get_sync(&slave->dev)Because slave->dev and bus->dev are RPM_SUSPENDED, rpm_resume(&slave->dev, RPM_GET_PUT) synchronously resumes the parent chain up to rpm_resume(amd_manager->dev, 0), which sees RPM_SUSPENDING and waits in TASK_UNINTERRUPTIBLE on amd_manager->dev->power.wait_queue for amd_suspend_runtime() to complete, while amd_suspend_runtime() is waiting in disable_work_sync() for amd_sdw_update_slave_status_work() to finish.
Quoted 17 lines
drivers/soundwire/amd_manager.c +10
-0+writel (0 , amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7 ); +writel (0 , amd_manager->mmio +ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11 ); +writel (0 , amd_manager->mmio +ACP_SW_ERROR_INTR_MASK ); +if (parent &&dev_is_pci (parent)) +synchronize_irq (to_pci_dev (parent)->irq); +cancel_work_sync (&amd_manager->amd_sdw_irq_thread); +enable_work (&amd_manager->amd_sdw_work); +} +static int amd_deinit_sdw_manager (struct amd_sdw_manager *amd_manager) {amd_disable_sdw_interrupts (amd_manager); +amd_sdw_quiesce_work (amd_manager);return amd_disable_sdw_manager (amd_manager); }
drivers/soundwire/amd_manager.c static void amd_sdw_irq_thread(struct work_struct *work) · line 1073
Quoted 10 lines
drivers/soundwire/amd_manager.c +7
-0@@ -1049,6 +1064,14 @@ static void amd_sdw_irq_thread(struct work_struct *work)u32 status_change_8to11;u32 status_change_0to7; +if (amd_manager->clk_stopped) { + status_change_8to11 =readl (amd_manager->mmio + +ACP_SW_STATE_CHANGE_STATUS_8TO11 ); +if (status_change_8to11 &AMD_SDW_WAKE_STAT_MASK ) +amd_sdw_process_wake_event (amd_manager); +return ; + }
[Severity: High]
Does this early return drop all non-wake interrupts after resuming from AMD_SDW_POWER_OFF_MODE when the parent ACP controller has been reset?
During amd_suspend() or amd_suspend_runtime() in AMD_SDW_POWER_OFF_MODE, amd_sdw_clock_stop() sets amd_manager->clk_stopped = true and amd_deinit_sdw_manager() clears ACP_SW_EN. When the parent ACP PCI device subsequently suspends and resumes, check_acp_sdw_enable_status() observes ACP_SW0_EN and ACP_SW1_EN cleared and performs a full ACP soft reset and power cycle, which resets ACP_SW_CLK_RESUME_CTRL to 0.
When amd_resume_runtime() then runs the AMD_SDW_POWER_OFF_MODE path in drivers/soundwire/amd_manager.c:
val = readl(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
if (val) {
val |= AMD_SDW_CLK_RESUME_REQ;
writel(val, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL, val,
(val & AMD_SDW_CLK_RESUME_DONE), ACP_DELAY_US,
AMD_SDW_TIMEOUT);
if (val & AMD_SDW_CLK_RESUME_DONE) {
writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
amd_manager->clk_stopped = false;
}
}Because val is 0 after the ACP reset, amd_manager->clk_stopped is never reset to false even after amd_sdw_clk_init_ctrl(), amd_init_sdw_manager(), amd_enable_sdw_interrupts(), and amd_enable_sdw_manager() reinitialize and restart the bus.
With this new check in amd_sdw_irq_thread(), wouldn't every subsequent interrupt return early without reading ACP_SW_STATE_CHANGE_STATUS_0TO7, processing peripheral attachment or ping status, scheduling amd_sdw_work, or clearing ACP_SW_STATE_CHANGE_STATUS_8TO11 and ACP_SW_STATE_CHANGE_STATUS_0TO7?
[ ... ]
Quoted 20 lines
drivers/soundwire/amd_manager.c +3
-2@@ -1391,25 +1414,29 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)return 0 ; }if (amd_manager->power_mode_mask &AMD_SDW_CLK_STOP_MODE ) { -amd_sdw_wake_enable (amd_manager, true);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, true);if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); ret =amd_sdw_clock_stop (amd_manager);if (ret)return ret; +if (amd_manager->clk_stopped) +amd_sdw_wake_enable (amd_manager, true); }else if (amd_manager->power_mode_mask &AMD_SDW_POWER_OFF_MODE ) { -amd_sdw_wake_enable (amd_manager, true);if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) { ret =amd_sdw_host_wake_enable (amd_manager, true);
drivers/soundwire/amd_manager.c static int __maybe_unused amd_suspend_runtime(struct device *dev) · line 1440
Quoted 10 lines
drivers/soundwire/amd_manager.c +3
-0if (ret)return ret; } +amd_sdw_quiesce_work (amd_manager); ret =amd_sdw_clock_stop (amd_manager);if (ret)return ret; +if (amd_manager->clk_stopped) +amd_sdw_wake_enable (amd_manager, true); ret =amd_deinit_sdw_manager (amd_manager);
[Severity: Medium]
In the AMD_SDW_POWER_OFF_MODE branch of amd_suspend_runtime(), amd_sdw_wake_enable(amd_manager, true) unmasks AMD_SDW_WAKE_INTR_MASK in ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11 while ACP_EXTERNAL_INTR_CNTL still has the manager interrupt enabled.
Immediately after that, amd_deinit_sdw_manager() is called:
amd_deinit_sdw_manager()
-> amd_disable_sdw_interrupts()
-> amd_sdw_quiesce_work()
-> cancel_work_sync(&amd_manager->amd_sdw_irq_thread)amd_disable_sdw_interrupts() zeroes ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11 and clears the manager interrupt bit in ACP_EXTERNAL_INTR_CNTL, and amd_sdw_quiesce_work() cancels amd_sdw_irq_thread.
If a wake interrupt fires in the window between amd_sdw_wake_enable() and amd_disable_sdw_interrupts(), the top-half IRQ handler acknowledges the interrupt in ACP_EXTERNAL_INTR_STAT and schedules amd_sdw_irq_thread, which is then cancelled by amd_deinit_sdw_manager() before amd_sdw_process_wake_event() can call pm_request_resume(). Why unmask AMD_SDW_WAKE_INTR_MASK here if amd_deinit_sdw_manager() immediately masks it and cancels the work?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=19
PATCH 20/21 soundwire: amd: Check clock resume timeout in amd_resume_runtime()
In reply to Lee JonesWhen resuming from AMD_SDW_POWER_OFF_MODE in amd_resume_runtime(), the return value of readl_poll_timeout() when polling for AMD_SDW_CLK_RESUME_DONE is stored in ret but never checked, allowing resume to proceed even when clock resume times out.
Return ret if polling for AMD_SDW_CLK_RESUME_DONE fails.
--- drivers/soundwire/amd_manager.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/soundwire/amd_manager.c +4 -4
@@ -1494,10 +1494,10 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) ret =readl_poll_timeout (amd_manager->mmio +ACP_SW_CLK_RESUME_CTRL , val, (val &AMD_SDW_CLK_RESUME_DONE ),ACP_DELAY_US ,AMD_SDW_TIMEOUT ); -if (val &AMD_SDW_CLK_RESUME_DONE ) { -writel (0 , amd_manager->mmio +ACP_SW_CLK_RESUME_CTRL ); - amd_manager->clk_stopped = false; - } +if (ret) +return ret; +writel (0 , amd_manager->mmio +ACP_SW_CLK_RESUME_CTRL ); + amd_manager->clk_stopped = false; }sdw_clear_slave_status (bus,SDW_UNATTACH_REQUEST_MASTER_RESET ); ret =amd_sdw_clk_init_ctrl (amd_manager);
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 20/21] soundwire: amd: Check clock resume timeout in amd_resume_runtime()
Quoted 21 lines · Lee Jones
When resuming from
AMD_SDW_POWER_OFF_MODEinamd_resume_runtime(), the return value ofreadl_poll_timeout()when polling forAMD_SDW_CLK_RESUME_DONEis stored in ret but never checked, allowing resume to proceed even when clock resume times out.Return ret if polling for
AMD_SDW_CLK_RESUME_DONEfails.Fixes 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/soundwire/amd_manager.c +0
-0@@ -1494,10 +1494,10 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL, val,
drivers/soundwire/amd_manager.c static int __maybe_unused amd_resume_runtime(struct device *dev) · line 1500
Quoted 10 lines
drivers/soundwire/amd_manager.c +4
-4(val &AMD_SDW_CLK_RESUME_DONE ),ACP_DELAY_US , AMD_SDW_TIMEOUT); -if (val &AMD_SDW_CLK_RESUME_DONE ) { -writel (0 , amd_manager->mmio +ACP_SW_CLK_RESUME_CTRL ); - amd_manager->clk_stopped = false; - } +if (ret) +return ret; +writel (0 , amd_manager->mmio +ACP_SW_CLK_RESUME_CTRL ); + amd_manager->clk_stopped = false;
++
The bug is valid. On clock resume timeout the code falls through to sdw_clear_slave_status() and full bus re-init without the clock being active, which is incorrect.
However, we have identified additional issues in this code path that need to be addressed together:
1. writel(0, ACP_SW_CLK_RESUME_CTRL) is incorrect — this register is not a plain zero-clear register. Only the Clk_Resume_Req bit should be cleared using a read-modify-write on the success path.
2. The same unchecked ret pattern exists in amd_acp63_sdw_clock_stop_exit() and needs to be fixed together.
We are planning to address all of these as part of our upcoming v4 patch series which will be posted shortly. Could you please hold this patch until v4 is on-list to avoid conflicts?
Quoted 3 lines
drivers/soundwire/amd_manager.c +0
-0}sdw_clear_slave_status (bus,SDW_UNATTACH_REQUEST_MASTER_RESET ); ret =amd_sdw_clk_init_ctrl (amd_manager);
Mukunda,Vijendar
reviewerRe: [PATCH 20/21] soundwire: amd: Check clock resume timeout in amd_resume_runtime()
Quoted 41 lines · Mukunda,Vijendar
Quoted 22 lines · Lee Jones
When resuming from
AMD_SDW_POWER_OFF_MODEinamd_resume_runtime(), the return value ofreadl_poll_timeout()when polling forAMD_SDW_CLK_RESUME_DONEis stored in ret but never checked, allowing resume to proceed even when clock resume times out.Return ret if polling for
AMD_SDW_CLK_RESUME_DONEfails.Fixes 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")Signed-off-by Lee Jones <[email protected]>---
drivers/soundwire/amd_manager.c| 8 ++++----1 file changed, 4 insertions(+), 4 deletions(-)drivers/soundwire/amd_manager.c +0
-0@@ -1494,10 +1494,10 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL, val,drivers/soundwire/amd_manager.c static int __maybe_unused · line 1501
Quoted 10 lines
drivers/soundwire/amd_manager.c +4
-4(val & AMD_SDW_CLK_RESUME_DONE), ACP_DELAY_US, AMD_SDW_TIMEOUT); - if (val & AMD_SDW_CLK_RESUME_DONE) { - writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL); - amd_manager->clk_stopped = false; - } + if (ret) + return ret; + writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL); + amd_manager->clk_stopped = false;++
The bug is valid. On clock resume timeout the code falls through to
sdw_clear_slave_status()and full bus re-init without the clock being active, which is incorrect.
Quoted 6 lines
However, we have identified additional issues in this code path that need to be addressed together:
1.
writel(0, ACP_SW_CLK_RESUME_CTRL)is incorrect — this register is not a plain zero-clear register. Only theClk_Resume_Reqbit should be cleared using a read-modify-write on the success path.
Slight correction. Ignore comment 1.
Quoted 11 lines
2. The same unchecked ret pattern exists in
amd_acp63_sdw_clock_stop_exit()and needs to be fixed together.We are planning to address all of these as part of our upcoming v4 patch series which will be posted shortly. Could you please hold this patch until v4 is on-list to avoid conflicts?
Quoted 3 lines
drivers/soundwire/amd_manager.c +0
-0} sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET);ret =
amd_sdw_clk_init_ctrl(amd_manager);
Mukunda,Vijendar
reviewerRe: [PATCH 20/21] soundwire: amd: Check clock resume timeout in amd_resume_runtime()
Quoted 5 lines · Mukunda,Vijendar
Quoted 2 lines · Mukunda,Vijendar
Quoted 7 lines
Quoted 7 lines · Lee Jones
Quoted 6 lines
When resuming from
AMD_SDW_POWER_OFF_MODEinamd_resume_runtime(), the return value ofreadl_poll_timeout()when polling forAMD_SDW_CLK_RESUME_DONEis stored in ret but never checked, allowing resume to proceed even when clock resume times out.Return ret if polling for
AMD_SDW_CLK_RESUME_DONEfails.
Correction to my earlier reply.
After further review, the existing timeout handling is intentional. Please disregard my earlier statement that the timeout should be propagated.
A similar CLK_RESUME_DONE timeout issue was discussed earlier:
https://lore.kernel.org/all/[email protected]/#t
As discussed there, propagating -ETIMEDOUT would abort the PM resume path rather than allowing the remaining recovery sequence to proceed. That discussion also noted the same timeout-handling pattern in the POWER_OFF_MODE path.
For POWER_OFF_MODE, returning an error at this point would skip the subsequent slave-state reset and SoundWire manager re-initialization, preventing the bus recovery and codec re-enumeration.
The preferred long-term approach is to recover the bus on such failures rather than propagate the error to the PM framework. This remains a TODO.
Therefore, we intend to retain the existing timeout behavior and will not change it in the upcoming respin.
Please drop this patch. Sorry for the confusion caused by my earlier reply.
Quoted 52 lines
Quoted 32 lines
Quoted 16 lines
Fixes 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")Signed-off-by Lee Jones <[email protected]>---
drivers/soundwire/amd_manager.c| 8 ++++----1 file changed, 4 insertions(+), 4 deletions(-)drivers/soundwire/amd_manager.c +0
-0@@ -1494,10 +1494,10 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) ret = readl_poll_timeout(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL, val,drivers/soundwire/amd_manager.c static int __maybe_unused · line 1501
Quoted 10 lines
drivers/soundwire/amd_manager.c +4
-4(val & AMD_SDW_CLK_RESUME_DONE), ACP_DELAY_US, AMD_SDW_TIMEOUT); - if (val & AMD_SDW_CLK_RESUME_DONE) { - writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL); - amd_manager->clk_stopped = false; - } + if (ret) + return ret; + writel(0, amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL); + amd_manager->clk_stopped = false;++
The bug is valid. On clock resume timeout the code falls through to
sdw_clear_slave_status()and full bus re-init without the clock being active, which is incorrect.Quoted 6 lines
However, we have identified additional issues in this code path that need to be addressed together:
1.
writel(0, ACP_SW_CLK_RESUME_CTRL)is incorrect — this register is not a plain zero-clear register. Only theClk_Resume_Reqbit should be cleared using a read-modify-write on the success path.Slight correction. Ignore comment 1.
Quoted 11 lines
2. The same unchecked ret pattern exists in
amd_acp63_sdw_clock_stop_exit()and needs to be fixed together.We are planning to address all of these as part of our upcoming v4 patch series which will be posted shortly. Could you please hold this patch until v4 is on-list to avoid conflicts?
Quoted 3 lines
drivers/soundwire/amd_manager.c +0
-0} sdw_clear_slave_status(bus, SDW_UNATTACH_REQUEST_MASTER_RESET);ret =
amd_sdw_clk_init_ctrl(amd_manager);
PATCH 21/21 soundwire: amd: Disable interrupts if enabling manager fails
In reply to Lee JonesIn amd_sdw_manager_start() and amd_resume_runtime(), amd_enable_sdw_interrupts() is called before amd_enable_sdw_manager(). If amd_enable_sdw_manager() fails, both functions return immediately with SoundWire interrupts left unmasked. In addition, amd_resume_runtime() ignores the return value of amd_init_sdw_manager().
Check the return value of amd_init_sdw_manager() in amd_resume_runtime(), and disable interrupts if amd_enable_sdw_manager() fails in amd_sdw_manager_start() and amd_resume_runtime().
--- drivers/soundwire/amd_manager.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/soundwire/amd_manager.c +9 -3
@@ -1109,8 +1109,10 @@ int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager)return ret;amd_enable_sdw_interrupts (amd_manager); ret =amd_enable_sdw_manager (amd_manager); -if (ret) +if (ret) { +amd_disable_sdw_interrupts (amd_manager);return ret; + }amd_sdw_set_frameshape (amd_manager); }/* Enable runtime PM */
@@ -1503,11 +1505,15 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) ret =amd_sdw_clk_init_ctrl (amd_manager);if (ret)return ret; -amd_init_sdw_manager (amd_manager); + ret =amd_init_sdw_manager (amd_manager); +if (ret) +return ret;amd_enable_sdw_interrupts (amd_manager); ret =amd_enable_sdw_manager (amd_manager); -if (ret) +if (ret) { +amd_disable_sdw_interrupts (amd_manager);return ret; + }amd_sdw_set_frameshape (amd_manager); }if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) {
Signature · 1 line
2.56.0.317.g454f8302e1-goog
Mukunda,Vijendar
reviewerRe: [PATCH 21/21] soundwire: amd: Disable interrupts if enabling manager fails
Quoted 10 lines · Lee Jones
In
amd_sdw_manager_start()andamd_resume_runtime(),amd_enable_sdw_interrupts()is called beforeamd_enable_sdw_manager(). Ifamd_enable_sdw_manager()fails, both functions return immediately with SoundWire interrupts left unmasked. In addition,amd_resume_runtime()ignores the return value ofamd_init_sdw_manager().Check the return value of
amd_init_sdw_manager()inamd_resume_runtime(), and disable interrupts ifamd_enable_sdw_manager()fails inamd_sdw_manager_start()andamd_resume_runtime().
++
The unchecked return value of amd_init_sdw_manager() in amd_resume_runtime() is already addressed in our v3 patch series:
https://lore.kernel.org/alsa-devel/[email protected]/T/#m7741edcedb1264764edc7f8559e39af8b23433fb
The missing amd_disable_sdw_interrupts() on amd_enable_sdw_manager() failure is a valid observation and will be addressed in our upcoming v4 patch series. Please hold this patch until v4 is on-list to avoid conflicts.
Quoted 43 lines
Fixes 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")Signed-off-by Lee Jones <[email protected]>--- drivers/soundwire/amd_manager.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/soundwire/amd_manager.c +9
-3@@ -1109,8 +1109,10 @@ int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager)return ret;amd_enable_sdw_interrupts (amd_manager); ret =amd_enable_sdw_manager (amd_manager); -if (ret) +if (ret) { +amd_disable_sdw_interrupts (amd_manager);return ret; + }amd_sdw_set_frameshape (amd_manager); }/* Enable runtime PM */ @@ -1503,11 +1505,15 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) ret =amd_sdw_clk_init_ctrl (amd_manager);if (ret)return ret; -amd_init_sdw_manager (amd_manager); + ret =amd_init_sdw_manager (amd_manager); +if (ret) +return ret;amd_enable_sdw_interrupts (amd_manager); ret =amd_enable_sdw_manager (amd_manager); -if (ret) +if (ret) { +amd_disable_sdw_interrupts (amd_manager);return ret; + }amd_sdw_set_frameshape (amd_manager); }if (amd_manager->acp_rev >=ACP70_PCI_REV_ID ) {
Mukunda,Vijendar
reviewerRe: [PATCH 21/21] soundwire: amd: Disable interrupts if enabling manager fails
Quoted 17 lines · Mukunda,Vijendar
Quoted 10 lines · Lee Jones
In
amd_sdw_manager_start()andamd_resume_runtime(),amd_enable_sdw_interrupts()is called beforeamd_enable_sdw_manager(). Ifamd_enable_sdw_manager()fails, both functions return immediately with SoundWire interrupts left unmasked. In addition,amd_resume_runtime()ignores the return value ofamd_init_sdw_manager().Check the return value of
amd_init_sdw_manager()inamd_resume_runtime(), and disable interrupts ifamd_enable_sdw_manager()fails inamd_sdw_manager_start()andamd_resume_runtime().++
The unchecked return value ofamd_init_sdw_manager()inamd_resume_runtime()is already addressed in our v3 patch series:
Quoted 7 lines
https://lore.kernel.org/alsa-devel/[email protected]/T/#m7741edcedb1264764edc7f8559e39af8b23433fb
The missing
amd_disable_sdw_interrupts()onamd_enable_sdw_manager()failure is a valid observation and will be addressed in our upcoming v4 patch series. Please hold this patch until v4 is on-list to avoid conflicts.
Correction to the above. After a more detailed analysis of the ACP hardware interrupt routing, the fix is not required. Please drop this change and disregard our earlier statement.
The AMD ACP IP is the parent device that dispatches SoundWire interrupts per manager instance. It only forwards an SDW interrupt to the CPU when the corresponding SoundWire manager is active. If acp_enable_sdw_manager() times out the ACP treats that manager instance as inactive and will not assert any SDW interrupt on its behalf, regardless of the mask register state.
Two independent hardware guarantees prevent any interrupt from firing after a manager-enable failure:
1. The masks written by acp_enable_sdw_interrupts() cover packet-based SoundWire status events. These require the SoundWire clock to be running, which in turn requires the manager to be enabled. Without an active manager there is no clock, no SoundWire packet can be received, and no interrupt is generated.
2. Wake interrupt masks are only functional once the SoundWire manager is enabled in the D0 sequence. Additionally, wake is explicitly disabled at the very start of the POWER_OFF_MODE resume path, well before acp_enable_sdw_manager() is reached:
writel(0x00, acp_mmio + ACP_SW_WAKE_EN(instance)); amd_sdw_host_wake_enable_by_rev(amd_manager, false);
...
acp_enable_sdw_interrupts(amd_manager);
ret = acp_enable_sdw_manager(amd_manager); /* may fail here */
v5 will not include a fix for this case.
Quoted 46 lines
Quoted 45 lines
Fixes 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")Signed-off-by Lee Jones <[email protected]>---
drivers/soundwire/amd_manager.c| 12 +++++++++---1 file changed, 9 insertions(+), 3 deletions(-)drivers/soundwire/amd_manager.c +3
-1@@ -1109,8 +1109,10 @@ int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager) return ret; amd_enable_sdw_interrupts(amd_manager); ret = amd_enable_sdw_manager(amd_manager); - if (ret) + if (ret) { + amd_disable_sdw_interrupts(amd_manager); return ret; + } amd_sdw_set_frameshape(amd_manager); }/* Enable runtime PM */
drivers/soundwire/amd_manager.c +6
-2@@ -1503,11 +1505,15 @@ static int __maybe_unused amd_resume_runtime(struct device *dev) ret = amd_sdw_clk_init_ctrl(amd_manager); if (ret) return ret; - amd_init_sdw_manager(amd_manager); + ret = amd_init_sdw_manager(amd_manager); + if (ret) + return ret; amd_enable_sdw_interrupts(amd_manager); ret = amd_enable_sdw_manager(amd_manager); - if (ret) + if (ret) { + amd_disable_sdw_interrupts(amd_manager); return ret; + } amd_sdw_set_frameshape(amd_manager); }if (
amd_manager->acp_rev>=ACP70_PCI_REV_ID) {
Vinod Koul
maintainerHi Lee,
Quoted 1 line · Lee Jones
These issues were reported by Sashiko.
Thanks for looking up issues and fixing them. Some of them do sound good to me. I would not want to cc stable and tag them to fixes though. These are bugs, but I have no evidence they are impacting real world scenarios. Mostly a good but theoretical exercise, so I would like to avoid churn and move them for next cycle...
Signature · 1 line
~Vinod
Quoted 8 lines · Vinod Koul
Quoted 1 line · Lee Jones
These issues were reported by Sashiko.
Thanks for looking up issues and fixing them. Some of them do sound good to me. I would not want to cc stable and tag them to fixes though. These are bugs, but I have no evidence they are impacting real world scenarios. Mostly a good but theoretical exercise, so I would like to avoid churn and move them for next cycle...
That is expected. I would like these to soak in -next.
I plan on preparing a v2 this week, taking into account all of the review comments and collected trailers.
Would you like me to strip the Stable tags as well?
Signature · 1 line
Lee Jones
Vinod Koul
maintainerQuoted 10 lines · Lee Jones
Quoted 8 lines · Vinod Koul
Quoted 1 line · Lee Jones
These issues were reported by Sashiko.
Thanks for looking up issues and fixing them. Some of them do sound good to me. I would not want to cc stable and tag them to fixes though. These are bugs, but I have no evidence they are impacting real world scenarios. Mostly a good but theoretical exercise, so I would like to avoid churn and move them for next cycle...
Quoted 6 lines
That is expected. I would like these to soak in -next.
I plan on preparing a v2 this week, taking into account all of the review comments and collected trailers.
Would you like me to strip the Stable tags as well?
Yes please, I do not consider this stable material.. Fixes doesn not help too... :-)
Signature · 1 line
~Vinod
Mark Brown
maintainerRe: (subset) [PATCH 00/21] soundwire: Fix Critical and High issues reported by Sashiko
In reply to Lee JonesQuoted 31 lines · Lee Jones
soundwire: Fix Critical and High issues reported by Sashiko
These issues were reported by Sashiko.
Cc "Uwe Kleine-König (The Capable Hub)" <[email protected]>Cc Bard Liao <[email protected]>Cc Bjorn Helgaas <[email protected]>Cc Daniel Baluta <[email protected]>Cc Danilo Krummrich <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Hardik T Shah <[email protected]>Cc Jaroslav Kysela <[email protected]>Cc Kai Vehmanen <[email protected]>Cc Liam Girdwood <[email protected]>Cc Mark Brown <[email protected]>Cc Mastan Katragadda <[email protected]>Cc Peter Ujfalusi <[email protected]>Cc Pierre-Louis Bossart <[email protected]>Cc Rander Wang <[email protected]>Cc Ranjani Sridharan <[email protected]>Cc Sanyog Kale <[email protected]>Cc Simon Trimmer <[email protected]>Cc Srinivas Kandagatla <[email protected]>Cc Takashi Iwai <[email protected]>Cc Takashi Sakamoto <[email protected]>Cc Vijendar Mukunda <[email protected]>Cc Vinod Koul <[email protected]>
Quoted 1 line
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-7.4
Thanks!
[05/21] ASoC: SOF: Intel: hda-sdw-bpt: Stop active DMA before closing BPT streams
https://git.kernel.org/broonie/sound/c/348bc756d912
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks,
Mark