Re: [PATCH 6/8] can: rcar_canfd: Add support for Renesas RZ/G3S

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing enforcement of CAN-FD mode when hardware does not support classical CAN mode.
- [High] The variable `fcan_freq` is used uninitialized when clock fallback branches fail.
--

--- Patch [6]: [PATCH 6/8] can: rcar_canfd: Add support for Renesas RZ/G3S ---
commit cd9e80c9be5a9e3af3e5266e74d95204fb8cf5b3
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

    can: rcar_canfd: Add support for Renesas RZ/G3S

This commit adds CAN-FD support for the Renesas RZ/G3S controller. It
configures the hardware info struct to reflect the controller's capabilities,
including its lack of support for classical CAN mode and its internal clock
divider.

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 7f01afe60a4c2..6d6b2d999b782 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -661,6 +661,25 @@ static const struct rcar_canfd_hw_info rzg2l_hw_info = {
[ ... ]
> +static const struct rcar_canfd_hw_info r9a08g045_hw_info = {
> +	.nom_bittiming = &rcar_canfd_gen4_nom_bittiming_const,
> +	.data_bittiming = &rcar_canfd_gen4_data_bittiming_const,
> +	.tdc_const = &rcar_canfd_gen4_tdc_const,
> +	.regs = &rcar_gen4_regs,
> +	.sh = &rcar_gen4_shift_data,
> +	.rnc_field_width = 16,
> +	.max_aflpn = 127,
> +	.max_cftml = 31,
> +	.max_channels = 2,
> +	.postdiv = 2,
> +	.multi_channel_irqs = 1,
> +	.ch_interface_mode = 1,
> +	.shared_can_regs = 1,
> +	.external_clk = 1,
> +	.fcan_pclk = 1,
> +	.mode_select_na = 1,

[Severity: High]
Since this hardware sets .mode_select_na = 1 because it does not support
classical CAN mode, could there be a mismatch if the device tree contains the
"renesas,no-can-fd" property?

In rcar_canfd_probe(), we set the software mode to classical CAN if this
property is present:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_probe() {
	if (of_property_read_bool(dev->of_node, "renesas,no-can-fd"))
		fdmode = false;			/* Classical CAN only mode */
    ...
}

Since the hardware configuration bits are bypassed by .mode_select_na = 1,
would this leave the hardware in CAN-FD mode while the software driver allocates
MTU and payload sizes for classical CAN frames?

[Severity: High]
While reviewing the clock configuration related to .postdiv = 2 and
.fcan_pclk = 1, I noticed a potential regression in the clock fallback logic
that might affect older platforms.

In rcar_canfd_probe(), if "can_clk" and "canfd" clocks are missing from the
device tree, devm_clk_get_optional() returns NULL instead of an error, which
skips the error path but leaves gpriv->can_clk as NULL:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_probe() {
	...
	gpriv->can_clk = devm_clk_get_optional(dev, "canfd");
	if (IS_ERR(gpriv->can_clk))
		return dev_err_probe(...);

	if (gpriv->can_clk)
		fcan_freq = clk_get_rate(gpriv->can_clk);
	else if (info->fcan_pclk)
		fcan_freq = clk_get_rate(gpriv->clkp);

	/* CANFD clock may be further divided within the IP */
	fcan_freq /= info->postdiv;
	...
}

If info->fcan_pclk is 0 (which is the case for Gen3/Gen4 platforms), does
this mean fcan_freq is used uninitialized when calculating the CAN bit timing?
Can this lead to unstable or non-functional CAN buses on those platforms?

> +};
> +

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