[v3] can: grcan: Enhance driver with CANFD Support and Improvemens

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

Posted by Arun Muthusamy 2 weeks, 4 days ago

Refactor echo socket buffer management by introducing dedicated indices
for current and next echo slots.

- Introduce "echo_skb_idx" to keep track of the current packet index in
  the echo buffer, and "next_echo_idx" for the next available slot.
- Adjust memory allocation for echo skb to calculate the number of slots
  based on slot size.
- Enhance logic in catch_up_echo_skb() to correctly process and free echo
  skbs.
- Initialize "next_echo_idx" in grcan_set_mode() to ensure proper starting
  conditions when the device enters proper modes.
- Improve memory and index handling in grcan_start_xmit() and
  added a check to stop the network queue when necessary.

Signed-off-by: Arun Muthusamy <arun.muthusamy@gaisler.com>
---
 drivers/net/can/grcan.c | 51 ++++++++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 13 deletions(-)

diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
index 39afb12c50d0..22c86e5d7002 100644
--- a/drivers/net/can/grcan.c
+++ b/drivers/net/can/grcan.c
@@ -298,6 +298,15 @@ struct grcan_priv {
 
 	struct sk_buff **echo_skb;	/* We allocate this on our own */
 
+	/*
+	 * Since the CAN FD frame has a variable length, this variable is used
+	 * to keep track of the index of the CAN echo skb (socket buffer) frame.
+	 */
+	u32 echo_skb_idx;
+
+	/* Next echo skb free slot index */
+	u32 next_echo_idx;
+
 	/* The echo skb pointer, pointing into echo_skb and indicating which
 	 * frames can be echoed back. See the "Notes on the tx cyclic buffer
 	 * handling"-comment for grcan_start_xmit for more details.
@@ -578,7 +587,7 @@ static int catch_up_echo_skb(struct net_device *dev, int budget, bool echo)
 	struct grcan_registers __iomem *regs = priv->regs;
 	struct grcan_dma *dma = &priv->dma;
 	struct net_device_stats *stats = &dev->stats;
-	int i, work_done;
+	int work_done;
 
 	/* Updates to priv->eskbp and wake-ups of the queue needs to
 	 * be atomic towards the reads of priv->eskbp and shut-downs
@@ -589,19 +598,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;
-		i = priv->eskbp / GRCAN_MSG_SIZE;
-		if (echo) {
-			/* Normal echo of messages */
-			stats->tx_packets++;
-			stats->tx_bytes += can_get_echo_skb(dev, i, NULL);
-		} else {
-			/* For cleanup of untransmitted messages */
-			can_free_echo_skb(dev, i, NULL);
-		}
 
 		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);
+			} else {
+				/* For cleanup of untransmitted messages */
+				can_free_echo_skb(dev, priv->echo_skb_idx, NULL);
+			}
+		}
 	}
 	return work_done;
 }
@@ -1109,6 +1121,7 @@ static int grcan_set_mode(struct net_device *dev, enum can_mode mode)
 			if (!(priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY))
 				netif_wake_queue(dev);
 		}
+		priv->next_echo_idx = 0;
 		spin_unlock_irqrestore(&priv->lock, flags);
 		return err;
 	}
@@ -1120,6 +1133,7 @@ static int grcan_open(struct net_device *dev)
 	struct grcan_priv *priv = netdev_priv(dev);
 	struct grcan_dma *dma = &priv->dma;
 	unsigned long flags;
+	u32 nr_echo_slots;
 	int err;
 
 	/* Allocate memory */
@@ -1130,13 +1144,15 @@ static int grcan_open(struct net_device *dev)
 		return err;
 	}
 
