Re: [RFC PATCH 3/8] ASoC: SDCA: expose class helpers with hw_ops for non-DisCo platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Pointer aliasing in `class_function_probe()` overwrites the dynamically registered function descriptor, breaking devices with multiple functions of the same type.
- [High] `sdca_class_probe()` unconditionally claims the slave device's driver data, preventing caller codec drivers from maintaining their own state and causing type confusion crashes.
- [High] Exported `sdca_class_probe()` queues asynchronous work but provides no cleanup mechanism, leading to a Use-After-Free.
- [High] The `duplicate` flag is not preserved when injecting static function descriptors, breaking ALSA controls for duplicate functions.

Pre-existing issues:
- [High] `sdca_class_read_prop()` ignores memory allocation failures from `sdw_slave_read_prop()`, leading to invalid property accesses.
--

--- Patch [3]: [RFC PATCH 3/8] ASoC: SDCA: expose class helpers with hw_ops for non-DisCo platforms ---
commit bcbc6a4337e2eb8e1a4a49f6313a6ae24f542dc0
Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

ASoC: SDCA: expose class helpers with hw_ops for non-DisCo platforms

This patch modifies the SDCA class driver to expose helpers for non-DisCo
(DT/ARM) platforms. It introduces a hw_ops structure allowing codec drivers
to supply static function data and initialization routines when ACPI tables
are absent.

> diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
> --- a/sound/soc/sdca/sdca_class.c
> +++ b/sound/soc/sdca/sdca_class.c
> @@ -24,7 +24,15 @@
>  
>  #define CLASS_SDW_ATTACH_TIMEOUT_MS	5000
>  
> -static int class_read_prop(struct sdw_slave *sdw)
> +/**
> + * sdca_class_read_prop - fill SDCA-common SoundWire slave properties
> + * @sdw: SoundWire slave
> + *
> + * Exported so codec-specific SoundWire drivers can invoke the SDCA
> + * common property setup from their own sdw_slave_ops.read_prop, and
> + * then apply codec-specific overrides inline.
> + */
> +int sdca_class_read_prop(struct sdw_slave *sdw)

[Severity: High]
This isn't a bug introduced by this patch, but since this function is now
being exported as a public API, does this safely handle allocation failures
from sdw_slave_read_prop()?

If sdw_slave_read_prop() encounters memory allocation failures, it returns
-ENOMEM. Because this function ignores the return value, the SoundWire core
might attempt to access partially initialized or NULL property structures
later on. Should we propagate the return value?

[ ... ]

> @@ -156,11 +179,47 @@ static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id
>  
>  	drv->dev = dev;
>  	drv->sdw = sdw;
> +	drv->hw_ops = hw_ops;
>  	mutex_init(&drv->regmap_lock);
>  	mutex_init(&drv->init_lock);
>  
>  	dev_set_drvdata(drv->dev, drv);

[Severity: High]
When codec drivers call this newly exported sdca_class_probe() helper, will
they still be able to maintain their own private state?

Because this helper unconditionally assigns its internal sdca_class_drv struct
to the device's driver data, any calling driver that later sets its own
drvdata will overwrite it. 

If the codec driver sets its own drvdata, could this cause type confusion
when the exported PM callbacks execute?

sound/soc/sdca/sdca_class.c:class_runtime_resume() {
    struct sdca_class_drv *drv = dev_get_drvdata(dev);
    ...
    ret = sdw_slave_wait_for_init(drv->sdw, ...);
}

This cast would result in an invalid pointer dereference. Could the allocated
drv pointer be returned to the caller, or embedded in a public structure,
so codec drivers can safely use this API?

> +	/*
> +	 * On ARM platforms without ACPI/DisCo tables, sdca_lookup_functions()
> +	 * is a no-op and num_functions stays 0. Inject the function descriptors
> +	 * from the device-specific static data so sdca_dev_register_functions()
> +	 * can create the auxiliary devices.
> +	 */
> +	if (data->num_functions == 0 && hw_ops && hw_ops->get_function_data) {
> +		struct sdca_function_data *fdata;
> +		unsigned int num = 0;
> +		unsigned int i;
> +
> +		fdata = hw_ops->get_function_data(&num);
> +		if (!fdata || num == 0 || num > SDCA_MAX_FUNCTION_COUNT)
> +			return -EINVAL;
> +
> +		for (i = 0; i < num; i++) {
> +			if (!fdata[i].desc)
> +				return -EINVAL;
> +			data->function[i].type = fdata[i].desc->type;
> +			data->function[i].adr  = fdata[i].desc->adr;
> +			data->function[i].name = fdata[i].desc->name;
> +			data->function[i].node = NULL;

[Severity: High]
Does this structure copy miss the duplicate flag from the static descriptor?

In SDCA, the duplicate flag is necessary for ensuring ALSA controls for
duplicate functions (like Left and Right SmartAmps) are properly prefixed
with their address. Without this flag, won't the dynamically registered
functions have duplicate = false, leading to ALSA control name collisions?

[ ... ]

> @@ -185,6 +244,13 @@ static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_NS_GPL(sdca_class_probe, "SND_SOC_SDCA_CLASS");

[Severity: High]
Could exposing this probe function lead to a Use-After-Free if a caller
subsequently unbinds or fails probe?

The sdca_class_probe() function queues an asynchronous boot_work, but its
teardown (cancel_work_sync) is only present in the unexported
class_sdw_remove(). The work lifecycle doesn't appear to be managed by
devres.

If a codec driver uses this helper and later unbinds, devres will free the
drv structure while the work remains queued, leading to a dereference of the
freed drv pointer when the work eventually executes. Should the work
cancellation be tied to devres or exposed via an un-probe helper?

[ ... ]

> diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
> --- a/sound/soc/sdca/sdca_class_function.c
> +++ b/sound/soc/sdca/sdca_class_function.c
> @@ -329,9 +329,43 @@ static int class_function_probe(struct auxiliary_device *auxdev,
>  	drv->core = core;
>  	drv->function = &sdev->function;
>  
> -	ret = sdca_parse_function(dev, core->sdw, drv->function);
> -	if (ret)
> -		return ret;
> +	if (drv->function->desc->node) {
> +		ret = sdca_parse_function(dev, core->sdw, drv->function);
> +		if (ret)
> +			return ret;
> +	} else if (core->hw_ops && core->hw_ops->get_function_data) {
[ ... ]
> +		for (i = 0; i < num; i++) {
> +			if (fdata[i].desc &&
> +			    fdata[i].desc->type == sdev->function.desc->type) {
> +				*drv->function = fdata[i];
> +				drv->function->desc = sdev->function.desc;

[Severity: High]
Can this structure assignment corrupt the dynamically registered function
descriptor?

Earlier in this function, drv->function is set as an alias to &sdev->function:

    drv->function = &sdev->function;

When *drv->function = fdata[i] executes, it completely overwrites
sdev->function with the static template, which destroys the original desc
pointer. The next line, drv->function->desc = sdev->function.desc, acts as a
self-assignment and fails to restore the original dynamic descriptor.

Consequently, the dynamically injected SoundWire address is lost. If a device
has multiple functions of the same type (like dual SmartAmps), they will both
blindly use the same address from the first template. Could this cause
SoundWire control collisions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722234221.884765-1-srinivas.kandagatla@oss.qualcomm.com?part=3