Re: [PATCH 10/11] ASoC: amd: ps: harden the ACP interrupt path for ACP6x

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: amd: ps: unacknowledged DMA IRQ and NULL sdw_data in acp63_irq_handler()
- [High] ASoC: amd: ps: NULL sdw->pdev[i] dereference in acp63_irq_handler()
--

commit 00d5bef454b30567a1333bcc6ac7ae59904adf98
Author: Corey Smith <corsmith@gmail.com>

ASoC: amd: ps: harden the ACP interrupt path for ACP6x

This commit adds NULL checks for adata->sdw, adata->pdm_dev, and
adata->sdw_dma_dev in acp63_irq_handler(), and extends
check_and_handle_sdw_dma_irq() to handle ACP6X_PCI_REV and ACP6XR_PCI_REV.

> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
> index 235cb1ef25f14..b06b399060288 100644
> --- a/sound/soc/amd/ps/pci-ps.c
> +++ b/sound/soc/amd/ps/pci-ps.c

[ ... ]

> @@ -209,18 +213,28 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>  	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
>  	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);
> +		/*
> +		 * The handler may run before the SoundWire manager devices
> +		 * have been created (shared IRQ lines can fire as soon as
> +		 * they are registered, and the ACP interrupts are enabled by
> +		 * acp_hw_init() before create_acp63_platform_devs() runs).
> +		 */
> +		if (adata->sdw) {
> +			amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> +			if (amd_manager)
> +				schedule_work(&amd_manager->amd_sdw_irq_thread);
> +		}
>  		irq_flag = 1;
>  	}
>  
>  	ext_intr_stat1 = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
>  	if (ext_intr_stat1 & ACP_SDW1_STAT) {
>  		writel(ACP_SDW1_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT1);
> -		amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
> -		if (amd_manager)
> -			schedule_work(&amd_manager->amd_sdw_irq_thread);
> +		if (adata->sdw) {
> +			amd_manager = dev_get_drvdata(&adata->sdw->pdev[1]->dev);
> +			if (amd_manager)
> +				schedule_work(&amd_manager->amd_sdw_irq_thread);
> +		}
>  		irq_flag = 1;
>  	}

[Severity: High]
Can adata->sdw->pdev[0] or adata->sdw->pdev[1] still be NULL here when
adata->sdw is non-NULL?

