Re: [PATCH v5 3/6] clk: anlogic: add cru support for Anlogic DR1V90 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Global state mutation on probe causes memory corruption upon driver rebind.
- [High] Data race in read-modify-write operations on shared clock registers.
- [High] Clocks gated at boot cannot be enabled and rate requests for disabled clocks are discarded.
- [High] Kernel panic due to division by zero in `cru_div_gate_determine_rate()`.
- [Medium] `cru_div_gate_recalc_rate()` erroneously reports 0 Hz when the clock is disabled, corrupting the CCF cache.
--

commit 49a46a5cd3719a2e4d6b1b89b13b6a45411cc337
Author: Junhui Liu <junhui.liu@pigmoral.tech>

clk: anlogic: add cru support for Anlogic DR1V90 SoC

The Clock and Reset Unit (CRU) in the Anlogic DR1V90 SoC provides management
for the clock and reset. The driver includes support for generic clocks,
PLLs, and dividers with gating, and registers an auxiliary device for resets.

> diff --git a/drivers/clk/anlogic/cru_dr1.c b/drivers/clk/anlogic/cru_dr1.c
> new file mode 100644
> index 0000000000000..10f8751a88383
> --- /dev/null
> +++ b/drivers/clk/anlogic/cru_dr1.c

[ ... ]

> @@ -48,16 +48,16 @@ static void cru_div_gate_endisable(struct clk_hw *hw, int enable)
>  {
>  	struct cru_div_gate *div_gate = hw_to_cru_div_gate(hw);
>  	struct clk_divider *divider = &div_gate->divider;
>  	u32 reg;
>  
>  	reg = readl(divider->reg);
>  	reg &= ~(clk_div_mask(divider->width) << divider->shift);
>  
>  	if (enable)
>  		reg |= div_gate->val << divider->shift;
>  
>  	writel(reg, divider->reg);
>  }

Multiple clocks (like qspi, spi, smc, and sdio) share the same MMIO register
0x030.

Since .set_rate runs under a global mutex (prepare_lock) and .enable runs
under a global spinlock (enable_lock), could concurrent calls to
cru_div_gate_set_rate() and cru_div_gate_endisable() on different clocks
mapped to the same register cause a data race and corrupt the hardware state
here?

Should there be a spinlock protecting these read-modify-write sequences?

[ ... ]

> @@ -86,17 +86,17 @@ static unsigned long cru_div_gate_recalc_rate(struct clk_hw *hw,
>  					      unsigned long parent_rate)
>  {
>  	struct cru_div_gate *div_gate = hw_to_cru_div_gate(hw);
>  	struct clk_divider *divider = &div_gate->divider;
>  	unsigned int val;
>  
>  	val = readl(divider->reg) >> divider->shift;
>  	val &= clk_div_mask(divider->width);
>  
>  	if (val < div_gate->min)
>  		return 0;

Does returning 0 here corrupt the CCF rate cache when the clock is gated?

The CCF expects .recalc_rate() to return the operational structural frequency
of the clock, which is persistently cached. If a gated clock returns 0, will
clk_get_rate() erroneously report 0 Hz even after the clock is enabled?

>  
>  	return divider_recalc_rate(hw, parent_rate, val, divider->table,
>  				   divider->flags, divider->width);
>  }

[ ... ]

> @@ -107,15 +107,15 @@ static int cru_div_gate_determine_rate(struct clk_hw *hw,
>  				       struct clk_rate_request *req)
>  {
>  	struct cru_div_gate *div_gate = hw_to_cru_div_gate(hw);
>  	struct clk_divider *divider = &div_gate->divider;
>  	unsigned long maxdiv, mindiv;
>  	int div = 0;
>  
>  	maxdiv = clk_div_mask(divider->width) + 1;
>  	mindiv = div_gate->min + 1;
>  
>  	div = DIV_ROUND_UP_ULL(req->best_parent_rate, req->rate);

Can req->rate be 0 here?

If the clock framework or userspace requests a target rate of 0 Hz (e.g., via
clk_round_rate(clk, 0)), does this cause an immediate division-by-zero kernel
panic?

>  	div = div > maxdiv ? maxdiv : div;
>  	div = div < mindiv ? mindiv : div;

[ ... ]

> @@ -128,14 +128,14 @@ static int cru_div_gate_set_rate(struct clk_hw *hw, unsigned long rate,
>  				 unsigned long parent_rate)
>  {
>  	struct cru_div_gate *div_gate = hw_to_cru_div_gate(hw);
>  	struct clk_divider *divider = &div_gate->divider;
>  	int value;
>  	u32 reg;
>  
>  	if (!__clk_is_enabled(hw->clk))
>  		return 0;

If cru_div_gate_set_rate() returns early when the clock is disabled, the
cached div_gate->val isn't updated.

If the clock was disabled by the bootloader (so the cached value initialized
in cru_div_gate_init() is 0), and a consumer calls clk_set_rate() followed by
clk_enable(), will cru_div_gate_enable() write 0 to the hardware and silently
leave the clock gated forever?

>  
>  	value = divider_get_val(rate, parent_rate, divider->table,

[ ... ]

> @@ -176,14 +176,14 @@ int dr1_cru_clk_register(struct device *dev, void __iomem *base,
>  			 const struct cru_clk *clks, int nr_clks)
>  {
>  	struct clk_hw_onecell_data *priv;
>  	int i, ret;
>  
>  	priv = devm_kzalloc(dev, struct_size(priv, hws, nr_clks), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < nr_clks; i++) {
>  		const struct cru_clk *clk = &clks[i];
>  
>  		if (clk->reg)
>  			*(clk->reg) += (uintptr_t)base;

Will this permanently modify the static global clock structures during probe?

If probe deferral occurs or the driver is unbound and rebound, wouldn't base
be added a second time to the already-modified address, leading to invalid
memory pointer dereferences?

>  
>  		ret = devm_clk_hw_register(dev, clk->hw);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-dr1v90-cru-v5-0-34f3021aab51@pigmoral.tech?part=3