Re: [PATCH v2 4/5] 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:
- [Critical] Clock operations access hardware registers without ensuring the provider device is active, bypassing runtime PM and risking a system hang.
- [High] The driver saves per-device register state during runtime suspend into a statically allocated global array, causing state corruption.
- [High] The driver incorrectly uses local consumer connection IDs as global parent clock names, leading to orphaned clocks.
- [High] Multiple independent logical clocks erroneously share the exact same hardware registers and bitfields.
- [High] Manual unregistration of composite clocks in `remove()` happens before devres unregisters the clock provider, creating a use-after-free window.
- [High] The driver fails to check the return value of regmap_read(), leading to uninitialized stack memory being used and written back to hardware.
- [High] Runtime resume blindly restores state from the last suspend, clobbering new hardware configurations made by consumers while the device was suspended.
- [Medium] Parent clock reference counts are leaked on probe failure and device removal if CONFIG_PM is disabled.
- [Low] The dev_pm_ops structure is unconditionally assigned without pm_ptr(), causing dead code when CONFIG_PM is disabled.
--

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

clk: cix: add sky1 audss clock controller

This patch adds a platform driver for the Cix Sky1 Audio Subsystem (AUDSS)
internal clock controller. It registers mux, divider, and gate composite clocks
for various audio components and brings up six SoC-level audio reference
clocks.

> diff --git a/drivers/clk/cix/clk-sky1-audss.c b/drivers/clk/cix/clk-sky1-audss.c
> new file mode 100644
> index 0000000000000..899452d5ed144
> --- /dev/null
> +++ b/drivers/clk/cix/clk-sky1-audss.c
> @@ -0,0 +1,1129 @@
[ ... ]
> +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 get overwritten by multiple instances of the driver?

During runtime suspend, the driver reads dynamic register values into
devtype_data->reg_save, which points to this statically allocated array. This
could mutate constant match data and corrupt state across multiple device
instances if more than one exists.

[ ... ]
> +static const char * const dsp_clk_parent[] = {
> +	"audio_clk4"
> +};

[Severity: High]
Will these clocks become orphaned?

The driver retrieves input clocks via devm_clk_get() using local connection
IDs like audio_clk4, but passes these string literals as parent_names to CCF
registration functions. Since CCF matches parent_names against the global
provider clock name, this might fail to find the parent. Should the driver
use parent_data with .fw_name instead?

[ ... ]
> +	/* dsp */
> +	CFG(CLK_DSP_CLK,
> +	    "audss_dsp_clk",
> +	    dsp_clk_parent,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_DIV, 0, 2, 0,
> +	    INFO_HIFI0, 0, 0,
> +	    0),
[ ... ]
> +	/* i2s */
> +	CFG(CLK_I2S0_APB,
> +	    "audss_i2s0_apb",
> +	    i2s_apb_parent,
> +	    -1, 0, 0, 0,
> +	    INFO_CLK_DIV, 0, 2, 0,
> +	    INFO_CLK_GATE, 0, 0,
> +	    0),

[Severity: High]
Is it intentional that multiple independent clocks share the exact same
INFO_CLK_DIV register offset, shift 0, and width 2?

If one clock's rate is modified, it will physically clobber the shared divider
bits without updating the internal CCF state for the other clocks (like
CLK_DSP_CLK and CLK_I2S0_APB through CLK_I2S9_APB), which could lead to
widespread clock rate corruption.

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

[Severity: High]
Can reg remain uninitialized here?

If regmap_read() fails and returns an error code, the reg stack variable will
contain uninitialized memory, which is then used in bitwise operations and
written back to the hardware via regmap_write(). Should the return value
of regmap_read() be checked?

> +	}
> +	val = val << mux->shift;
> +	reg |= val;
> +	regmap_write(sky1_mux->regmap, sky1_mux->offset, reg);
[ ... ]
> +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: Critical]
Does this write to the hardware while the device might be runtime suspended?

CCF allows operations like clk_set_rate to execute on unprepared clocks without
automatically waking the provider device. If pm_runtime_get_sync() is not
called before regmap_write(), accessing registers while the parent clocks are
cut in sky1_audss_clk_runtime_suspend() could trigger a synchronous external
abort and hang the system.

