Re: [PATCH v5 12/16] can: grcan: Refactor GRCAN DMA buffer to use structured memory layout

[Vincent Mailhol <mailhol@kernel.org>]

On 16/02/2026 at 14:53, Arun Muthusamy wrote:
>  Introduce a structured layout that accurately represents the hardware memory by using struct grcan_msg_slot.
>  This structure encapsulates the message parameters, including id, dlc, and data.
> 
> Signed-off-by: Arun Muthusamy <arun.muthusamy@gaisler.com>
> ---
>  drivers/net/can/grcan.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 3104946071dd..28fa219e1c3b 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -174,6 +174,7 @@ struct grcan_registers {
>  #define GRCAN_IRQ_DEFAULT (GRCAN_IRQ_RX | GRCAN_IRQ_TX | GRCAN_IRQ_ERRORS)
>  
>  #define GRCAN_MSG_SIZE		16
> +#define GRCAN_CLASSIC_DATA_SIZE 8
>  
>  #define GRCAN_MSG_IDE		0x80000000
>  #define GRCAN_MSG_RTR		0x40000000
> @@ -239,9 +240,23 @@ struct grcan_hwcap {
>  	bool fd;
>  };
>  
> +union grcan_msg_slot {
> +	/*  First slot: header + 8 bytes payload */
> +	struct {
> +		u32 id;
> +		u32 ctrl;
> +		u8  data[GRCAN_CLASSIC_DATA_SIZE];
                         ^^^^^^^^^^^^^^^^^^^^^^^

Remove this GRCAN_CLASSIC_DATA_SIZE macro and instead use CAN_MAX_DLEN.

> +	} __packed header;
> +
> +	/* Continuation slot: payload only */
> +	struct {
> +		u8 data[GRCAN_MSG_SIZE];
> +	} __packed frags;
> +} __packed;
> +
>  struct grcan_dma_buffer {
>  	size_t size;
> -	void *buf;
> +	union grcan_msg_slot *msg_slot;
>  	dma_addr_t handle;
>  };

Nitpick: you can use an anonymous union:

	/*  First slot: header + 8 bytes payload */
	struct grcan_msg_header {
		u32 id;
		u32 ctrl;
		u8  data[CAN_MAX_DLEN];
	} __packed;

	/* Continuation slot: payload only */
	struct grcan_msg_fragment {
		u8 data[GRCAN_MSG_SIZE];
	} __packed;
	
	struct grcan_dma_buffer {
		size_t size;
		void *buf;
		union {
			struct grcan_msg_header *header;
			struct grcan_msg_fragment *frag;
		};
		dma_addr_t handle;
	};

With this, you can do:

	dma->rx->header
	dma->rx->frag

instead of:

	dma->rx.msg_slot->header
	dma->rx.msg_slot->frag

> @@ -1019,8 +1034,8 @@ static int grcan_allocate_dma_buffers(struct net_device *dev,
>  	small->handle = large->handle + lsize;
>  	shift = large->handle - dma->base_handle;
>  
> -	large->buf = dma->base_buf + shift;
> -	small->buf = large->buf + lsize;
> +	large->msg_slot = (union grcan_msg_slot *)((u8 *)dma->base_buf + shift);
> +	small->msg_slot = (union grcan_msg_slot *)((u8 *)large->msg_slot + lsize);
>  
>  	return 0;
>  }
> @@ -1237,7 +1252,7 @@ static int grcan_receive(struct net_device *dev, int budget)
>  			continue;
>  		}
>  
> -		slot = dma->rx.buf + rd;
> +		slot = (u32 *)((u8 *)dma->rx.msg_slot + rd);
>  		eff = slot[0] & GRCAN_MSG_IDE;
>  		rtr = slot[0] & GRCAN_MSG_RTR;
>  		if (eff) {
> @@ -1418,7 +1433,7 @@ static netdev_tx_t grcan_start_xmit(struct sk_buff *skb,
>  	space = grcan_txspace(dma->tx.size, txwr, priv->eskbp);
>  
>  	slotindex = txwr / GRCAN_MSG_SIZE;
> -	slot = dma->tx.buf + txwr;
> +	slot = (u32 *)((u8 *)dma->tx.msg_slot + txwr);

As told in my previous message, refactor the code in this commit.

>  	if (unlikely(space == 1))
>  		netif_stop_queue(dev);


Yours sincerely,
Vincent Mailhol