In drivers/soundwire/amd_init.c:sdw_amd_probe_controller(), ctx->pdev[index]
is only registered for links enabled in link_mask:

	for (index = 0; index < count; index++) {
		if (!(link_mask & BIT(index)))
			continue;
		...
		ctx->pdev[index] = platform_device_register_full(&pdevinfo[index]);

On single-link platforms where only one link is set in link_mask (for example,
link_mask == BIT(0) where adata->sdw->pdev[1] remains NULL), if a pending or
spurious ACP_SDW1_STAT or ACP_SDW0_STAT bit is set in ACP_EXTERNAL_INTR_STAT1
or ACP_EXTERNAL_INTR_STAT, acp63_irq_handler() will pass the adata->sdw check
and pass &adata->sdw->pdev[i]->dev to dev_get_drvdata(), dereferencing a NULL
pdev[i] pointer in hard IRQ context.

Note that handle_acp70_sdw_wake_event() in sound/soc/amd/ps/pci-ps.c (called
from acp63_irq_handler() -> check_and_handle_acp70_sdw_wake_irq()) also
dereferences adata->sdw->pdev[0] and adata->sdw->pdev[1] without checking
adata->sdw or adata->sdw->pdev[i] first, whereas
handle_acp63_sdw_pme_event() in sound/soc/amd/ps/ps-common.c checks both
adata->sdw and adata->sdw->pdev[i]. Should both acp63_irq_handler() and
handle_acp70_sdw_wake_event() check adata->sdw->pdev[i] before calling
dev_get_drvdata()?

> @@ -237,16 +251,22 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>  		wake_irq_flag = check_and_handle_acp70_sdw_wake_irq(adata);
>  
>  	if (ext_intr_stat & BIT(PDM_DMA_STAT)) {
> -		ps_pdm_data = dev_get_drvdata(&adata->pdm_dev->dev);
>  		writel(BIT(PDM_DMA_STAT), adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> -		if (ps_pdm_data->capture_stream)
> -			snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
> +		if (adata->pdm_dev) {
> +			ps_pdm_data = dev_get_drvdata(&adata->pdm_dev->dev);
> +			if (ps_pdm_data && ps_pdm_data->capture_stream)
> +				snd_pcm_period_elapsed(ps_pdm_data->capture_stream);
> +		}
>  		irq_flag = 1;
>  	}
>  
> -	sdw_dma_irq_flag = check_and_handle_sdw_dma_irq(adata, ext_intr_stat, ext_intr_stat1);
> -	if (sdw_dma_irq_flag)
> -		return IRQ_WAKE_THREAD;
> +	/* The SoundWire DMA device may not exist yet during probe */
> +	if (adata->sdw_dma_dev) {
> +		sdw_dma_irq_flag = check_and_handle_sdw_dma_irq(adata, ext_intr_stat,
> +								ext_intr_stat1);
> +		if (sdw_dma_irq_flag)
> +			return IRQ_WAKE_THREAD;
> +	}

[Severity: High]
Does skipping check_and_handle_sdw_dma_irq() when adata->sdw_dma_dev is NULL
leave pending SoundWire DMA interrupt bits uncleared, and can waking the IRQ
thread as soon as adata->sdw_dma_dev is non-NULL dereference a NULL sdw_data?

Unlike the ACP_SDW0_STAT, ACP_SDW1_STAT, and BIT(PDM_DMA_STAT) branches above,
which clear their status bits in ACP_EXTERNAL_INTR_STAT /
ACP_EXTERNAL_INTR_STAT1 before checking the device pointers, the writel()
calls that clear the SoundWire DMA interrupt status bits are inside
check_and_handle_sdw_dma_irq():

	if (ext_intr_stat & ACP63_SDW_DMA_IRQ_MASK) {
		for (index = ACP_AUDIO2_RX_THRESHOLD; index <= ACP_AUDIO0_TX_THRESHOLD; index++) {
			if (ext_intr_stat & BIT(index)) {
				writel(BIT(index), adata->acp63_base + ACP_EXTERNAL_INTR_STAT);

If a SoundWire DMA interrupt bit is pending while adata->sdw_dma_dev is NULL
(during snd_acp63_probe() before create_acp63_platform_devs(), or on a
PDM-only configuration), skipping check_and_handle_sdw_dma_irq() leaves the
hardware status bit asserted and returns IRQ_NONE on a shared level-triggered
PCI interrupt line, causing an unhandled interrupt storm.

Furthermore, in sound/soc/amd/ps/pci-ps.c:create_acp63_platform_devs():

	adata->sdw_dma_dev = platform_device_register_full(&pdevinfo);

adata->sdw_dma_dev becomes non-NULL before the amd_ps_sdw_dma driver
(acp63_sdw_platform_probe() in sound/soc/amd/ps/ps-sdw-dma.c, which may be in
a separate module) binds to the device and sets drvdata (or holds an ERR_PTR
if registration fails). Because this patch also adds ACP6X_PCI_REV and
ACP6XR_PCI_REV to check_and_handle_sdw_dma_irq(), an interrupt in that window
returns IRQ_WAKE_THREAD and runs acp63_irq_thread() ->
acp63_sdw_dma_irq_thread() (or acp70_sdw_dma_irq_thread()) in
sound/soc/amd/ps/ps-common.c:

	sdw_data = dev_get_drvdata(&adata->sdw_dma_dev->dev);

	for (stream_id = 0; stream_id < ACP63_SDW0_DMA_MAX_STREAMS; stream_id++) {
		if (adata->acp63_sdw0_dma_intr_stat[stream_id]) {
			if (sdw_data->acp63_sdw0_dma_stream[stream_id])
				snd_pcm_period_elapsed(sdw_data->acp63_sdw0_dma_stream[stream_id]);

which dereferences sdw_data without checking if it is NULL.

Should check_and_handle_sdw_dma_irq() always run to clear the hardware status
bits, and should acp63_sdw_dma_irq_thread() / acp70_sdw_dma_irq_thread() (or
acp63_irq_handler() before returning IRQ_WAKE_THREAD) verify that
adata->sdw_dma_dev and dev_get_drvdata(&adata->sdw_dma_dev->dev) are valid?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028020552.35278-1-corsmith@gmail.com?part=10