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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Reading registers while the device is still in reset during the probe error path causes a system bus hang.
- [High] Mismatch between manual unregistration of clocks and devm-managed OF provider registration creates a use-after-free window during device removal.
- [High] Seventeen distinct clocks share the exact same physical divider configuration bits, breaking clock topology.
- [Medium] Parent clocks enabled during probe are permanently leaked on error paths and during device removal when `CONFIG_PM` is disabled.
- [Medium] The driver modifies a global static array to save instance-specific register states during runtime suspend.
- [Medium] Manual Read-Modify-Write (RMW) operations on a shared syscon regmap bypass atomicity guarantees.
- [Medium] Unchecked `regmap_read` calls in clock operations lead to uninitialized stack memory being written to hardware registers.
- [Low] Unconditional assignment of `dev_pm_ops` to the platform driver creates dead code when PM is disabled.
--

commit 0916ea34519831a84e69fe5dc0d82f878c51aef8
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. The driver binds to a cix,sky1-audss-clock device tree
node under the AUDSS syscon, obtains the parent regmap via
syscon_node_to_regmap(), and registers mux/divider/gate composite clocks
for DSP, SRAM, HDA, DMAC, watchdog, timer, mailbox and I2S outputs. Four
SoC-level audio reference clocks are brought up as inputs to the tree.

> diff --git a/drivers/clk/cix/clk-sky1-audss.c b/drivers/clk/cix/clk-sky1-audss.c
> new file mode 100644
> index 0000000000000..3ec6bea93b73c
> --- /dev/null
> +++ b/drivers/clk/cix/clk-sky1-audss.c
> @@ -0,0 +1,1175 @@
[ ... ]
> +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]
Since devtype_data->reg_save points to this statically defined sky1_reg_save
array, won't multiple instances of this device concurrently overwrite each
other's saved register states during runtime suspend?

Should the register save array be allocated per-instance in
sky1_audss_clks_priv instead?

[ ... ]
> +static const struct composite_clk_cfg sky1_audss_clks[] = {
> +	/* dsp */
> +	CFG(CLK_DSP_CLK,
> +	    "audss_dsp_clk",
> +	    dsp_clk_parent,
> +	    NULL,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_DIV, 0, 2, 0,
> +	    INFO_HIFI0, 0, 0,
> +	    0),

[Severity: High]
Seventeen distinct clocks (from CLK_DSP_CLK through CLK_I2S9_APB) are defined
with the exact same divider offset (INFO_CLK_DIV), shift (0), and width (2).

Won't changing the rate of one clock silently overwrite the physical divider
for the other 16 clocks without updating their cached rates in the clock core?

[ ... ]
> +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);
> +		reg &= ~(mux->mask << mux->shift);
> +	}
> +	val = val << mux->shift;
> +	reg |= val;
> +	regmap_write(sky1_mux->regmap, sky1_mux->offset, reg);

[Severity: Medium]
Since the regmap is obtained via syscon_node_to_regmap(), it may be shared
with other drivers.

Does relying on the local priv->lock to protect discrete regmap_read() and
regmap_write() calls leave a race condition against other syscon users?

Should regmap_update_bits() be used instead to ensure atomicity?

Also, if the regmap_read() call fails, reg remains uninitialized. Does this
result in writing raw stack memory back to the hardware register via the
subsequent regmap_write()?

[ ... ]
> +static void sky1_audss_clk_gate_endisable(struct clk_hw *hw, int enable)
> +{
> +	struct clk_gate *gate = to_clk_gate(hw);
> +	struct sky1_clk_gate *sky1_gate = to_sky1_clk_gate(gate);
> +	int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
> +	unsigned long flags = 0;
> +	u32 reg;
> +
> +	set ^= enable;
> +
> +	if (gate->lock)
> +		spin_lock_irqsave(gate->lock, flags);
> +	else
> +		__acquire(gate->lock);
> +
> +	if (gate->flags & CLK_GATE_HIWORD_MASK) {
> +		reg = BIT(gate->bit_idx + 16);
> +		if (set)
> +			reg |= BIT(gate->bit_idx);
> +	} else {
> +		regmap_read(sky1_gate->regmap, sky1_gate->offset, &reg);
> +
> +		if (set)
> +			reg |= BIT(gate->bit_idx);
> +		else
> +			reg &= ~BIT(gate->bit_idx);
> +	}
> +
> +	regmap_write(sky1_gate->regmap, sky1_gate->offset, reg);

[Severity: Medium]
If the regmap_read() call fails here, reg remains uninitialized.

Could this write uninitialized stack memory back to the hardware register?

[ ... ]
> +static int sky1_audss_clk_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = sky1_audss_clks_set_rate(priv);
> +	if (ret) {
> +		dev_err(dev, "failed to set clocks rate\n");
> +		goto fail_clks_set;
> +	}
> +
> +	/* assert reset */
> +	reset_control_assert(rst_noc);

[Severity: High]
If sky1_audss_clks_set_rate() fails, the code jumps to fail_clks_set and
calls pm_runtime_put_sync(), which invokes sky1_audss_clk_runtime_suspend().

Since reset_control_deassert(rst_noc) has not yet been executed, doesn't this
attempt to read registers from a subsystem still held in reset, potentially
causing a system bus hang?

[ ... ]
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, priv->clk_data);

[Severity: High]
The OF clock provider is registered using devm_of_clk_add_hw_provider(),
meaning its cleanup happens during the devres phase, after
sky1_audss_clk_remove() returns.

By manually freeing the composite clocks in sky1_audss_clk_remove() with
clk_hw_unregister_composite(), is there a window where the OF provider is
still globally active but the internal clk_hw pointers have already been
freed, leading to a use-after-free if a consumer probes during this time?

> +	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:
> +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]
If the kernel is built with CONFIG_PM=n, these runtime PM calls become
no-ops, which means the runtime suspend callback is never invoked.

Does this permanently leak the parent clocks explicitly enabled earlier in
probe?

Should the clocks be manually disabled in the error path and remove function?

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

[Severity: Medium]
If CONFIG_PM is disabled, pm_runtime_force_suspend() is a no-op.

Could this leave parent clocks enabled during device removal?

> +
> +	pm_runtime_disable(dev);
> +}
> +
> +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: High]
This register read can happen when the device is still in reset if the
probe function fails at sky1_audss_clks_set_rate().

Can this trigger a system bus hang?

[Severity: Medium]
This also writes to devtype_data->reg_save[i][1] which points to the global
sky1_reg_save array.

Does this corrupt the saved state if multiple instances of this driver are
active?

[ ... ]
> +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]
Should this be wrapped with pm_ptr() to avoid leaving dead code when CONFIG_PM
is disabled?

> +	},
> +};

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