Page MenuHomeFreeBSD

aq(4): interface lifecycle and link-state fixes
Needs ReviewPublic

Authored by nick_spun.io on Mon, Jul 27, 2:28 AM.
Tags
None
Referenced Files
F164400575: D58473.id182750.diff
Fri, Jul 31, 1:57 PM
F164374870: D58473.diff
Fri, Jul 31, 7:01 AM
F164358623: D58473.id182750.diff
Fri, Jul 31, 3:59 AM
F164353046: D58473.diff
Fri, Jul 31, 2:49 AM
Unknown Object (File)
Thu, Jul 30, 6:02 AM
Unknown Object (File)
Thu, Jul 30, 6:01 AM
Unknown Object (File)
Thu, Jul 30, 5:20 AM
Unknown Object (File)
Thu, Jul 30, 3:44 AM
Subscribers

Details

Reviewers
adrian
Summary

aq_if_init() programmed the address captured at attach, so an address set
with "ifconfig ether" or by lagg(4) enslavement was never written to
unicast filter slot 0: the interface transmitted with the new address but
the MAC still filtered on the old one, so it received nothing. Copy the
current if_getlladdr() the way the other iflib drivers do.

The link state could latch UP forever. aq_if_stop() cleared linkup
before calling aq_if_update_admin_status(), which suppressed the
LINK_STATE_DOWN transition the "link was UP" branch would have made.
Announce the down transition directly from aq_if_stop() instead, and do
not poll the admin status there at all: the MAC has just been reset, so a
stale link reading would re-announce the link as up.

The admin task itself had to stop reporting a link on a stopped
interface. iflib runs it while either IFF_DRV_RUNNING or IFF_DRV_OACTIVE
is set, and iflib_stop() sets OACTIVE, so the task kept polling after the
stop and re-announced LINK_STATE_UP behind the driver's back. Treat a
non-running interface as having no link. A lagg(4) parent otherwise
keeps hashing flows onto a port whose carrier is gone, because
LAGG_PORTACTIVE tests if_link_state together with IFF_UP. Stop the rest
of the task there as well: the PHY thermal poll and the initialization
retry both end in iflib_request_reset(), and _task_fn_admin() acts on
that with no test of its own, so either could re-initialize an interface
the operator had just taken down.

aq_if_update_admin_status() also only reacted to transitions in and out
of zero speed, so an autoneg downshift that kept the link up left
if_baudrate, ifmedia, RX pause and interrupt moderation programmed for
the old speed. Track the announced speed and re-run that work when it
changes.

aq_if_suspend() resets the MAC and stops the rings, but
iflib_device_suspend() only calls IFDI_SUSPEND and never stops the
interface, leaving IFF_DRV_RUNNING set over a suspended device. Clear it.

Signed-off-by: Nick Price <nick@spun.io>

Diff Detail

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

Event Timeline

Gate the driver's own link UP/DOWN messages behind bootverbose, as ix and igc do, since if_link_state_change() already announces the transition and the pair was reported twice. The retrain speed line stays unconditional: a retrain holds the link up, so it is not a state change and nothing else reports it.

Add the Differential Revision trailer and drop the explanatory block comments; the reasoning is in the commit message.

Trim the two block comments that restated commit-message paragraphs.

Collapse the multi-line block comments to single lines; the reasoning is already in the commit message.

Refreshed after the comment-cleanup rebase of the series.