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

[netdev-bot+sashiko]

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

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

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] In ifi_canfd_open() (drivers/net/can/ifi_canfd/ifi_canfd.c),
  enabling NAPI before ifi_canfd_start() lets ifi_canfd_poll() run in…
- [Low] The new open sequence never puts IFI_CANFD_IRQMASK into a known
  masked state before napi_enable().
- [Low] The comment added in ifi_canfd_open() (lines 857-859) reads "Keep
  between the reset and ifi_canfd_start(): the ISR masks the interrupts…

--

can: ifi_canfd: enable NAPI before starting the controller

This splits the IP reset, configuration and interrupt status clear out of
ifi_canfd_start() into a new ifi_canfd_reset_config(). ifi_canfd_open()
now calls ifi_canfd_reset_config(), synchronize_irq() and napi_enable()
before ifi_canfd_start(). An early RX interrupt can then schedule the poll
instead of having the schedule dropped. The bus-off restart path in
ifi_canfd_set_mode() calls both halves in sequence.

> diff --git a/drivers/net/can/ifi_canfd/ifi_canfd.c b/drivers/net/can/ifi_canfd/ifi_canfd.c
> index 0f83335e4d075..0ff766c7e786a 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)
>  {

[ ... ]

> @@ -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);
> +}

[Severity: Low]
The commit message says "The controller stays disabled until
ifi_canfd_start() unmasks the interrupts and enables it". However,
ifi_canfd_reset_config() never writes IFI_CANFD_IRQMASK. Does the driver
know the interrupt mask state in the window after napi_enable()?

On a re-open, the last IRQMASK write was in ifi_canfd_stop():

	/* Reset the IP */
	writel(IFI_CANFD_STCMD_HARDRESET, priv->base + IFI_CANFD_STCMD);

	/* Mask all interrupts */
	writel(~0, priv->base + IFI_CANFD_IRQMASK);

ifi_canfd_irq_enable() treats SET_ERR, SET_TS, SET_TX and SET_RX as group
write qualifiers. A source bit of 1 enables that source, and it masks by
writing only the SET_* bits. Under that encoding, ~0 would enable every
source, including ones that ifi_canfd_irq_enable(ndev, 1) never enables.

Suppose HARDRESET does not clear IRQMASK. On a dedicated IRQ line, an
rx-class status bit that latches after the final INTERRUPT clear here
would then raise the interrupt. With NAPI now enabled, the poll would run
alongside ifi_canfd_start().

The ~0 write predates this patch, and the v2 notes say it could not be
confirmed to mask. Enabling NAPI inside this window is what makes it
matter now.

Would calling ifi_canfd_irq_enable(ndev, 0) in ifi_canfd_reset_config(),
before the final INTERRUPT clear, put the mask into a known state? The ISR
already relies on that encoding to mask. ifi_canfd_stop() could also use
the same helper instead of ~0.

Neither change covers a shared-line invocation, because the ISR reads the
raw status regardless of the mask.

[ ... ]

> @@ -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.
> +	 */

[Severity: Low]
Is "only the poll unmasks them" accurate? ifi_canfd_start() is called
right after this and also unmasks, through ifi_canfd_irq_enable(ndev, 1).
The same happens from ifi_canfd_set_mode(CAN_MODE_START).

The comment also only explains why napi_enable() has to come before
ifi_canfd_start(). Could it also say why it has to come after
ifi_canfd_reset_config() and synchronize_irq()?

The IRQF_SHARED handler acts on the raw IFI_CANFD_INTERRUPT status, so
stale status could schedule a poll that races with the HARDRESET and FIFO
reset. synchronize_irq() makes sure the schedule from an in-flight
handler is still dropped.

As written, the comment gives no warning to a later change that moves
napi_enable() above the reset or drops synchronize_irq().

> +	napi_enable(&priv->napi);
>  	ifi_canfd_start(ndev);

[Severity: Medium]
With NAPI enabled here, can ifi_canfd_poll() now run at the same time as
the rest of ifi_canfd_start()?

synchronize_irq() only waits for handlers that are already running. The
handler is registered with IRQF_SHARED, so it can be invoked again after
that point, and it acts on the raw IFI_CANFD_INTERRUPT status:

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

On the open path this schedule used to be dropped, and now it succeeds.
The poll then overlaps these writes in ifi_canfd_start(), none of which
hold a lock:

	priv->can.state = CAN_STATE_ERROR_ACTIVE;

	ifi_canfd_irq_enable(ndev, 1);

	/* Unlock, reset and enable the error counter. */
	writel(IFI_CANFD_ERROR_CTR_UNLOCK_MAGIC,
	       priv->base + IFI_CANFD_ERROR_CTR);
	writel(IFI_CANFD_ERROR_CTR_ER_RESET, priv->base + IFI_CANFD_ERROR_CTR);
	writel(IFI_CANFD_ERROR_CTR_ER_ENABLE, priv->base + IFI_CANFD_ERROR_CTR);

On the poll side, ifi_canfd_poll()->ifi_canfd_handle_state_errors()
compares the STCMD status bits of the still-disabled controller with the
stale priv->can.state, which is CAN_STATE_STOPPED. If BUSOFF reads as set,
ifi_canfd_handle_state_change() does:

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

This turns the carrier off in the middle of open(). ifi_canfd_start() then
overwrites the state with CAN_STATE_ERROR_ACTIVE, so the driver state and
the carrier state disagree. In the milder case, a spurious ERROR_ACTIVE
error frame is sent and error_warning is incremented.

With CAN_CTRLMODE_BERR_REPORTING set, ifi_canfd_handle_lec_err() does:

	writel(IFI_CANFD_ERROR_CTR_ER_RESET, priv->base + IFI_CANFD_ERROR_CTR);
	writel(IFI_CANFD_INTERRUPT_ERROR_COUNTER,
	       priv->base + IFI_CANFD_INTERRUPT);
	writel(IFI_CANFD_ERROR_CTR_ER_ENABLE, priv->base + IFI_CANFD_ERROR_CTR);

Could these interleave with the unlock/reset/enable sequence in
ifi_canfd_start() and leave the error counter in an inconsistent state?

The driver alone doesn't show whether the IP latches these status bits, or
reports BUSOFF in STCMD, while STCMD_ENABLE is clear. The v2 notes
acknowledge this overlap. The bus-off restart path in ifi_canfd_set_mode()
already has it, but it is new on the open path.

Would it work to move the priv->can.state assignment and the ERROR_CTR
unlock/reset/enable writes into ifi_canfd_reset_config(), before
napi_enable()? That would leave only the interrupt unmask and the STCMD
enable in ifi_canfd_start().

[ ... ]

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