[PATCH] ASoC: amd: ps: fix snd_acp63_remove() teardown ordering

[PATCH] ASoC: amd: ps: fix snd_acp63_remove() teardown ordering

[Fan Wu]

The ACP threaded interrupt handler dereferences the SoundWire and PDM
child platform devices, but snd_acp63_remove() unregisters them while
the interrupt is still registered: devm_request_threaded_irq() ties its
release to devres cleanup, which runs only after the remove callback
returns. A completion in this window is a use-after-free.

Fix this by masking the ACP interrupt sources and calling devm_free_irq()
before the first child device is unregistered. The interrupt line is
shared, and acp_hw_deinit() clears the sources only after the children
are gone, which would leave the line raised with no handler left to
ack it. The window predates the tagged refactor, which only reshaped
the dereferences.

This issue was found by an in-house static analysis tool.

Fixes: eaf825037d6d ("ASoC: amd: ps: refactor acp child platform device creation code")
Cc: stable@vger.kernel.org
Co-developed-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Song Li <songl@zju.edu.cn>
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 sound/soc/amd/ps/pci-ps.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
index 729f9aa..01ee697 100644
--- a/sound/soc/amd/ps/pci-ps.c
+++ b/sound/soc/amd/ps/pci-ps.c
@@ -738,6 +738,12 @@ static void snd_acp63_remove(struct pci_dev *pci)
 	int ret;
 
 	adata = pci_get_drvdata(pci);
+	/* Mask the interrupt sources before freeing the shared IRQ. */
+	writel(ACP_EXT_INTR_STAT_CLEAR_MASK,
+	       adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
+	writel(0, adata->acp63_base + ACP_EXTERNAL_INTR_CNTL);
+	writel(0, adata->acp63_base + ACP_EXTERNAL_INTR_ENB);
+	devm_free_irq(&pci->dev, pci->irq, adata);
 	if (adata->sdw) {
 		amd_sdw_exit(adata);
 		platform_device_unregister(adata->sdw_dma_dev);

Re: [PATCH] ASoC: amd: ps: fix snd_acp63_remove() teardown ordering

[Mukunda,Vijendar]

On 9/23/26 14:56, Fan Wu wrote:
> The ACP threaded interrupt handler dereferences the SoundWire and PDM
> child platform devices, but snd_acp63_remove() unregisters them while
> the interrupt is still registered: devm_request_threaded_irq() ties its
> release to devres cleanup, which runs only after the remove callback
> returns. A completion in this window is a use-after-free.
>
> Fix this by masking the ACP interrupt sources and calling devm_free_irq()
> before the first child device is unregistered. The interrupt line is
> shared, and acp_hw_deinit() clears the sources only after the children
> are gone, which would leave the line raised with no handler left to
> ack it. The window predates the tagged refactor, which only reshaped
> the dereferences.
>
> This issue was found by an in-house static analysis tool.
>
> Fixes: eaf825037d6d ("ASoC: amd: ps: refactor acp child platform device creation code")
> Cc: stable@vger.kernel.org
> Co-developed-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>   sound/soc/amd/ps/pci-ps.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index 729f9aa..01ee697 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -738,6 +738,12 @@ static void snd_acp63_remove(struct pci_dev *pci)
>   	int ret;
>   
>   	adata = pci_get_drvdata(pci);
> +	/* Mask the interrupt sources before freeing the shared IRQ. */
> +	writel(ACP_EXT_INTR_STAT_CLEAR_MASK,
> +	       adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> +	writel(0, adata->acp63_base + ACP_EXTERNAL_INTR_CNTL);
> +	writel(0, adata->acp63_base + ACP_EXTERNAL_INTR_ENB);
++ The fix open-codes the register offsets (ACP_EXTERNAL_INTR_STAT, 
ACP_EXTERNAL_INTR_CNTL, ACP_EXTERNAL_INTR_ENB) directly in 
snd_acp63_remove(). Future platforms may have different interrupt 
control register offsets, which would require changes in this remove 
path as well. Consider adding a disable_interrupts callback to struct 
acp_hw_ops and invoking it here instead. This keeps the remove path 
platform-agnostic and the interrupt masking logic co-located with its 
platform-specific counterpart in ps-common.c.
> +	devm_free_irq(&pci->dev, pci->irq, adata);
>   	if (adata->sdw) {
>   		amd_sdw_exit(adata);
>   		platform_device_unregister(adata->sdw_dma_dev);

Re: [PATCH] ASoC: amd: ps: fix snd_acp63_remove() teardown ordering

[Fan Wu]

On 9/25/26 13:35, Mukunda,Vijendar wrote:
> The fix open-codes the register offsets (ACP_EXTERNAL_INTR_STAT,
> ACP_EXTERNAL_INTR_CNTL, ACP_EXTERNAL_INTR_ENB) directly in
> snd_acp63_remove(). Future platforms may have different interrupt
> control register offsets, which would require changes in this remove
> path as well. Consider adding a disable_interrupts callback to struct
> acp_hw_ops and invoking it here instead. This keeps the remove path
> platform-agnostic and the interrupt masking logic co-located with its
> platform-specific counterpart in ps-common.c.

Thank you for the review. Agreed — I have reworked the patch exactly
along these lines for v2:

- struct acp_hw_ops gains a disable_interrupts callback plus an
  acp_hw_disable_interrupts() wrapper, following the interrupt-control
  ops pattern of the acp family in amd.h (en_interrupts member and
  acp_disable_interrupts() helper).
- acp63_hw_init_ops() and acp70_hw_init_ops() wire it to the existing
  static acp63_disable_interrupts()/acp70_disable_interrupts() helpers
  in ps-common.c, so the masking sits next to its platform-specific
  counterparts.
- snd_acp63_remove() now calls acp_hw_disable_interrupts() followed by
  devm_free_irq() before the first child device is unregistered; the
  register writes are gone from the remove path.

The MMIO sequence is unchanged (clear STAT, CNTL = 0, ENB = 0 before
freeing the shared IRQ), and the Fixes tag stays on eaf825037d6d. I
will send v2 as a new thread with a link to this version.

Separately, I noticed your "[PATCH V2 0/9] soundwire: amd: SoundWire
manager driver bug fixes" series: patch 3/9, adding the amd_sdw_irq_thread
drain in amd_sdw_manager_remove(), is exactly the soundwire-side companion
this fix needs. The v2 commit message now notes that the manager-side
drain is handled separately, and with both in place the chain is closed
from the ACP hardirq down to the manager work.

Best regards,
Fan Wu