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 weekDetails
- 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
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
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.