Page MenuHomeFreeBSD
Differential D55682

ure: fix spurious link flaps from MII
AcceptedPublic
Actions

Authored by rkitover_gmail.com on Fri, Mar 6, 1:27 AM.
Tags
Referenced Files
F147512090: D55682.id173248.diff
Wed, Mar 11, 1:26 PM
F147485204: D55682.id.diff
Wed, Mar 11, 8:57 AM
Unknown Object (File)
Wed, Mar 11, 5:33 AM
Unknown Object (File)
Tue, Mar 10, 12:59 AM
Unknown Object (File)
Mon, Mar 9, 2:08 PM
Unknown Object (File)
Sun, Mar 8, 4:23 AM
Unknown Object (File)
Sat, Mar 7, 1:45 PM
Unknown Object (File)
Sat, Mar 7, 7:38 AM
View All 16 Files
Subscribers
imp
pouria

Details

Reviewers
adrian
kevlo
pouria
Group Reviewers
USB
network
Summary

A race condition in the MII layer causes spurious link down events, see:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=252165

. In statchg, on link down, check if the PHY reports the link as
actually down using the BMSR register, if not, force the status of the
link to back up and restart TX. Do the same in a MII linkchg handler.

On actual link up, restart TX in case it went idle and down.

I have tested this patch with a Realtek 8153 USB interface, and it fixes
the link flaps.

Signed-off-by: Rafael Kitover <rkitover@gmail.com>

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 71348
Build 68231: arc lint + arc unit

Event Timeline

rkitover_gmail.com created this revision.Fri, Mar 6, 1:27 AM
Herald added a subscriber: imp. · View Herald TranscriptFri, Mar 6, 1:27 AM
rkitover_gmail.com requested review of this revision.Fri, Mar 6, 1:27 AM
Harbormaster completed remote builds in B71213: Diff 173248.Fri, Mar 6, 1:27 AM
rkitover_gmail.com updated this revision to Diff 173249.Fri, Mar 6, 1:27 AM
rkitover_gmail.com added a parent revision: D55631: axge: fix spurious link down from MII, frozen TX.

Add Differential Review link to commit message

Harbormaster completed remote builds in B71214: Diff 173249.Fri, Mar 6, 1:28 AM
rkitover_gmail.com added projects: USB, network.Fri, Mar 6, 1:29 AM
rkitover_gmail.com added reviewers: adrian, USB, network.
lwhsu added a reviewer: kevlo.Fri, Mar 6, 2:25 AM
rkitover_gmail.com updated this revision to Diff 173252.Fri, Mar 6, 3:38 AM

Remove frozen TX fix, only needed for axge

Harbormaster completed remote builds in B71217: Diff 173252.Fri, Mar 6, 3:38 AM
pouria added a subscriber: pouria.Fri, Mar 6, 11:29 AM
pouria added inline comments.
sys/dev/usb/net/if_ure.c
536

style.9

rkitover_gmail.com updated this revision to Diff 173261.Fri, Mar 6, 12:25 PM

Fix for style(9)

Harbormaster completed remote builds in B71226: Diff 173261.Fri, Mar 6, 12:25 PM
rkitover_gmail.com updated this revision to Diff 173264.Fri, Mar 6, 12:38 PM

Update for style(9)

Harbormaster completed remote builds in B71228: Diff 173264.Fri, Mar 6, 12:38 PM
rkitover_gmail.com marked an inline comment as done.Fri, Mar 6, 12:41 PM

Thank you. I also have:

https://reviews.freebsd.org/D55631

pouria added a comment.Fri, Mar 6, 9:44 PM

Unfortunately, even after applying your patch, I still have the issue:

% mdo tail -n 500 /var/log/messages | grep -E 'ue0|8153'
Mar  7 00:57:55 cornelia kernel: rgephy0: <RTL8251/8153 1000BASE-T media interface> PHY 0 on miibus0
Mar  7 00:57:55 cornelia kernel: ue0: <USB Ethernet> on ure0
Mar  7 00:57:55 cornelia kernel: ue0: Ethernet address: 00:e0:4c:a4:4e:ef
Mar  7 00:57:55 cornelia kernel: ue0: link state changed to DOWN
Mar  7 00:58:16 cornelia kernel: ue0: link state changed to UP
Mar  7 01:01:44 cornelia kernel: ue0: link state changed to DOWN

usbconfig:

