Re: [PATCH can-next 06/10] can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

[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] The commit fixes a real bug introduced by commit 24bc41b45583
  ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception"),…

--

can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

The patch fixes one error path in gs_usb_receive_bulk_callback(). That path
runs when the host frame has GS_CAN_FLAG_OVERFLOW set and alloc_can_err_skb()
fails. It now jumps to a new can_rx_offload_irq_finish label instead of going
straight to resubmit_urb.

> If the overflow flag is set for a host frame and the allocation of the
> error SKB fails, the URB should not be resubmitted immediately; instead,
> can_rx_offload_irq_finish() should be called, since an SKB may have been
> added to rx-offload.
>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>

[Severity: Low]
The code change looks right. Should the commit message also have a Fixes
tag, so the fix gets backported to stable?

The regression appears to come from:

Fixes: 24bc41b45583 ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception")

That commit changed gs_usb_rx_offload() and gs_usb_get_echo_skb() to queue
SKBs on offload->skb_irq_queue. Only can_rx_offload_irq_finish() moves those
SKBs to skb_queue and calls napi_schedule(). The same commit added the
can_rx_offload_irq_finish() call just above resubmit_urb.

The goto resubmit_urb on the overflow path is older (from d08e973a77d1).
After 24bc41b45583, that goto started skipping the new
can_rx_offload_irq_finish() call.

The commit message could also say what the user sees. Here is the sequence:

gs_usb_receive_bulk_callback()
    gs_usb_rx_offload() or gs_usb_get_echo_skb()
        __skb_queue_tail(&offload->skb_irq_queue, skb)
    hf->flags & GS_CAN_FLAG_OVERFLOW
        alloc_can_err_skb() returns NULL
        goto resubmit_urb  /* skips can_rx_offload_irq_finish() */

The RX frame or TX echo frame queued earlier stays on skb_irq_queue, and
NAPI is not scheduled. It is only delivered when a later URB completion calls
can_rx_offload_irq_finish(). On a quiet bus, that could take a long time.

Would it help to mention this delayed delivery in the commit message, along
with the Fixes tag?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004-gs_usb-cleanups-and-fixes-v1-0-7b4b06244a77%40pengutronix.de