Re: [PATCH 7/11] ASoC: amd: ps: probe the ACP6x SoundWire stack

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: amd: ps: truncated ACP6x reg_range in snd_acp63_probe()
--

commit 936e882d9f0167141cc5ea00f82706fe1b5cf051
Author: Corey Smith <corsmith@gmail.com>

ASoC: amd: ps: probe the ACP6x SoundWire stack

This commit enables probing ACP6x (Rembrandt) PCI revisions in the Pink
Sardine PCI driver when the FLAG_AMD_LEGACY_SDW quirk is set, wiring up the
ACP6x register range, hardware ops, SoundWire controller address, and machine
tables.

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

[ ... ]

> @@ -487,7 +514,7 @@ static int create_acp63_platform_devs(struct pci_dev *pci, struct acp63_dev_data
>  		}
>  		adata->res->flags = IORESOURCE_MEM;
>  		adata->res->start = addr;
> -		adata->res->end = addr + (ACP63_REG_END - ACP63_REG_START);
> +		adata->res->end = addr + adata->reg_range;
>  		memset(&pdevinfo, 0, sizeof(pdevinfo));
>  	}

[ ... ]

> @@ -622,7 +664,10 @@ static int snd_acp63_probe(struct pci_dev *pci,
>  		goto release_regions;
>  	}
>  	adata->addr = addr;
> -	adata->reg_range = ACP63_REG_END - ACP63_REG_START;
> +	if (pci->revision == ACP6X_PCI_REV || pci->revision == ACP6XR_PCI_REV)
> +		adata->reg_range = ACP6X_REG_END - ACP6X_REG_START;
> +	else
> +		adata->reg_range = ACP63_REG_END - ACP63_REG_START;

[Severity: Medium]
Is ACP6X_REG_END - ACP6X_REG_START large enough for the amd_ps_sdw_dma
platform device resource on ACP6x?

For ACP6X_PCI_REV and ACP6XR_PCI_REV, snd_acp63_probe() sets adata->reg_range
to ACP6X_REG_END - ACP6X_REG_START (0x1250200 - 0x1240000 = 0x10200).
create_acp63_platform_devs() then uses adata->reg_range to set the end of the
IORESOURCE_MEM resource passed to amd_ps_sdw_dma, which
acp63_sdw_platform_probe() in sound/soc/amd/ps/ps-sdw-dma.c maps with a size
of 0x10201 bytes:

sound/soc/amd/ps/ps-sdw-dma.c:acp63_sdw_platform_probe() {
	...
	sdw_data->acp_base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
	...
}

However, when configuring a SoundWire PCM stream during hw_params
(acp63_sdw_dma_hw_params() -> acp63_config_dma()) or system resume
(acp63_restore_sdw_dma_config() -> acp63_config_dma()), acp63_config_dma() in
sound/soc/amd/ps/ps-sdw-dma.c writes page table entries to ACP_SCRATCH_REG_0
(0x10000) + val:

sound/soc/amd/ps/ps-sdw-dma.c:acp63_config_dma() {
	...
	sdw_dma_pte_offset = SDW_PTE_OFFSET(stream->instance);
	val = sdw_dma_pte_offset + (stream_id * ACP_SDW_PTE_OFFSET);
	...
	for (page_idx = 0; page_idx < stream->num_pages; page_idx++) {
		...
		writel(low, acp_base + ACP_SCRATCH_REG_0 + val);
		high |= BIT(31);
		writel(high, acp_base + ACP_SCRATCH_REG_0 + val + 4);
		...
	}
}

Because SDW_PTE_OFFSET(0) is 0x400, SDW_PTE_OFFSET(1) is 0xA00, and
ACP_SDW_PTE_OFFSET is 0x100, these SoundWire DMA PTE writes occur at offsets
0x10400..0x10B7C, past the 0x10200 end of adata->res and the requested
devm_ioremap() length.

ACP6X_REG_END (0x1250200) comes from the DMIC-only driver where PDM PTEs start
at PDM_PTE_OFFSET (0x0) and only need 64 bytes of ACP_SCRATCH_REG_0. Should
ACP6X_REG_END or adata->reg_range be large enough to cover the SoundWire PTE
region in ACP_SCRATCH_REG_0 as well?

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