Re: [PATCH] can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] can: mcp251xfd: netif_stop_queue() race in
  mcp251xfd_handle_cerrif()
- [Low] can: mcp251xfd: missing Fixes trailer for
  mcp251xfd_handle_cerrif() fix
--

commit 04d0094bdd9ffb626b769cf55b6bbc25e504875f
Author: Mike Looijmans <mike.looijmans@topic.nl>

can: mcp251xfd: Always stop on BUS_OFF and call netif_stop_queue

This commit changes mcp251xfd_handle_cerrif() on bus-off to log an error
instead of aborting when reading the BER counter fails, and calls
netif_stop_queue() after stopping the chip and invoking can_bus_off().

[Severity: Low]
In the commit message, since this is a bug fix for the bus-off handling
in mcp251xfd_handle_cerrif(), please consider adding a Fixes: trailer:

  Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")

[Severity: Medium]
In drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c, inside
mcp251xfd_handle_cerrif() (around line 1138), can calling
netif_stop_queue(priv->ndev) after mcp251xfd_chip_stop() and
can_bus_off() race with concurrent transmissions and bus-off recovery?

CPU0 (threaded IRQ)               CPU1 (kworker / restart)
mcp251xfd_handle_cerrif()
  mcp251xfd_chip_stop()
  can_bus_off()
    schedule_delayed_work()
                                  can_restart_work()
                                    can_restart()
                                      mcp251xfd_set_mode()
                                        netif_wake_queue(ndev)
  netif_stop_queue(priv->ndev)
    <-- TX queue left stopped

can_bus_off() schedules priv->can.restart_work, which runs can_restart()
and mcp251xfd_set_mode(), reinitializing the TX ring to empty and calling
netif_wake_queue(ndev).  If the threaded IRQ handler is preempted after
can_bus_off(), netif_stop_queue() runs after mcp251xfd_set_mode() wakes
the queue.  With an empty TX ring, no TEF completion interrupt will fire
to wake the queue again, leaving TX permanently stopped until the
interface is cycled down and up.

In addition, mcp251xfd_chip_stop() performs sleeping SPI transfers while
disabling interrupts and entering configuration mode.  Leaving the TX
queue active until after mcp251xfd_chip_stop() returns allows
mcp251xfd_start_xmit() to submit new frames concurrently while the chip
is shutting down.

Calling netif_stop_queue(priv->ndev) at the start of the
CAN_STATE_BUS_OFF block before mcp251xfd_chip_stop() and can_bus_off()
(matching the order in mcp251xfd_stop()) avoids both races.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109070605.9761-1-mike.looijmans@topic.nl?part=1