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

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 <[email protected]>

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.

[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?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2