Re: [PATCH v2 5/7] ASoC: Intel: avs: SoundWire support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: Intel: avs: premature sdw clear, IRQ race, and unmasked IRQ in teardown
- [Medium] ASoC: Intel: avs: 32-bit unaligned read and missing all-ones check on WAKESTS
--

commit 8ccf2c90780786f5e6483aeba7422e7c31a54429
Author: Cezary Rojewski <cezary.rojewski@intel.com>

ASoC: Intel: avs: SoundWire support

This commit adds SoundWire support to the Intel AVS driver by utilizing
the existing SoundWire library to probe, start up, tear down, and handle
interrupts for the SoundWire component.

> diff --git a/sound/soc/intel/avs/cnl.c b/sound/soc/intel/avs/cnl.c
> index 31fd9f251d6c9..7090b91572d88 100644
> --- a/sound/soc/intel/avs/cnl.c
> +++ b/sound/soc/intel/avs/cnl.c
[ ... ]
> @@ -61,16 +66,30 @@ static void avs_cnl_ipc_interrupt(struct avs_dev *adev)
>  irqreturn_t avs_cnl_dsp_interrupt(struct avs_dev *adev, int irq)
>  {
>  	u32 adspis = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS);
> +	u32 adspis2 = snd_hdac_adsp_readl(adev, AVS_ADSP_REG_ADSPIS2);
>  	irqreturn_t ret = IRQ_NONE;
>  
> -	if (adspis == UINT_MAX)
> +	if (adspis == UINT_MAX && adspis2 == UINT_MAX)
>  		return ret;
>  
> -	if (adspis != UINT_MAX && (adspis & AVS_ADSP_ADSPIS_IPC)) {
> +	if (adspis != UINT_MAX && (adspis & AVS_ADSP_ADSPIS_IPC)) {
>  		avs_cnl_ipc_interrupt(adev);
>  		ret = IRQ_HANDLED;
>  	}
>  
> +	if (adev->sdw) {
> +		u32 wakests;
> +
> +		if (adspis2 != UINT_MAX && (adspis2 & AVS_ADSP_ADSPIS2_SNDW))
> +			ret = avs_sdw_thread(irq, adev);
> +
> +		wakests = snd_hdac_adsp_readl(adev, adev->sdw->shim_base + SDW_SHIM_WAKESTS);
> +		if (wakests) {
> +			avs_sdw_process_wakeen_event(adev);
> +			ret = IRQ_HANDLED;
> +		}
> +	}

[Severity: Medium]
In avs_cnl_dsp_interrupt() in sound/soc/intel/avs/cnl.c, should
SDW_SHIM_WAKESTS be read with the 16-bit helper snd_hdac_adsp_readw() and
checked against U16_MAX?

In include/linux/soundwire/sdw_intel.h, SDW_SHIM_WAKESTS is defined at
2-byte-aligned offset 0x192 immediately after SDW_SHIM_WAKEEN at 0x190,
and drivers/soundwire/intel.c accesses it as a 16-bit register via
intel_readw() in intel_shim_check_wake(). Using snd_hdac_adsp_readl()
performs an unaligned 32-bit MMIO read across offsets 0x192..0x195.

Additionally, in avs_hda_probe_work() in sound/soc/intel/avs/core.c,
avs_sdw_probe() populates adev->sdw before avs_dsp_first_boot_firmware()
and avs_sdw_startup() run. When an IPC interrupt fires while the SoundWire
SHIM power domain is still unpowered (or powered down at runtime), the
MMIO read can return all-ones. Without filtering out U16_MAX (like the
UINT_MAX checks on adspis and adspis2 above), could wakests evaluate to
non-zero and spuriously trigger avs_sdw_process_wakeen_event()?

> diff --git a/sound/soc/intel/avs/soundwire.c b/sound/soc/intel/avs/soundwire.c
> new file mode 100644
> index 0000000000000..284a00ea794e7
> --- /dev/null
> +++ b/sound/soc/intel/avs/soundwire.c
[ ... ]
> +void avs_sdw_exit(struct avs_dev *adev)
> +{
> +	struct sdw_intel_ctx *ctx = adev->sdw;
> +
> +	if (ctx) {
> +		adev->sdw = NULL;
> +		sdw_intel_exit(ctx);
> +	}
> +}

[Severity: High]
As reported by Sashiko on v1 [1], in avs_sdw_exit() in
sound/soc/intel/avs/soundwire.c (called from avs_pci_remove() in
sound/soc/intel/avs/core.c), does setting adev->sdw to NULL before calling
sdw_intel_exit(ctx) while interrupts are still active cause several issues
during driver removal?

1. Inside sdw_intel_exit() in drivers/soundwire/intel_init.c, each link
synchronously runtime-resumes its child peripheral devices:

drivers/soundwire/intel_init.c:sdw_intel_exit() {
	list_for_each_entry(link, &ctx->link_list, list) {
		struct sdw_bus *bus = &link->cdns->bus;
		int ret;

		ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);
		...
	}
}

Because avs_sdw_probe() sets SDW_INTEL_CLK_STOP_BUS_RESET, resuming a
suspended link powers it up, enables Cadence MCP interrupts, restarts the
SoundWire bus, and performs register transfers waiting on completion.
However, since adev->sdw was already cleared to NULL,
avs_cnl_dsp_interrupt() skips avs_sdw_thread(), so SoundWire interrupts
are never serviced during sdw_intel_exit(), causing child device resume
and bus transfers to time out while the unacknowledged level-triggered
interrupt remains asserted.

2. Neither avs_pci_remove() nor avs_sdw_exit() calls
avs_sdw_enable_irq(adev, false) to clear AVS_ADSP_ADSPIC2_SNDW in
AVS_ADSP_REG_ADSPIC2.

3. In avs_pci_remove(), avs_sdw_exit() runs before pci_free_irq() without
synchronization against the IRQ thread. In avs_cnl_dsp_interrupt(),
adev->sdw is checked at the start of the block and then dereferenced again
for adev->sdw->shim_base:

sound/soc/intel/avs/cnl.c:avs_cnl_dsp_interrupt() {
	if (adev->sdw) {
		u32 wakests;

		if (adspis2 != UINT_MAX && (adspis2 & AVS_ADSP_ADSPIS2_SNDW))
			ret = avs_sdw_thread(irq, adev);

		wakests = snd_hdac_adsp_readl(adev, adev->sdw->shim_base + SDW_SHIM_WAKESTS);
		...
	}
}

If avs_sdw_exit() clears adev->sdw and calls sdw_intel_exit(ctx)
concurrently, can avs_cnl_dsp_interrupt() hit a NULL pointer dereference
on adev->sdw->shim_base or a use-after-free on ctx in sdw_intel_thread()
and sdw_intel_process_wakeen_event()?

[1] https://lore.kernel.org/r/20261102181042.1922005-6-cezary.rojewski@intel.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103134958.2089136-1-cezary.rojewski@intel.com?part=5