ugen0.14: <RTL8153 Gigabit Ethernet Adapter Realtek Semiconductor Corp.> at usbus0, cfg=0 md=HOST spd=SUPER (5.0Gbps) pwr=ON (28mA)

bLength = 0x0012
bDescriptorType = 0x0001
bcdUSB = 0x0320
bDeviceClass = 0x0000  <Probed by interface class>
bDeviceSubClass = 0x0000
bDeviceProtocol = 0x0000
bMaxPacketSize0 = 0x0009
idVendor = 0x0bda
idProduct = 0x8153
bcdDevice = 0x3100
iManufacturer = 0x0001  <retrieving string failed>
iProduct = 0x0002  <retrieving string failed>
iSerialNumber = 0x0007  <retrieving string failed>
bNumConfigurations = 0x0002

Also, shortly after plugging in the usb dock, ifconfig ue0 takes about ~2 seconds to respond with the media status:

% time ifconfig ue0
ue0: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
options=68009b<RXCSUM,TXCSUM,VLAN_MTU,VLAN_HWTAGGING,VLAN_HWCSUM,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
ether 00:e0:4c:a4:4e:ef
media: Ethernet autoselect (none <half-duplex>)
status: no carrier
nd6 options=823<PERFORMNUD,ACCEPT_RTADV,AUTO_LINKLOCAL,STABLEADDR>
ifconfig ue0  0.00s user 0.01s system 0% cpu 1.687 total
rkitover_gmail.com added a comment.Fri, Mar 6, 9:56 PM

The initial link flaps on device initialization is not fixed by this patch, but, if you run with and without the patch for like an hour or two, you will see the difference.

I don't know about the ifconfig slowness, may be MII related, I will investigate.

pouria added a comment.Fri, Mar 6, 9:57 PM

Despite the Ethernet port being connected, after replugging in the USB dock it reports that ue0 is DOWN, even though the Ethernet lights are on and the other Ethernet endpoint reports connected and UP:

% mdo tail -f /var/log/messages
Mar  7 01:20:09 cornelia kernel: ugen0.11: <GenesysLogic USB3.1 Hub> at usbus0
Mar  7 01:20:09 cornelia kernel: uhub3 on uhub0
Mar  7 01:20:09 cornelia kernel: uhub3: <GenesysLogic USB3.1 Hub, class 9/0, rev 3.20/6.63, addr 18> on usbus0
Mar  7 01:20:09 cornelia kernel: uhub3: 4 ports with 4 removable, self powered
Mar  7 01:20:10 cornelia kernel: ugen0.12: <Realtek USB 10/100/1000 LAN> at usbus0
Mar  7 01:20:10 cornelia kernel: ure0 on uhub3
Mar  7 01:20:10 cornelia kernel: ure0: <Realtek USB 10/100/1000 LAN, class 0/0, rev 3.20/31.00, addr 19> on usbus0
Mar  7 01:20:10 cornelia kernel: miibus0: <MII bus> on ure0
Mar  7 01:20:10 cornelia kernel: rgephy0: <RTL8251/8153 1000BASE-T media interface> PHY 0 on miibus0
Mar  7 01:20:10 cornelia kernel: rgephy0:  none, 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, 1000baseT-FDX, 1000baseT-FDX-master, auto
Mar  7 01:20:10 cornelia kernel: ue0: <USB Ethernet> on ure0
Mar  7 01:20:10 cornelia kernel: ue0: Ethernet address: 00:e0:4c:a4:4e:ef
Mar  7 01:20:10 cornelia kernel: ue0: link state changed to DOWN
Mar  7 01:20:10 cornelia kernel: ugen0.13: <GenesysLogic USB2.1 Hub> at usbus0
Mar  7 01:20:10 cornelia kernel: uhub4 on uhub0
Mar  7 01:20:10 cornelia kernel: uhub4: <GenesysLogic USB2.1 Hub, class 9/0, rev 2.10/6.63, addr 20> on usbus0
Mar  7 01:20:11 cornelia kernel: uhub4: 4 ports with 4 removable, self powered
pouria added a comment.Fri, Mar 6, 10:02 PM
In D55682#1274609, @pouria wrote:

Despite the Ethernet port being connected, after replugging in the USB dock it reports that ue0 is DOWN, even though the Ethernet lights are on and the other Ethernet endpoint reports connected and UP:

And now, after about eight minutes, ue0 suddenly reports that it's up.

