Re: [PATCH v4 4/8] ASoC: SDCA: add class_ops with populate_function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] soundwire: type confusion via dev_get_drvdata() in class_function_probe()
- [High] soundwire: infinite spin loop in regmap_read_poll_timeout() due to 0 delay
- [Medium] soundwire: DAPM widgets silently omitted due to missing Entity 0 padding
--

commit fda476b8426451c92a8bb60b96f3c646d1aed378
Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

ASoC: SDCA: add class_ops with populate_function

This commit introduces a populate_function callback in struct sdca_class_ops.
It allows codec drivers to supply SDCA topology data from static tables
instead of relying on sdca_parse_function(), supporting non-DisCo platforms.

> diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
> index 0e6dead414959..79fdb672be1d7 100644
> --- a/sound/soc/sdca/sdca_class.c
> +++ b/sound/soc/sdca/sdca_class.c
[ ... ]
> @@ -144,6 +144,8 @@ static void class_boot_work(struct work_struct *work)
>   *       allocation and sets its own dev_set_drvdata() -- the framework
>   *       does not touch drvdata.  Typically embedded in the codec's own
>   *       priv struct so codec drivers can keep per-slave state.
> + * @ops: optional codec-provided class callbacks (may be NULL for
> + *       pure-generic SDCA parts that need no quirks)
>   *
>   * Codec-specific SoundWire drivers call this from their .probe after
>   * allocating a struct sdca_class_drv (usually embedded in their own
[ ... ]
> diff --git a/sound/soc/sdca/sdca_class_function.c b/sound/soc/sdca/sdca_class_function.c
> index 411e3d717bb6b..d83be8474e7b8 100644
> --- a/sound/soc/sdca/sdca_class_function.c
> +++ b/sound/soc/sdca/sdca_class_function.c
> @@ -329,7 +329,14 @@ static int class_function_probe(struct auxiliary_device *auxdev,
>  	drv->core = core;
>  	drv->function = &sdev->function;
>  
> -	ret = sdca_parse_function(dev, drv->function);
> +	if (core->ops && core->ops->populate_function) {

[Severity: High]
Does dereferencing core->ops here cause type confusion and a potential
kernel panic?

The framework documentation for sdca_class_probe() explicitly allows
codec drivers to own and set their own drvdata:

 * @drv: caller-allocated sdca_class_drv storage.  The caller (a codec
 *       driver, or the built-in class_sdw_driver in this file) owns the
 *       allocation and sets its own dev_set_drvdata()

However, class_function_probe() retrieves the parent SoundWire device's
drvdata and unconditionally treats it as a struct sdca_class_drv:

    struct sdca_class_drv *core = dev_get_drvdata(dev->parent);

If a codec driver embeds struct sdca_class_drv anywhere but the first
position of its private data, won't dereferencing core->ops read
arbitrary memory?

> +		ret = core->ops->populate_function(core->sdw, drv->function);

[Severity: High]
Can bypassing sdca_parse_function() here lead to an infinite spin loop
during hardware reset?

When populate_function() is used, it skips the default initialization
of reset_max_delay to 100ms that happens in sdca_parse_function(). If a
codec driver forgets to explicitly initialize this parameter, it defaults
to 0.

Later, sdca_reset_function() passes this 0 value to
regmap_read_poll_timeout():

    poll_us = umin(function->reset_max_delay >> 4, 1000);
    ret = regmap_read_poll_timeout(regmap, reg, val, !val, poll_us,
                                   function->reset_max_delay);

Because the internal timeout check requires a non-zero timeout to break,
won't this cause an unbounded spin loop if the hardware condition is not
met?

[Severity: Medium]
Does supplying a raw entity array via populate_function() cause DAPM
widgets and routes to be silently omitted?

The core framework topological helpers explicitly skip the last element
of the entity array:

    for (i = 0; i < function->num_entities - 1; i++) {

This was safe when sdca_parse_function() artificially padded the array
with a dummy Entity 0. However, if a codec driver provides a raw entity
array without appending the dummy Entity 0 at the very end, won't the
framework silently skip the final legitimate audio entity?

> +	} else if (drv->function->desc->node) {
> +		ret = sdca_parse_function(dev, drv->function);
> +	} else {
> +		dev_err(dev, "no firmware node and no populate_function hook\n");
> +		return -ENOENT;
> +	}
>  	if (ret)
>  		return ret;

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