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

[Mukunda,Vijendar]

On 29/09/26 11:43, Fan Wu wrote:
> [Some people who received this message don't often get email from fanwu01@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> The ACP IRQ handlers dereference the SoundWire, SoundWire DMA, and PDM
> child platform devices, but snd_acp63_remove() unregisters them while
> the interrupt is still held by devres, which frees it only after remove
> returns. An interrupt in this window is a use-after-free.
>
> Mask the ACP interrupt sources and call devm_free_irq() before the
> first child device is unregistered. The masking goes through the new
> acp_hw_ops->disable_interrupts callback, keeping the remove path
> platform-agnostic. 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.
>
> devm_free_irq() waits for in-flight handlers, but not for work the
> hardirq has already queued: acp63_irq_handler() schedules
> amd_sdw_irq_thread on the SoundWire manager. Draining that work in
> the manager's remove path is a soundwire-side change and follows
> separately.
>
> 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@xxxxxxxxxxxxxxx
> Co-developed-by: Song Li <songl@xxxxxxxxxx>
> Signed-off-by: Song Li <songl@xxxxxxxxxx>
> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>
Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
> ---
>
> Link to v1: https://lore.kernel.org/linux-sound/20260923092640.502145-1-fanwu01@xxxxxxxxxx/
>
> - mask ACP interrupt sources via the new acp_hw_ops->disable_interrupts
>   callback instead of open-coding the register writes in
>   snd_acp63_remove(), as suggested by Vijendar Mukunda.
>
>  sound/soc/amd/ps/acp63.h     | 8 ++++++++
>  sound/soc/amd/ps/pci-ps.c    | 2 ++
>  sound/soc/amd/ps/ps-common.c | 2 ++
>  3 files changed, 12 insertions(+)
>
> diff --git a/sound/soc/amd/ps/acp63.h b/sound/soc/amd/ps/acp63.h
> index 62cb6bef1..d0cfbd4da 100644
> --- a/sound/soc/amd/ps/acp63.h
> +++ b/sound/soc/amd/ps/acp63.h
> @@ -294,6 +294,7 @@ struct acp63_dev_data;
>   * struct acp_hw_ops - ACP PCI driver platform specific ops
>   * @acp_init: ACP initialization
>   * @acp_deinit: ACP de-initialization
> + * @disable_interrupts: disable ACP interrupt sources
>   * @acp_get_config: function to read the acp pin configuration
>   * @acp_sdw_dma_irq_thread: ACP SoundWire DMA interrupt thread
>   * acp_suspend: ACP system level suspend callback
> @@ -304,6 +305,7 @@ struct acp63_dev_data;
>  struct acp_hw_ops {
>         int (*acp_init)(void __iomem *acp_base, struct device *dev);
>         int (*acp_deinit)(void __iomem *acp_base, struct device *dev);
> +       void (*disable_interrupts)(void __iomem *acp_base);
>         void (*acp_get_config)(struct pci_dev *pci, struct acp63_dev_data *acp_data);
>         void (*acp_sdw_dma_irq_thread)(struct acp63_dev_data *acp_data);
>         int (*acp_suspend)(struct device *dev);
> @@ -397,6 +399,12 @@ static inline int acp_hw_deinit(struct acp63_dev_data *adata, struct device *dev
>         return -EOPNOTSUPP;
>  }
>
> +static inline void acp_hw_disable_interrupts(struct acp63_dev_data *adata)
> +{
> +       if (adata && adata->hw_ops && adata->hw_ops->disable_interrupts)
> +               ACP_HW_OPS(adata, disable_interrupts)(adata->acp63_base);
> +}
> +
>  static inline void acp_hw_get_config(struct pci_dev *pci, struct acp63_dev_data *adata)
>  {
>         if (adata && adata->hw_ops && adata->hw_ops->acp_get_config)
> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index 729f9aaba..b604db494 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c
> @@ -738,6 +738,8 @@ static void snd_acp63_remove(struct pci_dev *pci)
>         int ret;
>
>         adata = pci_get_drvdata(pci);
> +       acp_hw_disable_interrupts(adata);
> +       devm_free_irq(&pci->dev, pci->irq, adata);
>         if (adata->sdw) {
>                 amd_sdw_exit(adata);
>                 platform_device_unregister(adata->sdw_dma_dev);
> diff --git a/sound/soc/amd/ps/ps-common.c b/sound/soc/amd/ps/ps-common.c
> index 7b4966b75..ea71cdf19 100644
> --- a/sound/soc/amd/ps/ps-common.c
> +++ b/sound/soc/amd/ps/ps-common.c
> @@ -248,6 +248,7 @@ void acp63_hw_init_ops(struct acp_hw_ops *hw_ops)
>  {
>         hw_ops->acp_init = acp63_init;
>         hw_ops->acp_deinit = acp63_deinit;
> +       hw_ops->disable_interrupts = acp63_disable_interrupts;
>         hw_ops->acp_get_config = acp63_get_config;
>         hw_ops->acp_sdw_dma_irq_thread = acp63_sdw_dma_irq_thread;
>         hw_ops->acp_suspend = snd_acp63_suspend;
> @@ -484,6 +485,7 @@ void acp70_hw_init_ops(struct acp_hw_ops *hw_ops)
>  {
>         hw_ops->acp_init = acp70_init;
>         hw_ops->acp_deinit = acp70_deinit;
> +       hw_ops->disable_interrupts = acp70_disable_interrupts;
>         hw_ops->acp_get_config = acp70_get_config;
>         hw_ops->acp_sdw_dma_irq_thread = acp70_sdw_dma_irq_thread;
>         hw_ops->acp_suspend = snd_acp70_suspend;
> --
> 2.34.1