Re: [PATCH 4/4] 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 7 potential issue(s) to consider:
- [Critical] Uninitialized `struct clk_init_data` allocated on the stack causes kernel panics.
- [High] Invalid downcast of grandparent clock causes type confusion and memory corruption.
- [High] State mutation in `determine_rate` callbacks violates Common Clock Framework constraints.
- [High] Missing locking during read-modify-write on the highly shared `SCKCR3` register.
- [High] Potential divide-by-zero kernel panic in `r9a09g077_cpg_pll3_clk_recalc_rate`.
- [Medium] Integer truncation of requested 64-bit clock rates leads to unexpected hardware frequencies.
- [Medium] Amplified rounding error during `best_parent_rate` calculation.
--

commit 92d34dcfa10f7893860a62c602064adfd5fe8364
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 patch adds the clock definitions and PLL logic required to supply the LCDC
blocks on the RZ/T2H SoC. It introduces a new PLL clock type, implements rate
recalculation and locking sequences, and adds the corresponding muxes and
divider entries.

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

[ ... ]

> +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);
> +	unsigned int 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);
> +}

Is it possible for pdiv to be 0 if the hardware boots in an uninitialized
state?

The DIV_ROUND_CLOSEST_ULL macro will cause a kernel panic if the divisor
is zero.

> +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);

Does this implicitly cast the 64-bit req->rate (which is an unsigned long on
ARM64) to a 32-bit integer, dropping the upper bits before multiplication?

> +	if (!rzv2h_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;
> +
> +	return 0;
> +}

Is it safe to mutate the driver state (pll_clk->pll_parameters and
pll_clk->cur_rate) inside a determine_rate callback?

The common clock framework can call this via clk_round_rate for queries.
This could overwrite the active configuration without actually changing the
hardware.

[ ... ]

> +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)));

Since the grandparent clock CLK_SEL_CLK_PLL3 is a multiplexer, could the
parent hardware actually be the fixed-rate loco clock instead of a
pll_clk structure?

If so, the to_pll macro would cast a fixed-rate clock to a PLL clock,
potentially leading to out-of-bounds memory access later in this function.

> +	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);

Similar to the PLL3 callback, does this also truncate the 64-bit rate down
to 32 bits?

> +
> +	if (!rzv2h_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;

Could calculating req->best_parent_rate using the already rounded req->rate
amplify the Hz-level rounding error?

Using the exact internal rate from dsi_params.pll.freq_millihz / MILLI
might be more accurate.

> +	dsi_div->divider = dsi_params.div.divider_value;
> +	pll_clk->cur_rate = req->best_parent_rate;
> +	pll_clk->pll_parameters = dsi_params.pll;
> +
> +	return 0;
> +}

Does mutating dsi_div->divider and pll_clk properties here break queries if
the framework only requested a rate check via clk_round_rate?

> +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) {
> +			divider_found = true;
> +			break;
> +		}
> +	}
> +
> +	if (!divider_found)
> +		return -EINVAL;
> +
> +	shift = GET_SHIFT(dsi_div->conf);
> +	val = readl(dsi_div->reg);
> +	val &= ~(clk_div_mask(GET_WIDTH(dsi_div->conf)) << shift);
> +	val |= clkt->val << shift;
> +	writel(val, dsi_div->reg);
> +
> +	return 0;
> +}

Does this read-modify-write sequence need to be protected by pub->rmw_lock?

It looks like SCKCR3 is highly shared with other dividers like DIVSCI0ASYNC,
and concurrent modifications could race.

[ ... ]

> +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;

Since this is allocated on the stack without zero initialization, could
fields like parent_hws and parent_data contain uninitialized garbage?

The common clock framework might dereference these pointers during
registration before falling back to checking parent_names.

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

[ ... ]

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