Re: [PATCH 3/3] net: macb: quiesce IRQs and drain BH on interface close

[Nicolai Buchwitz]

On 18.9.2026 22:32, Théo Lebrun wrote:
> The macb_close() operation is facing races as it disables IRQs late in
> its sequence and keeps BH primitives alive while shutdown.
>
> Non exhaustive list of races that could occur:
>
>  - macb_tx_error_task() could be scheduled and access the buffers freed
>    by macb_close().
>
>  - macb_tx_error_task() or macb_hresp_error_task() might re-enable
>    interrupts after the IDR write in macb_close().
>
>  - macb_close() calls napi_disable() meaning that if
>    macb_tx_error_task() occurs later, it will deadlock on 
> napi_disable()
>    that shouldn't be called if NAPI is already disabled.
>
>  - macb_hresp_error_task() might reinit every RX/TX ring under
>    macb_close()'s foot.
>
>  - macb_interrupt() might re-enable NAPI just after it has been
>    disabled by macb_close().
>
> Instead, disable all our primitives one by one:
>  - (1) mask and sync on IRQ handlers,
>  - (2) drain any scheduled bp->hresp_err_bh_work,
>  - (3) drain any scheduled queue->tx_error_task,
>  - (4) drain queue->napi_rx/napi_tx,
>  - (5) drain bp->tx_lpi_work.
>
> Careful! Ordering is important because our scheduling primitives can
> wake each other up. Recap table:
>
> |               |   enable/disable   |           schedule            |
> |               |----|-------|-------|------|----|----|--------|-----|
> |               |IRQs|napi_tx|napi_rx|tx_lpi|napi|napi|tx_error|hresp|
> | Context       |    |       |       | task | rx | tx |  task  |task |
> |===============|====|=======|=======|======|====|====|========|=====|
> | open          | X  |   X   |   X   |      |    |    |        |     |
> | link_up       | X  |       |       |      |    |    |        |     |
> | link_down     | X  |       |       |      |    |    |        |     |
> | close         | X  |   X   |   X   |      |    |    |        |     |
> | enable_tx_lpi |    |       |       |  X   |    |    |        |     |
> | swap          | X  |   X   |   X   |  X   |    |    |        |     |
> | suspend       | X  |   X   |   X   |      |    |    |        |     |
> | resume        | X  |   X   |   X   |      |    |    |        |     |
> |---------------|----|-------|-------|------|----|----|--------|-----|
> | irq & netpoll | X  |       |       |      | X  | X  |   X    | X   |
> |---------------|----|-------|-------|------|----|----|--------|-----|
> | napi_rx       | X  |       |       |      | X  |    |        |     |
> | napi_tx       | X  |       |       |  X   |    | X  |        |     |
> |---------------|----|-------|-------|------|----|----|--------|-----|
> | tx_error_task | X  |   X   |       |      |    |    |        |     |
> | hresp task    | X  |       |       |      |    |    |        |     |
>
> As example, one ordering constraint that can be deduced from the table:
> napi_tx can schedule tx_lpi_task meaning napi_tx must be disabled
> before tx_lpi_task, else we risk napi_tx re-enabling tx_lpi_task after
> it has been disabled by macb_close().
>
> We do *not* use IDR masking to shutdown IRQs because that risks
> conflicting with BH primitives we have not disabled yet. For example if
> we writel(IDR) in macb_close() and napi_rx is pending then IRQs might
> be unmasked by the NAPI poll. As for why we do not use disable_irq():
> we will have situations where we are quiesced but want to listen to
> some IRQs and (minor reason) we register shared IRQ handlers so we
> shouldn't disable the full IRQ line.
>
> Instead we introduce a bool that tells macb_interrupt() to self-disarm.
> Its default value is true as we start closed. It gets set to false
> while interface is active. Reading into my crystal ball, we'll reuse
> that flag in suspend/WOL, set_ringparam and change_mtu (context swap).
>
> Note that old IRQ handler tried preventing a race with close by
> self-disarming based on netif_running(). This might work, but it does
> not prevent a race with the error codepath of macb_open() which needs
> to run with IRQs dis-armed but netif_running() returns true during that
> time.
>
> Fixes: e86cd53afc59 ("net/macb: better manage tx errors")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> ---

> [...]

Reviewed-by: Nicolai Buchwitz <nb@xxxxxxxxxxx>

Regards
Nicolai