Re: [PATCH v2 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:
- [Critical] Unsafe cast of a dynamically-parented grandparent clock leads to type confusion and potential kernel panic.
- [Critical] Uninitialized stack variable `init` passed to clock registration framework causes undefined behavior and kernel panic.
- [High] The `determine_rate` callbacks illegally mutate internal hardware caching state, breaking the clock framework's stateless query design.
- [High] Unchecked hardware reads can cause a division by zero panic during clock tree initialization.
--

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

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

Add the clock definitions and PLL logic required to supply the LCDC
(VSPD/FCPVD/DU) blocks on the RZ/T2H (R9A09G077) SoC. Introduce a new PLL
clock type and implement rate recalculation, programming and locking sequences
for PLL3.

> diff --git a/drivers/clk/renesas/r9a09g077-cpg.c b/drivers/clk/renesas/r9a09g077-cpg.c
> index 93b15e06a19bc..44c8b5aaa2afb 100644
> --- a/drivers/clk/renesas/r9a09g077-cpg.c
> +++ b/drivers/clk/renesas/r9a09g077-cpg.c

[ ... ]

> @@ -480,6 +576,272 @@ r9a09g077_cpg_fselxspi_div_clk_register(struct device *dev,
>  	return hw->clk;
>  }
>  
> +static unsigned long r9a09g077_cpg_pll3_clk_recalc_rate(struct clk_hw *hw,
> +							unsigned long parent_rate)
> +{
> +	struct pll_clk *pll_clk = to_pll(hw);
> +	u32 ctr0, ctr1;
> +	u8 pdiv, sdiv;
> +	u64 rate;
> +	u16 mdiv;
> +	s16 kdiv;
> +
> +	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);
> +	mdiv = FIELD_GET(CPG_PLL3_VCO_CTR0_MDIV, ctr0);
> +	kdiv = (s16)FIELD_GET(CPG_PLL3_VCO_CTR1_KDIV, ctr1);
> +	sdiv = FIELD_GET(CPG_PLL3_VCO_CTR1_SDIV, ctr1);
> +
> +	rate = mul_u64_u32_shr(parent_rate, (mdiv << 16) + kdiv, 16 + sdiv);
> +
> +	return DIV_ROUND_CLOSEST_ULL(rate, pdiv);

[Severity: High]
Can pdiv be 0 here if the hardware defaults to a 0 state or the bootloader
has not initialized the PLL?

If pdiv is 0, will this DIV_ROUND_CLOSEST_ULL() macro trigger a division by
zero panic during clock tree initialization?

> +}
> +
> +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);
> +	if (!rzv2h_cpg_get_pll_pars(pll_clk->limits, &pll_clk->pll_parameters,
> +				    rate_millihz)) {
> +		dev_dbg(pll_clk->dev,
> +			"failed to determine rate for req->rate: %lu\n",
> +			req->rate);
> +		return -EINVAL;
> +	}
> +	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 the driver's cached state (pll_clk->cur_rate and
pll_clk->pll_parameters) inside a .determine_rate callback?

Since the Common Clock Framework expects .determine_rate to be a stateless
query, could speculative queries like clk_round_rate() corrupt the cached
parameters, causing a subsequent clk_set_rate() for a different rate to
program the hardware with the wrong values?

> +
> +	return 0;
> +}

[ ... ]

> +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 pll_clk *pll_clk = to_pll(clk_hw_get_parent(clk_hw_get_parent(hw)));

[Severity: Critical]
Does this safely cast the grandparent hardware structure to struct pll_clk?

The immediate parent of this clock is CLK_SEL_CLK_PLL3, which is a mux that
can be parented to either .loco or .pll3. If the bootloader leaves the mux
parented to the fixed .loco clock, will clk_hw_get_parent() return the
fixed clock's hw structure instead?

If so, would casting it to struct pll_clk via to_pll() and dereferencing
its fields (like pll_clk->limits below) cause out-of-bounds memory accesses
and a panic?

> +	u8 table[RZT2H_MAX_LCDC_DIV_TABLES] = { 0 };
> +	struct rzv2h_pll_div_pars dsi_params;
> +	const struct clk_div_table *div;
> +	unsigned int i = 0;
> +	u64 freq_millihz;
> +
> +	for (div = dsi_div->dtable; div->div; div++) {
> +		if (i >= RZT2H_MAX_LCDC_DIV_TABLES)
> +			return -EINVAL;
> +		table[i++] = div->div;
> +	}
> +
> +	freq_millihz = mul_u32_u32(req->rate, MILLI);
> +
> +	if (!rzv2h_cpg_get_pll_divs_pars(pll_clk->limits, &dsi_params, table,
> +					 i, freq_millihz)) {
> +		dev_err(dsi_div->dev,
> +			"LCDC divider failed to determine rate for req->rate: %lu\n",
> +			req->rate);
> +		return -EINVAL;
> +	}
> +
> +	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]
Does modifying dsi_div->divider, pll_clk->cur_rate, and
pll_clk->pll_parameters here violate the stateless design of CCF rate queries?

Similar to r9a09g077_cpg_pll3_determine_rate(), could speculative queries
corrupt the internal state of both this driver and its grandparent's PLL?

> +
> +	return 0;
> +}

[ ... ]

> +static struct clk * __init
> +r9a09g077_cpg_lcdc_div_clk_register(struct device *dev,
> +				    const struct cpg_core_clk *core,
> +				    void __iomem *addr,
> +				    struct cpg_mssr_pub *pub)
> +{
> +	struct r9a09g077_lcdc_div_clk *clk_hw_data;
> +	struct clk **clks = pub->clks;
> +	struct clk_init_data init;

[Severity: Critical]
Is it intentional that the struct clk_init_data init variable is allocated
on the stack without being zero-initialized (e.g., struct clk_init_data
init = {};)?

Will unassigned fields like parent_data and parent_hws contain random stack
garbage, causing the framework to attempt to dereference them in
devm_clk_hw_register() and resulting in a panic?

> +	const struct clk *parent;
> +	const char *parent_name;
> +	struct clk_hw *hw;
> +	int ret;

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