-	priv->echo_skb = kcalloc(dma->tx.size, sizeof(*priv->echo_skb),
+	nr_echo_slots =  dma->tx.size / GRCAN_MSG_SIZE;
+
+	priv->echo_skb = kcalloc(nr_echo_slots, sizeof(*priv->echo_skb),
 				 GFP_KERNEL);
 	if (!priv->echo_skb) {
 		err = -ENOMEM;
 		goto exit_free_dma_buffers;
 	}
-	priv->can.echo_skb_max = dma->tx.size;
+	priv->can.echo_skb_max = nr_echo_slots;
 	priv->can.echo_skb = priv->echo_skb;
 
 	/* Get can device up */
@@ -1575,7 +1591,16 @@ 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;
+
+	if (priv->can.echo_skb[priv->echo_skb_idx])
+		netif_stop_queue(dev);
 
 	/* Make sure everything is written before allowing hardware to
 	 * read from the memory
-- 
2.51.0

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

Posted by Marc Kleine-Budde 2 weeks, 3 days ago

On 22.01.2026 13:10:38, Arun Muthusamy wrote:
[...]

> @@ -1575,7 +1591,16 @@ 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;
> +
> +	if (priv->can.echo_skb[priv->echo_skb_idx])
> +		netif_stop_queue(dev);

You also use "if (unlikely(space == 1)) netif_stop_queue(dev);", that
looks suspicious. Why have 2 independent ways to check if the TX queue
is full?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Posted by Arun Muthusamy 1 week, 5 days ago

On 1/23/26 15:17, Marc Kleine-Budde wrote:
> On 22.01.2026 13:10:38, Arun Muthusamy wrote:
> [...]
>
>> @@ -1575,7 +1591,16 @@ 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;
>> +
>> +	if (priv->can.echo_skb[priv->echo_skb_idx])
>> +		netif_stop_queue(dev);
> You also use "if (unlikely(space == 1)) netif_stop_queue(dev);", that
> looks suspicious. Why have 2 independent ways to check if the TX queue
> is full?

That's correct.
Current implementation introduces two independent checks where space reflects hardware TX descriptor availability and echo slot availability.
I’ll rework this to use a single check, covering both TX ring space and echo slot availability.

-- 
-- 
BR,

Arun Muthusamy
Software Engineer
Frontgrade Gaisler
T : +46 (0) 700 558 528
arun.muthusamy@gaisler.com

Frontgrade Gaisler AB, Kungsgatan 12, SE-411 19 GÖTEBORG, Sweden.
+46 (0) 31 775 8650, www.gaisler.com

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

Posted by Marc Kleine-Budde 2 weeks, 3 days ago

On 22.01.2026 13:10:38, Arun Muthusamy wrote:
> Refactor echo socket buffer management by introducing dedicated indices
> for current and next echo slots.
>
> - Introduce "echo_skb_idx" to keep track of the current packet index in
>   the echo buffer, and "next_echo_idx" for the next available slot.
> - Adjust memory allocation for echo skb to calculate the number of slots
>   based on slot size.
> - Enhance logic in catch_up_echo_skb() to correctly process and free echo
>   skbs.
> - Initialize "next_echo_idx" in grcan_set_mode() to ensure proper starting
>   conditions when the device enters proper modes.
> - Improve memory and index handling in grcan_start_xmit() and
>   added a check to stop the network queue when necessary.

consider using
netif_subqueue_maybe_stop()/netif_subqueue_completed_wake() from
netdev_queues.h

> Signed-off-by: Arun Muthusamy <arun.muthusamy@gaisler.com>
> ---
>  drivers/net/can/grcan.c | 51 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 39afb12c50d0..22c86e5d7002 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -298,6 +298,15 @@ struct grcan_priv {
>
>  	struct sk_buff **echo_skb;	/* We allocate this on our own */
>
> +	/*
> +	 * Since the CAN FD frame has a variable length, this variable is used
> +	 * to keep track of the index of the CAN echo skb (socket buffer) frame.
> +	 */
> +	u32 echo_skb_idx;
> +
> +	/* Next echo skb free slot index */
> +	u32 next_echo_idx;
> +
>  	/* The echo skb pointer, pointing into echo_skb and indicating which
>  	 * frames can be echoed back. See the "Notes on the tx cyclic buffer
>  	 * handling"-comment for grcan_start_xmit for more details.
> @@ -578,7 +587,7 @@ static int catch_up_echo_skb(struct net_device *dev, int budget, bool echo)
>  	struct grcan_registers __iomem *regs = priv->regs;
>  	struct grcan_dma *dma = &priv->dma;
>  	struct net_device_stats *stats = &dev->stats;
> -	int i, work_done;
> +	int work_done;
>
>  	/* Updates to priv->eskbp and wake-ups of the queue needs to
>  	 * be atomic towards the reads of priv->eskbp and shut-downs
> @@ -589,19 +598,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;
> -		i = priv->eskbp / GRCAN_MSG_SIZE;
> -		if (echo) {
> -			/* Normal echo of messages */
> -			stats->tx_packets++;
> -			stats->tx_bytes += can_get_echo_skb(dev, i, NULL);
> -		} else {
> -			/* For cleanup of untransmitted messages */
> -			can_free_echo_skb(dev, i, NULL);
> -		}
>
>  		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*/
                                           ^^
one space is enough

> +		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);
> +			} else {
> +				/* For cleanup of untransmitted messages */
> +				can_free_echo_skb(dev, priv->echo_skb_idx, NULL);
> +			}
> +		}
>  	}
>  	return work_done;
>  }
> @@ -1109,6 +1121,7 @@ static int grcan_set_mode(struct net_device *dev, enum can_mode mode)
>  			if (!(priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY))
>  				netif_wake_queue(dev);
>  		}
> +		priv->next_echo_idx = 0;
>  		spin_unlock_irqrestore(&priv->lock, flags);
>  		return err;
>  	}
> @@ -1120,6 +1133,7 @@ static int grcan_open(struct net_device *dev)
>  	struct grcan_priv *priv = netdev_priv(dev);
>  	struct grcan_dma *dma = &priv->dma;
>  	unsigned long flags;
> +	u32 nr_echo_slots;
>  	int err;
>
>  	/* Allocate memory */
> @@ -1130,13 +1144,15 @@ static int grcan_open(struct net_device *dev)
>  		return err;
>  	}
>
> -	priv->echo_skb = kcalloc(dma->tx.size, sizeof(*priv->echo_skb),
> +	nr_echo_slots =  dma->tx.size / GRCAN_MSG_SIZE;
                       ^^
one space is enough

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |