Re: [PATCH 19/21] soundwire: amd: Cancel pending work before suspending manager
From: Mukunda,Vijendar
Date: Fri Oct 30 2026 - 08:36:07 EST
On 10/30/26 16:16, Lee Jones wrote:
amd_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.
Both issues are already addressed in our v3 patch series:
https://lore.kernel.org/alsa-devel/20261013085111.2009061-6-Vijendar.Mukunda@xxxxxxx/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.
Fixes: 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")
Closes: https://sashiko.dev/#/bug/linux-3b774ace-00cf-41e6-82eb-85cd43f5e1d6
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
---
drivers/soundwire/amd_manager.c | 40 ++++++++++++++++++++++++++++-----
1 file changed, 34 insertions(+), 6 deletions(-)
diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index f39f4f04db7e..ba8e6d42c467 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -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) {