Re: [PATCH v2 03/11] ASoC: SDCA: expose class SoundWire probe/remove/read_prop as library

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] sdca_class_probe() queues an async work item but does not tie its cancellation to devres, creating a potential use-after-free hazard for caller drivers on probe failure.

Pre-existing issues:
- [High] The PM runtime usage counter is permanently leaked if devm_pm_runtime_enable() fails during probe.
- [High] The PM runtime usage counter is permanently leaked if the deferred boot work is cancelled before execution.
- [High] The return value of sdw_slave_read_prop() is ignored, masking potential memory allocation failures and leaving properties uninitialized.
- [High] If the runtime force suspend fails during a system suspend transition, the device's interrupt is left permanently disabled.
--

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

ASoC: SDCA: expose class SoundWire probe/remove/read_prop as library

Split the internal class_sdw_probe/class_sdw_remove/class_read_prop
functions into caller-friendly library helpers. The class_sdw_probe and
class_sdw_remove callbacks of the built-in class_sdw_driver are now thin
wrappers that defer to the exported helpers.

> diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
> index 88a29116a334e..b952fa6eb802f 100644
> --- 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)
>  {
>  	struct sdw_slave_prop *prop = &sdw->prop;
>  
> @@ -36,9 +44,10 @@ static int class_read_prop(struct sdw_slave *sdw)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_NS_GPL(sdca_class_read_prop, "SND_SOC_SDCA_CLASS");

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
sdw_slave_read_prop() here?

If sdw_slave_read_prop() returns -ENOMEM upon memory exhaustion, ignoring it
might cause the SoundWire core to unknowingly operate on uninitialized
properties (e.g., null pointers for src_dpn_prop), leading to null pointer
dereferences during enumeration.

[ ... ]
> @@ -185,14 +205,47 @@ 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: Medium]
Does sdca_class_probe() create a use-after-free hazard for caller drivers on
probe failure?

If a caller allocates sdca_class_drv via devm, calls sdca_class_probe(), and
encounters an error before its own probe finishes, the async boot_work is left
queued:

    queue_work(system_long_wq, &drv->boot_work);
    return 0;

If the caller unwinds its devres allocations without manually cancelling the
work, the background class_boot_work() could dereference the freed drv pointer.
Should a devm action be added to guarantee the cancellation of boot_work?

[Severity: High]
This is a pre-existing issue, but does sdca_class_probe() leak the PM runtime
usage counter if devm_pm_runtime_enable() fails?

In sdca_class_probe(), pm_runtime_get_noresume(dev) increments the usage
counter. If devm_pm_runtime_enable(dev) fails immediately after, the function
returns directly without calling pm_runtime_put_noidle() to decrement the
usage counter:

    pm_runtime_get_noresume(dev);

    ret = devm_pm_runtime_enable(dev);
    if (ret)
        return ret;

Could this permanently prevent the device from entering runtime suspend?

>  
> -static void class_sdw_remove(struct sdw_slave *sdw)
> +static int class_sdw_probe(struct sdw_slave *sdw, const struct sdw_device_id *id)
> +{
[ ... ]
+
+/**
+ * sdca_class_remove - SDCA class SoundWire slave remove helper
+ * @drv: caller-owned sdca_class_drv (the one handed to sdca_class_probe()).
+ *
+ * Cancels the deferred boot work so devres can safely free @drv and the
+ * embedding codec priv without racing class_boot_work.  Codec-specific
+ * SoundWire drivers that call sdca_class_probe() must call this from
+ * their .remove with the same drv pointer they passed to probe.
+ */
+void sdca_class_remove(struct sdca_class_drv *drv)
+{
>  	cancel_work_sync(&drv->boot_work);
>  }
> +EXPORT_SYMBOL_NS_GPL(sdca_class_remove, "SND_SOC_SDCA_CLASS");

[Severity: High]
This is a pre-existing issue, but does this leak the PM runtime usage counter
if the deferred boot work is cancelled before execution?

sdca_class_probe() increments the PM usage count via pm_runtime_get_noresume().
The decrement exclusively occurs inside the deferred class_boot_work(). If the
device is removed and cancel_work_sync() successfully prevents the work from
running, the return value of cancel_work_sync() is ignored and the decrement
never occurs.

> +
> +static void class_sdw_remove(struct sdw_slave *sdw)
> +{
> +	struct sdca_class_drv *drv = dev_get_drvdata(&sdw->dev);
> +
> +	sdca_class_remove(drv);
> +}
>  
>  /**
>   * sdca_class_system_suspend - SDCA class system suspend helper

[Severity: High]
This is a pre-existing issue, but does sdca_class_system_suspend() leave the
device's interrupt permanently disabled if runtime force suspend fails?

In sdca_class_system_suspend():

    disable_irq(drv->sdw->irq);

    ret = pm_runtime_force_suspend(drv->dev);
    if (ret) {
        dev_err(drv->dev, "failed to force suspend: %d\n", ret);
        return ret;
    }

If pm_runtime_force_suspend() fails, the error is returned immediately and the
system suspend transition is aborted. However, enable_irq(drv->sdw->irq) is not
called in this error path, which might render the device unresponsive to
interrupts.

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