Re: [PATCH] net: usb: cdc_ether: add quirk for AMI BMC stale link events

[Jinhui Guo]

Hi,

To help review this patch, here is the runtime evidence captured on
one of the affected Genoa boxes. All timestamps below are from the
same failing boot; I have trimmed unrelated lines and replaced host-
specific addresses with placeholders.

1. usbmon: the two back-to-back NETWORK_CONNECTION notifications

Right after enumeration the BMC gadget delivers two class
notifications on the interrupt endpoint within ~130us of each other.
Both are USB_CDC_NOTIFY_NETWORK_CONNECTION (bNotificationType = 0x00,
bmRequestType = 0xa1); the first has wValue = 0 (link OFF), the
second has wValue = 1 (link ON):

ffff9c48c... Ii:1:007:1 -115:1 8
ffff9c48c... Ii:1:007:1 0:1 8 = a1000000 00000000 0000        <- OFF
ffff9c48c... Ii:1:007:1 -115:1 8
ffff9c48c... Ii:1:007:1 0:1 8 = a1000000 01000000 0000        <- ON

Timing between the two Ii completions is ~130us. There is no
carrier-off event in between and no earlier NETWORK_CONNECTION was
seen, so the OFF is purely a stale event generated by the BMC
firmware on cold boot.

2. Why the OFF is not filtered out

At the moment the OFF arrives, alloc_netdev() has already run but
netif_carrier_off() has not, so __LINK_STATE_NOCARRIER is clear and
netif_carrier_ok() returns 1. usbnet_cdc_status() therefore takes
the wValue-differs branch and calls usbnet_link_change(dev, 0, 0),
which schedules EVENT_LINK_CHANGE. This is confirmed by tracing at
the cdc_ether entry:

[Test] CDC status: set carrier 0, current carrier 1     <- OFF, propagated
[Test] CDC status: set carrier 1, current carrier 0     <- ON, propagated

The ~130us window between the two events is much shorter than the
kevent workqueue latency, so both are queued before
__handle_link_change() runs and are processed in order.

3. xHCI trace: rx URBs killed with data already DMA'd

When __handle_link_change() processes the OFF it takes the
!netif_carrier_ok branch and calls unlink_urbs() on dev->rxq (~60
URBs at the moment of failure). The xhci_ring events show that
several TRBs are already in a completed state when Stop Ring runs;
their Transfer Events carry a non-zero transferred length and are
delivered to the driver as -ECONNRESET:

xhci_handle_command:    Stop Ring, slot 3 ep 1
xhci_handle_transfer:   TRB[...] type 'Transfer' len 1514 status 'Stopped'
xhci_urb_giveback:      ep1in-bulk urb ffff8a... status -108 -> -104 length 1514/1514
xhci_urb_giveback:      ep1in-bulk urb ffff8a... status -104 length 1500/1514
...

(-104 == -ECONNRESET, -108 == -ESHUTDOWN. Both cases hit the same
goto block in rx_complete().)

rx_complete() unconditionally drops these frames:

switch (urb_status) {
case -ECONNRESET:
case -ESHUTDOWN:
        goto block;
}

so the ARP reply that happened to have been DMA'd into one of these
URBs just before Stop Ring is discarded, even though the payload was
already resident in memory. The subsequent ON re-arms the rx queue,
but by then the reply is gone and the interface stays "up but
silent" until the peer times out and re-ARPs, or until the operator
runs ifdown/ifup. This matches the observed intermittent behaviour:
whether the ARP reply falls into the tiny "DMA done, giveback not
yet" window is a race, so the failure only reproduces on some
fraction of AC cold boots.

4. Why DC reboots don't hit this

On DC reboot the BMC USB gadget state is preserved, so the stale
OFF/ON pair is not queued and the sequence above never starts. This
matches every failing boot we captured being an AC cold boot.

5. Why FLAG_LINK_INTR fixes it

With FLAG_LINK_INTR the probe path calls
usbnet_link_change(dev, 0, 0), which sets carrier off before
register_netdev() exposes the netdev. When the stale OFF arrives,
usbnet_cdc_status() sees netif_carrier_ok() == !!event->wValue
(both 0) and returns without calling usbnet_link_change(), so no
EVENT_LINK_CHANGE is queued and unlink_urbs() never runs. The
following ON is the first real state change and brings the link up
cleanly, without ever tearing down the rx queue.

I chose to add this as a device quirk rather than modifying cdc_info
because commit 71cc1fa9f2d7 ("cdc_ether: Set FLAG_LINK_INTR for all
devices with a status endpoint") was reverted in the same series
after it broke CDC devices that never send NETWORK_CONNECTION
notifications; restricting FLAG_LINK_INTR to 046b:ffb0 keeps those
devices untouched.

Verified across 100+ AC cold boot cycles on both Genoa and Turin
reference boards: first-packet ping success went from intermittent
to 100%. DC reboot and ifdown/ifup paths are unchanged.

Happy to provide fuller usbmon / xhci trace captures if that would
help.

Thanks,
Jinhui