Re: [PATCH v7 15/15] can: grcan: Advertise CANFD capability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing CAN FD Data Phase hardware configuration.
- [High] Advertised data bit timing constraints exceed hardware register limits.
- [High] Socket buffer allocation failures permanently stall the NAPI receive queue (infinite loop).
- [Medium] The CANFD_BRS flag is lost on received CAN FD frames.
- [Medium] Unconditional exposure of CAN FD data bit timing to userspace for non-FD hardware.
--

commit 73799692e455f9bd1a3d6243e60de78467c0a58b
Author: Arun Muthusamy <arun.muthusamy@gaisler.com>

can: grcan: Advertise CANFD capability

Advertise CANFD capability for GRCANFD hardware by enabling
CAN_CTRLMODE_FD flag and providing CANFD timing operations.

> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 06ef081970a24..060031df81924 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -1766,11 +1766,14 @@ static int grcan_setup_netdev(struct platform_device *ofdev,
>  	priv->ofdev_dev = &ofdev->dev;
>  	priv->regs = base;
>  	priv->can.bittiming_const = hwcap->bt_const;
> +	priv->can.fd.data_bittiming_const = hwcap->bt_const;

Are the data bit timing constraints advertised here too large for the
hardware registers? It looks like hwcap->bt_const (which sets
tseg1_max = 63) is assigned, but the hardware macro for the Data Bit
Timing Register (GRCANFD_FDBTR_PS1) defines only a 4-bit field (max value
15). Could this result in bitwise truncation and corrupted baud rates if the
register is programmed?

Also, is it safe to assign data_bittiming_const unconditionally, even for
hardware that does not support CAN FD? Since hwcap->fd is not checked
before this assignment, this seems to unconditionally expose
IFLA_CAN_DATA_BITTIMING_CONST to userspace tools for classic CAN controllers.

>  	priv->can.do_set_mode = grcan_set_mode;
>  	priv->can.do_get_berr_counter = grcan_get_berr_counter;
>  	priv->can.clock.freq = ambafreq;
>  	priv->can.ctrlmode_supported =
>  		CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_ONE_SHOT;
> +	if (hwcap->fd)
> +		priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;

With CAN FD being enabled here, there appear to be a few issues in other
parts of the code handling the FD functionality.

First, does the hardware timing configuration callback grcanfd_set_bittiming()
omit programming the CAN FD data phase? It seems to only read the nominal
bit timing (priv->can.bittiming) and write it to the Nominal Bit Timing
Register (regs->nbtr), completely ignoring priv->can.fd.data_bittiming
and never programming the Fast/Data Bit Timing Register (regs->fdbtr).
Will this cause immediate communication failures on the bus when transmitting
CAN FD frames with Bit Rate Switching (BRS)?

Second, is the CANFD_BRS flag lost on received CAN FD frames?
In grcan_receive(), when a CAN FD frame is received:

    hdr = grcan_msg_header_at(&dma->rx, rd);
    if (hdr->ctrl & GRCAN_RX_FDF)
        skb = alloc_canfd_skb(dev, &cf);

It doesn't look like the hardware's BRS bit (GRCAN_RX_BRS, BIT(25)) is
ever checked to set cf->flags |= CANFD_BRS. Does this mean userspace is
incorrectly informed that no received frames used bit-rate switching?

Finally, this code exists in grcan_receive():

    if (unlikely(!skb)) {
        netdev_err(dev, "dropping frame: skb allocation failed\n");
        stats->rx_dropped++;
        continue;
    }

If alloc_can_skb() or alloc_canfd_skb() fails, the code continues to the
next loop iteration. However, because rd is not advanced (via
grcan_ring_add()), won't the next iteration read the exact same message,
fail allocation again, and repeat until the NAPI budget is exhausted? Could
this permanently stall the receive ring and cause a NAPI livelock under
memory pressure?

>  	priv->need_txbug_workaround = txbug;
>  	priv->hwcap = hwcap;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508070121.6918-1-arun.muthusamy@gaisler.com?part=15