Re: [PATCH v2 02/11] ASoC: SDCA: export PM helpers keyed on sdca_class_drv

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Exporting `sdca_class_pm_ops` unconditionally with `RUNTIME_PM_OPS` causes dead code retention when CONFIG_PM is disabled.

Pre-existing issues:
- [High] Data race and invalid hardware access between `boot_work` and system suspend.
--

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

ASoC: SDCA: export PM helpers keyed on sdca_class_drv

The class PM callbacks pull sdca_class_drv out of drvdata, so the
built-in class_sdw_driver owns the drvdata slot. That works for the
generic case but blocks codec drivers that want to embed sdca_class_drv
in their own private struct. Split the four callbacks into exported
helpers that take a struct sdca_class_drv * directly.

> diff --git a/sound/soc/sdca/sdca_class.c b/sound/soc/sdca/sdca_class.c
> index d7444f442c71c..88a29116a334e 100644
> --- a/sound/soc/sdca/sdca_class.c
> +++ b/sound/soc/sdca/sdca_class.c
[ ... ]
> -static int class_suspend(struct device *dev)
> +/**
> + * sdca_class_system_suspend - SDCA class system suspend helper
> + * @drv: caller-owned sdca_class_drv.
> + *
> + * Codec drivers compose this into their own dev_pm_ops.  Disables the
> + * SoundWire interrupt and forces runtime suspend of the underlying
> + * class regmap.
> + */
> +int sdca_class_system_suspend(struct sdca_class_drv *drv)
>  {
> -	struct sdca_class_drv *drv = dev_get_drvdata(dev);
>  	int ret;
>  
>  	disable_irq(drv->sdw->irq);
>  
> -	ret = pm_runtime_force_suspend(dev);
> +	ret = pm_runtime_force_suspend(drv->dev);
>  	if (ret) {
> -		dev_err(dev, "failed to force suspend: %d\n", ret);
> +		dev_err(drv->dev, "failed to force suspend: %d\n", ret);
>  		return ret;
>  	}
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_NS_GPL(sdca_class_system_suspend, "SND_SOC_SDCA_CLASS");

[Severity: High]
This is a pre-existing issue, but does sdca_class_system_suspend() fail to
serialize against boot_work?

Because boot_work is queued on system_long_wq (an unbounded, non-freezable
workqueue), it can race with system suspend. If the system initiates suspend
while boot_work is waiting in sdw_slave_wait_for_init(),
sdca_class_system_suspend() will force the device into a suspended state by
disabling interrupts and setting regcache_cache_only(true).

Since sdca_class_system_suspend() does not flush or cancel the pending
initialization work, boot_work will eventually resume, blindly set
regcache_cache_only(false), and attempt hardware access on a physically
suspended bus, which could lead to bus timeouts or crashes.

[ ... ]
> +static int class_pm_runtime_resume(struct device *dev)
> +{
> +	return sdca_class_runtime_resume(dev_get_drvdata(dev));
> +}
>  
> -static const struct dev_pm_ops class_pm_ops = {
> -	SYSTEM_SLEEP_PM_OPS(class_suspend, class_resume)
> -	RUNTIME_PM_OPS(class_runtime_suspend, class_runtime_resume, NULL)
> +const struct dev_pm_ops sdca_class_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(class_pm_system_suspend, class_pm_system_resume)
> +	RUNTIME_PM_OPS(class_pm_runtime_suspend, class_pm_runtime_resume, NULL)
>  };
> +EXPORT_SYMBOL_NS_GPL(sdca_class_pm_ops, "SND_SOC_SDCA_CLASS");

[Severity: Low]
Does exporting sdca_class_pm_ops unconditionally with RUNTIME_PM_OPS cause
dead code retention when CONFIG_PM is disabled?

When CONFIG_PM is disabled, runtime PM callbacks are never used and should be
eliminated by the compiler. Because sdca_class_pm_ops is globally exported in
this patch and populated using the unconditional RUNTIME_PM_OPS macro instead
of conditionally-compiled macros, the compiler will retain the structure and
its referenced static functions (class_pm_runtime_suspend and
class_pm_runtime_resume) as dead code in the final binary.

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