[PATCH v2 3/3] can: ifi_canfd: enable NAPI before starting the controller

[Patrick Zhang <patrzhan@amazon.com>]

ifi_canfd_open() calls ifi_canfd_start(), which unmasks the interrupts
in IFI_CANFD_IRQMASK and then enables the controller through
IFI_CANFD_STCMD, and only then calls napi_enable(). A frame that
arrives in that window raises RXFIFO_NEMPTY. ifi_canfd_isr() masks all
interrupts with ifi_canfd_irq_enable(ndev, 0) and calls
napi_schedule(). NAPI is still disabled, so napi_schedule_prep() finds
NAPI_STATE_SCHED set and the schedule is dropped. Only ifi_canfd_poll()
unmasks the interrupts again, and it never runs. From then on the
interface receives nothing. ifi_canfd_start_xmit() stops the queue
after every frame and relies on the TX FIFO interrupt to wake it, and
that interrupt is masked too, so transmit stops after the next frame.
The stall lasts until the next close. Every re-open runs the same race
again.

Enable NAPI before the controller starts, so that an early RX interrupt
can schedule the poll. ifi_canfd_start() cannot fail, so no error path
changes.

NAPI must not be enabled before the IP has been reset, though. The
handler is registered with IRQF_SHARED and acts on the raw
IFI_CANFD_INTERRUPT status, and nothing resets the IP before the first
open, so stale status could schedule a poll that runs while
ifi_canfd_start() resets and programs the IP. Split the reset,
configuration and status clear out of ifi_canfd_start() into
ifi_canfd_reset_config(), and enable NAPI between the two. Before
enabling NAPI, wait with synchronize_irq() for any handler that read
the stale status before the reset, so that its schedule is still
dropped, as before. The controller stays disabled until
ifi_canfd_start() unmasks the interrupts and enables it. The bus-off
restart path calls both halves in sequence, so its register writes do
not change.

This is the same race as in "can: ctucanfd: enable NAPI before starting
the controller" and "can: xilinx_can: enable NAPI before starting the
controller". I do not have IFI CAN FD hardware, so this change is
compile tested only and the failure sequence is taken from the code.

Fixes: 0c4d9c94a130 ("can: ifi: Add IFI CANFD IP support")
Assisted-by: LLM
Signed-off-by: Patrick Zhang <patrzhan@amazon.com>
---

Notes:
    v2:
    - Split the reset, configuration and status clear out of
      ifi_canfd_start() into ifi_canfd_reset_config(), and enable NAPI
      between the two, so the poll cannot race with the reset when the
      shared handler sees stale status (reported by the netdev AI review)
    - This replaces the ifi_canfd_stop() call I proposed in reply to the
      review: ifi_canfd_stop() writes ~0 to IFI_CANFD_IRQMASK, and I cannot
      confirm from the driver alone that this masks the interrupts, while
      the split does not depend on it
    - Call synchronize_irq() before napi_enable(), so a handler that read
      stale status before the reset cannot schedule the poll afterwards
    - Explain the ordering in the commit message, and name the ctucanfd
      and xilinx_can fixes by subject instead of "the previous patches"
    - A poll can still overlap the interrupt unmask and error-counter writes
      at the end of ifi_canfd_start(), the same as on the existing bus-off
      restart path. I have not reordered those writes, because I cannot
      test the effect on hardware.
    - v1: https://lore.kernel.org/r/20261021205924.2538511-4-patrzhan@amazon.com

 drivers/net/can/ifi_canfd/ifi_canfd.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
index 0f83335e4..0ff766c7e 100644
--- a/drivers/net/can/ifi_canfd/ifi_canfd.c
+++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
@@ -734,10 +734,9 @@ static void ifi_canfd_set_filters(struct net_device *ndev)
 			     IFI_CANFD_FILTER_IDENT_IDE);
 }
 
-static void ifi_canfd_start(struct net_device *ndev)
+static void ifi_canfd_reset_config(struct net_device *ndev)
 {
 	struct ifi_canfd_priv *priv = netdev_priv(ndev);
-	u32 stcmd;
 
 	/* Reset the IP */
 	writel(IFI_CANFD_STCMD_HARDRESET, priv->base + IFI_CANFD_STCMD);
@@ -760,6 +759,12 @@ static void ifi_canfd_start(struct net_device *ndev)
 	/* Clear all pending interrupts */
 	writel((u32)(~IFI_CANFD_INTERRUPT_SET_IRQ),
 	       priv->base + IFI_CANFD_INTERRUPT);
+}
+
+static void ifi_canfd_start(struct net_device *ndev)
+{
+	struct ifi_canfd_priv *priv = netdev_priv(ndev);
+	u32 stcmd;
 
 	stcmd = IFI_CANFD_STCMD_ENABLE | IFI_CANFD_STCMD_NORMAL_MODE |
 		IFI_CANFD_STCMD_ENABLE_7_9_8_8_TIMING;
@@ -817,6 +822,7 @@ static int ifi_canfd_set_mode(struct net_device *ndev, enum can_mode mode)
 {
 	switch (mode) {
 	case CAN_MODE_START:
+		ifi_canfd_reset_config(ndev);
 		ifi_canfd_start(ndev);
 		netif_wake_queue(ndev);
 		break;
@@ -846,9 +852,14 @@ static int ifi_canfd_open(struct net_device *ndev)
 		goto err_irq;
 	}
 
+	ifi_canfd_reset_config(ndev);
+	synchronize_irq(ndev->irq);
+	/* Keep between the reset and ifi_canfd_start(): the ISR masks the
+	 * interrupts and only the poll unmasks them.
+	 */
+	napi_enable(&priv->napi);
 	ifi_canfd_start(ndev);
 
-	napi_enable(&priv->napi);
 	netif_start_queue(ndev);
 
 	return 0;
-- 
2.50.1