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

[Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>]

On 9/18/26 3:08 PM, sashiko-bot@kernel.org wrote:
> 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()
This is pre-existing issue, none of which is introduced by this patch.
I will try to send a separate fix for this. But this is not something
this patch introduced.

> - [High] soundwire: infinite spin loop in regmap_read_poll_timeout() due to 0 delay
THis is set to 25ms for tambora so it will not hit, but we should patch
the core to avoid this .

> - [Medium] soundwire: DAPM widgets silently omitted due to missing Entity 0 padding
Last entity in tambora codec is Entity 0, so its not an issue with this
codec.

> --
> 
> 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;
>