> +
> +	if (divider->lock)
> +		spin_unlock_irqrestore(divider->lock, flags);
> +	else
> +		__release(divider->lock);
> +
> +	return 0;
> +}
[ ... ]
> +	/* audio_clk4 clock fixed divider */
> +	clk_table[CLK_AUD_CLK4_DIV2] =
> +		devm_clk_hw_register_fixed_factor(dev,
> +						  "audio_clk4_div2",
> +						  "audio_clk4",
> +						  0,
> +						  1, 2);

[Severity: High]
This again uses the local connection name audio_clk4 as the parent_name for
the fixed factor clock. Is this expected to match the upstream provider's
global clock name?

[ ... ]
> +	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;
> +	}
> +
> +	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 CONFIG_PM is disabled, will the parent clocks prepared and enabled in
sky1_audss_clks_enable() remain permanently enabled?

The error path relies on pm_runtime_put_sync() to disable parent clocks,
but without CONFIG_PM, this becomes a no-op, leading to leaked reference
counts.

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

[Severity: High]
Does unregistering the composite clocks here create a use-after-free window?

The driver registers the OF clock provider using devm_of_clk_add_hw_provider()
which defers cleanup to the devres phase after remove() finishes. Since the
underlying clk_hw structures are explicitly freed here, the active OF clock
provider might return a dangling pointer if a consumer queries the clock
during this window.

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

[Severity: Medium]
Similar to the probe error path, if CONFIG_PM is disabled,
pm_runtime_force_suspend() does nothing. Won't this leak the parent clock
reference counts upon device removal since sky1_audss_clks_disable() is never
called?

> +
> +	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 writes dynamic device state directly into devtype_data->reg_save, which
points to the global sky1_reg_save array. Should the state array be
dynamically allocated per-instance instead?

> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	sky1_audss_clks_disable(priv);
> +
> +	return 0;
> +}
> +
> +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 = sky1_audss_clks_enable(priv);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks\n");
> +		return ret;
> +	}
> +
> +	reset_control_deassert(priv->rst_noc);
> +
> +	spin_lock_irqsave(&priv->lock, flags);
> +	for (i = 0; i < devtype_data->reg_save_size; i++)
> +		regmap_write(priv->regmap_cru,
> +			     devtype_data->reg_save[i][0], devtype_data->reg_save[i][1]);

[Severity: High]
Will this blindly clobber new hardware configurations made by consumers while
the device was suspended?

If a clock rate was changed via clk_set_rate during suspend, restoring all
register values from the statically saved array here will overwrite the
consumer's new configuration with stale values from the previous suspend cycle.

> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops sky1_audss_clk_pm_ops = {
> +	SET_RUNTIME_PM_OPS(sky1_audss_clk_runtime_suspend,
> +			   sky1_audss_clk_runtime_resume, NULL)
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				     pm_runtime_force_resume)
> +};
> +
> +static const struct sky1_audss_clks_devtype_data sky1_devtype_data = {
> +	.reg_save = sky1_reg_save,
> +	.reg_save_size = ARRAY_SIZE(sky1_reg_save),
> +	.clk_names = sky1_audss_clk_names,
> +	.clk_num = ARRAY_SIZE(sky1_audss_clk_names),
> +	.clk_rate_default = sky1_clk_rate_default,
> +	.clk_cfg = sky1_audss_clks,
> +	.clk_cfg_size = ARRAY_SIZE(sky1_audss_clks),
> +};
> +
> +static const struct of_device_id sky1_audss_clk_of_match[] = {
> +	{ .compatible = "cix,sky1-audss-clock", .data = &sky1_devtype_data, },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, sky1_audss_clk_of_match);
> +
> +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 assignment be wrapped with pm_ptr()?

Assigning the pointer unconditionally prevents the compiler from optimizing out
the dev_pm_ops structure and its __maybe_unused callbacks when CONFIG_PM is
disabled, leaving dead code in the binary.

> +	},
> +};

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