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] Device-specific suspend/resume register state is stored in a globally shared static array, breaking multi-instance support and causing state corruption.
- [High] Unchecked regmap_read() results in usage of uninitialized stack variables.
- [High] Clock operations access hardware registers without PM runtime protection, potentially causing bus faults or lost register updates.
- [High] Race condition during device removal leading to Use-After-Free of composite clock structures due to LIFO inversion.
- [High] A massive copy-paste error causes nearly all clocks in the system to write to the exact same divider register bits, leading to silently corrupted hardware states.
- [High] Non-atomic read-modify-write sequences are performed on a shared syscon regmap, leading to race conditions.
- [Medium] Parent clocks enabled during probe are leaked on device removal and probe error paths when CONFIG_PM is disabled.
- [Low] The `dev_pm_ops` structure pointer is assigned unconditionally in the driver, producing dead code when CONFIG_PM is disabled.
--

commit f6414eab975c93e0178027203d04f1647fb5efdb
Author: Joakim Zhang <joakim.zhang@cixtech.com>
Date:   Wed Jun 10 06:17:11 2026 +0000

    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
[ ... ]
> +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]
Will this global static array cause state corruption if there are multiple
instances of the device?

It looks like multiple instances would concurrently overwrite each other's
saved register context during suspend/resume. Should this save buffer be
allocated per-instance inside struct sky1_audss_clks_priv?

[ ... ]
> +	CFG(CLK_I2S0_APB,
> +	    "audss_i2s0_apb",
> +	    i2s_apb_parent,
> +	    NULL,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_DIV, 0, 2, 0,

[Severity: High]
Does this configuration accidentally assign the same register and bit shift
to multiple clocks?

Looking at the array, clocks like CLK_I2S0_APB through CLK_I2S9_APB, as
well as CLK_DSP_CLK, CLK_SRAM_AXI, and others, all specify
INFO_CLK_DIV, 0, 2, 0 for their divider. 

Will updating the rate for any of these clocks silently overwrite the
hardware divider for all the others?

[ ... ]
> +static u8 sky1_audss_clk_mux_get_parent(struct clk_hw *hw)
> +{
> +	struct clk_mux *mux = to_clk_mux(hw);
> +	struct sky1_clk_mux *sky1_mux = to_sky1_clk_mux(mux);
> +	u32 val;
> +
> +	regmap_read(sky1_mux->regmap, sky1_mux->offset, &val);
> +	val = val >> mux->shift;

[Severity: High]
If regmap_read() fails, could this use an uninitialized stack variable?

The return value of regmap_read() is ignored, meaning val could remain
uninitialized when shifted and returned. A similar pattern appears in
sky1_audss_clk_divider_recalc_rate().

[ ... ]
> +static int sky1_audss_clk_mux_set_parent(struct clk_hw *hw, u8 index)
> +{
[ ... ]
> +	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: High]
Is it safe to access hardware registers here without ensuring the device is
active via PM runtime functions?

If the clock callbacks are invoked while the subsystem is suspended, this
might access powered-down MMIO or the changes might be immediately
overwritten when sky1_audss_clk_runtime_resume() restores the old state.

[Severity: High]
Can this sequence lose concurrent register updates?

Since the regmap is shared via syscon_node_to_regmap(), the private lock
doesn't serialize these accesses against other drivers modifying different
bits in the same syscon.

Would regmap_update_bits() be safer here to atomically perform the update?
A similar pattern appears in sky1_audss_clk_divider_set_rate() and the gate
callbacks.

[ ... ]
> +fail_clks_set:
> +	pm_runtime_put_sync(dev);
> +	pm_runtime_disable(dev);
> +	return ret;
> +}

[Severity: Medium]
Will the parent clocks leak their prepare/enable references if CONFIG_PM is
disabled?

The driver relies on pm_runtime_put_sync() to trigger the suspend callback
to disable the clocks. If runtime PM is stubbed out, the clocks enabled
during probe might never be disabled on error paths or during device
removal.

[ ... ]
> +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 create a use-after-free window?

The OF clock provider is registered using devm_of_clk_add_hw_provider()
which will run its cleanup after remove() finishes. If we manually
unregister and free the composite clocks here, consumers might look up
a freed clock pointer via the device tree during that window.

[ ... ]
> +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 intended to leave the dev_pm_ops pointer assigned unconditionally?

If CONFIG_PM is disabled, this prevents the compiler from discarding the
sky1_audss_clk_pm_ops structure and its referenced callbacks as dead code.
Could we use the pm_ptr() wrapper here?

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