Re: [PATCH v7 11/15] can: grcan: Refactor GRCAN DMA buffer to use structured memory layout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unnecessary __packed attribute on struct grcan_msg_header causes inefficient and non-atomic DMA descriptor accesses.
- [High] Replacing word-wise bit shifts with memcpy() for CAN payload breaks endianness on little-endian architectures.
- [High] Pre-existing NAPI livelock when alloc_can_skb() fails under memory pressure.
--

commit be98321b7518fa6d89c56b12cd7c4501bba55c9b
Author: Arun Muthusamy <arun.muthusamy@gaisler.com>

can: grcan: Refactor GRCAN DMA buffer to use structured memory layout

This commit refactors the GRCAN DMA buffer handling to use structured memory
layouts instead of array indices. It introduces struct grcan_msg_header and
updates the transmit and receive functions to use memcpy for payload handling.

> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index b5527fd9e02e7..4921da349a0ef 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c
> @@ -242,9 +239,23 @@ struct grcan_hwcap {
>  	bool fd;
>  };
>  
> +struct grcan_msg_header {
> +	u32 id;
> +	u32 ctrl;
> +	u8 data[CAN_MAX_DLEN];
> +} __packed;

Since this structure is already naturally aligned to 16 bytes, is the __packed
attribute necessary? On strict-alignment architectures like SPARC, which are
typically used with Aeroflex Gaisler cores, this attribute might force the
compiler to synthesize non-atomic byte-by-byte memory accesses (ldub/stb) for
the 32-bit id and ctrl fields instead of atomic 32-bit instructions.

Could this disrupt the atomicity of descriptor reads and writes, leading the
GRCAN DMA hardware to observe partially updated descriptors?

[ ... ]

> @@ -1239,30 +1256,33 @@ static int grcan_receive(struct net_device *dev, int budget)
>  			continue;
>  		}

This is a pre-existing issue, but if alloc_can_skb() returns NULL due to memory
pressure, the code uses continue and skips the remainder of the loop.

Does this bypass the rd = grcan_ring_add(...) call at the end of the loop?
If the read pointer rd is never advanced, won't the loop process the same
packet repeatedly until budget is exhausted, potentially leading to a NAPI
livelock?

>  
> -		slot = dma->rx.buf + rd;
> -		eff = slot[0] & GRCAN_MSG_IDE;
> -		rtr = slot[0] & GRCAN_MSG_RTR;
> +		hdr = grcan_msg_header_at(&dma->rx, rd);
> +
> +		eff = hdr->id & GRCAN_MSG_IDE;
> +		rtr = hdr->id & GRCAN_MSG_RTR;
> +
>  		if (eff) {
> -			cf->can_id = ((slot[0] & GRCAN_MSG_EID)
> +			cf->can_id = ((hdr->id & GRCAN_MSG_EID)
>  				      >> GRCAN_MSG_EID_BIT);
>  			cf->can_id |= CAN_EFF_FLAG;
>  		} else {
> -			cf->can_id = ((slot[0] & GRCAN_MSG_BID)
> +			cf->can_id = ((hdr->id & GRCAN_MSG_BID)
>  				      >> GRCAN_MSG_BID_BIT);
>  		}
> -		cf->len = can_cc_dlc2len((slot[1] & GRCAN_MSG_DLC)
> -					  >> GRCAN_MSG_DLC_BIT);
> +
> +		cf->len = can_cc_dlc2len((hdr->ctrl & GRCAN_MSG_DLC)
> +					 >> GRCAN_MSG_DLC_BIT);
> +
>  		if (rtr) {
>  			cf->can_id |= CAN_RTR_FLAG;
>  		} else {
> -			for (i = 0; i < cf->len; i++) {
> -				j = GRCAN_MSG_DATA_SLOT_INDEX(i);
> -				shift = GRCAN_MSG_DATA_SHIFT(i);
> -				cf->data[i] = (u8)(slot[j] >> shift);
> -			}
> +			if (cf->len > 0)
> +				memcpy(cf->data, hdr->data,
> +				       min_t(u32, cf->len, CAN_MAX_DLEN));

The original code explicitly extracted payload bytes from 32-bit words using
bitwise shifts, which placed the bytes correctly regardless of the host
architecture's endianness.

Will replacing this with a byte-wise memcpy() break the payload memory layout
on little-endian systems, causing the CAN hardware to receive byte-swapped
corrupted payloads?

>  
>  			stats->rx_bytes += cf->len;
>  		}

[ ... ]

> @@ -1447,16 +1468,13 @@ static netdev_tx_t grcan_start_xmit(struct sk_buff *skb,
>  		tmp = (id << GRCAN_MSG_EID_BIT) & GRCAN_MSG_EID;
>  	else
>  		tmp = (id << GRCAN_MSG_BID_BIT) & GRCAN_MSG_BID;
> -	slot[0] = (eff ? GRCAN_MSG_IDE : 0) | (rtr ? GRCAN_MSG_RTR : 0) | tmp;
> -
> -	slot[1] = ((dlc << GRCAN_MSG_DLC_BIT) & GRCAN_MSG_DLC);
> -	slot[2] = 0;
> -	slot[3] = 0;
> -	for (i = 0; i < dlc; i++) {
> -		j = GRCAN_MSG_DATA_SLOT_INDEX(i);
> -		shift = GRCAN_MSG_DATA_SHIFT(i);
> -		slot[j] |= cf->data[i] << shift;
> -	}
> +
> +	hdr->id = (eff ? GRCAN_MSG_IDE : 0) | (rtr ? GRCAN_MSG_RTR : 0) | tmp;
> +
> +	hdr->ctrl = ((dlc << GRCAN_MSG_DLC_BIT) & GRCAN_MSG_DLC);
> +
> +	if (dlc > 0)
> +		memcpy(hdr->data, cf->data, min_t(u32, cf->len, CAN_MAX_DLEN));

Similarly, does changing this transmit packing from explicit bitwise shifts
to memcpy() risk transmitting byte-swapped payloads on little-endian
architectures?

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