Page MenuHomeFreeBSD

aq(4): mailbox, flow-control and firmware error-handling fixes
Needs ReviewPublic

Authored by nick_spun.io on Fri, Jul 24, 3:38 PM.
Tags
None
Referenced Files
F164160578: D58437.id182663.diff
Wed, Jul 29, 6:25 AM
F164109775: D58437.diff
Tue, Jul 28, 7:46 PM
F164093691: D58437.id182663.diff
Tue, Jul 28, 4:05 PM
Unknown Object (File)
Tue, Jul 28, 12:30 PM
Unknown Object (File)
Mon, Jul 27, 10:44 PM
Unknown Object (File)
Mon, Jul 27, 6:08 PM
Unknown Object (File)
Mon, Jul 27, 9:30 AM
Unknown Object (File)
Sun, Jul 26, 6:21 PM
Subscribers
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Fold the whole-driver-review correctness and hardening fixes for the
firmware and hardware layers.

Advance the firmware-mailbox address per word in aq_hw_fw_downld_dwords():
on B1 silicon each loop iteration waits for the mailbox address register
to differ from the expected address, but it was set once and never moved,
so after the first word every wait returned immediately and read stale
data. Advance it four bytes per word. B0 is unaffected (it polls the
busy bit). The same function also left err set to ETIMEDOUT after
successfully force-recovering the RAM CPU semaphore; the transfer loop is
guarded by "--cnt && !err", so it ran zero iterations and returned a
timeout with an untouched buffer, making the recovery path dead code.

aq_hw_get_mac_permanent() ignored the get_mac_addr() error and then
examined a buffer the firmware op never wrote on failure. A fresh softc
is zero, so the "invalid address" test fired, a random locally
administered MAC was substituted, and err was overwritten with 0 -- a
transient mailbox failure produced a card that attached with a different
MAC every boot. Fail instead; the random-address fallback still covers a
genuinely blank or multicast burned-in address. aq_fw1x_reset()
discarded the same download's return value and then read transaction_id
out of an uninitialized stack struct, so propagate that error too.

Encode RX-only flow control as PAUSE|ASYM_PAUSE rather than PAUSE alone:
firmware 2.x/3.x has no independent RX-only bit, so the old encoding
advertised symmetric pause when RX-only was requested. The MPI_INIT path
also never cleared the pause bits before OR-ing in the requested ones, so
flow control could be enabled and never disabled; clear them first, as
the Atlantic 2 and Linux implementations do.

Reject single-vector MSI in aq_if_attach_post() the same way legacy INTx
is rejected: ift_legacy_intr is NULL, so no driver filter would
acknowledge the not-clear-on-read, auto-masked device interrupt status;
every supported Atlantic device provides MSI-X.

Propagate firmware and MDIO errors instead of discarding them. The fw2x
MDIO primitive returned a data word with no way to report a controller
timeout; give aq_fw2x_mdio_op() a status return and a data out-parameter,
propagate it through phy_write/read/reset/thermal_arm, and stop advancing
the thermal recovery state machine when a PHY reset fails. Use that
error to end the PHY address scan early: aq_fw2x_init_phy_id() probed all
32 MDIO ports even when the controller itself was timing out, spending up
to ten seconds under fw_mtx and the iflib context lock. aq_fw2x_reset()
also drove the shared MIF mailbox without fw_mtx, unlike every other fw2x
mailbox user, so it could interleave with the temperature sysctl and load
the capability mask from the wrong window.

aq_hw_mpi_set() can return ETIMEDOUT when the Atlantic 2 shared firmware
buffer is not acknowledged; aq_hw_init() now aborts through its error
path rather than enabling rings with an unaccepted link state, and
aq_if_init() logs the later link-speed error.

Retry a failed initialization instead of leaving the link down. ifdi_init
has no return value, so iflib marks the interface running once aq_if_init()
returns; a propagated firmware-ack failure would otherwise leave it running
with no initialized hardware and no recovery. Record the failure and retry
from the admin task via iflib_request_reset(), paced by the once-per-second
timer, giving up after a bounded number of attempts. Ring and queue start
failures are deliberately left to the existing diagnostic, since they leave
the remaining queues usable.

Diff Detail

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

Event Timeline

Fold in the firmware/MDIO error-handling fixes from a second review round; they touch the same functions as this change.

nick_spun.io retitled this revision from aq(4): mailbox and flow-control fixes with init/error-handling hardening to aq(4): mailbox, flow-control and firmware error-handling fixes.Sat, Jul 25, 3:56 AM
nick_spun.io edited the summary of this revision. (Show Details)

Self-review pass, and three behavioural corrections. Keep the
generated-address fallback in aq_hw_get_mac_permanent() rather than
failing the attach, so a transient mailbox timeout cannot turn into a
missing interface; report the failure instead of substituting silently.
Keep aq_hw_init() going when the firmware link-mode acknowledgement times
out, since returning early skipped the interrupt-mode and IRQ-map
programming that follow. Retry the fw1x mailbox poll instead of returning
on the first error, which is what the surrounding 1000-iteration loop
exists to do while the MCP cold-starts. Also track the outstanding reset
so the init retry does not run one extra stop/init after the interface has
already recovered, and reset the retry budget per bring-up.

Rebased onto the updated parent; this commit's content is unchanged.

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.