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

Marc Kleine-Budde
2026-01-23T14:12:31Z

[-- Attachment #1: Type: text/plain, Size: 4540 bytes --]

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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]