Re: [PATCH 4/8] can: rcar_canfd: Allow the CAN FD clock to be sourced from fck

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Uninitialized variable usage when 'canfd' clock is missing on platforms where 'fcan_pclk' is 0.
--

--- Patch [4]: [PATCH 4/8] can: rcar_canfd: Allow the CAN FD clock to be sourced from fck ---
commit 0fffa3e660d5596983dd22ee45ce4eb746f70520
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

can: rcar_canfd: Allow the CAN FD clock to be sourced from fck

This commit adjusts the CAN FD clock handling in the R-Car CAN FD driver to
allow sourcing the clock from the peripheral clock (fck) for the RZ/G3S
platform, changing the "canfd" clock lookup to be optional.

Link: https://lore.kernel.org/all/1d9719e3-10ff-4cd8-b729-55fea93c37ce@wanadoo.fr

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index b40ae5998c1da..6ec2d7a031238 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c

[ ... ]

> @@ -2190,13 +2196,18 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	 */
>  	gpriv->can_clk = devm_clk_get(dev, "can_clk");
>  	if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {
> -		gpriv->can_clk = devm_clk_get(dev, "canfd");
> +		gpriv->can_clk = devm_clk_get_optional(dev, "canfd");
>  		if (IS_ERR(gpriv->can_clk))
>  			return dev_err_probe(dev, PTR_ERR(gpriv->can_clk),
>  					     "cannot get canfd clock\n");
>  
> +		if (gpriv->can_clk)
> +			fcan_freq = clk_get_rate(gpriv->can_clk);
> +		else if (info->fcan_pclk)
> +			fcan_freq = clk_get_rate(gpriv->clkp);
> +

[Severity: High]
Does this code leave fcan_freq uninitialized on legacy platforms if the device
tree is missing the "canfd" clock?

Because devm_clk_get_optional() returns NULL when the clock is missing,
gpriv->can_clk will be NULL. On platforms where info->fcan_pclk is 0, both
conditions in this if/else block evaluate to false.

>  		/* CANFD clock may be further divided within the IP */
> -		fcan_freq = clk_get_rate(gpriv->can_clk) / info->postdiv;
> +		fcan_freq /= info->postdiv;

If fcan_freq is uninitialized here, it will divide random stack memory and pass
that downstream to configure CAN bus bit timings. Should there be an error
returned if neither clock source is available?

>  	} else {
>  		fcan_freq = clk_get_rate(gpriv->can_clk);
>  		gpriv->extclk = gpriv->info->external_clk;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707102418.1646159-1-claudiu.beznea+renesas@tuxon.dev?part=4