Re: [PATCH v4 4/5] clk: cix: add sky1 audss clock controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Use-after-free race during device removal because composite clocks are unregistered manually before the devres-managed OF clock provider is unregistered.
- [High] Multiple independent clocks erroneously share the exact same hardware gate bits, causing Common Clock Framework (CCF) state corruption.
- [High] Calling `pm_runtime_force_suspend()` before `pm_runtime_disable()` in `remove()` creates a concurrency race condition.
- [Medium] Parent clock enables are permanently leaked in the probe error path and on device removal if `CONFIG_PM` is disabled.
- [Medium] Missing `pm_ptr()` wrapper for `dev_pm_ops` pointer causes dead code when `CONFIG_PM` is disabled.
- [Medium] Device register states are saved into a global static array during runtime suspend, causing an anti-pattern and potential data races.
--

commit ba255415d22d68eb57522fe8fe2cfadbf4ba75d6
Author: Joakim Zhang <joakim.zhang@cixtech.com>

clk: cix: add sky1 audss clock controller

Add a platform driver for the Cix Sky1 Audio Subsystem (AUDSS) internal
clock controller.

> diff --git a/drivers/clk/cix/clk-sky1-audss.c b/drivers/clk/cix/clk-sky1-audss.c
> new file mode 100644
> index 0000000000000..c1e7dfe45d12c
> --- /dev/null
> +++ b/drivers/clk/cix/clk-sky1-audss.c
> @@ -0,0 +1,1167 @@
[ ... ]
> +static u32 sky1_reg_save[][2] = {
> +	{ INFO_HIFI0,  0 },
> +	{ INFO_CLK_GATE,  0 },
> +	{ INFO_CLK_DIV, 0 },
> +	{ INFO_CLK_MUX, 0 },
> +	{ INFO_MCLK, 0 },
> +};

[Severity: Medium]
Is it safe to save device register states into a global static array during
runtime suspend? If multiple instances of this driver or SoC ever exist in a
system, wouldn't they overwrite each other's suspended states? Could this
state be allocated dynamically within the per-device priv structure instead?

[ ... ]
> +	/* wdg */
> +	CFG(CLK_WDG_APB,
> +	    "audss_wdg_apb",
> +	    wdg_apb_parent,
> +	    NULL,
> +	    -1, 0, 0, 0,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_GATE, 10, 0,
> +	    0),
> +	CFG(CLK_WDG_WDG,
> +	    "audss_wdg_wdg",
> +	    wdg_wdg_parent,
> +	    NULL,
> +	    -1, 0, 0, 0,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_GATE, 10, 0,
> +	    0),

[Severity: High]
Does defining two distinct clocks that map to the exact same gate bit
(shift 10) cause Common Clock Framework state corruption? Since CCF tracks
their states separately, if a driver disables CLK_WDG_APB, wouldn't it
silently turn off the physical clock for CLK_WDG_WDG as well? Should shared
gates be modeled as a single gate clock with multiple children?

> +	/* timer */
> +	CFG(CLK_TIMER_APB,
> +	    "audss_timer_apb",
> +	    timer_apb_parent,
> +	    NULL,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_DIV, 0, 2, 0,
> +	    INFO_CLK_GATE, 11, 0,
> +	    0),
> +	CFG(CLK_TIMER_TIMER,
> +	    "audss_timer_timer",
> +	    timer_timer_parent,
> +	    NULL,
> +	    -1, 0, 0, 0,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_GATE, 11, 0,
> +	    0),

[Severity: High]
Similarly, does having CLK_TIMER_APB and CLK_TIMER_TIMER share gate shift 11
cause the same CCF state tracking issues?

[ ... ]
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, priv->clk_data);
> +	if (ret) {
> +		dev_err(dev, "failed to add clock provider: %d\n", ret);
> +		goto fail_register;
> +	}
> +
> +	pm_runtime_put_sync(dev);
> +
> +	return 0;
> +
> +fail_register:

[Severity: High]
Does using devm_of_clk_add_hw_provider() here create a use-after-free race
condition during device removal?

Since devres defers unregistration until after the remove function completes,
but sky1_audss_clk_remove() manually destroys the composite clocks, will a
concurrent clk_get() return a dangling pointer while the OF provider is still
active?

> +fail_array_clk:
> +	while (i--)
> +		clk_hw_unregister_composite(clk_table[devtype_data->clk_cfg[i].id]);
> +fail_fixed_clk:
> +fail_clks_set:
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);

[Severity: Medium]
Will the parent clock enables be permanently leaked here if CONFIG_PM is
disabled?

The explicit sky1_audss_clks_enable() called earlier delegates teardown to
runtime PM wrappers like pm_runtime_put_sync(). If CONFIG_PM=n, these wrappers
are no-ops. Does the driver need explicit clock disablement when runtime PM
is unavailable?

> +err_link:
> +	device_link_remove(dev, dev->parent);
> +	return ret;
> +}
> +
> +static void sky1_audss_clk_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sky1_audss_clks_priv *priv = dev_get_drvdata(dev);
> +	const struct sky1_audss_clks_devtype_data *devtype_data = priv->devtype_data;
> +	int i = 0;
> +
> +	for (i = 0; i < devtype_data->clk_cfg_size; i++)
> +		clk_hw_unregister_composite(priv->clk_data->hws[devtype_data->clk_cfg[i].id]);
> +
> +	if (!pm_runtime_status_suspended(dev))
> +		pm_runtime_force_suspend(dev);
> +
> +	pm_runtime_disable(dev);

[Severity: High]
Does calling pm_runtime_force_suspend() before pm_runtime_disable() create a
concurrency race condition?

pm_runtime_force_suspend() temporarily re-enables runtime PM before returning,
which opens a window where a concurrent operation could trigger a runtime
resume. Would it be safer to call pm_runtime_disable() first to ensure the
device stays suspended?

[Severity: Medium]
Also, similar to the probe error path, does delegating hardware disablement to
pm_runtime_force_suspend() leak the parent clocks on removal if CONFIG_PM is
disabled?

> +	device_link_remove(dev, dev->parent);
> +}
> +
> +static int __maybe_unused sky1_audss_clk_runtime_suspend(struct device *dev)
> +{
> +	struct sky1_audss_clks_priv *priv = dev_get_drvdata(dev);
> +	const struct sky1_audss_clks_devtype_data *devtype_data = priv->devtype_data;
> +	unsigned long flags;
> +	int i;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	for (i = 0; i < devtype_data->reg_save_size; i++)
> +		regmap_read(priv->regmap_cru,
> +			    devtype_data->reg_save[i][0], &devtype_data->reg_save[i][1]);

[Severity: Medium]
Does writing to devtype_data->reg_save[i][1] mutate the statically allocated
global array sky1_reg_save from a per-device suspend handler? Could this lead
to data corruption if there are multiple devices, and shouldn't we write to
dynamically allocated per-device state instead?

[ ... ]
> +static struct platform_driver sky1_audss_clk_driver = {
> +	.probe = sky1_audss_clk_probe,
> +	.remove = sky1_audss_clk_remove,
> +	.driver = {
> +		.name = "sky1-audss-clk",
> +		.suppress_bind_attrs = true,
> +		.of_match_table = sky1_audss_clk_of_match,
> +		.pm = &sky1_audss_clk_pm_ops,

[Severity: Medium]
Will this unconditional assignment to .pm produce dead code when CONFIG_PM
is disabled? Should it be wrapped with pm_ptr(&sky1_audss_clk_pm_ops) to
allow the compiler to optimize out the unused structures?

> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617060437.1474816-1-joakim.zhang@cixtech.com?part=4