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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260123-sepia-marmoset-of-luxury-ad6d1f-mkl@pengutronix.de>
Date: Fri, 23 Jan 2026 15:12:22 +0100
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Arun Muthusamy <arun.muthusamy@...sler.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	mailhol@...nel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-can@...r.kernel.org
Subject: Re: [PATCH v3 15/15] can: grcan: Update echo skb handling to match
 variable length CANFD frame

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@...sler.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   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)