Re: [PATCH net v3 5/5] can: rockchip_canfd: serialize TX with bus-off and restart

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new tx_enabled gate in rkcanfd_handle_rx_int_one()
  (drivers/net/can/rockchip/rockchip_canfd-rx.c) changes how self-…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but CAN XL frames
  are accepted on this non-XL controller and are then parsed as CAN/CAN…

--

can: rockchip_canfd: serialize TX with bus-off and restart

This patch adds a private tx_lock and a tx_enabled flag to the
rockchip_canfd driver. The lock is held across the whole ndo_start_xmit()
submission, and the gate is closed before bus-off is published or the
controller is reset. The same gate covers TX completion and the erratum 6
retry on the RX side, and open/restart now enable TX, IRQ handling,
interrupts and the queue in a single critical section.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> index 24e87daa1df0..9b357bb8fc70 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c

[ ... ]

> @@ -237,15 +240,18 @@ static int rkcanfd_handle_rx_int_one(struct rkcanfd_priv *priv)
>  		return 0;
>  	}
>  
> -	if (rkcanfd_get_tx_pending(priv)) {
> -		bool tx_done = false;
> -
> +	/* Do not consume echo SKBs, retry TX or wake the queue after bus-off
> +	 * has handed ownership of the echo ring to the CAN restart path.
> +	 */
> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +	err = 0;
> +	if (priv->tx_enabled && rkcanfd_get_tx_pending(priv))
>  		err = rkcanfd_rxstx_filter(priv, cfd, header->ts, &tx_done);
> -		if (err)
> -			return err;
> -		if (tx_done && !(priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK))
> -			return 0;
> -	}
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +	if (err)
> +		return err;
> +	if (tx_done && !(priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK))
> +		return 0;

[Severity: Medium]
What happens to self-received copies of our own TX frames while
tx_enabled is false?

rkcanfd_chip_start() always sets RKCANFD_REG_MODE_RXSTX_MODE, so the
controller loops every transmitted frame back into the RX FIFO. With the
gate closed, rkcanfd_rxstx_filter() is skipped and tx_done stays false. The
copy then takes the normal RX path:

    if (priv->bec.rxerr)
        priv->bec.rxerr = min(CAN_ERROR_PASSIVE_THRESHOLD,
                              priv->bec.rxerr) - 1;
    ...
    err = can_rx_offload_queue_timestamp(&priv->offload, skb, header->ts);

The stack then sees it as a frame from another node. It also reaches
sockets that have recv_own_msgs disabled. Its echo SKB is never completed,
and can_flush_echo_skb() in close_candev() drops it later, even though the
frame did go out on the bus.

rkcanfd_stop() now closes the gate before it masks interrupts and frees
the IRQ:

    close_candev_cancel_restart_sync(ndev);
    rkcanfd_tx_disable(priv);

    rkcanfd_chip_interrupts_disable(priv);
    free_irq(ndev->irq, priv);
    can_rx_offload_disable(&priv->offload);
    rkcanfd_chip_stop_sync(priv, CAN_STATE_STOPPED);

The controller stays in WORK_MODE with RXSTX until rkcanfd_chip_stop_sync()
runs. Frames already in the hardware TX FIFO can still go out and loop back
in that window.

rkcanfd_irq() only checks irq_suspended and the software mask, and
irq_suspended is still false here. A handler already looping in
rkcanfd_handle_rx_int() on another CPU could therefore pass these copies
upstream.

Before this patch, rkcanfd_stop() only called netif_stop_queue(). The
filter still matched these frames and completed them as TX.

There seems to be a similar but narrower window after bus-off. It runs from
rkcanfd_tx_disable() in rkcanfd_handle_state_error_int() until set_mode()
sets irq_suspended.

Two comments also look out of date now. The erratum 6 comment in
rockchip_canfd.h says:

    * Work around this issue by activating self reception (RXSTX). If we
    * have pending TX CAN frames, check all RX'ed CAN frames in
    * rkcanfd_rxstx_filter().

The comment in rkcanfd_get_berr_counter_corrected() says RXSTX does not
affect the RX error counters:

    * Luckily the controller doesn't decrement the RX CAN bus
    * error counter in hardware for self received TX'ed CAN
    * frames (RKCANFD_REG_MODE_RXSTX_MODE), so RXSTX doesn't
    * interfere with proper RX CAN bus error counters.

The commit message only says that RX will not "consume echo SKBs" after
bus-off. It does not mention that these frames are now delivered as RX
traffic from another node.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index 12200dcfd338..f49ed6d2266d 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> @@ -69,12 +69,19 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	u32 reg_frameinfo, reg_id, reg_cmd;
>  	unsigned int tx_head, frame_len;
>  	const struct canfd_frame *cfd;
> +	unsigned long flags;
>  	int err;
>  	u8 i;
>  
>  	if (can_dev_dropped_skb(ndev, skb))
>  		return NETDEV_TX_OK;
>  
> +	spin_lock_irqsave(&priv->tx_lock, flags);

[Severity: High]
This isn't a bug introduced by this patch, but can a CAN XL frame get
here and loop forever with tx_lock held and IRQs disabled?

can_dev_dropped_skb() does not reject CAN XL skbs on a device without XL
support. can_dropped_invalid_skb() accepts ETH_P_CANXL whenever
can_is_canxl_skb() passes. That check only tests the length and the
CANXL_XLF bit:

    return cxl->flags & CANXL_XLF;

AF_PACKET can therefore send a 13-byte ETH_P_CANXL frame with len=1 and
flags=0xff, and it reaches rkcanfd_start_xmit().

rkcanfd_start_xmit() casts skb->data to struct canfd_frame. cfd->len then
reads the XL flags byte, which is at least 128 for any accepted XL frame
and 255 in this example. can_is_canfd_skb() is false, so the classic path
runs with a u8 index:

    for (i = 0; i < cfd->len; i += 4)
        rkcanfd_write(priv, RKCANFD_REG_FD_TXDATA0 + i,
                      *(u32 *)(cfd->data + i));

For cfd->len from 128 to 252, this reads past the skb data. It also writes
controller registers well beyond the TXDATA range. For 253 to 255, i wraps
from 252 to 0 and the loop never ends.

Without this patch the endless loop was a soft lockup. With the new
spin_lock_irqsave() held across it, it becomes a hard lockup on the xmit
CPU. rkcanfd_irq() on other CPUs would also spin on tx_lock in
rkcanfd_handle_rx_int_one().

Triggering this needs CAP_NET_RAW in the network namespace of the CAN
device.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-rockchip-canfd-restart-close-v3-0-f0eb41090242%40qq.com