Re: [PATCH 19/21] soundwire: amd: Cancel pending work before suspending manager

["Mukunda,Vijendar" <vijendar.mukunda@amd.com>]

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@amd.com/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@vger.kernel.org
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
>   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) {