Re: [PATCH v4 4/7] can: rcar_canfd: Do not set registers selecting the CAN mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Software state diverges from fixed hardware capabilities due to unvalidated device tree overrides, causing silent data corruption and CAN protocol violations.

Pre-existing issues:
- [High] Race condition on write-0-to-clear status register causes lost TX completion interrupts and permanently frozen TX queues.
--

commit f5c5dd93cfcd4bdec86a8b0c4bc20cba21d7f4dc
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

can: rcar_canfd: Do not set registers selecting the CAN mode

This commit adds a fixed_canfd flag to struct rcar_canfd_hw_info to handle
hardware like the Renesas RZ/G3S SoC that only supports CAN-FD mode. It skips
writing to unavailable CAN mode configuration bits during the reset controller
sequence.

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 2e84dc5dfa34d..bed7cde1aa01b 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c

[ ... ]

> @@ -846,6 +852,9 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
>  		}
>  
>  		/* Set the controller into appropriate mode */
> +		if (gpriv->info->fixed_canfd)
> +			continue;
> +

[Severity: High]
Does bypassing the hardware mode setup here create a state mismatch if
the software interface is forced into Classical CAN mode?

If the device tree contains the "renesas,no-can-fd" property, the probe
function sets fdmode = false in rcar_canfd_probe():

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 */
    ...
}

With this patch, if a device with fixed_canfd uses that DT override, the
hardware remains fixed in CAN-FD mode but the software expects Classical CAN.
Could this cause the driver to silently truncate 64-byte CAN-FD frames to
8 bytes while acknowledging CAN-FD frames on the bus?

>  		if (gpriv->info->ch_interface_mode) {
>  			/* Do not set CLOE and FDOE simultaneously */
>  			if (!gpriv->fdmode) {

[ ... ]

This isn't a bug introduced by this patch, but I also noticed a pre-existing
issue in the transmit completion path:

[Severity: High]
Can writing back the stale status mask unintentionally clear newly asserted
interrupt flags?

In rcar_canfd_tx_done():

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_tx_done() {
    ...
	/* Clear interrupt */
	rcar_canfd_write(priv->base, RCANFD_CFSTS(gpriv, ch, RCANFD_CFFIFO_IDX),
			 sts & ~RCANFD_CFSTS_CFTXIF);
    ...
}

If a new interrupt (like another TX completion or message lost flag) is
asserted by the hardware immediately after the status register is read but
before this write-back, writing the stale sts value with a bitwise AND to
a write-0-to-clear register will write '0' to the newly asserted flag.

Could this cause lost TX completion interrupts and permanently frozen
TX queues?

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