Re: [PATCH v2 2/3] can: xilinx_can: enable NAPI before starting the controller

["T, Harini" <harini.t@amd.com>]

Thanks Patrick,

On 10/31/2026 5:03 AM, Patrick Zhang wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> xcan_open() calls xcan_chip_start(), which writes XCAN_IER_OFFSET to
> unmask the RX interrupts and then sets XCAN_SRR_CEN_MASK to enable the
> controller, and only then calls napi_enable(). A frame that arrives in
> that window raises the RX interrupt. xcan_interrupt() clears the RX
> bits in XCAN_IER_OFFSET and calls napi_schedule(). NAPI is still
> disabled, so napi_schedule_prep() finds NAPI_STATE_SCHED set and the
> schedule is dropped. Only xcan_rx_poll() unmasks RX again, and it never
> runs. The interface receives no data frames until the next close.
> Transmit still works. The RX overflow interrupt stays enabled, so once
> the RX FIFO fills, xcan_err_interrupt() counts rx_over_errors and
> reports overflow error frames, but nothing reads the FIFO. Every
> re-open (ip link down/up, a driver rebind) runs the same race again.
> 
> The window is only the tail of xcan_chip_start(), so the failure is
> rare. Adding an mdelay(50) at the end of xcan_chip_start() makes it
> reproducible with continuous traffic from another node and a single
> "ip link set down/up" on the test device.
> 
> Enable NAPI before the controller starts, so that an early RX interrupt
> can schedule the poll. NAPI can be enabled while the controller is in
> reset, because nothing schedules it until an interrupt arrives. If
> xcan_chip_start() fails, disable NAPI again. Otherwise the next open
> would call napi_enable() on an already enabled instance and trip the
> BUG_ON() in napi_enable_locked().
> 
> This is the same race as in "can: ctucanfd: enable NAPI before starting
> the controller".
> 
> Tested on a Versal board.
> 
> Fixes: b1201e44f50b ("can: xilinx CAN controller support")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Patrick Zhang <patrzhan@amazon.com>

LGTM.

Reviewed-by: Harini T <harini.t@amd.com>

> ---
> 
> Notes:
>      v2:
>      - Correct the commit message: the RX overflow interrupt stays enabled
>        during the stall, so rx_over_errors is counted once the FIFO fills
>      - Name the ctucanfd fix by subject instead of "the previous patch"
>      - v1: https://lore.kernel.org/r/20261021205924.2538511-3-patrzhan@amazon.com
> 
>   drivers/net/can/xilinx_can.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
> index 43d7f2282..5c2412bf9 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -1570,18 +1570,22 @@ static int xcan_open(struct net_device *ndev)
>          if (ret)
>                  goto err_irq;
> 
> +       /* Keep before xcan_chip_start(): the ISR masks RX interrupts and only
> +        * the poll unmasks them.
> +        */
> +       napi_enable(&priv->napi);
>          ret = xcan_chip_start(ndev);
>          if (ret < 0) {
>                  netdev_err(ndev, "xcan_chip_start failed!\n");
>                  goto err_candev;
>          }
> 
> -       napi_enable(&priv->napi);
>          netif_start_queue(ndev);
> 
>          return 0;
> 
>   err_candev:
> +       napi_disable(&priv->napi);
>          close_candev(ndev);
>   err_irq:
>          free_irq(ndev->irq, ndev);
> --
> 2.50.1