Re: [PATCH] net: macb: fix race condition in TX queue initialization

[Ingo Rohloff <ingo.rohloff@lauterbach.com>]

Subject: Re: [PATCH] net: macb: fix race condition in TX queue initialization
Date: Thu, 19 Feb 2026 14:43:38 -0800

> > On Wed, 18 Feb 2026 11:43:25 +0100 Ingo Rohloff wrote:
> > Commit bf9cf80cab81e ("net: macb: Fix tx/rx malfunction after phy link
> > down and up") initializes queue->tx_head and queue->tx_tail to zero
> > without holding queue->tx_ptr_lock.

> That commit does look quite buggy, but I'm not sure whether the fix is
> effective.

Yes: I myself think that what I posted is definitely not enough.

I am not sure if I can come up with a real fix though:

Looking at ethernet drivers which use both:
   * phylink_mac_ops->mac_link_down()
   * net_device_ops->ndo_open()
the most commonly used strategy seems to be:
* When net_device_ops->ndo_open() is called:
  - Setup ring buffers
  - Call netif_tx_start_all_queues()
* DO NOT call netif_tx_stop_all_queues() or similar when
  phylink_mac_ops->mac_link_down() is called
* DO NOT re-initialize ring buffers when
  phylink_mac_ops->mac_link_up() is called.

So: Leave the ring buffers alone in
phylink_mac_ops->mac_link_up() and phylink_mac_ops->mac_link_down().

The driver in macb_main.c seems to be one of the exceptions
which calls netif_tx_stop_all_queues() in macb_mac_link_down() and
netif_tx_wake_all_queues() in macb_mac_link_up().

My problem is:
I don't understand why the OTHER drivers don't do it like this.
What's the expectation of the hardware, if I fill the TX ring buffer
with TX skb's even if the link is down?
Is the hardware supposed to throw all these TX entries away
and still produce completion or error(?) interrupts?
Or is this handled, because drivers which use "phylink",
automatically call netif_carrier_on() and netif_carrier_off() via
"phylink" when the link changes up/down state?


If you know the expected behavior:
Could you teach me what's expected ?

If I understand the expected behavior I might be able to come up
with an idea what the macb driver is supposed to do :-)


> But the fix should be locking and fixing whatever
> questionable dark arts the link_up handler is performing on the queues
> (we are resetting the pointers without freeing outstanding skbs!?).

Yeah: The last one also caught my eye; I think this might produce
an ever growing bunch of outstanding skb's.

I think the reason why the macb driver is unusual is,
because the hardware itself resets the pointers into the
TX ring buffer, when you disable transmit; which means you somehow
have to clean up the TX ring buffer after a link down
(if you disable transmit).

Most of the other drivers I have looked at, somehow seem to expect
that this cleanup happens automagically; but I have no clue why or
how this cleanup happens for other hardware.

I will be very thankful for any input what's the expected behavior!

so long
  Ingo



PS:
Just for reference,
I looked into the following drivers which use both:
    ->ndo_open() and ->mac_link_down()
not many surprisingly(?):

ethernet/altera/altera_tse_main.c
ethernet/atheros/ag71xx.c
ethernet/cadence/macb_main.c
ethernet/freescale/enetc/enetc4_pf.c
ethernet/freescale/enetc/enetc_pf.c
ethernet/freescale/fs_enet/fs_enet-main.c
ethernet/freescale/ucc_geth.c
ethernet/marvell/mvneta.c
ethernet/marvell/mvpp2/mvpp2_main.c
ethernet/marvell/prestera/prestera_main.c
ethernet/mediatek/mtk_eth_soc.c
ethernet/microchip/lan743x_main.c
ethernet/mscc/ocelot_net.c
ethernet/stmicro/stmmac/stmmac_main.c
ethernet/ti/am65-cpsw-nuss.c
ethernet/xilinx/xilinx_axienet_main.c


Drivers which stop netif tx queues in mac_link_down()
Note: Seems like these still start
with enabled tx queues in ->ndo_open() ?

   ethernet/microchip/lan743x_main.c
   ethernet/marvell/mvpp2/mvpp2_main.c
   ethernet/ti/am65-cpsw-nuss.c


Drivers which DO NOT stop netif tx queues in mac_link_down()

   ethernet/altera/altera_tse_main.c
   ethernet/atheros/ag71xx.c
   ethernet/freescale/enetc/enetc.c
   ethernet/marvell/mvneta.c
   ethernet/marvell/prestera/prestera_main.c
   ethernet/mediatek/mtk_eth_soc.c
   ethernet/mscc/ocelot_net.c
   ethernet/stmicro/stmmac/stmmac_main.c
   ethernet/xilinx/xilinx_axienet_main.c

And:
   ethernet/freescale/ucc_geth.c
Which seems to be another potential start_xmit/link_up race,
albeit with fancy handling. Look for usage of ugeth_quiesce()



-------------------------------------------------------------------------
Dipl.-Inform.
Ingo ROHLOFF
Senior Staff Embedded Systems Engineering
phone +49 8102 9876-142 - ingo.rohloff@lauterbach.com

Lauterbach Engineering GmbH & Co. KG
Altlaufstrasse 40, 85635 Hoehenkirchen-Siegertsbrunn, GERMANY
www.lauterbach.com

Registered Office: Hoehenkirchen-Siegertsbrunn, Germany,
Local Court: Munich, HRA 87406, VAT ID: DE246770537,
Managing Directors: Lothar Lauterbach, Stephan Lauterbach, Dr. Thomas Ullmann

-------------------------------------------------------------------------