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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
-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.
[ ... ]
quoted hunk ↗ jump to hunk
+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