[net PATCH v2 0/6] eth: fbnic: a collection of fixes

[net PATCH v2 0/6] eth: fbnic: a collection of fixes

[Alexander Duyck]

This series collects a handful of independent fbnic fixes for issues on
released kernels, plus one core ethtool fix needed by the fbnic offline
self test.

The first patch keeps rtnl_lock held on the ethtool ioctl path for the self
test. Since the ioctl path became rtnl-optional for ops-locked drivers,
fbnic's offline self test (which brings the interface down and up via
netif_close()/netif_open()) runs holding only the instance lock, tripping a
lockdep splat / ASSERT_RTNL and reconfiguring the device without the lock
it requires. A similar issue was found with Broadcom drivers so we expanded
the scope for v2 to just have the rtnl lock held for all selftest calls.

The second addresses a comparison issue in that we were limiting the
maximum number of standalone Tx queues to one less than the maximum number
of Tx queues. To resolve this it was just a matter of replacing a "<" with
a "<=".

The third addresses an indexing issue with netdev queues on fbnic in which
the NAPI vector was assumed to be findable as the Rx index modulo the
number of NAPI vectors. However this is actually not the case for if Tx
only and Rx only queues are setup. To resolve this we make use of the
cached NAPI pointer in the netdev Rx queues themselves.

The fourth patch fixes a NULL pointer dereference on unbind after a failed
PCIe error recovery: fbnic_pm_suspend() frees the napi vectors via a direct
ndo_stop() while leaving netif_running() true, and when slot_reset ->
resume fails the data path is never re-allocated. To prevent the panic we
reset num_napi to 0 before we free the IRQs which prevents walking the
unallocated napi vectors when we unbind the interface later.

The last two patches address the FW mailbox. One sets AW_FLUSH_MODE
alongside AW_FLUSH when tearing down the Rx ring, so the write pipeline
actually drains the staged requests instead of hanging on the BME halt.
The other handles completions flagged with FW_ERR on both mailboxes, which
the driver previously ignored. This resulted in us parsing a stale Rx page,
and spinning the capabilities poll to a timeout on a healthy ring.

---

Alexander Duyck (5):
      net: ethtool: keep rtnl_lock for the ioctl self test
      eth: fbnic: use the Rx queue napi pointer to find the napi vector
      eth: fbnic: reset num_napi when the napi vectors are freed
      eth: fbnic: Set AW_FLUSH_MODE alongside AW_FLUSH when flushing the mailbox
      eth: fbnic: Handle FW mailbox completions flagged with an error

Björn Töpel' via fbnic (1):
      eth: fbnic: Handle maximum standalone channels


 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  3 +-
 drivers/net/ethernet/meta/fbnic/fbnic_csr.h   |  5 ++
 .../net/ethernet/meta/fbnic/fbnic_debugfs.c   |  4 +-
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  3 +-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    | 47 ++++++++++++++++++-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |  1 +
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c   | 18 +++++--
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  | 28 +++++++++--
 include/linux/ethtool.h                       |  2 +
 net/ethtool/common.h                          |  2 +
 10 files changed, 99 insertions(+), 14 deletions(-)

--

[net PATCH v2 1/6] net: ethtool: keep rtnl_lock for the ioctl self test

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

An offline self test that brings the interface down and back up with
netif_close() / netif_open() requires rtnl_lock for both. Since the
ethtool IOCTL path became rtnl-optional for ops-locked drivers, the
ETHTOOL_TEST ioctl runs holding only the netdev instance lock, so on an
ops-locked driver the self test now tears the device down without
rtnl_lock.

With lockdep this reproduces deterministically on every offline self
test on such a driver; note the sole lock held is the instance lock, not
rtnl:

  WARNING: suspicious RCU usage
  net/core/netpoll.c:207 suspicious rcu_dereference_protected() usage!
  1 lock held by ethtool/107:
   #0: (&dev->lock){+.+.}, at: dev_ethtool
  Call Trace:
   netpoll_poll_disable
   __dev_close_many
   netif_close_many
   netif_close
   fbnic_self_test
   dev_ethtool_locked
   dev_ethtool
   dev_ioctl
   sock_ioctl
   __x64_sys_ioctl

Without lockdep the same condition trips ASSERT_RTNL() in
__dev_close_many() / __dev_open(); that check only samples the global
rtnl state, so it can be masked by a concurrent rtnl holder, but the
device is still being reconfigured without the lock it requires.

The ethtool self_test is a legacy ioctl-only command, so an ETHTOOL_TEST
case is only needed on the ioctl path. Add an opt-in bit for drivers whose
self test needs rtnl_lock and set it on the ops-locked drivers whose
offline self test tears the interface down and up:

  - fbnic (ops-locked via queue_mgmt_ops): fbnic_self_test() offline path
    uses netif_close() / netif_open().
  - bnxt (ops-locked via queue_mgmt_ops): bnxt_self_test() offline path
    goes through bnxt_close_nic() / bnxt_half_open_nic() /
    bnxt_half_close_nic() / bnxt_open_nic(), which close and reopen the
    device.

Fixes: f994752b1127 ("net: ethtool: optionally skip rtnl_lock on IOCTL path")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |    3 ++-
 drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c   |    3 ++-
 include/linux/ethtool.h                           |    2 ++
 net/ethtool/common.h                              |    2 ++
 4 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 62bc9cae613c..622e89587e5d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -5733,7 +5733,8 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 	.op_needs_rtnl			= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
 					  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
 					  ETHTOOL_OP_NEEDS_RTNL_SCOALESCE |
