Re: [PATCH net-next v10 00/15] ax88179_178a: Add support for AX88179A-based chips

Hi Birger,

I tested v9 on the same ASIX AX88179B adapter. By the time you read this,
v10 has already been posted. Given the relatively small differences between
v9 and v10, I expect the issue I found in v9 to apply to v10 as well.

First, as you already noticed, `data` is undeclared.

For QEMU runtime testing I also had to apply Chen-Yu Tsai's unrelated
`usb: xhci: Fix HCS_ERST_MAX conversion` patch. Without it, this base kernel
fails to initialize QEMU's xHCI controller before the network driver is
reached.

All three fresh functional starts completed cold DHCP at 1000baseT/Full
without reloading the driver and passed the normal 1000/100/10/1000 Mbit/s
matrix, EEE disable/restore, pause enable/restore, and EEPROM read.

During repeated speed transitions, I observed one intermittent carrier-loss
failure. Of 19 normally initiated restores from 100/full to the default
1000/full advertisement, 18 passed and one failed to regain carrier. After
that failure, ethtool reported unknown speed and no link, and further
advertisement changes did not recover it. Dmesg showed the preceding
100-Mbit Link Up followed by Link Down, with no subsequent Link Up before
the device was reattached.

I could not reproduce the failure in two later fresh starts or in a further
90 unmodified-v9 stress cycles. Counting the normal matrix and stress
points, all 22 tested 100/full points passed, so I also did not reproduce
the earlier 100-Mbit carrier-without-RX failure.

I repeated the QEMU deep-S3 tests as well. One fresh `wol d` cycle and five
fresh `wol g` cycles all passed. After `system_wakeup`, management SSH
returned, the adapter regained carrier at 1000baseT/Full, and gateway and
test-host traffic increased RX. These tests use QEMU-emulated xHCI and
monitor-triggered wake, not a physical xHCI controller or an actual magic
packet.

I then investigated the intermittent carrier loss with function tracing.
Each advertisement change generates hardware link-down and link-up status
notifications. V9 forwards both to phylib even though the ethtool-triggered
PHY state-machine run has already taken the link down before the hardware
down notification is handled. This results in three PHY state/read-status
runs per advertisement change, including a redundant read while
autonegotiation and controller link setup are still in progress.

In light of your previous observation [1]:

There is a race condition between the controller of the AX88179A
trying to set up and optimize the link and phylink trying to
configure the link on the mac-side.

At this point, the PHY may report that the link is up before the
controller is actually finished configuring it.

I suspected that the redundant PHY read during autonegotiation might
interact badly with the controller's own link-setup sequence.

As a focused experiment, I changed ax88179a_status() to inspect
AX_INT_PPLS_LINK and track whether phylink currently considers the MAC link
active. A down notification is ignored if phylink has already taken the MAC
down, while up notifications are always forwarded. mac_link_up() marks the
state active before configuring the MAC, so a genuine down event during
configuration is still forwarded.

The experimental kernel built successfully, including focused W=1 checks,
and strict checkpatch reported no findings. I then ran:

- 100 rapid 100/full -> 1000/full cycles;
- 30 paced cycles, holding each endpoint for 8 seconds;
- a 2-cycle trace smoke test.

All 264 requested-speed endpoints reached carrier at the correct speed. The
fixed traces consistently showed four raw status callbacks but only two
phylink interrupts and four PHY reads per complete cycle.

But this does not necessarily prove that the change fixes the original rare
failure: that failure occurred only once in 19 normally initiated restores
and could not be reproduced in another 90 unmodified-v9 stress cycles.

The experimental patch follows for review.

Thanks,
Jianhui

[1] https://lore.kernel.org/netdev/5b2c4498-2e3c-4ae6-b078-deeccf8b7a5c@birger-koblitz.de/ (local)

---
 drivers/net/usb/ax88179_lib.h      |  1 +
 drivers/net/usb/ax88179a_devices.c | 19 +++++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/ax88179_lib.h b/drivers/net/usb/ax88179_lib.h
--- a/drivers/net/usb/ax88179_lib.h
+++ b/drivers/net/usb/ax88179_lib.h

@@ -306,6 +306,7 @@ struct ax88179_data {
 	u8 is_ax88772d;
 	u8 ip_align;
 	u8 link;
+	bool mac_link_active;
 	u8 speed;
 	u8 full_duplex;
 	u8 rx_checksum;

diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
--- a/drivers/net/usb/ax88179a_devices.c
+++ b/drivers/net/usb/ax88179a_devices.c

@@ -118,11 +118,22 @@ static int ax88179_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
 static void ax88179a_status(struct usbnet *dev, struct urb *urb)
 {
 	struct ax88179_data *data = dev->driver_priv;
+	struct ax88179_int_data *event;
+	bool link;
 
 	if (urb->actual_length < 8)
 		return;
 
-	phylink_mac_interrupt(data->phylink);
+	event = urb->transfer_buffer;
+	link = le32_to_cpu(event->intdata1) & AX_INT_PPLS_LINK;
+
+	/* Changing the advertisement has already told phylib that the link is
+	 * down. Avoid another PHY read while the controller is still setting up
+	 * the new link, but always process link-up notifications so that speed
+	 * changes cannot be missed.
+	 */
+	if (link || READ_ONCE(data->mac_link_active))
+		phylink_mac_interrupt(data->phylink);
 }
 
 static int ax88179a_suspend(struct usb_interface *intf, pm_message_t message)

@@ -437,7 +448,9 @@ static void ax88179a_mac_config(struct phylink_config *config, unsigned int mode,
 static void ax88179a_mac_link_down(struct phylink_config *config,
 				   unsigned int mode, phy_interface_t interface)
 {
-	/* Nothing to do */
+	struct ax88179_data *data = netdev2data(to_net_dev(config->dev));
+
+	WRITE_ONCE(data->mac_link_active, false);
 }
 
 static void ax88179a_mac_link_up(struct phylink_config *config,

@@ -448,9 +461,11 @@ static void ax88179a_mac_link_up(struct phylink_config *config,
 {
 	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
 	struct ax88179_data *ax179_data = dev->driver_priv;
 	u8 tmp8, link_sts, reg8[3];
 	u8 bulk_config_speed = 0;
 	u16 tmp16, mode;
 
+	WRITE_ONCE(ax179_data->mac_link_active, true);
+
 	/* Stop RX/TX for link configuration */
 	tmp16 = AX_RX_CTL_STOP;