Mar  7 01:28:17 cornelia kernel: ue0: link state changed to UP
In D55682#1274608, @rkitover_gmail.com wrote:

The initial link flaps on device initialization is not fixed by this patch, but, if you run with and without the patch for like an hour or two, you will see the difference.

I don't know about the ifconfig slowness, may be MII related, I will investigate.

I'm not expert in usb driver. I just want to help by testing it.

rkitover_gmail.com added a comment.Fri, Mar 6, 11:01 PM

Can you please verify which behaviors happen for you with and without the patch?

pouria added a comment.Mon, Mar 9, 4:12 PM
In D55682#1274611, @rkitover_gmail.com wrote:

Can you please verify which behaviors happen for you with and without the patch?

Done.
It seems your patch decrease the flapping rate.

Mar  9 19:37:25 cornelia kernel: ue0: link state changed to UP
Mar  9 19:37:25 cornelia kernel: ue0: 2 link states coalesced
Mar  9 19:37:25 cornelia kernel: ue0: link state changed to UP
Mar  9 19:37:25 cornelia kernel: ue0: 2 link states coalesced
Mar  9 19:37:25 cornelia kernel: ue0: link state changed to UP

Without your patch, it stops at UP state.
With the patch, it can flap to DOWN, and I have to wait for the next random flap for it to become UP.

rkitover_gmail.com added a comment.Mon, Mar 9, 11:54 PM

My results are different, are you applying to CURRENT?

In my case, after the first 4 link flaps there are no more. Your link should not be going down at all.

Is it possible your dongle is defective?

pouria accepted this revision as: pouria.Tue, Mar 10, 11:28 AM
In D55682#1276189, @rkitover_gmail.com wrote:

My results are different, are you applying to CURRENT?

Yes

In my case, after the first 4 link flaps there are no more. Your link should not be going down at all.

It rarely happens.

Is it possible your dongle is defective?

It's a dock for my steam deck and it works fine with linux and windows.

To be clear, my comments are not meant to block this revision. instead, this change is LGTM.
But it doesn't completely fix the issue. I wanted you to be aware of that.

sys/dev/usb/net/if_ure.c
447–449

use bool for new_link and old_link

505–507

Up to your judgement, but try to not break strings as much as possible.

545

style: Do not test without a comparison, or with unary ! (except for booleans).

This revision is now accepted and ready to land.Tue, Mar 10, 11:28 AM
rkitover_gmail.com added a comment.Tue, Mar 10, 4:27 PM

Thanks!

So apparently the issue still manifests for you due to some kind of configuration difference between our systems.

I'd like to fix this as well.

Can you give me your log? Do a:

grep -E 'ure|ue0' /var/log/messages

, may be ue1 depending on configuration.

pouria added a comment.Tue, Mar 10, 6:01 PM
In D55682#1276410, @rkitover_gmail.com wrote:

Thanks!

So apparently the issue still manifests for you due to some kind of configuration difference between our systems.

I'd like to fix this as well.

Can you give me your log? Do a:

grep -E 'ure|ue0' /var/log/messages

, may be ue1 depending on configuration.

I've to apply your patch again and wait to see the new logs.
Will tell you.

rkitover_gmail.com added a comment.Wed, Mar 11, 3:25 PM

Sorry about the style violations, I will read the guide more carefully and adjust both DRs.

I'm working on one for axe now, that's the third remaining common USB Ethernet interface using MII, once we finish axge and this one (ure) that will be all of them.

pouria added a comment.Wed, Mar 11, 5:16 PM
In D55682#1276829, @rkitover_gmail.com wrote:

Sorry about the style violations, I will read the guide more carefully and adjust both DRs.

don't worry about it. we all make those mistakes.
that's why we review each other's work.

rkitover_gmail.com updated this revision to Diff 173585.Wed, Mar 11, 8:20 PM

Fix for style(9)

This revision now requires review to proceed.Wed, Mar 11, 8:20 PM
Harbormaster completed remote builds in B71348: Diff 173585.Wed, Mar 11, 8:20 PM
rkitover_gmail.com marked 2 inline comments as done.Wed, Mar 11, 8:21 PM
pouria accepted this revision as: pouria.Wed, Mar 11, 8:57 PM
This revision is now accepted and ready to land.Wed, Mar 11, 8:57 PM

Revision Contents
Changeset List

PathSize
sys/
dev/
usb/
net/
69 lines

Diff 173585

View Options

sys/dev/usb/net/if_ure.c

Loading...