-					  ETHTOOL_OP_NEEDS_RTNL_RSS,
+					  ETHTOOL_OP_NEEDS_RTNL_RSS |
+					  ETHTOOL_OP_NEEDS_RTNL_TEST,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES |
 				     ETHTOOL_COALESCE_USECS_IRQ |
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index 0e47088ec44b..423f179c9d47 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -2025,7 +2025,8 @@ static const struct ethtool_ops fbnic_ethtool_ops = {
 					  ETHTOOL_OP_NEEDS_RTNL_SPAUSEPARAM |
 					  ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
 					  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
-					  ETHTOOL_OP_NEEDS_RTNL_GLINK,
+					  ETHTOOL_OP_NEEDS_RTNL_GLINK |
+					  ETHTOOL_OP_NEEDS_RTNL_TEST,
 	.get_drvinfo			= fbnic_get_drvinfo,
 	.get_regs_len			= fbnic_get_regs_len,
 	.get_regs			= fbnic_get_regs,
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 253600c0eccd..c4c9ce038611 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -944,6 +944,7 @@ struct kernel_ethtool_ts_info {
 #define ETHTOOL_OP_NEEDS_RTNL_SPAUSEPARAM	BIT(6)
 #define ETHTOOL_OP_NEEDS_RTNL_RSS		BIT(7)
 #define ETHTOOL_OP_NEEDS_RTNL_GLINK		BIT(8)
+#define ETHTOOL_OP_NEEDS_RTNL_TEST		BIT(9)
 
 /**
  * struct ethtool_ops - optional netdev operations
@@ -981,6 +982,7 @@ struct kernel_ethtool_ts_info {
  *	 - netdev_update_features()
  *	 - netif_set_real_num_tx_queues()
  *	 - ethtool_op_get_link() (syncs link watch under rtnl_lock)
+ *	 - netif_open() / netif_close() (used by @self_test)
  *
  * @get_drvinfo: Report driver/device information. Modern drivers no
  *	longer have to implement this callback. Most fields are
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 4e5356e26f40..ae32e7fdb563 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -163,6 +163,8 @@ ethtool_ioctl_needs_rtnl(const struct net_device *dev, u32 ethcmd)
 		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_RSS;
 	case ETHTOOL_GLINK:
 		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_GLINK;
+	case ETHTOOL_TEST:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_TEST;
 	}
 	return false;
 }

[net PATCH v2 2/6] eth: fbnic: Handle maximum standalone channels

[Alexander Duyck]

From: Björn Töpel' via fbnic <fbnic@meta.com>

Standalone channels use one NAPI vector for each Tx and Rx queue.
fbnic's allocation path excludes FBNIC_MAX_TXQS from that layout. A
64-Tx/64-Rx configuration therefore records 128 vectors but allocates
only 64, leaving NULL entries that resource setup dereferences.

Include the maximum vector count in standalone allocation.

Fixes: bc6107771bb4 ("eth: fbnic: Allocate a netdevice and napi vectors with queues")
Signed-off-by: Björn Töpel <bjorn@kernel.org>
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 401f8b8ae1ca..81a30e2d449b 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1768,7 +1768,7 @@ int fbnic_alloc_napi_vectors(struct fbnic_net *fbn)
 	int err;
 
 	/* Allocate 1 Tx queue per napi vector */
-	if (num_napi < FBNIC_MAX_TXQS && num_napi == num_tx + num_rx) {
+	if (num_napi <= FBNIC_MAX_TXQS && num_napi == num_tx + num_rx) {
 		while (num_tx) {
 			err = fbnic_alloc_napi_vector(fbd, fbn,
 						      num_napi, v_idx,

[net PATCH v2 3/6] eth: fbnic: use the Rx queue napi pointer to find the napi vector

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The queue management ndos pick the napi vector for an Rx queue with:
	nv = fbn->napi[idx % fbn->num_napi];

The issue is this is only correct in the cases where there are no
standalone Tx vectors. In those cases we were allocating the Tx vectors
first and then the Rx so the queues would be pointing to Tx NAPI vectors
instead of the Rx ones.

The mapping the ndos want is already recorded.  fbnic_set_netif_napi()
publishes it with netif_queue_set_napi(), which stores the napi pointer
in netdev_rx_queue.napi, and fbnic_reset_netif_napi() clears it again.
Both run under the netdev instance lock that the queue management ndos
also hold, so the pointer can be read directly.

Use it and drop the divide.  The pointer is NULL exactly while the
datapath is down, so fbnic_queue_mem_alloc() can reject that case rather
than reaching into freed state: netdev_rx_queue_restart() calls it
before it tests netif_running(), and fbnic_pm_suspend() leaves
netif_running() true across a PCIe recovery that never completes, so a
queue restart can arrive after fbnic_stop() has freed the rings and the
vectors.  fbnic_stop() clears the association in
fbnic_reset_netif_queues() before fbnic_free_napi_vectors(), so the
NULL is always published first.  fbnic_queue_start() and
fbnic_queue_stop() need no check of their own, as
netdev_rx_queue_reconfig() only reaches them once fbnic_queue_mem_alloc()
has succeeded under the same instance lock.

Fixes: da43127a8edc ("eth: fbnic: support queue ops / zero-copy Rx")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c |   26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 81a30e2d449b..e93174fc1239 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -7,6 +7,7 @@
 #include <linux/iopoll.h>
 #include <linux/pci.h>
 #include <net/netdev_queues.h>
+#include <net/netdev_rx_queue.h>
 #include <net/page_pool/helpers.h>
 #include <net/tcp.h>
 #include <net/xdp.h>
@@ -2829,6 +2830,17 @@ void fbnic_napi_depletion_check(struct net_device *netdev)
 	fbnic_wrfl(fbd);
 }
 
+/* Returns the napi vector servicing an Rx queue, or NULL if the datapath
+ * is torn down.  The association is published by fbnic_set_netif_napi()
+ * and cleared by fbnic_reset_netif_napi(), both under the instance lock.
+ */
+static struct fbnic_napi_vector *fbnic_rxq_nv(struct net_device *dev, int idx)
+{
+	struct napi_struct *napi = __netif_get_rx_queue(dev, idx)->napi;
+
+	return napi ? container_of(napi, struct fbnic_napi_vector, napi) : NULL;
+}
+
 static int fbnic_queue_mem_alloc(struct net_device *dev,
 				 struct netdev_queue_config *qcfg,
 				 void *qmem, int idx)
@@ -2841,8 +2853,16 @@ static int fbnic_queue_mem_alloc(struct net_device *dev,
 	if (!netif_running(dev))
 		return fbnic_alloc_qt_page_pools(fbn, qt, idx);
 
+	/* A failed PCIe recovery or resume can leave the datapath torn down
+	 * while netif_running() is still true.  This ndo runs before
+	 * netdev_rx_queue_restart() checks netif_running(), so bail out
+	 * rather than touching rings and vectors that are already freed.
+	 */
+	nv = fbnic_rxq_nv(dev, idx);
+	if (!nv)
+		return -ENETDOWN;
+
 	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
-	nv = fbn->napi[idx % fbn->num_napi];
 
 	fbnic_ring_init(&qt->sub0, real->sub0.doorbell, real->sub0.q_idx,
 			real->sub0.flags);
@@ -2893,7 +2913,7 @@ static int fbnic_queue_start(struct net_device *dev,
 	struct fbnic_q_triad *real;
 
 	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
-	nv = fbn->napi[idx % fbn->num_napi];
+	nv = fbnic_rxq_nv(dev, idx);
 
 	fbnic_aggregate_ring_bdq_counters(fbn, &real->sub0);
 	fbnic_aggregate_ring_bdq_counters(fbn, &real->sub1);
@@ -2915,7 +2935,7 @@ static int fbnic_queue_stop(struct net_device *dev, void *qmem, int idx)
 	int err;
 
 	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
-	nv = fbn->napi[idx % fbn->num_napi];
+	nv = fbnic_rxq_nv(dev, idx);
 	fbnic_dbg_nv_exit(nv);
 
 	napi_disable_locked(&nv->napi);

[net PATCH v2 4/6] eth: fbnic: reset num_napi when the napi vectors are freed

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

fbn->num_napi is the count of live napi vectors, each of which owns an
IRQ.  The PM path had freed them without clearing the count.
fbnic_pm_suspend() tears the datapath down via ndo_stop() and frees the
IRQs, but leaves netif_running() true so resume knows to re-open.  Resume
rebuilds the datapath in __fbnic_pm_resume() and fbnic_reset_queues() sets
num_napi and __fbnic_open() re-allocates the vectors.

When the datapath is torn down but never rebuilt, num_napi is left
pointing at freed vectors under 2 different scenarios:
 - a PCIe error recovery that fails (fbnic_err_slot_reset() ->
   __fbnic_pm_resume() returns an error -> PCI_ERS_RESULT_DISCONNECT), so
   .resume never runs; or
 - an __fbnic_open() that fails partway on resume and unwinds, freeing
   the vectors after fbnic_reset_queues() has already set num_napi.

The netdev is then running with num_napi > 0 but napi[] freed, and the
eventual remove/unbind close re-enters fbnic_down() -> fbnic_dbg_down()
and dereferences the freed vectors:
  BUG: kernel NULL pointer dereference, address: 0000000000000210
  RIP: fbnic_dbg_down+0x28

Clear num_napi when the vectors are freed: in the suspend teardown (a
good resume re-establishes it before __fbnic_open()) and on the resume
open failure.  A redundant ndo_stop() then walks an empty napi[].  The
normal ndo_stop() down/up cycle is untouched and keeps num_napi for the
next ndo_open().

Fixes: bc6107771bb4 ("eth: fbnic: Allocate a netdevice and napi vectors with queues")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index 8b9bc9e8ea56..c6698e3002a1 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -434,6 +434,7 @@ static int fbnic_pm_suspend(struct device *dev)
 {
 	struct fbnic_dev *fbd = dev_get_drvdata(dev);
 	struct net_device *netdev = fbd->netdev;
+	struct fbnic_net *fbn;
 
 	if (fbnic_init_failure(fbd))
 		goto null_uc_addr;
@@ -441,11 +442,16 @@ static int fbnic_pm_suspend(struct device *dev)
 	rtnl_lock();
 	netdev_lock(netdev);
 
+	fbn = netdev_priv(netdev);
+
 	netif_device_detach(netdev);
 
 	if (netif_running(netdev))
 		netdev->netdev_ops->ndo_stop(netdev);
 
+	/* The IRQs are about to be freed, so drop the napi vector count */
+	fbn->num_napi = 0;
+
 	netdev_unlock(netdev);
 	rtnl_unlock();
 
@@ -508,16 +514,20 @@ static int __fbnic_pm_resume(struct device *dev)
 	if (fbnic_init_failure(fbd))
 		return 0;
 
+	rtnl_lock();
+	netdev_lock(netdev);
+
 	fbn = netdev_priv(netdev);
 
 	/* Reset the queues if needed */
 	fbnic_reset_queues(fbn, fbn->num_tx_queues, fbn->num_rx_queues);
 
-	rtnl_lock();
-	netdev_lock(netdev);
-
-	if (netif_running(netdev))
+	if (netif_running(netdev)) {
 		err = __fbnic_open(fbn);
+		/* On failure the vectors are freed, so drop the count */
+		if (err)
+			fbn->num_napi = 0;
+	}
 
 	netdev_unlock(netdev);
 	rtnl_unlock();

[net PATCH v2 5/6] eth: fbnic: Set AW_FLUSH_MODE alongside AW_FLUSH when flushing the mailbox

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

When tearing down the FW mailbox Rx ring, fbnic_mbx_reset_desc_ring()
writes AW_CFG with FLUSH set and everything else, BME included, cleared.
Clearing BME halts the device's writes to the host but leaves the staged
requests parked in the PUL write pipeline rather than draining them, so
on the write path FLUSH alone never terminates the outstanding requests
and the flush the firmware waits on never completes.

Add the FLUSH_MODE definition and set both bits so the staged writes
drain out of the pipeline on their own. BME stays cleared, so nothing
lands on the host; it is restored later in fbnic_mbx_init_desc_ring()
when the ring is rebuilt, once the outstanding writes are gone.

The read path is unaffected. AR_CFG has no equivalent mode bit and
AR_FLUSH terminates the outstanding reads by itself, so it is left as
is.

Both writes remain plain stores rather than read-modify-writes. That is
deliberate: the matching write in fbnic_mbx_init_desc_ring() restores
BME and the TLP attributes, and clears both flush bits as a side effect.

Fixes: 3b12f00ddd08 ("fbnic: Gate AXI read/write enabling on FW mailbox")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_csr.h |    1 +
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c  |    9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
index 64b958df7774..14af30e189d6 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
@@ -974,6 +974,7 @@ enum {
 /* PUL User Registers */
 #define FBNIC_CSR_START_PUL_USER	0x31000	/* CSR section delimiter */
 #define FBNIC_PUL_OB_TLP_HDR_AW_CFG	0x3103d		/* 0xc40f4 */
+#define FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH_MODE	CSR_BIT(20)
 #define FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH	CSR_BIT(19)
 #define FBNIC_PUL_OB_TLP_HDR_AW_CFG_BME		CSR_BIT(18)
 #define FBNIC_PUL_OB_TLP_HDR_AW_CFG_RDE_ATTR	CSR_GENMASK(17, 15)
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 283d25fae79e..59aa879798b9 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -60,8 +60,15 @@ static void fbnic_mbx_reset_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
 	 */
 	switch (mbx_idx) {
 	case FBNIC_IPC_MBX_RX_IDX:
+		/* Clearing BME blocks the device from writing to the host
+		 * but leaves the requests parked in the write pipeline. The
+		 * write path only clears outstanding requests when both FLUSH
+		 * and FLUSH_MODE are set; FLUSH_MODE lets them drain without
+		 * landing on the host.
+		 */
 		wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AW_CFG,
-		     FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH);
+		     FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH |
+		     FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH_MODE);
 		break;
 	case FBNIC_IPC_MBX_TX_IDX:
 		wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AR_CFG,

[net PATCH v2 6/6] eth: fbnic: Handle FW mailbox completions flagged with an error

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The firmware can complete a mailbox descriptor while also setting FW_ERR
to indicate it could not process the request, for example on a mailbox
DMA error. The completion carries no valid data.

The driver did not check FW_ERR. On the Rx mailbox it would sync and
parse the stale page as a normal message, and on the Tx mailbox it
silently freed the request. If the initial capabilities exchange in
fbnic_mbx_poll_tx_ready() hit FW_ERR -- on the Tx request or on the Rx
response descriptor -- no response was parsed and the poll spun until it
timed out even though the ring was healthy.

Check FW_ERR on both mailboxes. Count it per-mailbox in
fbnic_fw_mbx.resp_error, which is also shown in debugfs, warn (rate
limited, since the bit is firmware controlled), and drop the Rx page
instead of parsing it.

In fbnic_mbx_poll_tx_ready() re-issue the capabilities request when
either the Tx or the Rx resp_error counter advances, so a FW_ERR on the
request or on its response triggers a retry rather than a timeout. A
valid capabilities response is honored before the retry check, so a
response parsed in the same poll as an unrelated FW_ERR is not discarded.
The counters are mailbox-wide rather than keyed to the capabilities
request; that is sufficient here because the exchange runs during
bring-up before any other mailbox traffic, and any spurious retry is
bounded by the existing 10s timeout.

Fixes: da3cde08209e ("eth: fbnic: Add FW communication mechanism")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_csr.h     |    4 ++
 drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c |    4 +-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c      |   38 ++++++++++++++++++++++-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h      |    1 +
 4 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
index 14af30e189d6..baba3471bf5a 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
@@ -1216,6 +1216,10 @@ enum {
 #define FBNIC_IPC_MBX_DESC_LEN_MASK	DESC_GENMASK(63, 48)
 #define FBNIC_IPC_MBX_DESC_EOM		DESC_BIT(46)
 #define FBNIC_IPC_MBX_DESC_ADDR_MASK	DESC_GENMASK(45, 3)
+/* Set with FW_CMPL when the FW completed a descriptor without successfully
+ * processing it (e.g. a mailbox DMA error); the completion has no valid data.
+ */
+#define FBNIC_IPC_MBX_DESC_FW_ERR	DESC_BIT(2)
 #define FBNIC_IPC_MBX_DESC_FW_CMPL	DESC_BIT(1)
 #define FBNIC_IPC_MBX_DESC_HOST_CMPL	DESC_BIT(0)
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c b/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c
index 3c4563c8f403..6edfa0aa69f1 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c
@@ -539,8 +539,8 @@ static void fbnic_dbg_fw_mbx_display(struct seq_file *s,
 	/* Generate header */
 	seq_puts(s, mbx_idx == FBNIC_IPC_MBX_RX_IDX ? "Rx\n" : "Tx\n");
 
-	seq_printf(s, "Rdy: %d Head: %d Tail: %d\n",
-		   mbx->ready, mbx->head, mbx->tail);
+	seq_printf(s, "Rdy: %d Head: %d Tail: %d resp_error: %llu\n",
+		   mbx->ready, mbx->head, mbx->tail, mbx->resp_error);
 
 	snprintf(hdr, sizeof(hdr), "%3s %-4s %s %-12s %s %-3s %-16s\n",
 		 "Idx", "Len", "E", "Addr", "F", "H", "Raw");
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 59aa879798b9..6d7eb8479edf 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -292,6 +292,12 @@ static void fbnic_mbx_process_tx_msgs(struct fbnic_dev *fbd)
 		if (!(desc & FBNIC_IPC_MBX_DESC_FW_CMPL))
 			break;
 
+		if (desc & FBNIC_IPC_MBX_DESC_FW_ERR) {
+			tx_mbx->resp_error++;
+			dev_warn_ratelimited(fbd->dev,
+					     "FW completed a Tx mailbox request with an error\n");
+		}
+
 		fbnic_mbx_unmap_and_free_msg(fbd, FBNIC_IPC_MBX_TX_IDX, head);
 
 		head++;
@@ -1673,6 +1679,13 @@ static void fbnic_mbx_process_rx_msgs(struct fbnic_dev *fbd)
 		if (!(desc & FBNIC_IPC_MBX_DESC_FW_CMPL))
 			break;
 
+		if (desc & FBNIC_IPC_MBX_DESC_FW_ERR) {
+			rx_mbx->resp_error++;
+			dev_warn_ratelimited(fbd->dev,
+					     "FW reported an error on an Rx mailbox message; dropping\n");
+			goto next_page;
+		}
+
 		dma_sync_single_for_cpu(fbd->dev, rx_mbx->buf_info[head].addr,
 					FBNIC_RX_PAGE_SIZE, DMA_FROM_DEVICE);
 
@@ -1740,7 +1753,9 @@ void fbnic_mbx_poll(struct fbnic_dev *fbd)
 int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
 {
 	struct fbnic_fw_mbx *tx_mbx = &fbd->mbx[FBNIC_IPC_MBX_TX_IDX];
+	struct fbnic_fw_mbx *rx_mbx = &fbd->mbx[FBNIC_IPC_MBX_RX_IDX];
 	unsigned long timeout = jiffies + 10 * HZ + 1;
+	u64 tx_resp_error, rx_resp_error;
 	int err, i;
 
 	do {
@@ -1771,6 +1786,9 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
 	 * mgmt.version once we get the actual version from the firmware
 	 * in the capabilities request message.
 	 */
+send_cap_req:
+	tx_resp_error = tx_mbx->resp_error;
+	rx_resp_error = rx_mbx->resp_error;
 	err = fbnic_fw_xmit_simple_msg(fbd, FBNIC_TLV_MSG_ID_HOST_CAP_REQ);
 	if (err)
 		goto clean_mbx;
@@ -1788,9 +1806,27 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
 		msleep(20);
 		fbnic_mbx_poll(fbd);
 
+		/* A valid capabilities response ends the poll. Check it
+		 * before the FW_ERR retry below so a response parsed in the
+		 * same poll as an unrelated FW_ERR is not discarded.
+		 */
+		if (fbd->fw_cap.running.mgmt.version >= MIN_FW_VER_CODE)
+			break;
+
 		/* set err, but wait till mgmt.version check to report it */
-		if (!time_is_after_jiffies(timeout))
+		if (!time_is_after_jiffies(timeout)) {
 			err = -ETIMEDOUT;
+			continue;
+		}
+
+		/* The FW can flag our capabilities request (Tx) or its
+		 * response (Rx) with FW_ERR, in which case it produced no
+		 * usable response. The ring is not wedged, so re-issue the
+		 * request instead of spinning until the timeout.
+		 */
+		if (tx_mbx->resp_error != tx_resp_error ||
+		    rx_mbx->resp_error != rx_resp_error)
+			goto send_cap_req;
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index d84723e4cfa3..5f9969247e30 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -13,6 +13,7 @@ struct fbnic_tlv_msg;
 
 struct fbnic_fw_mbx {
 	u8 ready, head, tail;
+	u64 resp_error;
 	struct {
 		struct fbnic_tlv_msg	*msg;
 		dma_addr_t		addr;

Re: [net PATCH v2 3/6] eth: fbnic: use the Rx queue napi pointer to find the napi vector

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0

- [High] The invariant the patch relies on - "The pointer is NULL exactly
  while the datapath is down" - does not hold across PCIe…

Pre-existing issues:
- [High] fbnic_queue_stop() uses nv->qt[t].sub0.q_idx as the queue
  identifier for every triad, but that field is 0 for two kinds of…
- [High] fbnic_alloc_qt_page_pools(): when the queue has an unreadable
  memory provider, the second page_pool_create() overwrites the local…

--

Here is the review report.

eth: fbnic: use the Rx queue napi pointer to find the napi vector

The queue management ndos stop deriving the napi vector from
fbn->napi[idx % fbn->num_napi] and instead read the association that
fbnic_set_netif_napi() published in netdev_rx_queue.napi.  A new helper
fbnic_rxq_nv() performs the lookup, and fbnic_queue_mem_alloc() returns
-ENETDOWN when the pointer is NULL.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 81a30e2d449b2..e93174fc1239f 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c

[ ... ]

> @@ -2829,6 +2830,17 @@ void fbnic_napi_depletion_check(struct net_device *netdev)
>  	fbnic_wrfl(fbd);
>  }
>  
> +/* Returns the napi vector servicing an Rx queue, or NULL if the datapath
> + * is torn down.  The association is published by fbnic_set_netif_napi()
> + * and cleared by fbnic_reset_netif_napi(), both under the instance lock.
> + */
> +static struct fbnic_napi_vector *fbnic_rxq_nv(struct net_device *dev, int idx)
> +{
> +	struct napi_struct *napi = __netif_get_rx_queue(dev, idx)->napi;
> +
> +	return napi ? container_of(napi, struct fbnic_napi_vector, napi) : NULL;
> +}
> +

[Severity: High]
The commit message says "The pointer is NULL exactly while the datapath is
down".  Does that hold across the resume path?

In fbnic_pci.c, the publish and the enable are split across two calls with
the instance lock dropped in between:

__fbnic_pm_resume()
    netdev_lock()
    __fbnic_open()
        fbnic_set_netif_queues()
            fbnic_set_netif_napi()
                netif_queue_set_napi()   /* publishes rxq->napi */
    netdev_unlock()

__fbnic_pm_attach()
    if (netif_running(netdev))
        fbnic_up(fbn);               /* napi_enable_locked() happens here */
    netif_device_attach(netdev);

In the window between those two, netif_running() is true and rxq->napi is
already non-NULL, but the NAPIs have only been added, not enabled.

Can a queue restart arriving in that window get past the new guard and
reach fbnic_queue_stop()?  netdev_rx_queue_reconfig() has no
netif_device_present() gate:

	err = qops->ndo_queue_mem_alloc(dev, qcfg_new, new_mem, rxq_idx);
	...
	if (netif_running(dev)) {
		err = qops->ndo_queue_stop(dev, old_mem, rxq_idx);

One reachable caller is netdev_nl_sock_priv_destroy() on netlink socket
close, which takes netdev_lock(dev) and walks
net_devmem_unbind_dmabuf() -> netif_mp_close_rxq() ->
netdev_rx_queue_reconfig().

[ ... ]

> @@ -2841,8 +2853,16 @@ static int fbnic_queue_mem_alloc(struct net_device *dev,
>  	if (!netif_running(dev))
>  		return fbnic_alloc_qt_page_pools(fbn, qt, idx);
>  
> +	/* A failed PCIe recovery or resume can leave the datapath torn down
> +	 * while netif_running() is still true.  This ndo runs before
> +	 * netdev_rx_queue_restart() checks netif_running(), so bail out
> +	 * rather than touching rings and vectors that are already freed.
> +	 */
> +	nv = fbnic_rxq_nv(dev, idx);
> +	if (!nv)
> +		return -ENETDOWN;
> +

[Severity: High]
Continuing the point above: does a non-NULL rxq->napi actually imply the
datapath is up?  For the resume window the pointer is published while the
vectors exist but are not enabled, so this check passes and
fbnic_queue_stop() runs napi_disable_locked() on a NAPI that was only added
by netif_napi_add_config_locked(), which leaves NAPI_STATE_SCHED and
NAPI_STATE_NPSVC set:

net/core/dev.c:napi_disable_locked() {
	...
	do {
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}
	...
}

napi_enable_locked() is the only thing that clears those bits, and it has
not run yet, so does this loop spin forever with the netdev instance lock
held, blocking the pending fbnic_up(), dev_close(), unregister and ethtool?

Would a "datapath enabled" state test, or moving fbnic_up() inside the
instance-locked region of the resume path, be a better fit than keying off
the published pointer?

> +	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
> -	nv = fbn->napi[idx % fbn->num_napi];
>  
>  	fbnic_ring_init(&qt->sub0, real->sub0.doorbell, real->sub0.q_idx,
>  			real->sub0.flags);

[Severity: High]
This is a pre-existing issue, but since both branches of this ndo reach it,
could fbnic_alloc_qt_page_pools() be dereferencing an error pointer?  The
second page_pool_create() overwrites the local pp, and the error label then
destroys that value rather than the pool it should be releasing:

	qt->sub0.page_pool = pp;
	if (netif_rxq_has_unreadable_mp(fbn->netdev, rxq_idx)) {
		...
		pp = page_pool_create(&pp_params);
		if (IS_ERR(pp))
			goto err_destroy_sub0;
	...
err_destroy_sub0:
	page_pool_destroy(pp);
	return PTR_ERR(pp);

net/core/page_pool.c:page_pool_destroy() only filters NULL:

	if (!pool)
		return;

	if (!page_pool_put(pool))
		return;

so page_pool_put() would dereference the ERR_PTR, and qt->sub0.page_pool
leaks because netdev_rx_queue_reconfig() does not call ndo_queue_mem_free
on the mem_alloc failure path.  The reachable trigger is a devmem or
io_uring bind on a queue with an unreadable memory provider where the
second page_pool_create() fails with -ENOMEM, or where the provider's
->init() returns an error.  Would saving the error before destroying
qt->sub0.page_pool address this?

[ ... ]

> @@ -2915,7 +2935,7 @@ static int fbnic_queue_stop(struct net_device *dev, void *qmem, int idx)
>  	int err;
>  
>  	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
> -	nv = fbn->napi[idx % fbn->num_napi];
> +	nv = fbnic_rxq_nv(dev, idx);
>  	fbnic_dbg_nv_exit(nv);
>  
>  	napi_disable_locked(&nv->napi);

[Severity: High]
This isn't a bug introduced by this patch, but does the standalone-Tx
configuration this patch targets remain incorrect afterwards?  The rest of
fbnic_queue_stop() uses sub0.q_idx as the queue identifier:

	for (i = 0; i < nv->txt_count; i++)
		netif_stop_subqueue(dev, nv->qt[i].sub0.q_idx);
	fbnic_nv_disable(fbn, nv);

	for (t = 0; t < nv->txt_count + nv->rxt_count; t++) {
		err = fbnic_wait_queue_idle(fbn, t >= nv->txt_count,
					    nv->qt[t].sub0.q_idx);

but sub0.q_idx is 0 for two kinds of triads.  For the placeholder Tx triad
of an Rx-only vector, fbnic_alloc_napi_vector() sets:

		} else {
			fbnic_ring_init(&qt->sub0, db, 0,
					FBNIC_RING_F_DISABLED);
		}

and for the Rx BDQ rings:

		db = &uc_addr[FBNIC_QUEUE(rxq_idx) + FBNIC_QUEUE_BDQ_HPQ_TAIL];
		fbnic_ring_init(&qt->sub0, db, 0,
				FBNIC_RING_F_CTX | FBNIC_RING_F_STATS);

so the real HW block index only lives in cmpl.q_idx.

For an Rx-only vector, does netif_stop_subqueue(dev, 0) freeze netdev Tx
queue 0, a live ring owned by another vector, with __fbnic_nv_restart()
later re-waking it on both the success and err_restart paths?

	for (i = 0; i < nv->txt_count; i++)
		netif_wake_subqueue(fbn->netdev, nv->qt[i].sub0.q_idx);

And for Rx queue N > 0, does fbnic_wait_queue_idle(fbn, true, 0) poll Rx
queue 0's HPQ/PPQ/RCQ idle bits rather than queue N's, so quiescence of the
ring being torn down is never established before fbnic_nv_flush() cleans and
unmaps the BDQ pages and memsets the RCQ?  Similarly,
fbnic_wait_queue_idle(fbn, false, 0) on the disabled placeholder polls HW Tx
queue 0, which this path never disables, so under Tx load it would time out
and take the err_restart path naming the wrong queue.

fbnic_nv_flush() already carries the guard these two loops lack:

		/* Nothing else to do if Tx queue is disabled */
		if (qt->sub0.flags & FBNIC_RING_F_DISABLED)
			continue;

Should the stop/wake and idle-wait loops skip FBNIC_RING_F_DISABLED rings
and use the identifier appropriate to each triad?  A configuration that
reaches this today is combined 2 rx 2, which fbnic_set_channels() accepts
since only rx && tx && combined is rejected; that yields num_napi 4 with
vectors 2 and 3 having tqpv == 0.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/178941996343.7700.9376081102002673062.stgit%40ahduyck-xeon-server.home.arpa

Re: [net PATCH v2 6/6] eth: fbnic: Handle FW mailbox completions flagged with an error

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] FW_ERR handling does not release the outstanding-request
  bookkeeping: when the firmware completes a Tx descriptor with…

--

eth: fbnic: Handle FW mailbox completions flagged with an error

The patch adds a check for FBNIC_IPC_MBX_DESC_FW_ERR on both the Tx and
Rx firmware mailboxes, counts the occurrences per mailbox in
fbnic_fw_mbx.resp_error (also exported via debugfs), emits a rate limited
warning, and drops the Rx page instead of parsing it. It also re-issues
the bring-up capabilities request in fbnic_mbx_poll_tx_ready() when
either resp_error counter advances.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> index 14af30e189d6c..baba3471bf5a4 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> @@ -1216,6 +1216,10 @@ enum {
>  #define FBNIC_IPC_MBX_DESC_LEN_MASK	DESC_GENMASK(63, 48)
>  #define FBNIC_IPC_MBX_DESC_EOM		DESC_BIT(46)
>  #define FBNIC_IPC_MBX_DESC_ADDR_MASK	DESC_GENMASK(45, 3)
> +/* Set with FW_CMPL when the FW completed a descriptor without successfully
> + * processing it (e.g. a mailbox DMA error); the completion has no valid data.
> + */
> +#define FBNIC_IPC_MBX_DESC_FW_ERR	DESC_BIT(2)
>  #define FBNIC_IPC_MBX_DESC_FW_CMPL	DESC_BIT(1)
>  #define FBNIC_IPC_MBX_DESC_HOST_CMPL	DESC_BIT(0)
>  

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 59aa879798b9f..6d7eb8479edf4 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -292,6 +292,12 @@ static void fbnic_mbx_process_tx_msgs(struct fbnic_dev *fbd)
>  		if (!(desc & FBNIC_IPC_MBX_DESC_FW_CMPL))
>  			break;
>  
> +		if (desc & FBNIC_IPC_MBX_DESC_FW_ERR) {
> +			tx_mbx->resp_error++;
> +			dev_warn_ratelimited(fbd->dev,
> +					     "FW completed a Tx mailbox request with an error\n");
> +		}
> +
>  		fbnic_mbx_unmap_and_free_msg(fbd, FBNIC_IPC_MBX_TX_IDX, head);

[Severity: Medium]
Should this path also release the completion registered for the request
that the firmware just abandoned?

The new comment in fbnic_csr.h says the FW_ERR completion "has no valid
data", so no response TLV will follow. Requests submitted through
fbnic_mbx_map_req_w_cmpl() leave a struct fbnic_fw_completion installed in
fbd->cmpl_data[], and fbnic_mbx_process_tx_msgs() here only bumps
resp_error and frees the page, so that slot stays armed.

For example, with a TSENE read flagged FW_ERR on the Tx descriptor:

  fbnic_mac_get_sensor_asic()
    ...
	if (!wait_for_completion_timeout(&fw_cmpl->done, 10 * HZ)) {
		dev_err(fbd->dev, "Timed out waiting for TSENE read\n");

Does the hwmon reader then block for the full 10 seconds even though the
driver already knows at FW_ERR time that no answer is coming?

There is a second effect while that stale waiter is pending. In
fbnic_mbx_set_cmpl_slot():

	for (i = 0; i < FBNIC_MBX_CMPL_SLOTS; i++) {
		if (!fbd->cmpl_data[i])
			free = i;
		else if (fbd->cmpl_data[i]->msg_type == cmpl_data->msg_type)
			return -EEXIST;
	}

so a second request of the same msg_type in that window is rejected with
-EEXIST until the first waiter times out and clears its slot.

The driver already has a way to say "no response will arrive":

	static void __fbnic_fw_evict_cmpl(struct fbnic_fw_completion *cmpl_data)
	{
		cmpl_data->result = -EPIPE;
		complete(&cmpl_data->done);
	}

but that is only reached from fbnic_mbx_evict_all_cmpl() in
fbnic_mbx_flush_tx() during mailbox teardown, never from the new
per-descriptor FW_ERR handling. Could the FW_ERR path look up the
matching completion (the TLV, and therefore its msg type, is still in
tx_mbx->buf_info[head].msg at this point) and complete it with an error?

> @@ -1673,6 +1679,13 @@ static void fbnic_mbx_process_rx_msgs(struct fbnic_dev *fbd)
>  		if (!(desc & FBNIC_IPC_MBX_DESC_FW_CMPL))
>  			break;
>  
> +		if (desc & FBNIC_IPC_MBX_DESC_FW_ERR) {
> +			rx_mbx->resp_error++;
> +			dev_warn_ratelimited(fbd->dev,
> +					     "FW reported an error on an Rx mailbox message; dropping\n");
> +			goto next_page;
> +		}
> +
>  		dma_sync_single_for_cpu(fbd->dev, rx_mbx->buf_info[head].addr,
>  					FBNIC_RX_PAGE_SIZE, DMA_FROM_DEVICE);
>  

[Severity: Medium]
The same question applies on the Rx side: when the dropped descriptor was
the response to a request with a completion installed, nothing wakes the
waiter either, so callers such as fbnic_flash_start():

	if (fbnic_mbx_wait_for_cmpl(cmpl))
		err = cmpl->result;
	else
		err = -ETIMEDOUT;

burn the full FBNIC_MBX_RX_TO_SEC wait and report -ETIMEDOUT rather than
an error.

The retry added in fbnic_mbx_poll_tx_ready() covers the bring-up
capabilities exchange, but the QSFP EEPROM read, coredump info/read and
the FW upgrade start/write/finish requests all run after bring-up and are
not covered. Is that intentional for this patch, or should the completion
bookkeeping be released for those too?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/178941996343.7700.9376081102002673062.stgit%40ahduyck-xeon-server.home.arpa

Re: [net PATCH v2 3/6] eth: fbnic: use the Rx queue napi pointer to find the napi vector

[Alexander Duyck]

On Thu, Sep 17, 2026 at 2:12 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0
>
> - [High] The invariant the patch relies on - "The pointer is NULL exactly
>   while the datapath is down" - does not hold across PCIe…
>
> Pre-existing issues:
> - [High] fbnic_queue_stop() uses nv->qt[t].sub0.q_idx as the queue
>   identifier for every triad, but that field is 0 for two kinds of…
> - [High] fbnic_alloc_qt_page_pools(): when the queue has an unreadable
>   memory provider, the second page_pool_create() overwrites the local…
>
> --
>
> Here is the review report.
>
> eth: fbnic: use the Rx queue napi pointer to find the napi vector
>
> The queue management ndos stop deriving the napi vector from
> fbn->napi[idx % fbn->num_napi] and instead read the association that
> fbnic_set_netif_napi() published in netdev_rx_queue.napi.  A new helper
> fbnic_rxq_nv() performs the lookup, and fbnic_queue_mem_alloc() returns
> -ENETDOWN when the pointer is NULL.
>
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> > index 81a30e2d449b2..e93174fc1239f 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
>
> [ ... ]
>
> > @@ -2829,6 +2830,17 @@ void fbnic_napi_depletion_check(struct net_device *netdev)
> >       fbnic_wrfl(fbd);
> >  }
> >
> > +/* Returns the napi vector servicing an Rx queue, or NULL if the datapath
> > + * is torn down.  The association is published by fbnic_set_netif_napi()
> > + * and cleared by fbnic_reset_netif_napi(), both under the instance lock.
> > + */
> > +static struct fbnic_napi_vector *fbnic_rxq_nv(struct net_device *dev, int idx)
> > +{
> > +     struct napi_struct *napi = __netif_get_rx_queue(dev, idx)->napi;
> > +
> > +     return napi ? container_of(napi, struct fbnic_napi_vector, napi) : NULL;
> > +}
> > +
>
> [Severity: High]
> The commit message says "The pointer is NULL exactly while the datapath is
> down".  Does that hold across the resume path?
>
> In fbnic_pci.c, the publish and the enable are split across two calls with
> the instance lock dropped in between:
>
> __fbnic_pm_resume()
>     netdev_lock()
>     __fbnic_open()
>         fbnic_set_netif_queues()
>             fbnic_set_netif_napi()
>                 netif_queue_set_napi()   /* publishes rxq->napi */
>     netdev_unlock()
>
> __fbnic_pm_attach()
>     if (netif_running(netdev))
>         fbnic_up(fbn);               /* napi_enable_locked() happens here */
>     netif_device_attach(netdev);

This one has me scratching my head in how the code was able to get
into this state in the first place and has been this way for so long.
I am assuming it was relying on the netif_device_detach/attach for
some security. I would probably fix this as a separate patch as it
requires restructuring for several reasons, and I would classify it as
a pre-existing issue.

First the fact that the fbnic_up operating outside of any locking is
problematic at best as it could easily race with a dev_close request
and end up leaving things in a bad state. Second it looks like this
pattern exists in other drivers as well such as igb so we will need to
likely fix it across several drivers.

Basically we should be holding the rtnl and the netdev_lock and run
things from open through up all in one shot, likely including the
netdev_device_attach. What I will probably look at doing is merging
the __fbnic_pm_resume and __fbnic_pm_attach calls all into
fbnic_pm_resume and then only call it from the err_resume instead of
running down two different paths.

> [ ... ]
>
> > @@ -2841,8 +2853,16 @@ static int fbnic_queue_mem_alloc(struct net_device *dev,
> >       if (!netif_running(dev))
> >               return fbnic_alloc_qt_page_pools(fbn, qt, idx);
> >
> > +     /* A failed PCIe recovery or resume can leave the datapath torn down
> > +      * while netif_running() is still true.  This ndo runs before
> > +      * netdev_rx_queue_restart() checks netif_running(), so bail out
> > +      * rather than touching rings and vectors that are already freed.
> > +      */
> > +     nv = fbnic_rxq_nv(dev, idx);
> > +     if (!nv)
> > +             return -ENETDOWN;
> > +
>
> [Severity: High]
> Continuing the point above: does a non-NULL rxq->napi actually imply the
> datapath is up?  For the resume window the pointer is published while the
> vectors exist but are not enabled, so this check passes and
> fbnic_queue_stop() runs napi_disable_locked() on a NAPI that was only added
> by netif_napi_add_config_locked(), which leaves NAPI_STATE_SCHED and
> NAPI_STATE_NPSVC set:
>
> net/core/dev.c:napi_disable_locked() {
>         ...
>         do {
>                 while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
>                         usleep_range(20, 200);
>                         val = READ_ONCE(n->state);
>                 }
>         ...
> }
>
> napi_enable_locked() is the only thing that clears those bits, and it has
> not run yet, so does this loop spin forever with the netdev instance lock
> held, blocking the pending fbnic_up(), dev_close(), unregister and ethtool?
>
> Would a "datapath enabled" state test, or moving fbnic_up() inside the
> instance-locked region of the resume path, be a better fit than keying off
> the published pointer?

Odds are pretty slim for hitting this race, but this would be
addressed via the locking bug fix I called out above. Again the
pattern before was more of a NULL pointer dereference that was a
guaranteed hit versus this locking race.

> > +     real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
> > -     nv = fbn->napi[idx % fbn->num_napi];
> >
> >       fbnic_ring_init(&qt->sub0, real->sub0.doorbell, real->sub0.q_idx,
> >                       real->sub0.flags);
>
> [Severity: High]
> This is a pre-existing issue, but since both branches of this ndo reach it,
> could fbnic_alloc_qt_page_pools() be dereferencing an error pointer?  The
> second page_pool_create() overwrites the local pp, and the error label then
> destroys that value rather than the pool it should be releasing:
>
>         qt->sub0.page_pool = pp;
>         if (netif_rxq_has_unreadable_mp(fbn->netdev, rxq_idx)) {
>                 ...
>                 pp = page_pool_create(&pp_params);
>                 if (IS_ERR(pp))
>                         goto err_destroy_sub0;
>         ...
> err_destroy_sub0:
>         page_pool_destroy(pp);
>         return PTR_ERR(pp);
>
> net/core/page_pool.c:page_pool_destroy() only filters NULL:
>
>         if (!pool)
>                 return;
>
>         if (!page_pool_put(pool))
>                 return;
>
> so page_pool_put() would dereference the ERR_PTR, and qt->sub0.page_pool
> leaks because netdev_rx_queue_reconfig() does not call ndo_queue_mem_free
> on the mem_alloc failure path.  The reachable trigger is a devmem or
> io_uring bind on a queue with an unreadable memory provider where the
> second page_pool_create() fails with -ENOMEM, or where the provider's
> ->init() returns an error.  Would saving the error before destroying
> qt->sub0.page_pool address this?
>
> [ ... ]

This is yet another pre-existing issue that would likely be better
fixed as it is an unrelated problem. Looks like it should be a 2 liner
to use sub0.page_pool instead of pp in page_pool_destroy, and then to
reset that back to NULL.

> > @@ -2915,7 +2935,7 @@ static int fbnic_queue_stop(struct net_device *dev, void *qmem, int idx)
> >       int err;
> >
> >       real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
> > -     nv = fbn->napi[idx % fbn->num_napi];
> > +     nv = fbnic_rxq_nv(dev, idx);
> >       fbnic_dbg_nv_exit(nv);
> >
> >       napi_disable_locked(&nv->napi);
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does the standalone-Tx
> configuration this patch targets remain incorrect afterwards?  The rest of
> fbnic_queue_stop() uses sub0.q_idx as the queue identifier:
>
>         for (i = 0; i < nv->txt_count; i++)
>                 netif_stop_subqueue(dev, nv->qt[i].sub0.q_idx);
>         fbnic_nv_disable(fbn, nv);
>
>         for (t = 0; t < nv->txt_count + nv->rxt_count; t++) {
>                 err = fbnic_wait_queue_idle(fbn, t >= nv->txt_count,
>                                             nv->qt[t].sub0.q_idx);
>
> but sub0.q_idx is 0 for two kinds of triads.  For the placeholder Tx triad
> of an Rx-only vector, fbnic_alloc_napi_vector() sets:
>
>                 } else {
>                         fbnic_ring_init(&qt->sub0, db, 0,
>                                         FBNIC_RING_F_DISABLED);
>                 }
>
> and for the Rx BDQ rings:
>
>                 db = &uc_addr[FBNIC_QUEUE(rxq_idx) + FBNIC_QUEUE_BDQ_HPQ_TAIL];
>                 fbnic_ring_init(&qt->sub0, db, 0,
>                                 FBNIC_RING_F_CTX | FBNIC_RING_F_STATS);
>
> so the real HW block index only lives in cmpl.q_idx.

I agree that this is a pre-existing bug, but I wouldn't fix it in this
patch. I would probably introduce yet another patch to resolve it.

By my count we have 3 pre-existing issues that aren't directly related
to the original problem. If possible I would prefer to address them
separately, otherwise I can tack on 3 additional diffs to the set and
resubmit.

Re: [net PATCH v2 6/6] eth: fbnic: Handle FW mailbox completions flagged with an error

[Alexander Duyck]

On Thu, Sep 17, 2026 at 2:12 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> - [Medium] FW_ERR handling does not release the outstanding-request
>   bookkeeping: when the firmware completes a Tx descriptor with…
>
> --
>
> eth: fbnic: Handle FW mailbox completions flagged with an error
>
> The patch adds a check for FBNIC_IPC_MBX_DESC_FW_ERR on both the Tx and
> Rx firmware mailboxes, counts the occurrences per mailbox in
> fbnic_fw_mbx.resp_error (also exported via debugfs), emits a rate limited
> warning, and drops the Rx page instead of parsing it. It also re-issues
> the bring-up capabilities request in fbnic_mbx_poll_tx_ready() when
> either resp_error counter advances.
>
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> > index 14af30e189d6c..baba3471bf5a4 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
> > @@ -1216,6 +1216,10 @@ enum {
> >  #define FBNIC_IPC_MBX_DESC_LEN_MASK  DESC_GENMASK(63, 48)
> >  #define FBNIC_IPC_MBX_DESC_EOM               DESC_BIT(46)
> >  #define FBNIC_IPC_MBX_DESC_ADDR_MASK DESC_GENMASK(45, 3)
> > +/* Set with FW_CMPL when the FW completed a descriptor without successfully
> > + * processing it (e.g. a mailbox DMA error); the completion has no valid data.
> > + */
> > +#define FBNIC_IPC_MBX_DESC_FW_ERR    DESC_BIT(2)
> >  #define FBNIC_IPC_MBX_DESC_FW_CMPL   DESC_BIT(1)
> >  #define FBNIC_IPC_MBX_DESC_HOST_CMPL DESC_BIT(0)
> >
>
> [ ... ]
>

[...]

> but that is only reached from fbnic_mbx_evict_all_cmpl() in
> fbnic_mbx_flush_tx() during mailbox teardown, never from the new
> per-descriptor FW_ERR handling. Could the FW_ERR path look up the
> matching completion (the TLV, and therefore its msg type, is still in
> tx_mbx->buf_info[head].msg at this point) and complete it with an error?
>
> > @@ -1673,6 +1679,13 @@ static void fbnic_mbx_process_rx_msgs(struct fbnic_dev *fbd)
> >               if (!(desc & FBNIC_IPC_MBX_DESC_FW_CMPL))
> >                       break;
> >
> > +             if (desc & FBNIC_IPC_MBX_DESC_FW_ERR) {
> > +                     rx_mbx->resp_error++;
> > +                     dev_warn_ratelimited(fbd->dev,
> > +                                          "FW reported an error on an Rx mailbox message; dropping\n");
> > +                     goto next_page;
> > +             }
> > +
> >               dma_sync_single_for_cpu(fbd->dev, rx_mbx->buf_info[head].addr,
> >                                       FBNIC_RX_PAGE_SIZE, DMA_FROM_DEVICE);
> >
>
> [Severity: Medium]
> The same question applies on the Rx side: when the dropped descriptor was
> the response to a request with a completion installed, nothing wakes the
> waiter either, so callers such as fbnic_flash_start():
>
>         if (fbnic_mbx_wait_for_cmpl(cmpl))
>                 err = cmpl->result;
>         else
>                 err = -ETIMEDOUT;
>
> burn the full FBNIC_MBX_RX_TO_SEC wait and report -ETIMEDOUT rather than
> an error.
>
> The retry added in fbnic_mbx_poll_tx_ready() covers the bring-up
> capabilities exchange, but the QSFP EEPROM read, coredump info/read and
> the FW upgrade start/write/finish requests all run after bring-up and are
> not covered. Is that intentional for this patch, or should the completion
> bookkeeping be released for those too?

The modification this way was intentional. Basically the most likely
place for us to ever encounter an issue with  DMA is on mailbox
initialization. Doing it this way reduces the driver init time as
instead of waiting 10s for the mailbox polling to fail we just burn
through the DMA failures and get to the result quickly indicating that
the mailbox is dead.

Doing the same for the other paths adds extra complexity as they are
interrupt driver versus the mailbox init which is synchronous. In
addition at mailbox init we can only have one completion in flight,
whereas for the other calls we can actually have multiple so without
context we won't know which completion it is we need to release.

Re: [net PATCH v2 1/6] net: ethtool: keep rtnl_lock for the ioctl self test

[Simon Horman]

On Mon, Sep 14, 2026 at 02:09:57PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> An offline self test that brings the interface down and back up with
> netif_close() / netif_open() requires rtnl_lock for both. Since the
> ethtool IOCTL path became rtnl-optional for ops-locked drivers, the
> ETHTOOL_TEST ioctl runs holding only the netdev instance lock, so on an
> ops-locked driver the self test now tears the device down without
> rtnl_lock.
> 
> With lockdep this reproduces deterministically on every offline self
> test on such a driver; note the sole lock held is the instance lock, not
> rtnl:
> 
>   WARNING: suspicious RCU usage
>   net/core/netpoll.c:207 suspicious rcu_dereference_protected() usage!
>   1 lock held by ethtool/107:
>    #0: (&dev->lock){+.+.}, at: dev_ethtool
>   Call Trace:
>    netpoll_poll_disable
>    __dev_close_many
>    netif_close_many
>    netif_close
>    fbnic_self_test
>    dev_ethtool_locked
>    dev_ethtool
>    dev_ioctl
>    sock_ioctl
>    __x64_sys_ioctl
> 
> Without lockdep the same condition trips ASSERT_RTNL() in
> __dev_close_many() / __dev_open(); that check only samples the global
> rtnl state, so it can be masked by a concurrent rtnl holder, but the
> device is still being reconfigured without the lock it requires.
> 
> The ethtool self_test is a legacy ioctl-only command, so an ETHTOOL_TEST
> case is only needed on the ioctl path. Add an opt-in bit for drivers whose
> self test needs rtnl_lock and set it on the ops-locked drivers whose
> offline self test tears the interface down and up:
> 
>   - fbnic (ops-locked via queue_mgmt_ops): fbnic_self_test() offline path
>     uses netif_close() / netif_open().
>   - bnxt (ops-locked via queue_mgmt_ops): bnxt_self_test() offline path
>     goes through bnxt_close_nic() / bnxt_half_open_nic() /
>     bnxt_half_close_nic() / bnxt_open_nic(), which close and reopen the
>     device.
> 
> Fixes: f994752b1127 ("net: ethtool: optionally skip rtnl_lock on IOCTL path")
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [net PATCH v2 2/6] eth: fbnic: Handle maximum standalone channels

[Simon Horman]

On Mon, Sep 14, 2026 at 02:10:04PM -0700, Alexander Duyck wrote:
> From: Björn Töpel' via fbnic <fbnic@meta.com>
> 
> Standalone channels use one NAPI vector for each Tx and Rx queue.
> fbnic's allocation path excludes FBNIC_MAX_TXQS from that layout. A
> 64-Tx/64-Rx configuration therefore records 128 vectors but allocates
> only 64, leaving NULL entries that resource setup dereferences.
> 
> Include the maximum vector count in standalone allocation.
> 
> Fixes: bc6107771bb4 ("eth: fbnic: Allocate a netdevice and napi vectors with queues")
> Signed-off-by: Björn Töpel <bjorn@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [net PATCH v2 3/6] eth: fbnic: use the Rx queue napi pointer to find the napi vector

[Simon Horman]

On Fri, Sep 18, 2026 at 08:11:04AM -0700, Alexander Duyck wrote:
> On Thu, Sep 17, 2026 at 2:12 PM <netdev-bot+sashiko@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue(s) to consider:

...

> I agree that this is a pre-existing bug, but I wouldn't fix it in this
> patch. I would probably introduce yet another patch to resolve it.
> 
> By my count we have 3 pre-existing issues that aren't directly related
> to the original problem. If possible I would prefer to address them
> separately, otherwise I can tack on 3 additional diffs to the set and
> resubmit.

Yes, I agree that is a good approach.

I think that in the age of AI-generated reviews finding related bugs it is
best to adhere to a strict scope for a patch(set) combined with follow-up
fixes. Else a patchset may drown in scope creep.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [net PATCH v2 4/6] eth: fbnic: reset num_napi when the napi vectors are freed

[Simon Horman]

On Mon, Sep 14, 2026 at 02:10:18PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> fbn->num_napi is the count of live napi vectors, each of which owns an
> IRQ.  The PM path had freed them without clearing the count.
> fbnic_pm_suspend() tears the datapath down via ndo_stop() and frees the
> IRQs, but leaves netif_running() true so resume knows to re-open.  Resume
> rebuilds the datapath in __fbnic_pm_resume() and fbnic_reset_queues() sets
> num_napi and __fbnic_open() re-allocates the vectors.
> 
> When the datapath is torn down but never rebuilt, num_napi is left
> pointing at freed vectors under 2 different scenarios:
>  - a PCIe error recovery that fails (fbnic_err_slot_reset() ->
>    __fbnic_pm_resume() returns an error -> PCI_ERS_RESULT_DISCONNECT), so
>    .resume never runs; or
>  - an __fbnic_open() that fails partway on resume and unwinds, freeing
>    the vectors after fbnic_reset_queues() has already set num_napi.
> 
> The netdev is then running with num_napi > 0 but napi[] freed, and the
> eventual remove/unbind close re-enters fbnic_down() -> fbnic_dbg_down()
> and dereferences the freed vectors:
>   BUG: kernel NULL pointer dereference, address: 0000000000000210
>   RIP: fbnic_dbg_down+0x28
> 
> Clear num_napi when the vectors are freed: in the suspend teardown (a
> good resume re-establishes it before __fbnic_open()) and on the resume
> open failure.  A redundant ndo_stop() then walks an empty napi[].  The
> normal ndo_stop() down/up cycle is untouched and keeps num_napi for the
> next ndo_open().
> 
> Fixes: bc6107771bb4 ("eth: fbnic: Allocate a netdevice and napi vectors with queues")
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [net PATCH v2 5/6] eth: fbnic: Set AW_FLUSH_MODE alongside AW_FLUSH when flushing the mailbox

[Simon Horman]

On Mon, Sep 14, 2026 at 02:10:25PM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> When tearing down the FW mailbox Rx ring, fbnic_mbx_reset_desc_ring()
> writes AW_CFG with FLUSH set and everything else, BME included, cleared.
> Clearing BME halts the device's writes to the host but leaves the staged
> requests parked in the PUL write pipeline rather than draining them, so
> on the write path FLUSH alone never terminates the outstanding requests
> and the flush the firmware waits on never completes.
> 
> Add the FLUSH_MODE definition and set both bits so the staged writes
> drain out of the pipeline on their own. BME stays cleared, so nothing
> lands on the host; it is restored later in fbnic_mbx_init_desc_ring()
> when the ring is rebuilt, once the outstanding writes are gone.
> 
> The read path is unaffected. AR_CFG has no equivalent mode bit and
> AR_FLUSH terminates the outstanding reads by itself, so it is left as
> is.
> 
> Both writes remain plain stores rather than read-modify-writes. That is
> deliberate: the matching write in fbnic_mbx_init_desc_ring() restores
> BME and the TLP attributes, and clears both flush bits as a side effect.
> 
> Fixes: 3b12f00ddd08 ("fbnic: Gate AXI read/write enabling on FW mailbox")
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [net PATCH v2 6/6] eth: fbnic: Handle FW mailbox completions flagged with an error

[Simon Horman]

On Fri, Sep 18, 2026 at 08:17:06AM -0700, Alexander Duyck wrote:
> On Thu, Sep 17, 2026 at 2:12 PM <netdev-bot+sashiko@kernel.org> wrote:

...

> The modification this way was intentional. Basically the most likely
> place for us to ever encounter an issue with  DMA is on mailbox
> initialization. Doing it this way reduces the driver init time as
> instead of waiting 10s for the mailbox polling to fail we just burn
> through the DMA failures and get to the result quickly indicating that
> the mailbox is dead.
> 
> Doing the same for the other paths adds extra complexity as they are
> interrupt driver versus the mailbox init which is synchronous. In
> addition at mailbox init we can only have one completion in flight,
> whereas for the other calls we can actually have multiple so without
> context we won't know which completion it is we need to release.

Thanks for responding to Sashiko with your reasoning.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [net PATCH v2 3/6] eth: fbnic: use the Rx queue napi pointer to find the napi vector

[Alexander Duyck]

On Fri, Sep 18, 2026 at 9:14 AM Simon Horman <horms@kernel.org> wrote:
>
> On Fri, Sep 18, 2026 at 08:11:04AM -0700, Alexander Duyck wrote:
> > On Thu, Sep 17, 2026 at 2:12 PM <netdev-bot+sashiko@kernel.org> wrote:
> > >
> > > Thank you for your contribution! Sashiko AI review found 1 potential
> > > issue(s) to consider:
>
> ...
>
> > I agree that this is a pre-existing bug, but I wouldn't fix it in this
> > patch. I would probably introduce yet another patch to resolve it.
> >
> > By my count we have 3 pre-existing issues that aren't directly related
> > to the original problem. If possible I would prefer to address them
> > separately, otherwise I can tack on 3 additional diffs to the set and
> > resubmit.
>
> Yes, I agree that is a good approach.
>
> I think that in the age of AI-generated reviews finding related bugs it is
> best to adhere to a strict scope for a patch(set) combined with follow-up
> fixes. Else a patchset may drown in scope creep.
>
> Reviewed-by: Simon Horman <horms@kernel.org>

Thanks for the review. Also it looks like there are only 2 outstanding
since Bjorn submitted a fix for the page pool issue already in
8e0b235bd918 ("eth: fbnic: Fix payload page pool error cleanup").
Guess I wasn't the only one Sashiko dinged for that.

Re: [net PATCH v2 0/6] eth: fbnic: a collection of fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 14 Sep 2026 14:09:48 -0700 you wrote:
> This series collects a handful of independent fbnic fixes for issues on
> released kernels, plus one core ethtool fix needed by the fbnic offline
> self test.
> 
> The first patch keeps rtnl_lock held on the ethtool ioctl path for the self
> test. Since the ioctl path became rtnl-optional for ops-locked drivers,
> fbnic's offline self test (which brings the interface down and up via
> netif_close()/netif_open()) runs holding only the instance lock, tripping a
> lockdep splat / ASSERT_RTNL and reconfiguring the device without the lock
> it requires. A similar issue was found with Broadcom drivers so we expanded
> the scope for v2 to just have the rtnl lock held for all selftest calls.
> 
> [...]

Here is the summary with links:
  - [net,v2,1/6] net: ethtool: keep rtnl_lock for the ioctl self test
    https://git.kernel.org/netdev/net/c/1b82958f3f03
  - [net,v2,2/6] eth: fbnic: Handle maximum standalone channels
    https://git.kernel.org/netdev/net/c/1f4c73064a50
  - [net,v2,3/6] eth: fbnic: use the Rx queue napi pointer to find the napi vector
    https://git.kernel.org/netdev/net/c/b5d9e9d4d0c1
  - [net,v2,4/6] eth: fbnic: reset num_napi when the napi vectors are freed
    https://git.kernel.org/netdev/net/c/4bcc4a92c603
  - [net,v2,5/6] eth: fbnic: Set AW_FLUSH_MODE alongside AW_FLUSH when flushing the mailbox
    https://git.kernel.org/netdev/net/c/8947f13e436a
  - [net,v2,6/6] eth: fbnic: Handle FW mailbox completions flagged with an error
    https://git.kernel.org/netdev/net/c/1b97a269a5bd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html