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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Static initialization of the flexible array in `sf21_hw_clks` creates a critical out-of-bounds memory access vulnerability.
- [High] Lock nesting violation on PREEMPT_RT due to `spinlock_t` usage inside `.enable`/`.disable` callbacks.
- [High] Global static clock definition structures are modified with per-device dynamically allocated pointers.
- [High] Missing `select RATIONAL` in Kconfig leads to linker errors.
- [Medium] Integer truncation in PLL rate calculations causes precision loss.
- [Medium] Silent acceptance of invalid or unsupported soft reset IDs.
--

commit 83e3e7ec9410a85a7d5912f8b3485089ca10bdce
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 support for
multiple PLLs, mux/gate/divider blocks, and a reset register.

> diff --git a/drivers/clk/siflower/Kconfig b/drivers/clk/siflower/Kconfig
> new file mode 100644
> index 0000000000000..03cbfbdbdb8db
> --- /dev/null
> +++ b/drivers/clk/siflower/Kconfig
> @@ -0,0 +1,22 @@
[ ... ]
> +config CLK_SF21_TOPCRM
> +	tristate "Clock driver for Siflower SF21 toplevel clock & reset module"
> +	depends on ARCH_SIFLOWER || COMPILE_TEST
> +	default ARCH_SIFLOWER
> +	select RESET_CONTROLLER

Should this select RATIONAL? The driver calls rational_best_approximation()
in rate calculations, which could lead to linker errors if RATIONAL is not
enabled during a COMPILE_TEST build.

[ ... ]
> diff --git a/drivers/clk/siflower/clk-sf21-topcrm.c b/drivers/clk/siflower/clk-sf21-topcrm.c
> new file mode 100644
> index 0000000000000..7d4c5e370d6d2
> --- /dev/null
> +++ b/drivers/clk/siflower/clk-sf21-topcrm.c
> @@ -0,0 +1,1053 @@
[ ... ]
> +struct sf_clk_common {
> +	void __iomem	*base;
> +	/* Serializes register RMW sequences shared by clocks and resets. */
> +	spinlock_t	*lock;

Could this lock nesting cause issues on PREEMPT_RT?

The Common Clock Framework invokes .enable and .disable callbacks while
holding its own enable_lock, which is a raw_spinlock_t. Acquiring a
spinlock_t inside a raw spinlock critical section is a lock nesting
violation and can lead to a scheduling-while-atomic panic on PREEMPT_RT
kernels.

Does this hardware lock need to be a raw_spinlock_t?

> +	struct clk_hw	hw;
> +};
[ ... ]
> +static unsigned long sf21_cmnpll_vco_recalc_rate(struct clk_hw *hw,
> +						 unsigned long parent_rate)
> +{
> +	struct sf_clk_common *priv = hw_to_sf_clk_common(hw);
> +	u32 cfg = sf_readl(priv, PLL_CMN_CFG1);
> +	unsigned long refdiv = FIELD_GET(PLL_CMN_REFDIV, cfg);
> +	unsigned long fbdiv = FIELD_GET(PLL_CMN_FBDIV, cfg);
> +
> +	if (!refdiv || !fbdiv)
> +		return 0;
> +
> +	return (parent_rate / refdiv) * fbdiv;

Could this integer division truncate the remainder and cause precision
loss in the reported frequency?

This pattern also appears in other rate calculation functions like
sf21_pciepll_vco_recalc_rate and sf21_ddrpll_postdiv_recalc_rate. Would it
be better to use 64-bit math such as div_u64((u64)parent_rate * fbdiv,
refdiv) or mult_frac()?

> +}
[ ... ]
> +static struct clk_hw_onecell_data sf21_hw_clks = {
> +	.num = SF21_CLK_MAX,
> +	.hws = {
> +		[SF21_CLK_CMNPLL_VCO] = &cmnpll_vco.hw,
[ ... ]
> +		[SF21_CLK_PCIE_REFN] = &pcie_refclk_n.common.hw,
> +		[SF21_CLK_PCIE_REFP] = &pcie_refclk_p.common.hw,
> +	}
> +};

If SF21_CLK_MAX is larger than the number of items explicitly initialized
in this array, could this lead to an out-of-bounds memory read?

When statically initialized, the compiler only allocates space up to the
highest explicitly initialized index (SF21_CLK_PCIE_REFP). If the loop in
the probe function reads up to sf21_hw_clks.num, it could read past the end
of the allocated structure.

[ ... ]
> +struct sf21_clk_ctrl {
> +	void __iomem *base;
> +	/* Serializes register RMW sequences shared by clocks and resets. */
> +	spinlock_t lock;
> +	struct reset_controller_dev rcdev;
> +	const u32 *reset_bits;
> +	unsigned int nr_resets;
> +};
[ ... ]
> +static int sf21_topcrm_reset_update(struct reset_controller_dev *rcdev,
> +				    unsigned long id, bool assert)
> +{
> +	struct sf21_clk_ctrl *ctrl = rcdev_to_sf21_topcrm(rcdev);
> +	u32 bit = ctrl->reset_bits[id];

Can this accept invalid or unsupported reset IDs? Since the
sf21_topcrm_reset_bits array has unpopulated gaps, bit will evaluate to 0
for unsupported IDs.

If this happens, the code blindly performs reg &= ~0 or reg |= 0 and
returns success without throwing an error. Should it verify that the bit
is valid and return -EINVAL if not?

> +	unsigned long flags;
> +	u32 reg;
> +
> +	spin_lock_irqsave(&ctrl->lock, flags);
> +	reg = readl(ctrl->base + SF21_SOFT_RESET);
> +	if (assert)
> +		reg &= ~bit;
> +	else
> +		reg |= bit;
> +	writel(reg, ctrl->base + SF21_SOFT_RESET);
> +	spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +	return 0;
> +}
[ ... ]
> +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");
> +
> +	spin_lock_init(&ctrl->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->lock;

Could modifying global static clock definitions with per-device pointers
lead to a use-after-free?

If probe defers or fails later, ctrl is freed by devres, leaving the
global clock objects pointing to dangling memory. Subsequent probes could
then encounter corrupted pointers.

> +		ret = devm_clk_hw_register(dev, hw);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to register clock %d\n",
> +					     i);
> +	}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-sf21-topcrm-v1-0-438f2e0513ff@gmail.com?part=4