Re: [PATCH v7 13/15] can: grcan: Add CANFD RX support alongside legacy CAN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Failure to advance the DMA ring read pointer on SKB allocation failure causes a CPU livelock.
- [Medium] The driver silently ignores the Bit Rate Switch (BRS) flag for received CAN FD frames.
--

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

can: grcan: Add CANFD RX support alongside legacy CAN

This commit adds CAN FD reception capabilities to the grcan driver,
allowing it to handle extended data payloads and higher bit rates.

> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 690e07c7b1ef8..eba52701c882e 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
[ ... ]
> @@ -1269,47 +1276,62 @@ static int grcan_receive(struct net_device *dev, int budget)
>  		if (rd == wr)
>  			break;
>  
> -		/* Take care of packet */
> -		skb = alloc_can_skb(dev, &cf);
> -		if (skb == NULL) {
> -			netdev_err(dev,
> -				   "dropping frame: skb allocation failed\n");
> +		hdr = grcan_msg_header_at(&dma->rx, rd);
> +		if (hdr->ctrl & GRCAN_RX_FDF)
> +			skb = alloc_canfd_skb(dev, &cf);
> +		else
> +			skb = alloc_can_skb(dev, (struct can_frame **)&cf);
> +
> +		if (unlikely(!skb)) {
> +			netdev_err(dev, "dropping frame: skb allocation failed\n");
>  			stats->rx_dropped++;
>  			continue;
>  		}

This isn't a bug introduced by this patch, but if the skb allocation fails
here under memory pressure, the loop executes a continue without advancing the
hardware read pointer (rd).

Because rd is not advanced, the next iteration of the loop will attempt to read
the exact same packet, fail allocation again, and repeat this until the NAPI
budget is exhausted. Will this cause NAPI to continuously reschedule the poll
function, leading to a CPU softirq lockup?

Should the driver calculate the number of descriptors and advance rd
appropriately before continuing?

>  
> -		hdr = grcan_msg_header_at(&dma->rx, rd);
> +		dlc = FIELD_GET(GRCAN_MSG_DLC_MASK, hdr->ctrl);
> +		if (hdr->ctrl & GRCAN_RX_FDF)
> +			cf->len = can_fd_dlc2len(dlc);
> +		else
> +			cf->len = can_cc_dlc2len(dlc);
> +

The patch defines GRCAN_RX_BRS as BIT(25) to indicate that the hardware
received a CAN FD frame with Bit Rate Switch enabled, but the receive path
does not appear to check this bit.

If hdr->ctrl & GRCAN_RX_BRS is set, does the driver need to set the CANFD_BRS
bit in cf->flags before passing the frame to the network stack?

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