git: b445000158d3 - main - aq(4): mailbox, flow-control and firmware error-handling fixes
Adrian Chadd <[email protected]> Sun, 02 Aug 2026 23:10:35 +0000
| Newsgroups | gmane.os.freebsd.devel.cvs.src |
|---|---|
| Message-ID | <6a6fce6b.4335e.409e4a2f__16248.1603025057$1785712346$gmane$org@gitrepo.freebsd.org> |
The branch main has been updated by adrian: URL: https://cgit.FreeBSD.org/src/commit/?id=b445000158d39a126f5fcd6e18bbc32248f0bc68 commit b445000158d39a126f5fcd6e18bbc32248f0bc68 Author: Nick Price <[email protected]> AuthorDate: 2026-08-02 22:44:53 +0000 Commit: Adrian Chadd <[email protected]> CommitDate: 2026-08-02 22:44:53 +0000 aq(4): mailbox, flow-control and firmware error-handling fixes 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. Reviewed by: adrian Differential Revision: https://reviews.freebsd.org/D58437 --- sys/dev/aq/aq_device.h | 3 ++ sys/dev/aq/aq_fw1x.c | 8 ++++- sys/dev/aq/aq_fw2x.c | 85 ++++++++++++++++++++++++++++++++++---------------- sys/dev/aq/aq_hw.c | 13 +++++++- sys/dev/aq/aq_irq.c | 27 +++++++++++++++- sys/dev/aq/aq_main.c | 19 ++++++++--- 6 files changed, 121 insertions(+), 34 deletions(-) diff --git a/sys/dev/aq/aq_device.h b/sys/dev/aq/aq_device.h index 07482e5402ca..c8942e19cf13 100644 --- a/sys/dev/aq/aq_device.h +++ b/sys/dev/aq/aq_device.h @@ -131,6 +131,9 @@ struct aq_dev { int thermal_retry_ticks; /* earliest tick to retry at */ int thermal_temp_mc; /* temp at the last shutdown/cool */ int thermal_recover_mc; /* recover once cooled to here */ + bool init_failed; /* aq_hw_init() left the hw down */ + int init_retries; + bool reset_pending; /* a re-init is already queued */ int media_active; struct aq_hw_stats last_stats; diff --git a/sys/dev/aq/aq_fw1x.c b/sys/dev/aq/aq_fw1x.c index 771c62cc1303..66a9f40f7c8a 100644 --- a/sys/dev/aq/aq_fw1x.c +++ b/sys/dev/aq/aq_fw1x.c @@ -167,12 +167,18 @@ aq_fw1x_reset(struct aq_hw* hw) uint32_t tid0 = ~0u; /*< Initial value of MBOX transactionId. */ struct aq_hw_fw_mbox mbox; const int retryCount = 1000; + int err; for (int i = 0; i < retryCount; ++i) { // Read the beginning of Statistics structure to capture the // Transaction ID. - aq_hw_fw_downld_dwords(hw, hw->mbox_addr, (uint32_t*)&mbox, + err = aq_hw_fw_downld_dwords(hw, hw->mbox_addr, (uint32_t*)&mbox, (uint32_t)((char*)&mbox.stats - (char*)&mbox) / sizeof(uint32_t)); + /* The MCP is still cold-starting; that is what we wait for. */ + if (err != 0) { + DELAY(10); + continue; + } // Successfully read the stats. if (tid0 == ~0U) { diff --git a/sys/dev/aq/aq_fw2x.c b/sys/dev/aq/aq_fw2x.c index be1baa93c2ab..91a20cebb7d0 100644 --- a/sys/dev/aq/aq_fw2x.c +++ b/sys/dev/aq/aq_fw2x.c @@ -245,9 +245,11 @@ aq_fw2x_reset(struct aq_hw* hw) { struct aq_fw2x_capabilities caps = {0}; AQ_DBG_ENTER(); + mtx_lock(&hw->fw_mtx); int err = aq_hw_fw_downld_dwords(hw, hw->mbox_addr + offsetof(struct aq_fw2x_mailbox, caps), (uint32_t*)&caps, sizeof caps/sizeof(uint32_t)); + mtx_unlock(&hw->fw_mtx); if (err == 0) { hw->fw_caps = caps.caps_lo | ((uint64_t)caps.caps_hi << 32); trace(hw, dbg_init, @@ -304,13 +306,14 @@ aq_fw2x_set_mode(struct aq_hw* hw, enum aq_hw_fw_mpi_state mode, mpi_ctrl &= ~FW2X_RATE_MASK; mpi_ctrl |= link_speed_mask_to_fw2x(speed); mpi_ctrl &= ~FW2X_CAP_LINK_DROP; + mpi_ctrl &= ~(FW2X_FW_CAP_PAUSE | FW2X_FW_CAP_ASYM_PAUSE); #if 0 // #todo #flowcontrol #pause #eee if (pHal->pCfg->eee) mpi_ctrl |= FW2X_EEE_MASK; #endif if (hw->fc.fc_rx) - mpi_ctrl |= FW2X_FW_CAP_PAUSE; - if (hw->fc.fc_tx) + mpi_ctrl |= FW2X_FW_CAP_PAUSE | FW2X_FW_CAP_ASYM_PAUSE; + else if (hw->fc.fc_tx) mpi_ctrl |= FW2X_FW_CAP_ASYM_PAUSE; break; @@ -539,17 +542,20 @@ aq_fw2x_get_phy_fault(struct aq_hw* hw, uint16_t* fault) #define AQ_PHY_THERMAL_CTRL_REG 0xc478 /* 1E.C478 thermal control */ #define AQ_PHY_THERMAL_SD_EN 0x0400 /* .A thermalShutdownEnable */ -static uint16_t +static int aq_fw2x_mdio_op(struct aq_hw* hw, uint16_t mmd, uint16_t addr, int write, - uint16_t data) + uint16_t data, uint16_t* val) { uint32_t pa = (((uint32_t)hw->phy_id & 0x1f) << 5) | (mmd & 0x1f); + int err; AQ_WRITE_REG(hw, AQ_MDIO_IFACE(4), addr); AQ_WRITE_REG(hw, AQ_MDIO_IFACE(2), AQ_MDIO_EXECUTE | (AQ_MDIO_OP_ADDR << AQ_MDIO_OP_S) | pa); - AQ_HW_WAIT_FOR((AQ_READ_REG(hw, AQ_MDIO_IFACE(2)) & AQ_MDIO_BUSY) == 0, - 10, 10000); + err = AQ_HW_WAIT_FOR((AQ_READ_REG(hw, AQ_MDIO_IFACE(2)) & + AQ_MDIO_BUSY) == 0, 10, 10000); + if (err != 0) + return (err); if (write) { AQ_WRITE_REG(hw, AQ_MDIO_IFACE(3), data); AQ_WRITE_REG(hw, AQ_MDIO_IFACE(2), @@ -558,32 +564,45 @@ aq_fw2x_mdio_op(struct aq_hw* hw, uint16_t mmd, uint16_t addr, int write, AQ_WRITE_REG(hw, AQ_MDIO_IFACE(2), AQ_MDIO_EXECUTE | (AQ_MDIO_OP_READ << AQ_MDIO_OP_S) | pa); } - AQ_HW_WAIT_FOR((AQ_READ_REG(hw, AQ_MDIO_IFACE(2)) & AQ_MDIO_BUSY) == 0, - 10, 10000); - return ((uint16_t)AQ_READ_REG(hw, AQ_MDIO_IFACE(5))); + err = AQ_HW_WAIT_FOR((AQ_READ_REG(hw, AQ_MDIO_IFACE(2)) & + AQ_MDIO_BUSY) == 0, 10, 10000); + if (err != 0) + return (err); + if (val != NULL) + *val = (uint16_t)AQ_READ_REG(hw, AQ_MDIO_IFACE(5)); + + return (0); } /* MDIO is serialized against the F/W by cpu semaphore 0. */ -static void +static int aq_fw2x_phy_write(struct aq_hw* hw, uint16_t mmd, uint16_t addr, uint16_t data) { - if (AQ_HW_WAIT_FOR(reg_glb_cpu_sem_get(hw, AQ_FW_SM_MDIO) == 1U, - 10, 10000) != 0) - return; - aq_fw2x_mdio_op(hw, mmd, addr, 1, data); + int err; + + err = AQ_HW_WAIT_FOR(reg_glb_cpu_sem_get(hw, AQ_FW_SM_MDIO) == 1U, + 10, 10000); + if (err != 0) + return (err); + err = aq_fw2x_mdio_op(hw, mmd, addr, 1, data, NULL); reg_glb_cpu_sem_set(hw, 1U, AQ_FW_SM_MDIO); + + return (err); } static int aq_fw2x_phy_read(struct aq_hw* hw, uint16_t mmd, uint16_t addr, uint16_t* val) { - if (AQ_HW_WAIT_FOR(reg_glb_cpu_sem_get(hw, AQ_FW_SM_MDIO) == 1U, - 10, 10000) != 0) - return (ETIMEDOUT); - *val = aq_fw2x_mdio_op(hw, mmd, addr, 0, 0); + int err; + + err = AQ_HW_WAIT_FOR(reg_glb_cpu_sem_get(hw, AQ_FW_SM_MDIO) == 1U, + 10, 10000); + if (err != 0) + return (err); + err = aq_fw2x_mdio_op(hw, mmd, addr, 0, 0, val); reg_glb_cpu_sem_set(hw, 1U, AQ_FW_SM_MDIO); - return (0); + return (err); } /* Discover the PHY's MDIO port address; it is strap-selectable, not fixed at 0. */ @@ -592,12 +611,17 @@ aq_fw2x_init_phy_id(struct aq_hw* hw) { uint16_t val; uint8_t id; + int err; for (id = 0; id < AQ_PHY_ID_MAX; id++) { hw->phy_id = id; - if (aq_fw2x_phy_read(hw, AQ_MDIO_MMD_PMAPMD, AQ_PHY_ID2_REG, - &val) == 0 && val != 0xffff) + err = aq_fw2x_phy_read(hw, AQ_MDIO_MMD_PMAPMD, AQ_PHY_ID2_REG, + &val); + if (err == 0 && val != 0xffff) return (true); + /* A timeout means the controller is wedged, not this port. */ + if (err == ETIMEDOUT) + break; } hw->phy_id = 0; return (false); @@ -615,12 +639,15 @@ aq_fw2x_phy_id_probe(struct aq_hw* hw) static int aq_fw2x_phy_reset(struct aq_hw* hw) { + int err; + mtx_lock(&hw->fw_mtx); aq_fw2x_phy_id_probe(hw); - aq_fw2x_phy_write(hw, AQ_PHY_MMD_GLOBAL, AQ_PHY_RESET_REG, AQ_PHY_RESET); + err = aq_fw2x_phy_write(hw, AQ_PHY_MMD_GLOBAL, AQ_PHY_RESET_REG, + AQ_PHY_RESET); mtx_unlock(&hw->fw_mtx); - return (0); + return (err); } /* Arm autonomous thermal shutdown (1E.C478.A), cleared by any PHY reset. */ @@ -628,16 +655,20 @@ static int aq_fw2x_thermal_arm(struct aq_hw* hw) { uint16_t ctrl; + int err; mtx_lock(&hw->fw_mtx); aq_fw2x_phy_id_probe(hw); - if (aq_fw2x_phy_read(hw, AQ_PHY_MMD_GLOBAL, AQ_PHY_THERMAL_CTRL_REG, - &ctrl) == 0 && ctrl != 0xffff && (ctrl & AQ_PHY_THERMAL_SD_EN) == 0) - aq_fw2x_phy_write(hw, AQ_PHY_MMD_GLOBAL, + err = aq_fw2x_phy_read(hw, AQ_PHY_MMD_GLOBAL, AQ_PHY_THERMAL_CTRL_REG, + &ctrl); + if (err == 0 && ctrl == 0xffff) + err = ENXIO; + if (err == 0 && (ctrl & AQ_PHY_THERMAL_SD_EN) == 0) + err = aq_fw2x_phy_write(hw, AQ_PHY_MMD_GLOBAL, AQ_PHY_THERMAL_CTRL_REG, ctrl | AQ_PHY_THERMAL_SD_EN); mtx_unlock(&hw->fw_mtx); - return (0); + return (err); } /* 1E.C421 high-temp shutdown threshold, degrees C in Q8.8 fixed point. */ diff --git a/sys/dev/aq/aq_hw.c b/sys/dev/aq/aq_hw.c index a7dae250c66e..934543937a57 100644 --- a/sys/dev/aq/aq_hw.c +++ b/sys/dev/aq/aq_hw.c @@ -109,6 +109,7 @@ aq_hw_fw_downld_dwords(struct aq_hw *hw, uint32_t a, uint32_t *p, uint32_t cnt) err = ETIMEDOUT; goto err_exit; } + err = 0; } mif_mcp_up_mailbox_addr_set(hw, a); @@ -124,6 +125,7 @@ aq_hw_fw_downld_dwords(struct aq_hw *hw, uint32_t a, uint32_t *p, uint32_t cnt) 1000U); *(p++) = mif_mcp_up_mailbox_data_get(hw); + a += 4; } reg_glb_cpu_sem_set(hw, 1U, AQ_HW_FW_SM_RAM); @@ -311,6 +313,12 @@ aq_hw_get_mac_permanent(struct aq_hw *hw, uint8_t *mac) AQ_DBG_ENTER(); err = hw->fw_ops->get_mac_addr(hw, mac); + if (err != 0) { + /* A transient mailbox failure must not fail the attach. */ + device_printf(hw->dev, "could not read the MAC address: %d\n", + err); + memset(mac, 0, ETHER_ADDR_LEN); + } /* Couldn't get MAC address from HW. Use auto-generated one. */ if ((mac[0] & 1) || ((mac[0] | mac[1] | mac[2]) == 0)) { @@ -815,7 +823,10 @@ aq_hw_init(struct aq_hw *hw, uint8_t *mac_addr, uint8_t adm_irq, bool msix) aq_hw_mac_addr_set(hw, mac_addr, AQ_HW_MAC); - aq_hw_mpi_set(hw, MPI_INIT, hw->link_rate); + /* A lost ack must not skip the setup that follows. */ + err = aq_hw_mpi_set(hw, MPI_INIT, hw->link_rate); + if (err != 0) + device_printf(hw->dev, "could not set F/W link mode: %d\n", err); aq_hw_qos_set(hw); diff --git a/sys/dev/aq/aq_irq.c b/sys/dev/aq/aq_irq.c index dc1b0ac351ff..a15424cbeeba 100644 --- a/sys/dev/aq/aq_irq.c +++ b/sys/dev/aq/aq_irq.c @@ -118,6 +118,7 @@ aq_update_hw_stats(struct aq_dev *aq_dev) #define AQ_THERMAL_RECOVER_MC 90000 /* fallback when the limit is unreadable */ #define AQ_THERMAL_SETTLE_POLLS 5 /* ~5 s for the PHY reset to settle */ #define AQ_THERMAL_RETRY_SECS 60 /* minimum spacing between recoveries */ +#define AQ_INIT_MAX_RETRIES 5 /* re-init attempts after a failed init */ /* Temperature here is post-trip; the PHY is already dropping to low power. */ static void @@ -182,7 +183,8 @@ aq_thermal_poll(struct aq_dev *aq_dev) return; aq_dev->thermal_temp_mc = temp_mc; if (hw->fw_ops->phy_reset != NULL) { - hw->fw_ops->phy_reset(hw); + if (hw->fw_ops->phy_reset(hw) != 0) + return; aq_dev->thermal_settle = 0; aq_dev->thermal_state = AQ_THERMAL_SETTLING; return; @@ -204,6 +206,7 @@ aq_thermal_poll(struct aq_dev *aq_dev) device_printf(aq_dev->dev, "PHY cooled to %d C; restoring " "link\n", aq_dev->thermal_temp_mc / 1000); aq_dev->thermal_state = AQ_THERMAL_NORMAL; + aq_dev->reset_pending = true; iflib_request_reset(aq_dev->ctx); iflib_admin_intr_deferred(aq_dev->ctx); } @@ -218,6 +221,11 @@ aq_if_update_admin_status(if_ctx_t ctx) struct aq_hw_fc_info fc_neg; aq_hw_get_link_state(hw, &link_speed, &fc_neg); + + /* An interface whose initialization did not complete has no link. */ + if (aq_dev->init_failed) + link_speed = 0; + if (link_speed && !aq_dev->linkup) { /* link was DOWN */ device_printf(aq_dev->dev, "link UP: speed=%d\n", link_speed); @@ -245,6 +253,23 @@ aq_if_update_admin_status(if_ctx_t ctx) aq_mediastatus_update(aq_dev, link_speed, &fc_neg); } + /* Re-arming while a reset is queued would re-init once too often. */ + if (aq_dev->init_failed) { + if (aq_dev->reset_pending) + return; + if (aq_dev->init_retries < AQ_INIT_MAX_RETRIES) { + aq_dev->init_retries++; + aq_dev->reset_pending = true; + iflib_request_reset(ctx); + } else if (aq_dev->init_retries == AQ_INIT_MAX_RETRIES) { + aq_dev->init_retries++; + device_printf(aq_dev->dev, "initialization failed; " + "giving up after %d retries, link held down\n", + AQ_INIT_MAX_RETRIES); + } + return; + } + if (hw->fw_ops->get_phy_fault != NULL) aq_thermal_poll(aq_dev); diff --git a/sys/dev/aq/aq_main.c b/sys/dev/aq/aq_main.c index 445d02da3257..cb04ccf5d158 100644 --- a/sys/dev/aq/aq_main.c +++ b/sys/dev/aq/aq_main.c @@ -486,7 +486,8 @@ aq_if_attach_post(if_ctx_t ctx) goto exit; break; case IFLIB_INTR_MSI: - break; + rc = EOPNOTSUPP; + goto exit; case IFLIB_INTR_MSIX: break; default: @@ -751,19 +752,25 @@ aq_if_init(if_ctx_t ctx) softc->phy_fault_last = 0; softc->thermal_state = AQ_THERMAL_NORMAL; + softc->reset_pending = false; hw->tx_rings_count = softc->tx_rings_count; err = aq_hw_init(&softc->hw, softc->hw.mac_addr, softc->msix, softc->scctx->isc_intr == IFLIB_INTR_MSIX); if (err != 0) { device_printf(softc->dev, "aq_hw_init: %d\n", err); + softc->init_failed = true; AQ_DBG_EXIT(err); return; } + softc->init_failed = false; + softc->init_retries = 0; /* aq_hw_init reloads the PHY, resetting the thermal-shutdown arming. */ - if (hw->fw_ops->thermal_arm != NULL) - hw->fw_ops->thermal_arm(hw); + if (hw->fw_ops->thermal_arm != NULL && + hw->fw_ops->thermal_arm(hw) != 0) + device_printf(softc->dev, + "could not arm PHY thermal shutdown\n"); aq_if_media_status(ctx, &ifmr); @@ -807,7 +814,9 @@ aq_if_init(if_ctx_t ctx) aq_hw_udp_rss_enable(hw, (aq_rss_hashconfig() & (RSS_HASHTYPE_RSS_UDP_IPV4 | RSS_HASHTYPE_RSS_UDP_IPV6 | RSS_HASHTYPE_RSS_UDP_IPV6_EX)) != 0); - aq_hw_set_link_speed(hw, hw->link_rate); + err = aq_hw_set_link_speed(hw, hw->link_rate); + if (err != 0) + device_printf(softc->dev, "could not set link speed: %d\n", err); /* iflib does not replay filter state after init; aq_hw_init() clears it. */ aq_if_multi_set(ctx); @@ -844,6 +853,8 @@ aq_if_stop(if_ctx_t ctx) aq_hw_reset(&softc->hw, true); memset(&softc->last_stats, 0, sizeof(softc->last_stats)); + /* Each bring-up gets its own budget of re-init attempts. */ + softc->init_retries = 0; softc->linkup = false; aq_if_update_admin_status(ctx); AQ_DBG_EXIT(0);