Re: [PATCH v2 3/3] clk: add support for siflower sf21-topcrm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Holding a raw spinlock and disabling interrupts while busy-waiting for up to 1ms.
- [High] Global clock structures are mutated with pointers to dynamically allocated memory, causing dangling pointers on probe failure or driver unbind.
--

commit 479eb0c843e2bfaa965d0cb0f125d0ed4dd0cf0b
Author: Chuanhong Guo <gch981213@gmail.com>

clk: add support for siflower sf21-topcrm

This commit adds a driver for the Toplevel clock and reset controller
found on Siflower SF21A6826/SF21H8898 SoCs. It includes control for 3
PLLs, several clock mux/gate/divider blocks, and a reset register.

> diff --git a/drivers/clk/siflower/clk-sf21-topcrm.c b/drivers/clk/siflower/clk-sf21-topcrm.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/siflower/clk-sf21-topcrm.c
[ ... ]
> +static int sf21_cmnpll_vco_set_rate(struct clk_hw *hw, unsigned long rate,
> +				    unsigned long parent_rate)
> +{
> +	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> +	unsigned long fbdiv, refdiv;
> +	u32 val;
> +
> +	rational_best_approximation(rate, parent_rate, FIELD_MAX(PLL_CMN_FBDIV),
> +				    FIELD_MAX(PLL_CMN_REFDIV), &fbdiv, &refdiv);
> +	if (!refdiv || !fbdiv)
> +		return -EINVAL;
> +
> +	guard(raw_spinlock_irqsave)(priv->lock);
> +
> +	sf_rmw(priv, PLL_CMN_CFG1, PLL_CMN_REFDIV | PLL_CMN_FBDIV | PLL_CMN_PD,
> +	       FIELD_PREP(PLL_CMN_REFDIV, refdiv) |
> +		       FIELD_PREP(PLL_CMN_FBDIV, fbdiv));
> +	sf_writel(priv, CFG_LOAD, BIT(CFG_LOAD_CMN_PLL_BIT));
> +	sf_writel(priv, CFG_LOAD, 0);
> +
> +	return readl_poll_timeout_atomic(priv->base + PLL_CMN_LOCK, val, val & 1,
> +					0, PLL_LOCK_TIMEOUT_US);
> +}

[Severity: High]
Does this code hold the raw spinlock with interrupts disabled for up to 1ms
while polling for the PLL to lock?

Because guard() releases the lock at the end of the variable's scope, the
lock remains held during the readl_poll_timeout_atomic() call. Tightly
spinning for 1ms with interrupts disabled could cause system latency spikes
or RCU stalls.

Would it be better to use scoped_guard() for the sf_rmw() and sf_writel()
calls so the lock is dropped before the polling loop?

[ ... ]
> +static int sf21_pciepll_vco_enable(struct clk_hw *hw)
> +{
> +	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> +	u32 val;
> +
> +	guard(raw_spinlock_irqsave)(priv->lock);
> +	sf_rmw(priv, PLL_PCIE_CFG1, 0, PLL_PCIE_PLLEN);
> +	sf_writel(priv, CFG_LOAD, BIT(CFG_LOAD_PCIE_PLL_BIT));
> +	sf_writel(priv, CFG_LOAD, 0);
> +	return readl_poll_timeout_atomic(priv->base + PLL_PCIE_LOCK, val,
> +					 val & 1, 0, PLL_LOCK_TIMEOUT_US);
> +}

[Severity: High]
This appears to have the same issue mentioned above where a raw spinlock
is held with interrupts disabled during a 1ms polling loop.

[ ... ]
> +static struct sf_clk_common cmnpll_vco = {
> +	.hw.init = CLK_HW_INIT_PARENTS_DATA("cmnpll_vco", sf21_ref_parent,
> +					    &sf21_cmnpll_vco_ops, 0),
> +};
[ ... ]
> +static struct clk_hw_onecell_data sf21_hw_clks = {
> +	.num = SF21_NUM_CLKS,
> +	.hws = {
> +		[SF21_CLK_CMNPLL_VCO] = &cmnpll_vco.hw,
[ ... ]
> +static int sf21_topcrm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sf21_clk_ctrl *ctrl;
> +	int i, ret;
> +
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ctrl->base))
> +		return dev_err_probe(dev, PTR_ERR(ctrl->base),
> +				     "failed to map resources\n");
> +
> +	raw_spin_lock_init(&ctrl->clk_lock);
> +	spin_lock_init(&ctrl->rst_lock);
> +
> +	for (i = 0; i < sf21_hw_clks.num; i++) {
> +		struct clk_hw *hw = sf21_hw_clks.hws[i];
> +		struct sf_clk_common *common;
> +
> +		if (!hw)
> +			continue;
> +		common = hw_to_sf_clk_common(hw);
> +		common->base = ctrl->base;
> +		common->lock = &ctrl->clk_lock;
> +		ret = devm_clk_hw_register(dev, hw);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to register clock %d\n",
> +					     i);
> +	}

[Severity: High]
Does this setup mutate the global static clock structures (like cmnpll_vco)
with pointers to the dynamically allocated ctrl memory?

If probe() fails later or the driver unbinds, devm_kzalloc will free ctrl,
potentially leaving dangling pointers in the global clock structures. This
could result in a use-after-free or corrupt the clock framework's internal
tree if the driver is probed again later.

Should the clock structures be dynamically allocated per-device or embedded
in a driver-private struct instead of using global state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-sf21-topcrm-v2-0-4527aae05c3d@gmail.com?part=3