git: b445000158d3 - main - aq(4): mailbox, flow-control and firmware error-handling fixes
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Sun, 02 Aug 2026 23:10:35 UTC
The branch main has been updated by adrian:
URL: https://cgit.FreeBSD.org/src/commit/?id=b445000158d39a126f5fcd6e18bbc32248f0bc68
commit b445000158d39a126f5fcd6e18bbc32248f0bc68
Author: Nick Price <nick@spun.io>
AuthorDate: 2026-08-02 22:44:53 +0000
Commit: Adrian Chadd <adrian@FreeBSD.org>
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);