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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] ASoC: Intel: avs: premature IRQ disable and UAF in avs_sdw_exit()
- [Medium] ASoC: Intel: avs: disabling IRQs before avs_sdw_exit() in avs_pci_remove() breaks child resume
- [Medium] ASoC: Intel: avs: missing link_mask check in avs_sdw_probe()
- [Medium] ASoC: Intel: avs: cAVS SoundWire MMIO offsets used on ACE platforms
- [Low] ASoC: Intel: avs: vmlinux link failure when SND_SOC_INTEL_AVS=y and SOUNDWIRE_INTEL=m
--

commit 99a0d9e68cf08e7f2ab6024f81ba2c7db0e51ac2
Author: Cezary Rojewski <cezary.rojewski@intel.com>

ASoC: Intel: avs: SoundWire support

This commit adds SoundWire enumeration, startup, interrupt handling, and
teardown support to the Intel AVS driver using the SoundWire Intel library.

> diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
> index 63367364916ae..4e453b55fc043 100644
> --- a/sound/soc/intel/Kconfig
> +++ b/sound/soc/intel/Kconfig
> @@ -106,6 +106,7 @@ config SND_SOC_INTEL_AVS
>  	select SND_HDA_EXT_CORE
>  	select SND_HDA_DSP_LOADER
>  	select SND_INTEL_DSP_CONFIG
> +	select SND_INTEL_SOUNDWIRE_ACPI if SOUNDWIRE_INTEL != n
>  	select WANT_DEV_COREDUMP
>  	help
>  	  Enable support for Intel(R) cAVS 1.5 platforms with DSP
> diff --git a/sound/soc/intel/avs/Makefile b/sound/soc/intel/avs/Makefile
> index 576dc0da381d8..1d48953c7a31c 100644
> --- a/sound/soc/intel/avs/Makefile
> +++ b/sound/soc/intel/avs/Makefile
> @@ -14,6 +14,10 @@ ifneq ($(CONFIG_DEBUG_FS),)
>  snd-soc-avs-y += probes.o debugfs.o
>  endif
>  
> +ifneq ($(CONFIG_SOUNDWIRE_INTEL),)
> +snd-soc-avs-y += soundwire.o
> +endif

[Severity: Low]
Will vmlinux fail to link when CONFIG_SND_SOC_INTEL_AVS=y and
CONFIG_SOUNDWIRE_INTEL=m?

In that configuration, sound/soc/intel/avs/Makefile compiles soundwire.o into
the built-in snd-soc-avs.o, and sound/soc/intel/avs/soundwire.h enables the
external declarations under IS_ENABLED(CONFIG_SOUNDWIRE_INTEL). However,
avs_sdw_probe(), avs_sdw_startup(), avs_sdw_exit(),
avs_sdw_process_wakeen_event(), and avs_sdw_thread() in
sound/soc/intel/avs/soundwire.c call sdw_intel_cnl_hw_ops, sdw_intel_probe(),
sdw_intel_startup(), sdw_intel_exit(), sdw_intel_process_wakeen_event(), and
sdw_intel_thread(), which live in the loadable soundwire-intel.ko module.

Should SND_SOC_INTEL_AVS depend on SOUNDWIRE_INTEL || !SOUNDWIRE_INTEL (or
use IS_REACHABLE), similar to how other ASoC drivers handle optional
tristate dependencies?

[ ... ]

> diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c
> index 9413027e1e322..f9f25f651a9e2 100644
> --- a/sound/soc/intel/avs/core.c
> +++ b/sound/soc/intel/avs/core.c

[ ... ]

> @@ -587,6 +603,9 @@ static void avs_pci_remove(struct pci_dev *pci)
>  	avs_dsp_op(adev, int_control, false);
>  	snd_hdac_ext_bus_ppcap_int_enable(bus, false);
>  
> +	avs_sdw_enable_irq(adev, false);
> +	avs_sdw_exit(adev);

[Severity: Medium]
In avs_pci_remove(), won't disabling interrupts before calling avs_sdw_exit()
cause SoundWire child device resume to time out during driver removal?

Before avs_sdw_exit(adev) is called, avs_pci_remove() runs:

    snd_hdac_stop_streams_and_chip(bus);
    avs_dsp_op(adev, int_control, false);
    snd_hdac_ext_bus_ppcap_int_enable(bus, false);

    avs_sdw_enable_irq(adev, false);
    avs_sdw_exit(adev);

Inside avs_sdw_exit() -> sdw_intel_exit(), the SoundWire teardown iterates
over ctx->link_list and synchronously runtime-resumes all child peripheral
devices and links via device_for_each_child(bus->dev, NULL,
intel_resume_child_device) before cleaning them up.

Because avs_sdw_probe() sets res.clock_stop_quirks =
SDW_INTEL_CLK_STOP_BUS_RESET, resuming a runtime-suspended link and its
peripherals restarts the bus via sdw_intel_start_bus_after_reset(), waits for
peripheral attachment, and performs Cadence MCP register transfers in
cdns_xfer_msg() waiting on cdns->tx_complete. Those completions and status
updates depend on sdw_cdns_irq() running from avs_dsp_irq_thread() ->
avs_cnl_dsp_interrupt() -> avs_sdw_thread(), which cannot fire once HDA,
PPCTL, and AVS_ADSP_ADSPIC2_SNDW interrupts have been disabled.

