Re: [PATCH v5 17/25] soundwire: amd: use interrupt_enabled flag to guard IRQ work scheduling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] soundwire: amd: lost IRQ in amd_enable_sdw_interrupts() due
  to late flag
- [Medium] soundwire: amd: unguarded IRQ work queueing in
  amd_disable_sdw_interrupts()
- [Medium] soundwire: amd: race re-enabling IRQ masks in
  amd_disable_sdw_interrupts()
--

commit c646d5fedeac998edd11296e03517aec14fbed88
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: use interrupt_enabled flag to guard IRQ work scheduling

This commit adds an interrupt_enabled boolean flag to struct
amd_sdw_manager to guard scheduling of amd_sdw_work when interrupts are
being disabled.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 01aa051cf63fa..108553b0f32e8 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -129,12 +129,22 @@ static void amd_enable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
>  	writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
>  		       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
>  	writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
> +	amd_manager->interrupt_enabled = true;
>  }

[Severity: Medium]
Can setting amd_manager->interrupt_enabled after unmasking the hardware
registers drop an initial peripheral status-change interrupt?

amd_init_sdw_manager() has already enabled the bus before
amd_enable_sdw_interrupts() runs, so a peripheral state-change interrupt
can fire as soon as the mask registers are written:

CPU0                                     CPU1 (kworker)
amd_enable_sdw_interrupts()
  amd_updatel(..., ACP_EXTERNAL_INTR_CNTL, ...)
  writel(..., ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7)
  <IRQ fires, schedules amd_sdw_irq_thread>
                                         amd_sdw_irq_thread()
                                           status_change_0to7 = readl(...)
                                           amd_sdw_update_slave_status(...)
                                           if (... && amd_manager->interrupt_enabled)
                                             /* false: skips amd_sdw_work */
                                           writel(0x00, ..._STATUS_0TO7)
                                             <-- event lost
  amd_manager->interrupt_enabled = true

Because amd_sdw_irq_thread() skips schedule_work(&amd_manager->amd_sdw_work)
when interrupt_enabled is false but still clears
ACP_SW_STATE_CHANGE_STATUS_8TO11 and ACP_SW_STATE_CHANGE_STATUS_0TO7 to
0x00, the peripheral attachment or alert is permanently lost.  Setting
amd_manager->interrupt_enabled = true before unmasking the hardware
registers avoids this race.

>  static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
>  {
>  	u32 irq_mask;
>  
> +	/*
> +	 * Clear the flag before updating hardware masks so that any IRQ
> +	 * that fires during the mask-register writes sees the flag as false
> +	 * and does not queue new work. Once the mask registers are written,
> +	 * the ACP hardware will not assert any new SDW interrupt, so no
> +	 * synchronize_irq() is required after this function returns.
> +	 */
> +	amd_manager->interrupt_enabled = false;
> +
>  	mutex_lock(amd_manager->acp_sdw_lock);
>  	irq_mask = sdw_manager_reg_mask_array[amd_manager->instance];
>  	amd_updatel(amd_manager->acp_mmio, ACP_EXTERNAL_INTR_CNTL(amd_manager->instance),

[Severity: Medium]
Does clearing amd_manager->interrupt_enabled and zeroing the mask
registers before cancel_work_sync(&amd_manager->amd_sdw_work) allow
amd_sdw_update_slave_status_work() to re-enable the status masks?

drivers/soundwire/amd_manager.c:amd_sdw_update_slave_status_work() {
    ...
	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);
    ...
}

CPU0                                     CPU1 (kworker)
                                         amd_sdw_update_slave_status_work()
                                           sdw_handle_slave_status(...)
amd_disable_sdw_interrupts()
  amd_manager->interrupt_enabled = false
  writel(0x00, ..._STATUS_MASK_0TO7)
  writel(0x00, ..._STATUS_MASK_8TO11)
                                           writel(AMD_SDW_IRQ_MASK_0TO7, ...)
                                           writel(AMD_SDW_IRQ_MASK_8TO11, ...)
                                             <-- masks re-enabled
cancel_work_sync(&amd_manager->amd_sdw_work)

During runtime suspend in AMD_SDW_POWER_OFF_MODE, amd_sdw_wake_enable()
then performs a read-modify-write on ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11
while ACP_EXTERNAL_INTR_CNTL is re-enabled for wake on ACP 6.3, leaving
non-wake state-change interrupts unmasked during suspend.

> @@ -1143,7 +1153,7 @@ static void amd_sdw_irq_thread(struct work_struct *work)
>  		/* 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)
> +	if ((status_change_8to11 || status_change_0to7) && amd_manager->interrupt_enabled)
>  		schedule_work(&amd_manager->amd_sdw_work);
>  	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
>  	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);

[Severity: Medium]
Does guarding only schedule_work(&amd_manager->amd_sdw_work) inside
amd_sdw_irq_thread() still leave amd_sdw_irq_thread itself unguarded
against an in-flight hard IRQ handler?

amd_sdw_irq_thread() is a work_struct callback rather than a threaded IRQ
handler.  The actual hard IRQ handlers (such as acp63_irq_handler() in
sound/soc/amd/ps/pci-ps.c and acp_irq_handler() in
sound/soc/sof/amd/acp.c) schedule amd_sdw_irq_thread without checking
amd_manager->interrupt_enabled:

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
    ...
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
    ...
}

Without synchronize_irq() or a check before schedule_work() in the hard
IRQ handler, an IRQ that fires just before ACP_EXTERNAL_INTR_CNTL is
masked can queue amd_sdw_irq_thread after cancel_work_sync() returns:

CPU0                                     CPU1 (<hardirq>)
                                         acp63_irq_handler()
                                           ext_intr_stat & ACP_SDW0_STAT
amd_sdw_manager_remove()
  amd_disable_sdw_interrupts()
    amd_manager->interrupt_enabled = false
    amd_updatel(..., ACP_EXTERNAL_INTR_CNTL, ...)
  cancel_work_sync(&amd_manager->amd_sdw_irq_thread)
  cancel_work_sync(&amd_manager->amd_sdw_work)
                                           schedule_work(&...->amd_sdw_irq_thread)
  /* devm frees amd_manager */
                                         amd_sdw_irq_thread()
                                           readl(amd_manager->mmio + ...)
                                             <-- use-after-free

Even when amd_sdw_irq_thread() runs with interrupt_enabled == false before
cancel_work_sync(), it still reads and writes MMIO registers and calls
amd_sdw_read_and_process_ping_status() before reaching the
interrupt_enabled check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=17