Re: [PATCH net v9 01/10] can: rockchip_canfd: prevent TX stall on echo skb failure

[netdev-bot+sashiko@kernel.org]

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] The commit message understates how bad the bug is.

--

can: rockchip_canfd: prevent TX stall on echo skb failure

This commit moves can_put_echo_skb() in rkcanfd_start_xmit() so it runs
before the hardware TX buffer is written. If no echo skb was installed, the
frame is dropped and tx_head and the BQL bytes are left unchanged. It also
passes an extra skb reference to can_put_echo_skb() and releases the
original skb once submission finishes or fails.

> rkcanfd_start_xmit() advances tx_head and requests transmission even when
> can_put_echo_skb() fails. Completion matches self-received frames against
> the echo ring, so an entry without an echo skb cannot be completed and
> can leave the TX queue stopped.

[Severity: Low]
The code change looks right to me. Does this description cover the whole
impact, though? It looks like RX stops as well as TX.

can_create_echo_skb() uses skb_clone(GFP_ATOMIC), so can_put_echo_skb() can
fail with -ENOMEM under memory pressure. Before this patch, the empty slot
still became tx_tail, so rkcanfd_get_tx_pending() stayed non-zero.

After that, every received frame goes through rkcanfd_rxstx_filter():

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;
	}
    ...
}

By this point rkcanfd_handle_rx_int_one() has already pulled the frame out
of the FIFO with rkcanfd_read_rep(). It then returns before
alloc_can_skb() and can_rx_offload_queue_timestamp() are called:

drivers/net/can/rockchip/rockchip_canfd-rx.c:rkcanfd_handle_rx_int_one() {
    ...
	if (rkcanfd_get_tx_pending(priv)) {
		bool tx_done = false;

		err = rkcanfd_rxstx_filter(priv, cfd, header->ts, &tx_done);
		if (err)
			return err;
    ...
}

rkcanfd_handle_rx_int() then leaves its FIFO loop on that error. tx_tail
never advances, so the same thing happens on every later RX interrupt, with
a netdev_err() each time. Frames from other nodes on the bus are lost as
well until the interface is restarted.

Could the commit message say that both RX and TX stop? That would help
stable backporters judge how urgent this fix is.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-rockchip-canfd-restart-close-v9-0-6643f3825ba3%40qq.com