Re: [PATCH net-next 30/37] can: hi311x: drop hi3110_lock before free_irq() on open failure

[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 (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] The rollback in hi3110_open() (drivers/net/can/spi/hi311x.c,
  lines 789-794) sets priv->force_quit = 1 and drops hi3110_lock before…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the driver's
  own restart_work is never cancelled or flushed during shutdown.

--

can: hi311x: drop hi3110_lock before free_irq() on open failure

This patch changes the hi3110_open() error path. It now sets force_quit
and drops hi3110_lock before calling free_irq(), then takes the mutex
again for the remaining hardware cleanup. This avoids a deadlock against
the threaded IRQ handler, which takes hi3110_lock before it checks
force_quit.

Link: https://patch.msgid.link/20260820020631.316418-1-runyu.xiao@seu.edu.cn

> diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
> index 076a3efa1bfff..927555b5d581e 100644
> --- a/drivers/net/can/spi/hi311x.c
> +++ b/drivers/net/can/spi/hi311x.c
> @@ -787,7 +787,10 @@ static int hi3110_open(struct net_device *net)
>  	return 0;
>  
>   out_free_irq:
> +	priv->force_quit = 1;
> +	mutex_unlock(&priv->hi3110_lock);
>  	free_irq(spi->irq, priv);

[Severity: Medium]
Can this rollback turn the old deadlock into an IRQ storm when
hi3110_set_normal_mode() fails?

The IRQ is requested with IRQF_ONESHOT | IRQF_TRIGGER_HIGH. Before
hi3110_set_normal_mode() returns -EBUSY, it has already armed the chip:

drivers/net/can/spi/hi311x.c:hi3110_set_normal_mode() {
    hi3110_write(spi, HI3110_WRITE_INTE, HI3110_INT_BUSERR |
                 HI3110_INT_RXFIFO | HI3110_INT_TXCPLT);
    hi3110_write(spi, HI3110_WRITE_CTRL1, HI3110_CTRL1_TXEN);
    ...
    hi3110_write(spi, HI3110_WRITE_CTRL0, reg);
    mdelay(HI3110_OST_DELAY_MS);
    reg = hi3110_read(spi, HI3110_READ_CTRL0);
    if ((reg & HI3110_CTRL0_MODE_MASK) != reg)
        return -EBUSY;
    ...
}

A received frame or a bus error during the mdelay() window can assert
INT. Once force_quit is set, hi3110_can_ist() skips the whole
while (!priv->force_quit) loop. It never reads STATF, the FIFO or INTF,
but it still returns IRQ_HANDLED.

irq_finalize_oneshot() then unmasks the line, which is still asserted.
The interrupt fires again right away, so the SCHED_FIFO IRQ thread spins
until free_irq() finally shuts the line down.

If the IRQ thread was blocked on hi3110_lock, the new mutex_unlock()
wakes it. On the same CPU (always the case on UP), it can preempt the
opener, which holds RTNL, before the opener reaches free_irq().

The opener then only runs again through RT throttling or the fair
dl_server. That bounds the stall to about one RT period by default, but
the stall can be indefinite if those safeguards are disabled.

Would it be better to quiet the chip while still holding the mutex,
before force_quit is set and the lock is dropped? That could mirror
what hi3110_stop() does:

    hi3110_write(spi, HI3110_WRITE_CTRL1, 0x0);
    hi3110_write(spi, HI3110_WRITE_INTE, 0x0);
    hi3110_read(spi, HI3110_READ_INTF);

Another option is calling disable_irq_nosync() before mutex_unlock().

hi3110_stop() already uses the same force_quit then free_irq() ordering
while INTE is still enabled, so the same hazard exists there. In the
open() rollback path, though, it is new with this patch.

> +	mutex_lock(&priv->hi3110_lock);
>  	hi3110_hw_sleep(spi);
>   out_close:
>  	hi3110_power_enable(priv->transceiver, 0);

[Severity: Medium]
This is a pre-existing issue that this patch did not introduce, but is
priv->restart_work ever cancelled or flushed on shutdown?

hi3110_do_set_mode() queues it for CAN_MODE_START:

drivers/net/can/spi/hi311x.c:hi3110_do_set_mode() {
    ...
    priv->restart_tx = 1;
    if (priv->can.restart_ms == 0)
        priv->after_suspend = HI3110_AFTER_SUSPEND_RESTART;
    queue_work(priv->wq, &priv->restart_work);
    ...
}

hi3110_stop() calls close_candev(), which only cancels the CAN core's
delayed restart_work. It never calls
cancel_work_sync(&priv->restart_work), and neither does the hi3110_open()
rollback.

If the work is still pending at ifdown or during a failed open(),
hi3110_restart_work_handler() can run after cleanup. It takes
hi3110_lock and calls hi3110_hw_reset(), hi3110_setup() and
hi3110_set_normal_mode(), which re-arms INTE, TXEN and normal mode. It
then clears force_quit and calls netif_wake_queue() on the downed
interface.

The controller would then be back on the bus while the interface is
administratively down.

The new unlocked window around free_irq() in hi3110_open() gives the
worker one more place to run. It does not seem to make things worse than
the existing post-stop case, because free_irq() still completes and the
rollback then calls hi3110_hw_sleep().

There is no use-after-free here, because hi3110_can_remove() runs
destroy_workqueue() before free_candev().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009134323.64064-2-mkl%40pengutronix.de