Re: [PATCH v7 14/15] can: grcan: Update echo skb handling to match variable length CANFD frame

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Flawed queue space check artificially limits TX queue depth to 1 packet.
- [High] Decoupled echo SKB tracking corrupts TX completions and leaks SKBs when multiple frames are batched.
- [Medium] Data race on echo SKB indices between transmission and completion contexts.
--

commit 0d1b72a766dc2d38f5550398f1494cc94c0dd17e
Author: Arun Muthusamy <arun.muthusamy@gaisler.com>
Subject: can: grcan: Update echo skb handling to match variable length CANFD frame

The commit refactors echo socket buffer management in the grcan driver to
support variable length CANFD frames. It introduces echo_skb_idx and
next_echo_idx to decouple the echo skb index from the ring buffer slots, and
adjusts the echo skb allocation based on the number of slots.

[ ... ]

> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index eba52701c882e..06ef081970a24 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c

[ ... ]

> @@ -604,19 +613,22 @@ static int catch_up_echo_skb(struct net_device *dev, int budget, bool echo)
>  	for (work_done = 0; work_done < budget || budget < 0; work_done++) {
>  		if (priv->eskbp == txrd)
>  			break;
>  
>  		priv->eskbp = grcan_ring_add(priv->eskbp, GRCAN_MSG_SIZE,
>  					     dma->tx.size);
>  		txrd = grcan_read_reg(&regs->txrd);
> +
> +		/* Grab the packet once the packet is send or free untransmitted packet */
> +		if (priv->eskbp == txrd) {
> +			if (echo) {
> +				/* Normal echo of messages */
> +				stats->tx_packets++;
> +				stats->tx_bytes += can_get_echo_skb(dev, priv->echo_skb_idx, NULL);

If multiple frames complete at the same time, the hardware will advance txrd
by multiple slots. The condition if (priv->eskbp == txrd) will only be true
for the last frame, skipping any intermediate frames. Does this logic miss
tracking and leak the skbs for the intermediate frames?

Additionally, priv->echo_skb_idx is a single scalar value updated in
grcan_start_xmit() to point to the most recently queued packet. If multiple
packets are in flight, wouldn't this echo the newest packet repeatedly
instead of the oldest, breaking FIFO ordering and leaking the earlier ones?

Finally, priv->echo_skb_idx is read here in the completion context, but it
is modified in grcan_start_xmit() outside of any lock. Is a lock required
here to prevent reading a stale or partially updated index?

> +			} else {
> +				/* For cleanup of untransmitted messages */
> +				can_free_echo_skb(dev, priv->echo_skb_idx, NULL);
> +			}
> +		}
>  	}
>  	return work_done;
>  }

[ ... ]

> @@ -1504,11 +1519,9 @@ static netdev_tx_t grcan_start_xmit(struct sk_buff *skb,
>  
>  	txwr = grcan_read_reg(&regs->txwr);
>  	space = grcan_txspace(dma->tx.size, txwr, priv->eskbp);
> -
> -	slotindex = txwr / GRCAN_MSG_SIZE;
>  	bds = grcan_numbds(len);
>  
> -	if (unlikely(space < bds)) {
> +	if (unlikely(space < bds || priv->can.echo_skb[priv->echo_skb_idx])) {

At this point in the function, priv->echo_skb_idx still holds the index of
the previously queued packet (it is only updated to priv->next_echo_idx at
the end of the function).

If the previous packet is still in flight, its slot in priv->can.echo_skb
is occupied. Does this cause the condition to evaluate to true, returning
NETDEV_TX_BUSY and artificially limiting the transmission queue depth to a
single packet?

>  		netif_stop_queue(dev);
>  		spin_unlock_irqrestore(&priv->lock, flags);
>  		return NETDEV_TX_BUSY;

[ ... ]

> @@ -1588,7 +1601,13 @@ static netdev_tx_t grcan_start_xmit(struct sk_buff *skb,
>  	 * can_put_echo_skb would be an error unless other measures are
>  	 * taken.
>  	 */
> -	can_put_echo_skb(skb, dev, slotindex, 0);
> +
> +	priv->echo_skb_idx = priv->next_echo_idx;
> +
> +	can_put_echo_skb(skb, dev, priv->next_echo_idx, 0);
> +
> +	/* Move to the next index in the echo skb buffer */
> +	priv->next_echo_idx = (priv->next_echo_idx + 1) % priv->can.echo_skb_max;

These variables are updated here outside of the priv->lock critical section
that was unlocked earlier in the function. Since they are read concurrently
in catch_up_echo_skb() during softirq, does this introduce a data race?

>  	/* Make sure everything is written before allowing hardware to
>  	 * read from the memory
>  	 */
>  	wmb();

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