Re: [PATCH 1/7] soundwire: intel: Define stubs when library is disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] soundwire: intel: link failure when SND_INTEL_DSP_CONFIG=y, SOUNDWIRE_INTEL=m
- [Medium] soundwire: intel: sdw_intel_acpi_scan() stub returns 0 leaving info uninit
--

commit 36be5679dc6f7a17f83bd829bfffe5d60fd5b81f
Author: Cezary Rojewski <cezary.rojewski@intel.com>

soundwire: intel: Define stubs when library is disabled

This commit adds inline stub functions in include/linux/soundwire/sdw_intel.h
when CONFIG_SOUNDWIRE_INTEL is disabled and updates sound/hda/core/Kconfig
and Makefile so SND_INTEL_SOUNDWIRE_ACPI is only selected and built when
SOUNDWIRE_INTEL is enabled.

> diff --git a/include/linux/soundwire/sdw_intel.h b/include/linux/soundwire/sdw_intel.h
> index 9710f2dc04e29..83021cc0f5b86 100644
> --- a/include/linux/soundwire/sdw_intel.h
> +++ b/include/linux/soundwire/sdw_intel.h
> @@ -358,6 +358,8 @@ struct sdw_intel_res {
>  	struct mutex *eml_lock;
>  };
>  
> +#if IS_ENABLED(CONFIG_SOUNDWIRE_INTEL)
> +
>  /*
>   * On Intel platforms, the SoundWire IP has dependencies on power
>   * rails shared with the DSP, and the initialization steps are split
> @@ -384,6 +386,38 @@ void sdw_intel_exit(struct sdw_intel_ctx *ctx);
>  
>  irqreturn_t sdw_intel_thread(int irq, void *dev_id);
>  
> +#else
> +
> +static inline int sdw_intel_acpi_scan(acpi_handle parent_handle, struct sdw_intel_acpi_info *info)
> +{
> +	return 0;
> +}

[Severity: Medium]
Should the sdw_intel_acpi_scan() stub return -ENODEV instead of 0?

In sound/hda/core/intel-sdw-acpi.c, sdw_intel_acpi_scan() returns -ENODEV
when no SoundWire controller is found, and only returns 0 after populating
info->handle, info->count, and info->link_mask.

Callers pass an uninitialized struct sdw_intel_acpi_info on the stack and
check for a negative return value before reading its fields, for example in
sound/hda/core/intel-dsp-config.c:snd_intel_dsp_check_soundwire():

	struct sdw_intel_acpi_info info;
	acpi_handle handle;
	int ret;

	handle = ACPI_HANDLE(&pci->dev);
	if (!handle)
		return -ENODEV;

	ret = sdw_intel_acpi_scan(handle, &info);
	if (ret < 0)
		return ret;

	return info.link_mask;

With the stub returning 0 when CONFIG_SOUNDWIRE_INTEL=n, callers such as
snd_intel_dsp_check_soundwire(), hda_sdw_acpi_scan() in
sound/soc/sof/intel/hda.c, and avs_sdw_probe() in
sound/soc/intel/avs/soundwire.c will read uninitialized stack memory from
info and treat a SoundWire controller as present, causing subsequent
sdw_intel_probe() calls to return NULL and fail probing with -EINVAL.

[ ... ]

> diff --git a/sound/hda/core/Kconfig b/sound/hda/core/Kconfig
> index bfdcf6384c521..f91053dd9f589 100644
> --- a/sound/hda/core/Kconfig
> +++ b/sound/hda/core/Kconfig
> @@ -29,7 +29,7 @@ config SND_INTEL_DSP_CONFIG
>  	tristate
>  	select ACPI_NHLT if ACPI
>  	select SND_INTEL_NHLT if ACPI
> -	select SND_INTEL_SOUNDWIRE_ACPI if ACPI
> +	select SND_INTEL_SOUNDWIRE_ACPI if SOUNDWIRE_INTEL

[Severity: Medium]
Can this cause a vmlinux link failure when CONFIG_SND_INTEL_DSP_CONFIG=y
and CONFIG_SOUNDWIRE_INTEL=m?

Because SOUNDWIRE_INTEL is a tristate symbol, "select
SND_INTEL_SOUNDWIRE_ACPI if SOUNDWIRE_INTEL" bounds SND_INTEL_SOUNDWIRE_ACPI
to the minimum of SND_INTEL_DSP_CONFIG and SOUNDWIRE_INTEL. When a built-in
driver selects SND_INTEL_DSP_CONFIG=y (such as CONFIG_SND_HDA_INTEL=y) while
CONFIG_SOUNDWIRE_INTEL=m and CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE=m,
CONFIG_SND_INTEL_SOUNDWIRE_ACPI is built as a loadable module (=m).

In sound/hda/core/intel-dsp-config.c (built-in via SND_INTEL_DSP_CONFIG=y),
IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE) and
IS_ENABLED(CONFIG_SOUNDWIRE_INTEL) both evaluate to true for =m:

sound/hda/core/intel-dsp-config.c:snd_intel_dsp_check_soundwire() {
	...
	ret = sdw_intel_acpi_scan(handle, &info);

This causes built-in code in snd-intel-dspcfg to reference
sdw_intel_acpi_scan() in the loadable module snd-intel-sdw-acpi.ko. Should
this use a boolean condition instead, matching sound/soc/sof/intel/Kconfig
and sound/soc/intel/Kconfig:

	select SND_INTEL_SOUNDWIRE_ACPI if SOUNDWIRE_INTEL != n

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