net: macb: quiesce IRQs and drain BH on interface close

Browse source

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.

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Link: https://patch.msgid.link/20260925-macb-close-v2-3-360efa565914@bootlin.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

...

This commit is contained in:

Théo Lebrun 2026-09-25 15:59:36 +02:00 • committed by Jakub Kicinski

parent 9ea465c4fb

commit 4fcaed2cb1

2 changed files with 69 additions and 20 deletions

Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL

Download patch file Download diff file Expand all files Collapse all files

5

drivers/net/ethernet/cadence/macb.h

View file

84

drivers/net/ethernet/cadence/macb_main.c

View file