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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Because napi_enable() now runs before ifi_canfd_start(),
  ifi_canfd_poll() can run against an IFI CAN FD IP that has not been…

--

can: ifi_canfd: enable NAPI before starting the controller

This patch moves napi_enable() ahead of ifi_canfd_start() in
ifi_canfd_open(). The goal is that an RX interrupt raised right after the
controller is enabled can schedule the poll, instead of being dropped and
leaving the interrupts masked.

> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index 0f83335e4d07..feb738eb0c9a 100644
> --- a/drivers/net/can/ifi_canfd/ifi_canfd.c
> +++ b/drivers/net/can/ifi_canfd/ifi_canfd.c
> @@ -846,9 +846,12 @@ static int ifi_canfd_open(struct net_device *ndev)
>  		goto err_irq;
>  	}
>  
> +	/* Keep before ifi_canfd_start(): the ISR masks the interrupts and only
> +	 * the poll unmasks them.
> +	 */
> +	napi_enable(&priv->napi);
>  	ifi_canfd_start(ndev);

[Severity: Medium]
With NAPI enabled before ifi_canfd_start(), can ifi_canfd_poll() now run
against an IP that has not been reset yet? Can it also run alongside the
reset and programming sequence in ifi_canfd_start()?

The handler is installed with IRQF_SHARED just above. So ifi_canfd_isr()
can run because of another device on the line, or because of a line that
is still asserted. The ISR acts on the raw status and does not AND it with
IRQMASK:

ifi_canfd_isr() {
    ...
	isr = readl(priv->base + IFI_CANFD_INTERRUPT);
    ...
	if (isr & rx_irq_mask) {
		ifi_canfd_irq_enable(ndev, 0);
		napi_schedule(&priv->napi);
	}
    ...
}

Before this patch, NAPI was still disabled, so this schedule was dropped.
The ifi_canfd_irq_enable(ndev, 1) inside ifi_canfd_start() then recovered.
Now the schedule succeeds.

ifi_canfd_plat_probe() only reads IP_ID, VER and CANCLOCK, and the driver
has no .shutdown hook. After a kexec or warm boot, could the IP still hold
RX data or error status when the device is first opened?

If so, the poll can run on another CPU, or at irq_exit in the middle of
ifi_canfd_start(). It would then do the following with no lock:

  - read STCMD and RXSTCMD
  - drain the RX FIFO through ifi_canfd_read_fifo()
  - possibly write ERROR_CTR from ifi_canfd_handle_lec_err()
  - call ifi_canfd_irq_enable(ndev, 1)

Each of these could interleave with the HARDRESET, the RX/TX FIFO resets
and the ERROR_CTR unlock/reset/enable writes in ifi_canfd_start().

Possible results:

  - a stale or half-reset FIFO entry delivered as a received frame
  - a spurious ERROR_ACTIVE state-change frame with error_warning++,
    because priv->can.state is still CAN_STATE_STOPPED
  - a stale BUSOFF bit in STCMD, which makes
    ifi_canfd_handle_state_change() do this:

	priv->can.state = CAN_STATE_BUS_OFF;
	ifi_canfd_irq_enable(ndev, 0);
	priv->can.can_stats.bus_off++;
	can_bus_off(ndev);

In the bus-off case, ifi_canfd_start() then overwrites the state:

	priv->can.state = CAN_STATE_ERROR_ACTIVE;

With restart-ms set to 0, can_restart_now() would return -EBUSY, because
the state is no longer CAN_STATE_BUS_OFF. Would the interface then stay
carrier-off until the next down/up?

The commit message says:

  "NAPI can be enabled while the controller is in reset, because nothing
  schedules it until an interrupt arrives."

That seems to hold only after a previous close, where ifi_canfd_stop()
writes HARDRESET, masks IRQMASK and clears INTERRUPT. It does not seem to
hold for the first open after boot or kexec, or for ISR calls coming from
the shared line.

Would it be safer to quiesce the IP first? That would mean HARDRESET,
masking IRQMASK and clearing INTERRUPT, either in probe or before
napi_enable(). Another option is to move napi_enable() into
ifi_canfd_start(), after the reset and interrupt clear and before
ifi_canfd_irq_enable(ndev, 1).

The bus-off restart path, ifi_canfd_set_mode() -> ifi_canfd_start(),
already runs with NAPI enabled, so a similar overlap exists there. This
change extends that existing weakness to the open path.

>  
> -	napi_enable(&priv->napi);
>  	netif_start_queue(ndev);
>  
>  	return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021205924.2538511-1-patrzhan%40amazon.com