Re: [PATCH v5 2/4] 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] Clock operations access MMIO registers without waking the device from runtime suspend, causing fatal bus errors.
- [High] Error paths in probe and runtime resume leak the PM usage counter, leave hardware out of reset, and leak clocks if CONFIG_PM is disabled.
- [High] Mixing devres clock provider registration with manual composite clock cleanup creates a Use-After-Free window.
- [High] Instance-specific register state is saved into a global static array during runtime suspend, causing state corruption.
- [High] Unbalanced `pm_runtime_disable` depth in `sky1_audss_clk_remove` causes driver rebind failures.
- [Low] Unconditional assignment of `dev_pm_ops` structure pointer without the `pm_ptr()` wrapper.
--

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

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.

Four SoC-level audio reference clocks are enabled as inputs to the
internal clock tree. The driver releases the AUDSS NOC reset, enables
runtime PM and instantiates the auxiliary reset device.

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

[ ... ]
> +static int sky1_audss_clk_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct clk_mux *mux = to_clk_mux(hw);
> +	u32 val = clk_mux_index_to_val(mux->table, mux->flags, index);
> +	struct sky1_clk_mux *sky1_mux = to_sky1_clk_mux(mux);
> +	unsigned long flags = 0;
> +	u32 reg;
> +
> +	if (mux->lock)
> +		spin_lock_irqsave(mux->lock, flags);
> +	else
> +		__acquire(mux->lock);
> +
> +	if (mux->flags & CLK_MUX_HIWORD_MASK) {
> +		reg = mux->mask << (mux->shift + 16);
> +	} else {
> +		regmap_read(sky1_mux->regmap, sky1_mux->offset, &reg);

[Severity: High]
Is it safe to access the MMIO registers directly in the CCF clock operations
without first requesting a PM runtime wake?

The probe function puts the device into runtime suspend at the end of
execution. Since the Common Clock Framework does not automatically wake
provider devices for operations like set_parent or recalc_rate, could
accessing the registers of a powered-off hardware block trigger a
synchronous external abort?

[ ... ]
> +static int sky1_audss_clk_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	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;
> +	}
> +
> +	ret = sky1_audss_reset_controller_register(dev);
> +	if (ret)
> +		goto fail_register;
> +
> +	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: High]
Do these error paths properly clean up resources?

If initialization fails and jumps to err_pm, it skips calling
pm_runtime_put_noidle() and reset_control_assert(), which appears to
permanently leak the PM usage counter and leave the hardware out of reset.

Additionally, if it fails and branches to fail_clks_set, it relies on
pm_runtime_put_sync(). If CONFIG_PM is disabled, will this leave the
clocks permanently enabled?

[ ... ]
> +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 mixing devm_of_clk_add_hw_provider() with manual unregistration
create a use-after-free window?

Since devres executes after sky1_audss_clk_remove() returns, the OF clock
provider will remain active while the composite clock memory has already
been freed. Could a concurrent clk_get() lookup during this window return
a pointer to freed clk_hw memory?

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

[Severity: High]
Could this sequence cause driver rebind failures?

The pm_runtime_force_suspend() function internally increments the disable
depth. Calling pm_runtime_disable() immediately after increments it a
second time. Since probe() only enables PM once, would unbinding and
rebinding this driver leave the device with a disable_depth of 1,
permanently preventing runtime PM from enabling?

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

[Severity: High]
Could this cause data corruption if multiple instances of this device exist?

The sky1_reg_save variable is a global static array, but the register state
is being written directly into it during runtime suspend. If there are dual
audio subsystems, will one instance overwrite the other's saved clock
configurations?

[ ... ]
> +static int __maybe_unused sky1_audss_clk_runtime_resume(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, ret;
> +
> +	ret = reset_control_deassert(priv->rst_noc);
> +	if (ret)
> +		return ret;
> +
> +	ret = sky1_audss_clks_enable(priv);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks\n");
> +		return ret;
> +	}

[Severity: High]
Does the runtime resume error path miss asserting the reset control?

If sky1_audss_clks_enable() fails, the function returns without calling
reset_control_assert(), leaving the hardware out of reset while the system
considers it suspended.

[ ... ]
> +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]
Is it necessary to wrap the dev_pm_ops pointer assignment with pm_ptr()?

Without the wrapper, will the structure and its referenced callbacks be
compiled into the kernel as dead code when CONFIG_PM is disabled?

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