Re: [PATCH v7 12/15] can: grcan: Add CANFD TX support alongside legacy CAN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch breaks the `grcan_txbug_workaround` mitigation for a known hardware hang, causing it to bypass the trigger condition or unconditionally time out.
--

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

can: grcan: Add CANFD TX support alongside legacy CAN

This commit includes CANFD TX support with the legacy CAN support, enabling
extended data payloads to provide higher bit rates.

> diff --git a/drivers/net/can/grcan.c b/drivers/net/can/grcan.c
> index 4921da349a0ef..690e07c7b1ef8 100644
> --- a/drivers/net/can/grcan.c
> +++ b/drivers/net/can/grcan.c

[ ... ]

> @@ -1441,40 +1484,55 @@ static netdev_tx_t grcan_start_xmit(struct sk_buff *skb,
>  	spin_lock_irqsave(&priv->lock, flags);
>  
>  	txwr = grcan_read_reg(&regs->txwr);
>  	space = grcan_txspace(dma->tx.size, txwr, priv->eskbp);
>  
>  	slotindex = txwr / GRCAN_MSG_SIZE;
> +	bds = grcan_numbds(len);

[ ... ]

> +	copy_len = min_t(u32, len, CAN_MAX_DLEN);
> +	memcpy(hdr->data, payload, copy_len);
> +	payload_offset = copy_len;
>  
> -	if (dlc > 0)
> -		memcpy(hdr->data, cf->data, min_t(u32, cf->len, CAN_MAX_DLEN));
> +	txwr = grcan_ring_add(txwr, GRCAN_MSG_SIZE, dma->tx.size);
> +
> +	for (i = 1; i < bds; i++) {
> +		frag = grcan_msg_frag_at(&dma->tx, txwr);
> +
> +		memset(frag, 0, sizeof(*frag));
> +		copy_len = min_t(u32, (u32)len - payload_offset, (u32)GRCAN_MSG_SIZE);
> +		memcpy(frag->data, payload + payload_offset, copy_len);
> +		payload_offset += copy_len;
> +
> +		txwr = grcan_ring_add(txwr, GRCAN_MSG_SIZE, dma->tx.size);
> +	}

[ ... ]

>  	/* Bug workaround for old version of grcan where updating txwr
>  	 * in the same clock cycle as the controller updates txrd to
>  	 * the current txwr could hang the can controller
>  	 */
>  	if (priv->need_txbug_workaround) {
>  		txrd = grcan_read_reg(&regs->txrd);
>  		if (unlikely(grcan_ring_sub(txwr, txrd, dma->tx.size) == 1)) {

Does this break the grcan_txbug_workaround mitigation?

In the original code, txwr represented the current hardware write pointer 
when it was used in this check. Now, txwr is updated in the loop above to 
calculate the end of the new message fragments before the workaround check 
is evaluated.

Because txwr is now the new advanced write pointer, won't the trigger
condition grcan_ring_sub(txwr, txrd, dma->tx.size) == 1 evaluate against 
the wrong offset and fail to prevent the controller hang?

Additionally, grcan_txbug_workaround() waits for the hardware read pointer
to catch up to the provided txwr. Since the advanced txwr hasn't been 
written to the hardware yet (it is updated in the hardware at the end of 
the function), will this guarantee a timeout, returning -EBUSY and 
permanently stalling the TX queue?

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