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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch takes spin_lock_irqsave(&priv->tx_lock, flags) near
  the top of rkcanfd_start_xmit()…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  rkcanfd_start_xmit() does not handle a can_put_echo_skb() failure.

--

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

This adds a private tx_lock and tx_enabled flag. They serialize
rkcanfd_start_xmit() and the RX-side echo completion and retry against
bus-off handling and CAN restart. TX is disabled before CAN_STATE_BUS_OFF
is published, and carrier, TX, IRQs and the queue are re-enabled together
under the lock after the controller has been reinitialized.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index 12200dcfd3389..f49ed6d2266d9 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)

[ ... ]

>  	if (can_dev_dropped_skb(ndev, skb))
>  		return NETDEV_TX_OK;
>  
> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +	if (unlikely(!priv->tx_enabled)) {
> +		netif_stop_queue(ndev);
> +		goto out_busy;
> +	}
> +

[Severity: Low]
This lock stays held with hard IRQs disabled until the end of
rkcanfd_start_xmit(). The locked region includes the echo skb handling:

drivers/net/can/rockchip/rockchip_canfd-tx.c:rkcanfd_start_xmit() {
    ...
	frame_len = can_skb_get_frame_len(skb);
	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
	if (!err)
		netdev_sent_queue(priv->ndev, frame_len);
    ...
}

can_put_echo_skb()->can_create_echo_skb() clones the skb and then calls
consume_skb() on the original. If the clone fails, it calls kfree_skb()
instead. can_put_echo_skb() also calls kfree_skb() in three cases: IFF_ECHO
is clear, the protocol check fails, or the echo slot is still occupied.

Freeing the original skb runs the socket destructor (sock_wfree()). If this
is the last TX skb of a closed socket, that can continue into __sk_free().

The comment above dev_kfree_skb_irq() in include/linux/netdevice.h says
kfree_skb() and consume_skb() must not be called with hardware interrupts
disabled. Before this patch, ndo_start_xmit() ran with only BH disabled.

Nothing seems to fail visibly today. skb_release_head_state() only warns
on in_hardirq(), not on irqs_disabled(), and peak_canfd uses a similar
pattern. Even so, should the clone and the free of the original skb happen
outside tx_lock, with only the echo slot pointer and tx_head published
under the lock?

[ ... ]

> @@ -137,7 +144,14 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  				  RKCANFD_TX_STOP_THRESHOLD,
>  				  RKCANFD_TX_START_THRESHOLD);
>  
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
>  	return NETDEV_TX_OK;

[Severity: Medium]
This is a pre-existing issue that this patch did not introduce. What
happens when can_put_echo_skb() fails earlier in this function?

If the GFP_ATOMIC clone in can_create_echo_skb() fails, can_put_echo_skb()
frees the original skb and returns -ENOMEM. That leaves
priv->can.echo_skb[tx_head] set to NULL. In that case rkcanfd_start_xmit()
only skips netdev_sent_queue():

	err = can_put_echo_skb(skb, ndev, tx_head, frame_len);
	if (!err)
		netdev_sent_queue(priv->ndev, frame_len);

	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);

	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);

When the self-received frame comes back, rkcanfd_handle_rx_int_one() sees
tx_pending != 0 and calls rkcanfd_rxstx_filter(). That function then hits
the NULL slot:

drivers/net/can/rockchip/rockchip_canfd-rx.c:rkcanfd_rxstx_filter() {
    ...
	skb = priv->can.echo_skb[tx_tail];
	if (!skb) {
		netdev_err(priv->ndev, ...);

		return -ENOMSG;
	}
    ...
}

tx_tail never advances, because the only normal advance happens after a
successful echo match. rkcanfd_handle_rx_int() also stops draining the
FIFO when it gets the error.

After that, does every RX IRQ consume and drop one FIFO entry with another
-ENOMSG? TX would then stall once the ring is full, until a bus-off
restart or an interface down/up reinitializes the controller.

This patch keeps the same error propagation in rkcanfd_handle_rx_int_one().

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