Re: [PATCH v6 07/13] clk: ambarella: add CV75 CCU driver

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Hardcoded `ctrl_val = 0` breaks parameter configuration for PLL v2.
- [High] `amb_pll_determine_rate` bypasses rate validation and clamping for fractional PLLs.
- [High] Out-of-bounds write in `ccu->data->hws` array due to improper bounds allocation.
- [High] `amb_pll_set_rate` calculates the VCO range using stale hardware register state.
- [High] PLL `pre_scaler` and `post_scaler` are ignored during parameter calculation.
- [Medium] Clock gating/power down is improperly implemented via `set_rate(0)`.
- [Low] Bypassing clock framework abstractions using `__clk_get_hw()`.
--

commit 54fae2ddb7651a9b5e3a861a2b9b96fa672eb7cb
Author: Long Zhao [off-list ref]

clk: ambarella: add CV75 CCU driver

Add the Ambarella CV75 clock driver covering the core PLL and the
UART0 / AHB / APB clocks needed for early console bring-up.

[ ... ]

+static int amb_cv75_rct_probe(struct platform_device *pdev)
+{

[ ... ]

+	osc_clk = devm_clk_get(&pdev->dev, "osc");
+	if (IS_ERR(osc_clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(osc_clk),
+				     "missing osc clock\n");
+	osc = __clk_get_hw(osc_clk);

[Severity: Low]
Is unwrapping the consumer clock pointer directly via __clk_get_hw()
bypassing the CCF abstractions? Could this be avoided by referencing
the parent natively via struct clk_parent_data (e.g., using 
.fw_name = "osc"), as the driver already does for composite clocks?

+
+	for (i = 0; i < ARRAY_SIZE(cv75_clks); i++) {
+		hw = amb_cv75_register_clk(&pdev->dev, ccu, &cv75_clks[i],
+					   osc, dummy);
+		if (IS_ERR(hw))
+			return dev_err_probe(&pdev->dev, PTR_ERR(hw),
+					     "failed to register %s\n",
+					     cv75_clks[i].name);
+
+		if (cv75_clks[i].id == AMB_CV75_CLK_REF_DUMMY)
+			dummy = hw;
+		else
+			ccu->data->hws[cv75_clks[i].id] = hw;

[Severity: High]
Does this code risk an out-of-bounds write?

The ccu->data array is allocated based on the number of elements:

drivers/clk/ambarella/ccu_common.c:amb_ccu_init() {
    ...
    ccu->data = devm_kzalloc(&pdev->dev,
                             struct_size(ccu->data, hws, num_clks),
                             GFP_KERNEL);
    ...
}

However, the array is indexed here using the id from the DT bindings
(cv75_clks[i].id). If the binding IDs are sparse or start from 1, the max
ID could exceed the array size (ARRAY_SIZE(cv75_clks)), corrupting memory.

+	}
+
+	return amb_ccu_register(ccu);
+}

[ ... ]

[ ... ]

+static int amb_div_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct amb_div *div = to_amb_div(hw);
+	int val, mask;
+
+	if (div->fix_divider)
+		rate *= div->fix_divider;
+
+	if (!rate) {
+		/* Assert the reset/disable bit above the divider field. */
+		val = BIT(div->width);
+		mask = clk_div_mask(div->width + 1);
+	} else {

[Severity: Medium]
Is clock gating properly handled here? By placing the power-down logic 
inside .set_rate(0) without providing standard .enable and .disable 
callbacks in clk_ops, standard drivers calling clk_disable_unprepare() 
will leave the hardware powered on.

+		val = divider_get_val(rate, parent_rate, NULL,
+				      div->width, div->flags);

[ ... ]

[ ... ]

+static int amb_pll_calc_params(struct amb_pll *pll, unsigned long rate,
+			       unsigned long parent_rate, u32 ctrl2_val,
+			       u32 *intp, u32 *sdiv, u32 *sout,
+			       u32 *vcodiv, u32 *fsdiv, u32 *fsout)
+{
+	const struct amb_pll_soc_data *soc_data = pll->soc_data;
+	unsigned long max_numerator, max_denominator;
+	unsigned long intp_ul, sout_ul, rate_tmp, parent_scaled;
+	u32 ctrl_val;
+
+	if (!parent_rate)
+		return -EINVAL;
+
+	*sdiv = 1;
+
+	if (soc_data->pll_version >= 2) {
+		ctrl_val = 0;
+		*vcodiv = (ctrl_val & soc_data->vcodiv_mask) ==
+			  soc_data->vcodiv_val ? 2 : 1;

[Severity: High]
Because ctrl_val is hardcoded to 0 here, the bitwise AND operations 
statically evaluate to 1 for vcodiv, fsdiv, and fsout on v2 PLLs. 
Does this incorrectly restrict v2 PLLs to use dividers of 1 and ignore 
existing hardware configurations?

+		*fsdiv = (ctrl_val & soc_data->fsdiv_mask) ==
+			 soc_data->fsdiv_val ? 2 : 1;
+		*fsout = (ctrl_val & soc_data->fsout_mask) ==
+			 soc_data->fsout_val ? 2 : 1;
+	} else {

[ ... ]

+	if (rate < parent_rate)
+		return -EINVAL;
+
+	max_numerator = soc_data->vco_max_mhz;
+	max_numerator = div_u64(max_numerator * 1000000ULL, parent_rate);

[Severity: High]
Does this calculation operate directly on the unscaled parent_rate? 
It appears that reg[PRES_OFFSET] and reg[POST_OFFSET] are completely 
ignored here. Will this result in incorrect PLL parameters if a PLL 
instance is configured with non-zero prescalers or postscalers?

+	max_numerator = div_u64(max_numerator, *vcodiv * *fsdiv);
+	max_numerator = min(128UL, max_numerator);

[ ... ]

+static int amb_pll_determine_rate(struct clk_hw *hw,
+				  struct clk_rate_request *req)
+{
+	struct amb_pll *pll = to_amb_pll(hw);
+	const struct amb_pll_soc_data *soc_data = pll->soc_data;
+	u32 ctrl2_val = 0;
+	u32 intp, sdiv, sout, vcodiv, fsdiv, fsout;
+	unsigned long rate;
+	u64 rounded;
+	int ret;
+
+	if (!req->best_parent_rate)
+		return -EINVAL;
+
+	if (!req->rate || pll->frac_mode)
+		return 0;

[Severity: High]
Does returning 0 immediately for fractional PLLs bypass mandatory rate 
clamping and boundary validation? It looks like the framework could pass 
unbounded or unachievable rates down to the hardware, which would later 
fail the VCO limits in amb_pll_calc_params().

+
+	if (req->rate > ULONG_MAX / pll->fix_divider)
+		return -ERANGE;

[ ... ]

+static int amb_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct amb_pll *pll = to_amb_pll(hw);
+	const struct amb_pll_soc_data *soc_data = pll->soc_data;
+	u32 *reg = pll->reg_offset;
+	u32 ctrl_val, ctrl2_val = 0, ctrl3_val, frac_val = 0;
+	u32 intp, sdiv, sout, vcodiv, fsdiv, fsout;
+	unsigned long old_rate, new_rate, rate_tmp;
+	int ret;
+
+	if (!rate) {
+		regmap_read(pll->map, reg[CTRL_OFFSET], &ctrl_val);
+		ctrl_val |= CTRL_POWER_DOWN | CTRL_HALT_VCO;
+		amb_pll_write_enable(pll->map, reg[CTRL_OFFSET], ctrl_val);
+		return 0;
+	}

[Severity: Medium]
Similarly to the divider above, is clock gating properly handled here? 
Calling .set_rate(0) instead of implementing .disable and .enable 
callbacks leaves the clock powered on if a standard CCF driver calls 
clk_disable_unprepare().

+
+	if (!parent_rate)
+		return -EINVAL;

[ ... ]

+	ctrl_val = ((intp - 1) & 0x7f) << 24;
+	ctrl_val |= ((sdiv - 1) & 0xf) << 12;
+	ctrl_val |= ((sout - 1) & 0xf) << 16;
+	if (soc_data->pll_version >= 2) {
+		ctrl_val |= vcodiv == 2 ? soc_data->vcodiv_val : 0;

[Severity: High]
Because the vcodiv parameter (along with fsdiv and fsout) was computed 
with a hardcoded 0 in amb_pll_calc_params(), will this write those 
incorrectly calculated bits back to the hardware and destroy the 
existing divider configuration?

+		ctrl_val |= fsdiv == 2 ? soc_data->fsdiv_val : 0;
+		ctrl_val |= fsout == 2 ? soc_data->fsout_val : 0;
+	}
+
+	regmap_write(pll->map, reg[CTRL_OFFSET], ctrl_val);
+	regmap_write(pll->map, reg[FRAC_OFFSET], 0);
+
+	old_rate = amb_pll_recalc_rate(hw, parent_rate) * pll->fix_divider;
+	rate_tmp = old_rate > rate ? 0 : rate - old_rate;
+	if (rate_tmp && pll->frac_mode) {
+		u64 dividend, divider;
+
+		dividend = (u64)rate_tmp * sout * fsout;
+		dividend <<= 32;
+		divider = (u64)sdiv * fsdiv * parent_rate;
+		frac_val = DIV64_U64_ROUND_CLOSEST(dividend, divider);
+		regmap_write(pll->map, reg[FRAC_OFFSET], frac_val);
+		ctrl_val |= CTRL_FRAC_MODE;
+	}
+
+	if (soc_data->pll_version >= 2) {
+		ctrl3_val = soc_data->ctrl3_val;

[ ... ]

+	} else {
+		u32 fvco_mhz, range;
+
+		fvco_mhz = amb_pll_calc_vco(pll, parent_rate) / 1000000UL;

[Severity: High]
Does amb_pll_calc_vco() use stale register state here? The ctrl_val was 
updated with CTRL_FRAC_MODE locally, but it hasn't been written to the 
hardware yet. Because amb_pll_calc_vco() performs a direct regmap_read 
of CTRL_OFFSET, it will miss the fractional component, leading to an 
incorrect VCO range assignment for the CTRL3 register below.

+		for (range = 0; range < ARRAY_SIZE(soc_data->vco_range);
+		     range++) {
+			if (fvco_mhz > soc_data->vco_range[range])
+				break;
+		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-cv75-v5-v6-0-e918514cb3b1@ambarella.com?part=7