[PATCH net] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[PATCH net] bnxt_en: Don't keep MSI-X vectors added before FW reserves them

[Vlad Poenaru]

When dynamic MSI-X is supported, bnxt_check_rings() grows the MSI-X
vectors with bnxt_change_msix() once FW says the new rings would fit,
and keeps them.  FW only reserves the extra IRQs later, in
__bnxt_reserve_rings() on the following open.  The MSI-X table entries
beyond the function's reserved IRQs are not backed until then: the
MSI message written when the new vectors are activated is lost, and
the entries read back as zero.  After FW reserves them they are live
but still zero, so the NQ rings bound to them signal address 0 and
every completion on those rings is lost.  The next ring change (for
example ethtool -G) then times out freeing each of those rings and
the TX watchdog fires:

  bnxt_en 0002:01:00.0 eth0: Resp cmpl intr err msg: 0x51
  bnxt_en 0002:01:00.0 eth0: hwrm_ring_free type 1 failed. rc:fffffff0 err:0
  ...
  bnxt_en 0002:01:00.0 eth0: NETDEV WATCHDOG: CPU: 17: transmit queue 18 timed out 5716 ms

On x86 this is hidden: GENERIC_IRQ_RESERVATION_MODE makes the PCI MSI
domains MSI_FLAG_MUST_REACTIVATE, so the message is composed and
written again when the IRQ is requested, after FW has reserved the
vector.  Since commit 0d6a7c417983 ("irqchip/gic-v3-its: Enable
dynamic MSI-X allocation"), arm64 GICv3 systems take the dynamic path
as well, but write the message only once, at allocation.  On an
NVIDIA Grace system with a BCM57508, raising the channel count from
18 to 32 left MSI-X entries 18-31 all zero, and the 14 rings using
them lost their interrupts.

Keep the allocation in bnxt_check_rings() as a check that enough
vectors are available, but free the new vectors again right away when
FW manages IRQ reservations (BNXT_NEW_RM).  bnxt_reserve_rings() then
allocates them on the following open, after __bnxt_reserve_rings() has
reserved them.  Without BNXT_NEW_RM there is no IRQ reservation and
bnxt_reserve_rings() does not grow the vectors, so keep the current
behaviour there.

Fixes: 2d51eb0bd81c ("bnxt_en: Add MSIX check in bnxt_check_rings()")
Reported-by: Andrew Preece <apreece@meta.com>
Closes: https://lore.kernel.org/all/20261022193620.2152038-1-vlad.wing@gmail.com/
Signed-off-by: Vlad Poenaru <vlad.wing@gmail.com>
---
Tested on an NVIDIA Grace system (GICv3 ITS, ACPI) with a BCM57508,
FW 230.0.165.3: the boot-time channel change (18 -> 32) followed by a
ring resize no longer loses completions.  With instrumentation, MSI-X
entries 18-31 read back as all zero after bnxt_check_rings() allocated
them, and read back correctly once bnxt_reserve_rings() allocated them
after the FW reservation.

This conflicts trivially with "bnxt_en: Clear bp->total_irqs when
disabling IRQs" from the "bnxt_en: Ring accounting fixes" series on
net-next, which adds a bp->irq_tbl check to the same condition.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 0682060b048ce..2d14d991a3ead 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -15093,6 +15093,7 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
 			hwr.cp = min_t(int, hwr.cp, bnxt_get_max_func_irqs(bp));
 		}
 		if (hwr.cp > bp->total_irqs) {
+			int old_total = bp->total_irqs;
 			int total_msix = bnxt_change_msix(bp, hwr.cp);
 
 			if (total_msix < hwr.cp) {
@@ -15100,6 +15101,17 @@ int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
 					    hwr.cp, total_msix);
 				rc = -ENOSPC;
 			}
+			/* FW has not reserved the new vectors yet, and writes to
+			 * MSI-X table entries beyond the reserved IRQs don't
+			 * stick (seen on BCM57508).  Free them again;
+			 * bnxt_reserve_rings() allocates them after
+			 * __bnxt_reserve_rings().  Without the new resource
+			 * manager there is no IRQ reservation and
+			 * bnxt_reserve_rings() does not grow the vectors, so
+			 * keep them.
+			 */
+			if (BNXT_NEW_RM(bp))
+				bnxt_change_msix(bp, old_total);
 		}
 	}
 	return rc;