[PATCH net] net: usb: asix: ax88772: replace usbnet_link_change() with queue_work() and usbnet_unlink_rx_urbs()

[PATCH net] net: usb: asix: ax88772: replace usbnet_link_change() with queue_work() and usbnet_unlink_rx_urbs()

[Markus Baier]

Commit 36bdc0e815b4 ("net: usb: asix: ax88772: re-add usbnet_link_change()
in phylink callbacks") restored the link-change notification that was
lost during the phylink migration, by calling usbnet_link_change() from
the phylink mac_link_up() / mac_link_down() callbacks.

While this fixed the original symptom (RX URB submission not being
initiated after link up), usbnet_link_change() also calls
netif_carrier_off() on link-down, which is redundant in a phylink-based
driver because phylink manages the carrier state itself.

Replace the usbnet_link_change() calls with the minimal operations:

  - In ax88772_mac_link_up(), schedule dev->bh_work directly via
    queue_work(system_bh_wq, &dev->bh_work). This is the same
    mechanism usbnet_open() uses to schedule the bottom-half
    that submits RX URBs.

  - In ax88772_mac_link_down(), call usbnet_unlink_rx_urbs() to
    return any in-flight RX URBs to the host controller.
    This releases USB bandwidth and avoids keeping unused buffers
    queued while the link is down. This is the symmetric
    counterpart to scheduling new RX URBs on link up.

Tested with the Apple A1277 USB Ethernet Adapter (05ac:1402,
AX88772A based) on a Banana Pro (Allwinner A20).

Fixes: 36bdc0e815b4 ("net: usb: asix: ax88772: re-add usbnet_link_change() in phylink callbacks")
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Markus Baier <Markus.Baier@soslab.tu-darmstadt.de>
---

Transparency notice: The English formulation of this commit message
was prepared with AI assistance. The actual system testing and
verification of the issue were performed manually without AI
involvement.

This is a follow-up to commit 36bdc0e815b4 (just applied to net.git).
While that patch correctly restored the missing RX URB submission,
I realized during further analysis that usbnet_link_change() carries
side effects (notably calling netif_carrier_off() on link-down) that
are redundant in a phylink-based driver. After studying how
usbnet_open() handles the same task (direct queue_work() call), this
minimal approach better matches the existing usbnet patterns.

I am sending this now rather than waiting, because the previous patch
just landed and I would like the cleanup to be considered before it
propagates further into stable kernels via the Fixes: tag.

 drivers/net/usb/asix_devices.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 293ef80c4e30..4230ff611c4b 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -756,7 +756,11 @@ static void ax88772_mac_link_down(struct phylink_config *config,
 	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
 
 	asix_write_medium_mode(dev, 0, 0);
-	usbnet_link_change(dev, false, false);
+
+	/* Phylink will call netif_carrier_off(), but we should explicitly
+	 * stop RX URBs to save USB bandwidth.
+	 */
+	usbnet_unlink_rx_urbs(dev);
 }
 
 static void ax88772_mac_link_up(struct phylink_config *config,
@@ -787,7 +791,11 @@ static void ax88772_mac_link_up(struct phylink_config *config,
 		m |= AX_MEDIUM_RFC;
 
 	asix_write_medium_mode(dev, m, 0);
-	usbnet_link_change(dev, true, false);
+
+	/* Phylink will call netif_carrier_on(), but we need to explicitly
+	 * kick off RX URB submission in usbnet.
+	 */
+	queue_work(system_bh_wq, &dev->bh_work);
 }
 
 static const struct phylink_mac_ops ax88772_phylink_mac_ops = {
-- 
2.52.0

Re: [PATCH net] net: usb: asix: ax88772: replace usbnet_link_change() with queue_work() and usbnet_unlink_rx_urbs()

[Oleksij Rempel]

On Wed, May 06, 2026 at 03:03:44AM +0200, Markus Baier wrote:
> Commit 36bdc0e815b4 ("net: usb: asix: ax88772: re-add usbnet_link_change()
> in phylink callbacks") restored the link-change notification that was
> lost during the phylink migration, by calling usbnet_link_change() from
> the phylink mac_link_up() / mac_link_down() callbacks.
> 
> While this fixed the original symptom (RX URB submission not being
> initiated after link up), usbnet_link_change() also calls
> netif_carrier_off() on link-down, which is redundant in a phylink-based
> driver because phylink manages the carrier state itself.
> 
> Replace the usbnet_link_change() calls with the minimal operations:
> 
>   - In ax88772_mac_link_up(), schedule dev->bh_work directly via
>     queue_work(system_bh_wq, &dev->bh_work). This is the same
>     mechanism usbnet_open() uses to schedule the bottom-half
>     that submits RX URBs.
> 
>   - In ax88772_mac_link_down(), call usbnet_unlink_rx_urbs() to
>     return any in-flight RX URBs to the host controller.
>     This releases USB bandwidth and avoids keeping unused buffers
>     queued while the link is down. This is the symmetric
>     counterpart to scheduling new RX URBs on link up.
> 
> Tested with the Apple A1277 USB Ethernet Adapter (05ac:1402,
> AX88772A based) on a Banana Pro (Allwinner A20).
> 
> Fixes: 36bdc0e815b4 ("net: usb: asix: ax88772: re-add usbnet_link_change() in phylink callbacks")
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Markus Baier <Markus.Baier@soslab.tu-darmstadt.de>

Tested on AX88772C. Thx!

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net] net: usb: asix: ax88772: replace usbnet_link_change() with queue_work() and usbnet_unlink_rx_urbs()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: usb: asix: ax88772: replace usbnet_link_change() with queue_work() and usbnet_unlink_rx_urbs()

This patch replaces the usbnet_link_change() calls in the ax88772
phylink mac_link_up()/mac_link_down() callbacks with direct
queue_work(system_bh_wq, &dev->bh_work) and usbnet_unlink_rx_urbs(),
with the stated rationale that phylink already manages the carrier
state. It is tagged Fixes: 36bdc0e815b4 ("net: usb: asix: ax88772:
re-add usbnet_link_change() in phylink callbacks").

> diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
> index 293ef80c4e30..4230ff611c4b 100644
> --- a/drivers/net/usb/asix_devices.c
> +++ b/drivers/net/usb/asix_devices.c

[ ... mac_link_down hunk snipped, not relevant to the comment below ... ]

> @@ -787,7 +791,11 @@ static void ax88772_mac_link_up(struct phylink_config *config,
>  		m |= AX_MEDIUM_RFC;
>  
>  	asix_write_medium_mode(dev, m, 0);
> -	usbnet_link_change(dev, true, false);
> +
> +	/* Phylink will call netif_carrier_on(), but we need to explicitly
> +	 * kick off RX URB submission in usbnet.
> +	 */
> +	queue_work(system_bh_wq, &dev->bh_work);
>  }

Can this reintroduce the "no RX until first TX" symptom that commit
805206e66fab ("net: asix: fix 'can't send until first packet is send'
issue") and the Fixes: target 36bdc0e815b4 addressed?

Looking at the ordering in phylink_link_up() in drivers/net/phy/phylink.c,
the mac_link_up() callback is invoked before netif_carrier_on():

	pl->mac_ops->mac_link_up(pl->config, pl->phydev, pl->act_link_an_mode,
				 pl->cur_interface, speed, duplex,
				 !!(link_state.pause & MLO_PAUSE_TX), rx_pause);

	if (pl->mac_supports_eee && pl->phy_enable_tx_lpi)
		phylink_activate_lpi(pl);

	if (ndev)
		netif_carrier_on(ndev);

So when ax88772_mac_link_up() runs queue_work(system_bh_wq, &dev->bh_work),
the netdev carrier is still off.

system_bh_wq is a WQ_BH workqueue, so queue_work() on it raises
TASKLET_SOFTIRQ, which can be serviced on the next hard-IRQ exit (or by
ksoftirqd) before phylink_link_up() reaches netif_carrier_on().

When bh_work runs, usbnet_bh() in drivers/net/usb/usbnet.c gates RX URB
submission on netif_carrier_ok():

	} else if (netif_running (dev->net) &&
		   netif_device_present (dev->net) &&
		   netif_carrier_ok(dev->net) &&
		   ...
		   !test_bit(EVENT_RX_HALT, &dev->flags)) {
		...
		if (temp < RX_QLEN(dev)) {
			if (rx_alloc_submit(dev, GFP_ATOMIC) == -ENOLINK)
				return;
			...
			if (dev->rxq.qlen < RX_QLEN(dev))
				queue_work(system_bh_wq, &dev->bh_work);
		}

If carrier is still off when usbnet_bh() runs, the rx_alloc_submit()
branch is skipped and bh_work does not reschedule itself. Once phylink
eventually calls netif_carrier_on(), nothing kicks bh_work again: the
in-flight RX URBs were unlinked by the prior ax88772_mac_link_down(),
so there are no URB completions to re-queue work either.

Does the previous usbnet_link_change() path avoid this race because
__handle_link_change() in drivers/net/usb/usbnet.c flips the carrier
before queuing work?

	if (test_and_clear_bit(EVENT_LINK_CARRIER_ON, &dev->flags))
		netif_carrier_on(dev->net);

	if (!netif_carrier_ok(dev->net)) {
		/* kill URBs for reading packets to save bus bandwidth */
		unlink_urbs(dev, &dev->rxq);
		...
	} else {
		/* submitting URBs for reading packets */
		queue_work(system_bh_wq, &dev->bh_work);
	}

Here bh_work only runs after carrier_ok() is already true, so the gate
in usbnet_bh() cannot close on it.

Given the Fixes: tag targets -stable, is there a concern that this
patch partly reintroduces the original "RX silent until first TX or
link bounce" regression?

Would scheduling bh_work from a phylink_resolve() follow-up, or keeping
the usbnet_link_change() path (which sets EVENT_LINK_CARRIER_ON and
lets __handle_link_change() order carrier-on before queue_work), be a
safer sequencing?

As a side note, the comment added by this hunk states "Phylink will
call netif_carrier_on(), but we need to explicitly kick off RX URB
submission in usbnet." Should the comment reflect that phylink calls
netif_carrier_on() after this callback returns, rather than before, so
the ordering assumption is explicit?
-- 
pw-bot: cr