[ ... ]

> diff --git a/sound/soc/intel/avs/soundwire.c b/sound/soc/intel/avs/soundwire.c
> new file mode 100644
> index 0000000000000..c2a6db08a3afb
> --- /dev/null
> +++ b/sound/soc/intel/avs/soundwire.c
> @@ -0,0 +1,112 @@

[ ... ]

> +int avs_sdw_probe(struct avs_dev *adev)
> +{
> +	struct hdac_bus *hbus = &adev->base.core;
> +	struct sdw_intel_acpi_info info;
> +	struct sdw_intel_ctx *ctx;
> +	struct sdw_intel_res res;
> +	int ret;
> +
> +	ret = sdw_intel_acpi_scan(ACPI_HANDLE(adev->dev), &info);
> +	if (ret < 0)
> +		return -ENODEV;

[Severity: Medium]
Should avs_sdw_probe() also check if (!info.link_mask) before proceeding?

In sdw_intel_acpi_scan() -> sdw_intel_scan_controller(), when the ACPI
controller node is present with a valid master count but all links are
disabled in firmware or masked via sdw_link_mask, sdw_intel_acpi_scan()
returns 0 with info.link_mask == 0.

Without checking !info.link_mask, avs_sdw_probe() passes res.count > 0 and
res.link_mask == 0 to sdw_intel_probe(), which allocates and returns a
non-NULL struct sdw_intel_ctx with zero active links. That leaves adev->sdw
non-NULL with the SoundWire SHIM unpowered, causing avs_sdw_enable_irq() to
unmask AVS_ADSP_ADSPIC2_SNDW and avs_cnl_dsp_interrupt() to read
SDW_SHIM_WAKESTS (returning 0xffffffff when unpowered and falsely reporting
IRQ_HANDLED) on every DSP interrupt.

> +
> +	memset(&res, 0, sizeof(res));
> +	res.mmio_base = adev->dsp_ba;
> +	res.shim_base = SDW_SHIM_BASE;
> +	res.alh_base = SDW_ALH_BASE;
> +	res.ext = false;
> +	res.irq = adev->base.core.irq;
> +	res.handle = info.handle;
> +	res.parent = adev->dev;
> +	res.dev = adev->dev;
> +	res.clock_stop_quirks = SDW_INTEL_CLK_STOP_BUS_RESET;
> +	res.hbus = hbus;
> +
> +	memcpy(&avs_sdw_cnl_ops, &sdw_intel_cnl_hw_ops, sizeof(avs_sdw_cnl_ops));
> +	avs_sdw_cnl_ops.register_dai = NULL;
> +	res.hw_ops = &avs_sdw_cnl_ops;

[Severity: Medium]
What happens when avs_hda_probe_work() calls avs_sdw_probe(),
avs_sdw_startup(), and avs_sdw_enable_irq() on ACE platforms such as
fcl_desc (which sets AVS_PLATATTR_ACE)?

Here avs_sdw_probe() unconditionally sets cAVS 1.8/2.5 BAR4 offsets
(res.mmio_base = adev->dsp_ba, res.shim_base = SDW_SHIM_BASE, res.alh_base =
SDW_ALH_BASE, res.ext = false) and sdw_intel_cnl_hw_ops, and
sdw_intel_enable_irq() unconditionally accesses AVS_ADSP_REG_ADSPIC2 at
dsp_ba + 0x10. Because sdw_intel_cnl_hw_ops.get_link_count is NULL,
sdw_intel_probe() succeeds without touching hardware if SoundWire is present
in ACPI, and subsequent calls to avs_sdw_startup() and avs_sdw_enable_irq()
will read and write cAVS BAR4 offsets (0x2c000, 0x30000, and 0x10) instead of
the ACE multi-link and MTL_DWICTL_BASE / MTL_HfINT_BASE registers.

Should avs_sdw_probe() guard against avs_platattr_test(adev, ACE) until ACE
SoundWire support is wired up?

[ ... ]

> +void avs_sdw_exit(struct avs_dev *adev)
> +{
> +	if (adev->sdw) {
> +		sdw_intel_exit(adev->sdw);
> +		adev->sdw = NULL;
> +	}
> +}

[Severity: High]
Can avs_cnl_dsp_interrupt() race with avs_sdw_exit() and dereference
adev->sdw after sdw_intel_exit() has freed it?

In avs_pci_remove(), pci_free_irq(pci, 0, adev) is called after
avs_sdw_exit(adev), and in avs_sdw_exit(), sdw_intel_exit(adev->sdw) frees
ctx->peripherals, ctx->ldev, and ctx before adev->sdw is cleared to NULL.
Meanwhile, avs_cnl_dsp_interrupt() checks if (adev->sdw) without
synchronization and accesses adev->sdw->shim_base, adev->sdw->link_list (via
avs_sdw_thread() -> sdw_intel_thread()), and adev->sdw->ldev (via
avs_sdw_process_wakeen_event() -> sdw_intel_process_wakeen_event()).

Even if MMIO interrupt bits are cleared beforehand, an already-running
avs_dsp_irq_thread() is not synchronized until pci_free_irq(), and if
interrupts remain enabled during sdw_intel_exit() so child devices can
runtime-resume, avs_cnl_dsp_interrupt() can run concurrently while
sdw_intel_exit() frees adev->sdw.

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