Page MenuHomeFreeBSD

e1000: Defer link-up notification until after TSO reset
AbandonedPublic

Authored by kbowling on Thu, Jul 23, 9:51 PM.
Tags
None
Referenced Files
F164578401: D58425.id182655.diff
Sun, Aug 2, 4:38 AM
Unknown Object (File)
Sat, Aug 1, 5:38 AM
Unknown Object (File)
Sat, Aug 1, 2:01 AM
Unknown Object (File)
Fri, Jul 31, 4:04 AM
Unknown Object (File)
Thu, Jul 30, 1:30 PM
Unknown Object (File)
Thu, Jul 30, 5:40 AM
Unknown Object (File)
Wed, Jul 29, 11:50 PM
Unknown Object (File)
Wed, Jul 29, 10:16 PM
Subscribers

Details

Reviewers
kfv_kfv.io
Group Reviewers
Intel Networking
Restricted Owners Package(Owns No Changed Paths)
Commits
rG0bd6a167c156: e1000: Defer link-up notification until after TSO reset
Summary
em_automask_tso() changes the enabled TSO capabilities when the link
moves between 10/100 and 1000 Mb/s.  A running interface must be
reinitialized to apply the new capability set.  Do not publish
LINK_STATE_UP until the requested iflib reset has completed.

Replace link_active with an explicit state machine that distinguishes
the physical link, its publication to iflib, and an outstanding reset
barrier.  Preserve that barrier across a link flap with
DOWN_RESET_PENDING, and only publish DOWN if UP was previously
published.

Only request a reset for a running interface or for an initialization
while the interface is administratively up.  In other states the next
initialization will apply the capability changes, avoiding a reset
request that iflib's admin task could discard.

Reported by:    Faraz Vahedi <kfv@kfv.io>
Fixes:          2ddf24f8f525 ("e1000: Automask TSO on lem(4)/em(4) 10/100 Ethernet")
MFC after:      1 week

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

Owners added a reviewer: Restricted Owners Package.Thu, Jul 23, 9:51 PM

The tri-state reads beautifully, and the link_was_published guard is a neat touch.

With this patch, on the normal path the driver recovers fine and the deferred UP gets republished. There's one thing I'd like to flag before we land it, though. As far as I can understand, recovery hinges entirely on em_if_init() running again after the automask's reset, and that isn't guaranteed. iflib_request_reset() just sets IFC_DO_RESET and the only thing that acts on it is _task_fn_admin(), which reads-and-clears the flag up top and then bails out with an early-return if the iface is neither IFF_DRV_RUNNING nor IFF_DRV_OACTIVE. So if the automasked link-up happens before the interface is actually RUNNING (possibly first bring-up or an attach racing the admin task), that reset just gets dropped. And since a periodic admin poll is a no-op while we're in UP_RESET_PENDING, there's no fallback, and the link latches down for good, which is the same failure mode the patch is trying to eliminate. As a diagnostic to prove what was eating the reset, I tried flipping IFLIB_ADMIN_ALWAYS_RUN on and off, where on makes the exact same case recover and off makes it stick. So it's the not-running early-return swallowing IFC_DO_RESET, right? Please correct me if I’m mistaken.

By the way, thanks a lot for asking my review, it means a lot.

kbowling edited the summary of this revision. (Show Details)

Handle edge cases reported by @kfv_kfv.io and add a fourth state to handle flap.

This beautifully addresses everything. Thanks, Kevin! Happy to see this land.