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