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

[sashiko-bot@kernel.org]

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

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.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index f39f4f04db7e9..ba8e6d42c467f 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);

[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.

> +	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;
> +	}

[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?

[ ... ]

> @@ -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);

[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?

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