Re: [PATCH v7 2/4] clk: cix: add sky1 audss clock controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Manual unregistration of composite clocks creates a Use-After-Free window for the OF clock provider.
- [High] The driver saves per-device PM runtime state into a statically allocated global shared array.
- [High] Missing hardware state rollback in PM runtime suspend callbacks upon failure.
- [High] Clock operations access MMIO registers without ensuring the device is runtime resumed, leading to bus faults.
- [High] The reset controller auxiliary device remains accessible after the hardware is powered down in `remove()`.
- [High] Multiple distinct clocks are mapped to the same physical gate bit, leading to unbalanced hardware state.
- [Medium] Unbalanced PM runtime usage counter and inconsistent reset state on probe error paths.
- [Medium] Race condition in device removal due to incorrect PM runtime teardown order.
- [Low] The dev_pm_ops structure is unconditionally assigned without the pm_ptr() wrapper.
--

commit 1f90bbe464605b65e1fc0b224f338cd666c9fd20
Author: Joakim Zhang <joakim.zhang@cixtech.com>
Subject: clk: cix: add sky1 audss clock controller

Add a platform driver for the Cix Sky1 AUDSS CRU. The driver maps
the CRU registers and registers mux, divider and gate clocks for
DSP, SRAM, HDA, DMAC, I2S, mailbox, watchdog and timer blocks.

> diff --git a/drivers/clk/cix/clk-sky1-audss.c b/drivers/clk/cix/clk-sky1-audss.c
> new file mode 100644
> index 0000000000000..fbc0ec9c47e55
> --- /dev/null
> +++ b/drivers/clk/cix/clk-sky1-audss.c
[ ... ]
> +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: High]
Does this global array cause data corruption if multiple driver instances
suspend or resume concurrently? It looks like instance-specific state might be
better placed dynamically inside the private struct instead of shared globally.

[ ... ]
> +static const struct composite_clk_cfg sky1_audss_clks[] = {
[ ... ]
> +	/* hda */
> +	CFG(CLK_HDA_SYS,
> +	    "audss_hda_sys",
> +	    hda_sys_parent,
> +	    NULL,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_DIV, 0, 2, 0,
> +	    INFO_CLK_GATE, 14, 0,
> +	    0),
> +	CFG(CLK_HDA_HDA,
> +	    "audss_hda_hda",
> +	    hda_hda_parent,
> +	    NULL,
> +	    -1, 0, 0, 0,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_GATE, 14, 0,
> +	    0),

[Severity: High]
Can assigning multiple distinct clocks to the same physical gate bit lead to
unbalanced hardware state?

If a consumer disables CLK_HDA_SYS, does the driver clear bit 14,
unintentionally turning off the clock for CLK_HDA_HDA? If the HDA consumer
accesses its hardware afterward, could it hit an unclocked peripheral?

[ ... ]
> +static const struct regmap_config sky1_audss_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};

[Severity: High]
Is it possible to trigger a bus fault here? If a consumer modifies a clock,
the framework invokes atomic clock callbacks (like sky1_audss_clk_gate_endisable)
which run under a spinlock and cannot wake up the device synchronously.
Because the regmap config omits pm_runtime integration, could the MMIO writes
be issued to unclocked hardware if the CRU is suspended?

[ ... ]
> +static int sky1_audss_clk_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = sky1_audss_clks_enable(priv);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks\n");
> +		goto err_pm;
> +	}
[ ... ]
> +	ret = sky1_audss_reset_controller_register(dev);
> +	if (ret)
> +		goto fail_register;

[Severity: High]
Is there a use-after-free window if a consumer accesses the reset controller
after the driver is unbound?

sky1_audss_reset_controller_register() uses devm_auxiliary_device_create(),
which means the auxiliary device is unregistered after sky1_audss_clk_remove()
completes. In sky1_audss_clk_remove(), the hardware is forcefully suspended
and powered down. Does this mean any consumer asserting a reset during this
window accesses powered-down registers?

> +
> +	pm_runtime_put_sync(dev);
> +
> +	return 0;
> +
> +fail_register:
> +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);
> +err_pm:
> +	pm_runtime_disable(dev);
> +	return ret;

[Severity: Medium]
Should the probe error path balance the pm_runtime usage counter and hardware
state? The pm_runtime_disable() call doesn't drop the usage counter incremented
by pm_runtime_get_noresume() earlier, potentially leaving a permanent leak.
Also, is it necessary to re-assert the reset line here?

> +}
> +
> +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]);

[Severity: High]
Does this manual unregistration create a use-after-free window for the OF
clock provider?

The provider is registered using devm_of_clk_add_hw_provider(), so it remains
active until after remove() completes. Meanwhile, clk_hw_unregister_composite()
immediately frees the clock structures, leaving dangling pointers if a consumer
looks up the provider during this window.

> +
> +	if (!pm_runtime_status_suspended(dev))
> +		pm_runtime_force_suspend(dev);
> +
> +	pm_runtime_disable(dev);

[Severity: Medium]
Could this teardown order cause a race condition? Since pm_runtime is still
enabled when pm_runtime_force_suspend() is called, could a concurrent runtime
resume request happen immediately after, putting the device back into an active
state before pm_runtime_disable() runs?

> +}
> +
> +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]);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	sky1_audss_clks_disable(priv);
> +
> +	return reset_control_assert(priv->rst_noc);

[Severity: High]
If reset_control_assert() fails, does this leave the hardware in a broken
state? Returning an error to the PM core leaves the device marked as
RPM_ACTIVE, but the clocks were already disabled by sky1_audss_clks_disable().
Could subsequent accesses to the device registers trigger a bus fault?

> +}
[ ... ]
> +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: Low]
Does this unconditional assignment cause dead code when CONFIG_PM is disabled?
Should this be wrapped with pm_ptr(&sky1_audss_clk_pm_ops)?

> +	},
> +};

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