Date: Sun, 19 Jul 2026 16:49:14 +0000 From: Adrian Chadd <adrian@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Cc: Nick Price <nick@spun.io> Subject: git: 65fd3b4165ef - main - aq(4): harden the interrupt and MAC-statistics paths Message-ID: <6a5d000a.31812.9606322@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch main has been updated by adrian: URL: https://cgit.FreeBSD.org/src/commit/?id=65fd3b4165eff52f3447c70db8d10ef67875c022 commit 65fd3b4165eff52f3447c70db8d10ef67875c022 Author: Nick Price <nick@spun.io> AuthorDate: 2026-07-19 16:46:29 +0000 Commit: Adrian Chadd <adrian@FreeBSD.org> CommitDate: 2026-07-19 16:46:29 +0000 aq(4): harden the interrupt and MAC-statistics paths Firmware-statistics accounting and interrupt-routing fixes. Stats delta underflow: guard the MAC statistics delta accumulation against counter wrap or a firmware counter reset, so a snapshot smaller than the previous one does not underflow into a huge spurious delta. Skip stats on a failed read: aq_update_hw_stats() ignored aq_hw_mpi_read_stats()'s return and committed the on-stack mbox into last_stats unconditionally. On a failed read that snapshot is garbage or zero and poisons the delta baseline (a zeroed snapshot wipes last_stats, so the next good read double-counts). Check the return and skip the accumulation and the last_stats commit on failure. Mailbox/stats separation: struct aq_hw_stats served both as the raw fw1x MCP mailbox layout and as the driver's canonical stats snapshot, so any field added to it would silently shift the fw1x mailbox read. Give the fw1x mailbox its own raw layout in struct aq_hw_fw_mbox and let aq_hw_stats become purely driver-owned; with the coupling gone, add first-class aggregate octet fields (brc/btc) that Atlantic 2 B0 firmware can populate directly. No A1 behavior change. The raw block is a named struct (aq_fw1x_mbox_stats) with a _Static_assert tying its size to aq_hw_stats' matching prefix, so the fw1x memcpy cannot silently misalign if either field list drifts. Also drop the unused FW1X_MPI_STATE_ADR / FW1X_MPI_CONTROL_ADR macros and the redundant fw1x_get_stats() dpc assignment that the caller immediately overwrites. Per-speed interrupt moderation: aq_hw_interrupt_moderation_set() hardcoded speed_index = 0, so every link speed got the 10G timer pair and the other rows were dead. Record the negotiated rate and index the tables by ffs(speed) - 1, reordering the rows to match the enum aq_fw_link_speed bit positions so the index cannot drift from the enum. Rename the two per-speed timer tables (AQ_HW_NIC_timers_table_ {rx,tx}_ -> aq_itr_timers_{rx,tx}), function-local static arrays whose SCREAMING_CASE vendor names read like macros. Hardware error interrupts: route both hardware error causes (interrupt map register 0) to the admin vector so they are actually delivered. Reviewed by: adrian Differential Revision: https://reviews.freebsd.org/D58139 --- sys/dev/aq/aq_fw1x.c | 17 ++++++++--------- sys/dev/aq/aq_hw.c | 52 +++++++++++++++++++++++++++++----------------------- sys/dev/aq/aq_hw.h | 32 +++++++++++++++++++++++++++++--- sys/dev/aq/aq_irq.c | 35 +++++++++++++++++++++++++---------- 4 files changed, 91 insertions(+), 45 deletions(-) diff --git a/sys/dev/aq/aq_fw1x.c b/sys/dev/aq/aq_fw1x.c index 72b7f0456ddb..3149e979825f 100644 --- a/sys/dev/aq/aq_fw1x.c +++ b/sys/dev/aq/aq_fw1x.c @@ -45,8 +45,6 @@ __FBSDID("$FreeBSD$"); #include "aq_dbg.h" -#define FW1X_MPI_CONTROL_ADR 0x368 -#define FW1X_MPI_STATE_ADR 0x36C enum fw1x_mode { @@ -211,7 +209,7 @@ fw1x_set_mode(struct aq_hw* hw, enum aq_hw_fw_mpi_state mode, trace(dbg_init, "fw1x> set mode %d, rate mask = %#x; raw = %#x", state.mode, state.speed, state.val); - AQ_WRITE_REG(hw, FW1X_MPI_CONTROL_ADR, state.val); + AQ_WRITE_REG(hw, AQ_HW_MPI_CONTROL_ADR, state.val); return (0); } @@ -291,6 +289,11 @@ fw1x_get_mac_addr(struct aq_hw* hw, uint8_t* mac) return (0); } +/* fw1x_get_stats() memcpy's this raw block onto aq_hw_stats' prefix. */ +_Static_assert(sizeof(struct aq_fw1x_mbox_stats) == + __offsetof(struct aq_hw_stats, brc), + "fw1x mailbox stats must match the aq_hw_stats prefix"); + static int fw1x_get_stats(struct aq_hw* hw, struct aq_hw_stats* stats) { @@ -300,12 +303,8 @@ fw1x_get_stats(struct aq_hw* hw, struct aq_hw_stats* stats) err = aq_hw_fw_downld_dwords(hw, hw->mbox_addr, (uint32_t*)(void*)&hw->mbox, sizeof hw->mbox / sizeof(uint32_t)); - if (err == 0) { - if (stats != &hw->mbox.stats) - memcpy(stats, &hw->mbox.stats, sizeof *stats); - - stats->dpc = reg_rx_dma_stat_counter7get(hw); - } + if (err == 0) + memcpy(stats, &hw->mbox.stats, sizeof hw->mbox.stats); AQ_DBG_EXIT(err); return (err); diff --git a/sys/dev/aq/aq_hw.c b/sys/dev/aq/aq_hw.c index 7fc122482125..74c003d6e099 100644 --- a/sys/dev/aq/aq_hw.c +++ b/sys/dev/aq/aq_hw.c @@ -208,16 +208,16 @@ err_exit: } int -aq_hw_mpi_read_stats(struct aq_hw *hw, struct aq_hw_fw_mbox *pmbox) +aq_hw_mpi_read_stats(struct aq_hw *hw, struct aq_hw_stats *stats) { int err; // AQ_DBG_ENTER(); - err = hw->fw_ops->get_stats(hw, &pmbox->stats); + err = hw->fw_ops->get_stats(hw, stats); if (err == 0) { - pmbox->stats.dpc = reg_rx_dma_stat_counter7get(hw); - pmbox->stats.cprc = stats_rx_lro_coalesced_pkt_count0_get(hw); + stats->dpc = reg_rx_dma_stat_counter7get(hw); + stats->cprc = stats_rx_lro_coalesced_pkt_count0_get(hw); } // AQ_DBG_EXIT(err); @@ -267,6 +267,8 @@ aq_hw_get_link_state(struct aq_hw *hw, uint32_t *link_speed, struct aq_hw_fc_inf if (mode != MPI_INIT) return (0); + hw->link_speed = speed; /* remember negotiated rate for interrupt moderation */ + switch (speed) { case aq_fw_10G: *link_speed = 10000U; @@ -675,6 +677,9 @@ aq_hw_init(struct aq_hw *hw, uint8_t *mac_addr, uint8_t adm_irq, bool msix) else itr_irq_mode_set(hw, 0x5); //MSI + multi vector + /* Route both hardware error causes (map reg 0) to the admin vector. */ + reg_gen_irq_map_set(hw, + ((0x80U | adm_irq) << 24) | ((0x80U | adm_irq) << 16), 0); reg_gen_irq_map_set(hw, 0x80 | adm_irq, 3); err = aq_hw_offload_set(hw); @@ -702,24 +707,25 @@ aq_hw_start(struct aq_hw *hw) int aq_hw_interrupt_moderation_set(struct aq_hw *hw) { - static unsigned int AQ_HW_NIC_timers_table_rx_[][2] = { - {80, 120},//{0x6U, 0x38U},/* 10Gbit */ - {0xCU, 0x70U},/* 5Gbit */ - {0xCU, 0x70U},/* 5Gbit 5GS */ - {0x18U, 0xE0U},/* 2.5Gbit */ - {0x30U, 0x80U},/* 1Gbit */ - {0x4U, 0x50U},/* 100Mbit */ + /* Rows in enum aq_fw_link_speed bit order (10M=0 .. 10G=5); index = ffs(speed)-1. */ + static unsigned int aq_itr_timers_rx[][2] = { + {0x4U, 0x50U}, /* 10Mbit */ + {0x4U, 0x50U}, /* 100Mbit */ + {0x30U, 0x80U}, /* 1Gbit */ + {0x18U, 0xE0U}, /* 2.5Gbit */ + {0xCU, 0x70U}, /* 5Gbit */ + {80, 120}, /* 10Gbit */ }; - static unsigned int AQ_HW_NIC_timers_table_tx_[][2] = { - {0x4fU, 0x1ff},//{0xffU, 0xffU}, /* 10Gbit */ - {0x4fU, 0xffU}, /* 5Gbit */ - {0x4fU, 0xffU}, /* 5Gbit 5GS */ - {0x4fU, 0xffU}, /* 2.5Gbit */ - {0x4fU, 0xffU}, /* 1Gbit */ - {0x4fU, 0xffU}, /* 100Mbit */ + static unsigned int aq_itr_timers_tx[][2] = { + {0x4fU, 0xffU}, /* 10Mbit */ + {0x4fU, 0xffU}, /* 100Mbit */ + {0x4fU, 0xffU}, /* 1Gbit */ + {0x4fU, 0xffU}, /* 2.5Gbit */ + {0x4fU, 0xffU}, /* 5Gbit */ + {0x4fU, 0x1ff}, /* 10Gbit */ }; - uint32_t speed_index = 0U; //itr settings for 10 g + uint32_t speed_index = ffs(hw->link_speed ? hw->link_speed : aq_fw_10G) - 1; uint32_t itr_rx = 2U; uint32_t itr_tx = 2U; int custom_itr = hw->itr; @@ -731,14 +737,14 @@ aq_hw_interrupt_moderation_set(struct aq_hw *hw) if (custom_itr == -1) { /* set min timer value */ - itr_rx |= AQ_HW_NIC_timers_table_rx_[speed_index][0] << 0x8U; + itr_rx |= aq_itr_timers_rx[speed_index][0] << 0x8U; /* set max timer value */ - itr_rx |= AQ_HW_NIC_timers_table_rx_[speed_index][1] << 0x10U; + itr_rx |= aq_itr_timers_rx[speed_index][1] << 0x10U; /* set min timer value */ - itr_tx |= AQ_HW_NIC_timers_table_tx_[speed_index][0] << 0x8U; + itr_tx |= aq_itr_timers_tx[speed_index][0] << 0x8U; /* set max timer value */ - itr_tx |= AQ_HW_NIC_timers_table_tx_[speed_index][1] << 0x10U; + itr_tx |= aq_itr_timers_tx[speed_index][1] << 0x10U; } else { if (custom_itr > 0x1FF) custom_itr = 0x1FF; diff --git a/sys/dev/aq/aq_hw.h b/sys/dev/aq/aq_hw.h index 0619910d012c..33c4873292d7 100644 --- a/sys/dev/aq/aq_hw.h +++ b/sys/dev/aq/aq_hw.h @@ -77,7 +77,7 @@ extern uint32_t aq_hw_read_reg(struct aq_hw *hw, uint32_t reg); #define aq_hw_write_reg AQ_WRITE_REG -/* Statistics */ +/* Driver-side per-backend statistics snapshot. */ struct aq_hw_stats { uint32_t uprc; uint32_t mprc; @@ -97,6 +97,8 @@ struct aq_hw_stats { uint32_t prc; uint32_t dpc; uint32_t cprc; + uint32_t brc; /* aggregate rx octets */ + uint32_t btc; /* aggregate tx octets */ } __packed; union ip_addr { @@ -109,11 +111,34 @@ union ip_addr { } v4; } __packed; +/* Raw fw1x MCP mailbox stats block, in hardware order. */ +struct aq_fw1x_mbox_stats { + uint32_t uprc; + uint32_t mprc; + uint32_t bprc; + uint32_t erpt; + uint32_t uptc; + uint32_t mptc; + uint32_t bptc; + uint32_t erpr; + uint32_t mbtc; + uint32_t bbtc; + uint32_t mbrc; + uint32_t bbrc; + uint32_t ubrc; + uint32_t ubtc; + uint32_t ptc; + uint32_t prc; + uint32_t dpc; + uint32_t cprc; +} __packed; + +/* fw1x MCP mailbox: raw hardware layout, read verbatim. */ struct aq_hw_fw_mbox { uint32_t version; uint32_t transaction_id; int error; - struct aq_hw_stats stats; + struct aq_fw1x_mbox_stats stats; } __packed; struct aq_hw_fw_version { @@ -152,6 +177,7 @@ struct aq_hw { struct aq_hw_fc_info fc; uint16_t link_rate; + uint16_t link_speed; /* last negotiated rate (for ITR moderation) */ uint16_t device_id; uint16_t subsystem_vendor_id; @@ -339,7 +365,7 @@ int aq_hw_reset(struct aq_hw *hw); int aq_hw_mpi_create(struct aq_hw *hw); -int aq_hw_mpi_read_stats(struct aq_hw *hw, struct aq_hw_fw_mbox *pmbox); +int aq_hw_mpi_read_stats(struct aq_hw *hw, struct aq_hw_stats *stats); int aq_hw_init(struct aq_hw *hw, uint8_t *mac_addr, uint8_t adm_irq, bool msix); diff --git a/sys/dev/aq/aq_irq.c b/sys/dev/aq/aq_irq.c index efd37b359d6f..428a4baecab7 100644 --- a/sys/dev/aq/aq_irq.c +++ b/sys/dev/aq/aq_irq.c @@ -56,12 +56,17 @@ int aq_update_hw_stats(struct aq_dev *aq_dev) { struct aq_hw *hw = &aq_dev->hw; - struct aq_hw_fw_mbox mbox; + struct aq_hw_stats stats; - aq_hw_mpi_read_stats(hw, &mbox); + memset(&stats, 0, sizeof(stats)); + if (aq_hw_mpi_read_stats(hw, &stats) != 0) + return (0); -#define AQ_SDELTA(_N_) (aq_dev->curr_stats._N_ += \ - mbox.stats._N_ - aq_dev->last_stats._N_) +#define AQ_SDELTA(_N_) do { \ + int32_t _d = (int32_t)(stats._N_ - aq_dev->last_stats._N_); \ + if (_d > 0) \ + aq_dev->curr_stats._N_ += _d; \ +} while (0) if (aq_dev->linkup) { AQ_SDELTA(uprc); AQ_SDELTA(mprc); @@ -86,15 +91,25 @@ aq_update_hw_stats(struct aq_dev *aq_dev) AQ_SDELTA(dpc); - aq_dev->curr_stats.brc = aq_dev->curr_stats.ubrc + - aq_dev->curr_stats.mbrc + aq_dev->curr_stats.bbrc; - aq_dev->curr_stats.btc = aq_dev->curr_stats.ubtc + - aq_dev->curr_stats.mbtc + aq_dev->curr_stats.bbtc; - + /* + * Per-cast octets present: derive the aggregate from them. + * Otherwise (B0) accumulate the firmware-reported aggregate. + */ + if (stats.ubrc | stats.mbrc | stats.bbrc) + aq_dev->curr_stats.brc = aq_dev->curr_stats.ubrc + + aq_dev->curr_stats.mbrc + aq_dev->curr_stats.bbrc; + else + AQ_SDELTA(brc); + + if (stats.ubtc | stats.mbtc | stats.bbtc) + aq_dev->curr_stats.btc = aq_dev->curr_stats.ubtc + + aq_dev->curr_stats.mbtc + aq_dev->curr_stats.bbtc; + else + AQ_SDELTA(btc); } #undef AQ_SDELTA - memcpy(&aq_dev->last_stats, &mbox.stats, sizeof(mbox.stats)); + memcpy(&aq_dev->last_stats, &stats, sizeof(stats)); return (0); }home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6a5d000a.31812.9606322>