Re: [PATCH v6 2/6] can: flexcan: disable all IRQ lines in flexcan_chip_interrupts_enable()

[Bough Chen <haibo.chen@oss.nxp.com>]

On Mon, Jul 13, 2026 at 09:07:04AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] On multi-IRQ platforms, Wake-on-CAN is broken because `flexcan_suspend` only arms the primary IRQ (`dev->irq`) for wakeup, completely ignoring the state/BOFF interrupt (`priv->irq_boff`) which actually handles the wakeup event.
> - [High] In `flexcan_resume`, the network queue is restarted (`netif_device_attach` and `netif_start_queue`) before the hardware is initialized (`flexcan_chip_start`). This creates a race condition where a packet can be queued for transmission just before the hardware undergoes a soft reset, leading to a permanent TX stall.
> --
> 
> --- Patch [2]: [PATCH v6 2/6] can: flexcan: disable all IRQ lines in flexcan_chip_interrupts_enable() ---
> commit ca7ea75d45df1ecf13f11dcec0bd19377eccf3c6
> Author: Ciprian Marian Costea <ciprianmarian.costea@oss.nxp.com>
> 
> can: flexcan: disable all IRQ lines in flexcan_chip_interrupts_enable()
> 
> flexcan_chip_interrupts_enable() disables only the primary IRQ line while
> writing to the IMASK and CTRL registers. On multi-IRQ platforms, the
> additional IRQ lines remain active so their handlers can fire while registers
> are inconsistent. This commit disables all registered IRQ lines around the
> IMASK/CTRL writes.
> 
> > diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c
> > index f73ff442d5304..7dde2e623defe 100644
> > --- a/drivers/net/can/flexcan/flexcan-core.c
> > +++ b/drivers/net/can/flexcan/flexcan-core.c
> > @@ -1519,14 +1519,28 @@ static void flexcan_chip_interrupts_enable(const struct net_device *dev)
> >  {
> >  	const struct flexcan_priv *priv = netdev_priv(dev);
> >  	struct flexcan_regs __iomem *regs = priv->regs;
> > +	u32 quirks = priv->devtype_data.quirks;
> >  	u64 reg_imask;
> >  
> >  	disable_irq(dev->irq);
> > +	if (quirks & FLEXCAN_QUIRK_NR_IRQ_3) {
> > +		disable_irq(priv->irq_boff);
> 
> [Severity: High]
> This is a pre-existing issue, but on multi-IRQ platforms, is Wake-on-CAN
> broken because we only arm the primary IRQ during suspend?
> 
> In drivers/net/can/flexcan/flexcan-core.c:flexcan_suspend(), we see:
> 
> 	if (device_may_wakeup(device)) {
> 		enable_irq_wake(dev->irq);
> 		err = flexcan_enter_stop_mode(priv);
> 
> Since the hardware asserts irq_boff (which handles FLEXCAN_ESR_WAK_INT) for
> wakeup events, doesn't irq_boff need to be armed with enable_irq_wake() as
> well? Otherwise, the system might fail to wake from suspend since the
> interrupt controller won't treat the asserted IRQ line as a wakeup source.

Yes, this need to be addressed.

> 
> [Severity: High]
> This is also a pre-existing issue, but does flexcan_resume() restart the
> network queue prematurely?
> 
> In drivers/net/can/flexcan/flexcan-core.c:flexcan_resume(), the queue is
> restarted before hardware initialization:
> 
> 	if (netif_running(dev)) {
> 		netif_device_attach(dev);
> 		netif_start_queue(dev);
> 		if (device_may_wakeup(device)) {
> 			...
> 		} else {
> 			...
> 			err = flexcan_chip_start(dev);
> 
> If netif_start_queue() is called before flexcan_chip_start() reinitializes
> the hardware, could the networking stack concurrently call ndo_start_xmit
> (flexcan_start_xmit) and queue a packet?
> 
> If so, would the subsequent flexcan_chip_start() soft reset wipe out the
> freshly populated message buffers, aborting the transmission and leading to a
> permanent TX stall since the completion interrupt will never fire?

Good catch, those two findings should be addressed in another patch set.

Regards
Haibo Chen
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713085306.2643794-1-ciprianmarian.costea@oss.nxp.com?part=2