Re: [PATCH v4 5/5] clk: renesas: r9a09g077: Add LCDC and PLL3 clock support for RZ/T2H display pipeline

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `.determine_rate` callbacks unlawfully mutate internal driver state and break clock encapsulation, violating Common Clock Framework (CCF) semantics.
- [High] Kernel panic due to divide-by-zero in `r9a09g077_cpg_pll3_clk_recalc_rate` if the bootloader leaves the PLL uninitialized.
- [High] Unchecked return values from CCF parent resolution functions lead to a kernel panic via NULL pointer dereference.
- [Medium] Passing `unsigned long req->rate` to `mul_u32_u32()` causes silent 32-bit truncation on 64-bit platforms.
--

commit 1caebb13201d322aab2b2dd055d26412e4129ce0
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

clk: renesas: r9a09g077: Add LCDC and PLL3 clock support for RZ/T2H display pipeline

This commit adds clock definitions and PLL logic for the LCDC blocks on the
RZ/T2H SoC. It introduces a new PLL clock type, implementing rate
recalculation, programming, and locking sequences for PLL3. It also exposes
the LCDC core and module clocks to enable display pipeline integration.

diff --git a/drivers/clk/renesas/r9a09g077-cpg.c b/drivers/clk/renesas/r9a09g077-cpg.c
--- a/drivers/clk/renesas/r9a09g077-cpg.c
+++ b/drivers/clk/renesas/r9a09g077-cpg.c
@@ -577,6 +577,276 @@ r9a09g077_cpg_fselxspi_div_clk_register(struct device *dev,
> +static unsigned long r9a09g077_cpg_pll3_clk_recalc_rate(struct clk_hw *hw,
> +							unsigned long parent_rate)
> +{
[ ... ]
> +	ctr0 = readl(CPG_PLL3_VCO_CTR0(pll_clk->reg));
> +	ctr1 = readl(CPG_PLL3_VCO_CTR1(pll_clk->reg));
> +
> +	pdiv = FIELD_GET(CPG_PLL3_VCO_CTR0_PDIV, ctr0);
[ ... ]
> +	return DIV_ROUND_CLOSEST_ULL(rate, pdiv);

[Severity: High]
Does this code risk a divide-by-zero if the bootloader leaves the PLL
uninitialized?

If the clock controller block is in a standby state, the register might read
as 0, making pdiv 0. Unconditionally dividing by pdiv via
DIV_ROUND_CLOSEST_ULL() without validation would trigger a fatal kernel
divide-by-zero exception during the clock probe phase.

> +}
> +
> +static int r9a09g077_cpg_pll3_determine_rate(struct clk_hw *hw,
> +					     struct clk_rate_request *req)
> +{
> +	struct pll_clk *pll_clk = to_pll(hw);
> +	u64 rate_millihz;
> +
> +	if (req->rate == pll_clk->cur_rate)
> +		return 0;
> +
> +	rate_millihz = mul_u32_u32(req->rate, MILLI);

[Severity: Medium]
Will this silently truncate req->rate to 32 bits on 64-bit platforms?

mul_u32_u32() accepts 32-bit integers, but req->rate is an unsigned long
(which is 64-bit on architectures like ARM64). If a consumer requests a very
high rate, such as using clk_round_rate(clk, ULONG_MAX) to find the maximum
possible supported rate, the argument will be silently truncated to its
lower 32 bits before multiplication.

> +	if (!rzv2h_cpg_get_pll_pars(pll_clk->limits, &pll_clk->pll_parameters,
> +				    rate_millihz)) {
[ ... ]
> +	}
> +	req->rate = DIV_ROUND_CLOSEST_ULL(pll_clk->pll_parameters.freq_millihz, MILLI);
> +	pll_clk->cur_rate = req->rate;

[Severity: High]
Is it safe to mutate internal driver state like pll_clk->cur_rate and
pll_clk->pll_parameters during a read-only rate query?

Common Clock Framework semantics strictly require .determine_rate() callbacks
to be side-effect free, as they are used by the framework for speculative
evaluations (e.g., clk_round_rate()).

> +
> +	return 0;
> +}
> +
> +static int r9a09g077_cpg_pll3_set_rate(struct clk_hw *hw, unsigned long rate,
> +				       unsigned long parent_rate)
> +{
> +	struct pll_clk *pll_clk = to_pll(hw);
> +	struct rzv2h_pll_pars *params = &pll_clk->pll_parameters;

[Severity: High]
Does this code ignore the passed rate argument and program the hardware
using the cached pll_parameters?

Because .determine_rate() mutates driver-internal state, if the framework
evaluates multiple rates before applying one, the hardware will be programmed
with incorrect values from previous speculative checks.

[ ... ]
> +static int r9a09g077_cpg_lcdc_div_determine_rate(struct clk_hw *hw,
> +						 struct clk_rate_request *req)
> +{
> +	struct r9a09g077_lcdc_div_clk *dsi_div = to_lcdc_div_clk(hw);
> +	struct clk_hw *mux_hw = clk_hw_get_parent(hw);
[ ... ]
> +	/* index 1 is always .pll3 in sel_clk_pll3[] */
> +	pll_clk = to_pll(clk_hw_get_parent_by_index(mux_hw, 1));

[Severity: High]
Can mux_hw be NULL if the current clock is an orphan?

clk_hw_get_parent(hw) can return NULL during intermediate setup phases or
if a parent fails to register. Passing a potentially NULL mux_hw to
clk_hw_get_parent_by_index() unconditionally dereferences hw->core internally,
causing a kernel panic.

Furthermore, if clk_hw_get_parent_by_index itself returns NULL, to_pll()
performs unsafe pointer arithmetic yielding an invalid address that leads
to a crash later.

[ ... ]
> +	freq_millihz = mul_u32_u32(req->rate, MILLI);

[Severity: Medium]
Will this silently truncate the 64-bit req->rate to 32 bits on 64-bit
platforms, similar to r9a09g077_cpg_pll3_determine_rate()?

[ ... ]
> +	req->rate = DIV_ROUND_CLOSEST_ULL(dsi_params.div.freq_millihz, MILLI);
> +	req->best_parent_rate = req->rate * dsi_params.div.divider_value;
> +	dsi_div->divider = dsi_params.div.divider_value;
> +	pll_clk->cur_rate = req->best_parent_rate;
> +	pll_clk->pll_parameters = dsi_params.pll;

[Severity: High]
Is it safe to mutate internal driver state during this read-only query?

This function mutates both dsi_div->divider and its grandparent PLL3's cached
parameters (pll_clk->cur_rate and pll_clk->pll_parameters) during a
speculative rate evaluation.

> +
> +	return 0;
> +}
> +
> +static int r9a09g077_cpg_lcdc_div_set_rate(struct clk_hw *hw,
> +					   unsigned long rate,
> +					   unsigned long parent_rate)
> +{
> +	struct r9a09g077_lcdc_div_clk *dsi_div = to_lcdc_div_clk(hw);
> +	const struct clk_div_table *clkt;
> +	bool divider_found = false;
> +	u32 val, shift;
> +
> +	for (clkt = dsi_div->dtable; clkt->div; clkt++) {
> +		if (clkt->div == dsi_div->divider) {

[Severity: High]
Does this code rely on the mutated dsi_div->divider state instead of
recalculating the divider from the passed rate argument?

If the Common Clock Framework evaluates multiple rates before applying one,
the divider will hold an incorrect value from the last speculative check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618181949.3036280-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=5