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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The error handling paths in `sky1_audss_clk_probe` double-decrement the PM runtime usage counter.
- [High] Device-specific suspend and resume state is incorrectly saved into a mutable global static array.
- [High] Clock operations (`.set_rate` and `.set_parent`) access hardware registers without ensuring the provider device is active, and their writes are blindly clobbered on resume.
- [High] Hardware is suspended and PM runtime disabled before `devm`-managed child devices and interfaces are unregistered.
- [Medium] Probe error paths improperly bypass reset assertion and rely solely on runtime PM for cleanup, causing resource leaks.
- [Medium] Unchecked `regmap_read` return values lead to the use of uninitialized stack variables.
- [Medium] During runtime resume, clock gates are ungated before clock dividers and muxes are configured.
--

--- Patch [2]: [PATCH v9 2/4] clk: cix: add sky1 audss clock controller ---
commit 3d18b0af56a4158655e0330ed09981d99ba1869a
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
> --- /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 data corruption if the system
instantiates multiple instances of the audio subsystem clock controller?

If sky1_audss_clk_runtime_suspend() reads hardware registers and writes
their values directly into this shared global array, one instance could
overwrite the saved PM context of another. Should register context arrays
be dynamically allocated within the per-instance sky1_audss_clks_priv
structure instead?

[Severity: Medium]
Could restoring the gate register before the mux and divider registers
cause out-of-spec clock glitches?

Since sky1_audss_clk_runtime_resume() restores registers strictly following
the sequence in this array, active clocks might be ungated while their
corresponding muxes and dividers are still at their post-reset default
states. Should MUX and DIV configuration registers be restored before
GATE output controls?

[ ... ]

> +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;
> +	val &= mux->mask;

[Severity: Medium]
Is it possible for val to remain uninitialized if regmap_read() fails?

If the underlying MMIO read fails, the uninitialized stack variable is
immediately used in bitwise shifts to index the parent clock array, which
might lead to unpredictable clock framework behavior or out-of-bounds array
access. Should the return value of regmap_read() be checked?

[ ... ]

> +static int sky1_audss_clk_divider_set_rate(struct clk_hw *hw,
> +					   unsigned long rate,
> +					   unsigned long parent_rate)
> +{
> +	struct clk_divider *divider = to_clk_divider(hw);
> +	struct sky1_clk_divider *sky1_div = to_sky1_clk_divider(divider);
> +	int value;
> +	unsigned long flags = 0;
> +	u32 val;
> +
> +	value = divider_get_val(rate, parent_rate, divider->table,
> +				divider->width, divider->flags);
> +	if (value < 0)
> +		return value;
> +
> +	if (divider->lock)
> +		spin_lock_irqsave(divider->lock, flags);
> +	else
> +		__acquire(divider->lock);
> +
> +	if (divider->flags & CLK_DIVIDER_HIWORD_MASK) {
> +		val = clk_div_mask(divider->width) << (divider->shift + 16);
> +	} else {
> +		regmap_read(sky1_div->regmap, sky1_div->offset, &val);
> +		val &= ~(clk_div_mask(divider->width) << divider->shift);
> +	}
> +	val |= (u32)value << divider->shift;
> +	regmap_write(sky1_div->regmap, sky1_div->offset, val);

[Severity: High]
Does this write safely reach the hardware, and does the new state survive a
suspend/resume cycle?

The Common Clock Framework can call operations like set_rate or set_parent
on an unprepared clock. Since the controller might be in RPM_SUSPENDED,
this could lead to unclocked register accesses that hang the bus.

Additionally, since sky1_audss_clk_runtime_resume() unconditionally
restores all registers from the stale reg_save array captured during the
last suspend, won't any new rate or parent state written here be
permanently lost on resume? Should these ops call pm_runtime_resume_and_get()
and update the cached state?

[ ... ]

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

[Severity: Medium]
Does jumping to err_pm bypass reset assertion and potentially leak resources?

The driver deasserts priv->rst_noc before calling sky1_audss_clks_enable().
If sky1_audss_clks_enable() fails, it jumps to err_pm, which bypasses
pm_runtime_put_sync(). This leaves the NOC reset deasserted. 

Furthermore, if CONFIG_PM is disabled, relying entirely on
pm_runtime_put_sync() to clean up clocks and resets means the suspend
callback won't execute, leaving parent clocks permanently enabled.

[ ... ]

> +fail_register:
> +fail_fixed_clk:
> +fail_clks_set:
> +	pm_runtime_put_sync(dev);
> +err_pm:
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_disable(dev);
> +	return ret;
> +}

[Severity: High]
Will this fallthrough cause the PM runtime usage counter to double-decrement?

The PM runtime usage counter is incremented exactly once via
pm_runtime_get_noresume() earlier in probe. If the code jumps to
fail_clks_set, it explicitly calls pm_runtime_put_sync(). Execution then
directly falls through to the err_pm label, which executes
pm_runtime_put_noidle(). Could this underflow the PM usage counter and
corrupt the device's PM state machine?

> +static void sky1_audss_clk_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	if (!pm_runtime_status_suspended(dev))
> +		pm_runtime_force_suspend(dev);
> +
> +	pm_runtime_disable(dev);
> +}

[Severity: High]
Is it safe to force suspend and disable PM runtime before devm-managed child
devices and interfaces are unregistered?

The driver uses devm_ interfaces to register CCF clocks and the auxiliary
reset device. These are automatically unregistered in the devres cleanup
phase, which executes after sky1_audss_clk_remove() fully returns.

During the window between remove() completing and devres executing, the clock
and reset interfaces remain exposed to consumers. If a consumer accesses them,
could it trigger a fault because the underlying hardware controller is already
suspended and disabled?

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