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

[net PATCH 0/4] 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 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 next 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.

The last 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. An opt-in flag restores rtnl_lock for
drivers that need it.

---

Alexander Duyck (4):
      fbnic: reset num_napi when the IRQ vectors are freed
      fbnic: Set AW_FLUSH_MODE alongside AW_FLUSH when flushing the mailbox
      fbnic: Handle FW mailbox completions flagged with an error
      net: ethtool: keep rtnl_lock for the ioctl self test


 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    | 35 +++++++++++++++++--
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |  1 +
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c   | 18 +++++++---
 include/linux/ethtool.h                       |  2 ++
 net/ethtool/common.h                          |  2 ++
 8 files changed, 61 insertions(+), 9 deletions(-)

--

[net PATCH 1/4] fbnic: reset num_napi when the IRQ 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 frees the IRQs and vectors without clearing the count.
It must do this to avoid leaving too many IRQs active which could overwhelm
CPU 0 on some systems and end up in the suspend failing. See b980c0634fe5
("i40e: shutdown all IRQs and disable MSI-X when suspended") for more info.

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()
overwrites 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. Two cases that can trigger this are:

 - A PCIe error recovery that fails (fbnic_err_slot_reset() ->
   __fbnic_pm_resume() returns an error -> PCI_ERS_RESULT_DISCONNECT), so
   .resume never runs
 - 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 in the suspend are torn down (a good resume
re-establishes it before __fbnic_open()) and clear it on a resume open
failure. A redundant ndo_stop() then walks an empty napi[].  The normal
ndo_stop() down/up cycle is untouched and retains 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();

Re: [net PATCH 1/4] fbnic: reset num_napi when the IRQ vectors are freed

[netdev-bot+sashiko]

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

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

- [Medium] The patch encodes "datapath torn down" as `fbn->num_napi == 0`
  while deliberately leaving `netif_running(netdev)` true…

Pre-existing issues:
- [High] __fbnic_pm_attach()
  (drivers/net/ethernet/meta/fbnic/fbnic_pci.c) drops RTNL after…

--

fbnic: reset num_napi when the IRQ vectors are freed

This clears fbn->num_napi in fbnic_pm_suspend() after the datapath has
been torn down and the IRQs are about to be freed, and also clears it in
__fbnic_pm_resume() when __fbnic_open() fails, so the count-bounded
consumers such as fbnic_dbg_down() no longer walk freed napi vectors.

> 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;
> +

[Severity: Medium]
This makes num_napi == 0 the marker for "datapath torn down" while
netif_running(netdev) is deliberately left true (ndo_stop is called
directly, so __LINK_STATE_START stays set).  Do the netdev queue
management ops cope with that combination?

fbnic_queue_mem_alloc() in fbnic_txrx.c uses netif_running() as the test
for "the napi vectors exist" and then divides by the count:

	if (!netif_running(dev))
		return fbnic_alloc_qt_page_pools(fbn, qt, idx);

	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
	nv = fbn->napi[idx % fbn->num_napi];

fbnic_queue_start() and fbnic_queue_stop() do the same modulo with no
guard at all:

	real = container_of(fbn->rx[idx], struct fbnic_q_triad, cmpl);
	nv = fbn->napi[idx % fbn->num_napi];

With num_napi == 0, is idx % fbn->num_napi a divide error oops?

These ops look reachable in that state.  fbnic_netdev_alloc() sets
netdev->queue_mgmt_ops = &fbnic_queue_mgmt_ops, so a memory provider can
be bound to an fbnic Rx queue (io_uring zcrx via netif_mp_open_rxq(), or
devmem).  On teardown of the io_uring fd or the netlink socket,
__netif_mp_close_rxq() -> netdev_rx_queue_reconfig() in
net/core/netdev_rx_queue.c calls ndo_queue_mem_alloc unconditionally and
ndo_queue_stop/ndo_queue_start under netif_running(dev), holding only the
netdev instance lock and doing no netif_device_present() check, unlike
the netdev-genl bind entry points.  real_num_rx_queues is not reset by
ndo_stop, so the index checks still pass.

The window is not short: userspace is not frozen during PCIe error
recovery, and the state persists indefinitely when __fbnic_open() fails
on resume or when fbnic_err_slot_reset() returns
PCI_ERS_RESULT_DISCONNECT so .resume and __fbnic_pm_attach() never run.

For completeness, the same sequence already oopsed before this patch,
since fbnic_free_napi_vector() sets fbn->napi[i] = NULL and
fbnic_remove_rx_ring() sets fbn->rx[q_idx] = NULL, so the pre-patch
crash was a NULL dereference rather than a divide error.  The question is
whether the torn-down state can be made unambiguous for all consumers,
for example by gating the queue management ops on
netif_device_present() or on an explicit datapath-up flag, rather than
only fixing the count-bounded consumers.

>  	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();

[Severity: High]
This isn't a bug introduced by this patch, but while the resume path is
being reworked to take rtnl_lock() plus netdev_lock() around the
netif_running() test, __fbnic_pm_attach() in the same file still does the
equivalent test with neither lock held:

	rtnl_lock();
	fbnic_reset_hw_stats(fbd);
	rtnl_unlock();
	...
	fbn = netdev_priv(netdev);

	if (netif_running(netdev))
		fbnic_up(fbn);

	netif_device_attach(netdev);

Can a close slip in between the netif_running() test and fbnic_up()?
__dev_close_many() in net/core/dev.c explicitly allows ndo_stop after a
detach:

		/*
		 *	Call the device specific close. This cannot fail.
		 *	Only if device is UP
		 *
		 *	We allow it to be called even after a DETACH hot-plug
		 *	event.
		 */
		if (ops->ndo_stop)
			ops->ndo_stop(dev);

so fbnic_stop() -> fbnic_free_napi_vectors() can NULL and kfree() every
entry of fbn->napi[] while fbn->num_napi keeps the normal queue count.
fbnic_up() -> fbnic_napi_enable() then walks that array:

	for (i = 0; i < fbn->num_napi; i++) {
		struct fbnic_napi_vector *nv = fbn->napi[i];

		napi_enable_locked(&nv->napi);

Is that a NULL dereference, or a use-after-free of the vector, when the
close won the race?

The path is fbnic_err_slot_reset() succeeding -> fbnic_err_resume() ->
__fbnic_pm_attach(), which runs from the PCI recovery context with
userspace not frozen, and that context takes only device_lock, never
rtnl or netdev->lock, so an "ip link set dev X down" on another CPU is
free to run concurrently.  Would it make sense to serialize the
netif_running() check and the dependent fbnic_up() under the same
rtnl_lock() and netdev_lock() pair now used by suspend and resume?

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

[net PATCH 2/4] 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.
On the write path that is not enough to terminate the outstanding
requests. The PUL write pipeline only forces the staged requests out
when both FLUSH and FLUSH_MODE are set; with FLUSH alone the writes keep
obeying the halt that comes from clearing BME, so nothing drains and the
flush never completes.

Add the FLUSH_MODE definition and set both bits so the flush terminates
the outstanding writes on its own.

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  |    8 +++++++-
 2 files changed, 8 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..ace6685df039 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -60,8 +60,14 @@ static void fbnic_mbx_reset_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
 	 */
 	switch (mbx_idx) {
 	case FBNIC_IPC_MBX_RX_IDX:
+		/* The write path only terminates outstanding requests when
+		 * both FLUSH and FLUSH_MODE are set. With FLUSH alone the
+		 * writes still obey the halt asserted by clearing BME, so
+		 * nothing drains and AW_FLUSH_DONE never asserts.
+		 */
 		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,

Re: [net PATCH 2/4] fbnic: Set AW_FLUSH_MODE alongside AW_FLUSH when flushing the mailbox

[netdev-bot+sashiko]

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

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

- [High] fbnic_mbx_reset_desc_ring()
  (drivers/net/ethernet/meta/fbnic/fbnic_fw.c:54-95) now arms the PUL…

--

fbnic: Set AW_FLUSH_MODE alongside AW_FLUSH when flushing the mailbox

This adds a FLUSH_MODE definition for FBNIC_PUL_OB_TLP_HDR_AW_CFG and sets
it together with FLUSH when tearing down the FW mailbox Rx ring, so the PUL
write pipeline forces the staged requests out instead of stalling behind the
halt that comes from clearing BME.  The read path is left alone.

> 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..ace6685df039 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -60,8 +60,14 @@ static void fbnic_mbx_reset_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
>  	 */
>  	switch (mbx_idx) {
>  	case FBNIC_IPC_MBX_RX_IDX:
> +		/* The write path only terminates outstanding requests when
> +		 * both FLUSH and FLUSH_MODE are set. With FLUSH alone the
> +		 * writes still obey the halt asserted by clearing BME, so
> +		 * nothing drains and AW_FLUSH_DONE never asserts.
> +		 */
>  		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;

[Severity: High]

Once the flush actually starts terminating the outstanding writes, does
anything wait for it to finish before the mailbox pages go away?

The only thing after this store in fbnic_mbx_reset_desc_ring() is wrfl(),
which is just a dummy CSR read:

drivers/net/ethernet/meta/fbnic/fbnic.h:
	static inline void fbnic_wrfl(struct fbnic_dev *fbd)
	{
		fbnic_rd32(fbd, FBNIC_MASTER_SPARE_0);
	}

That pushes the posted register write out, but it doesn't say anything about
whether the AW pipeline has drained.  The caller then goes straight to the
unmap and free:

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_mbx_clean_desc_ring() {
	fbnic_mbx_reset_desc_ring(fbd, mbx_idx);

	for (i = FBNIC_IPC_MBX_DESC_LEN; i--;)
		fbnic_mbx_unmap_and_free_msg(fbd, mbx_idx, i);
}

drivers/net/ethernet/meta/fbnic/fbnic_fw.c:fbnic_mbx_unmap_and_free_msg() {
	dma_unmap_single(fbd->dev, mbx->buf_info[desc_idx].addr,
			 PAGE_SIZE, direction);

	free_page((unsigned long)mbx->buf_info[desc_idx].msg);
}

Can a write that the now-effective flush is in the middle of forcing out
land after dma_unmap_single() and free_page()?  With an IOMMU that would be
a translation fault; without one it would be a device write into a page the
allocator has already handed to someone else.

This path is reached on ordinary teardown, via
fbnic_fw_disable_mbx() -> fbnic_mbx_flush_tx() -> fbnic_mbx_clean() ->
fbnic_mbx_clean_desc_ring(), so unbind/remove, PM suspend and PCIe error
recovery all go through it, as does the clean_mbx error label in
fbnic_mbx_poll_tx_ready().

Would a bounded poll for flush completion between the AW_CFG store and
fbnic_mbx_unmap_and_free_msg() be appropriate here?  The driver already
uses read_poll_timeout_atomic() for drain confirmation in fbnic_txrx.c, and
an fbnic_fw_present() style escape would cover surprise removal.

>  	case FBNIC_IPC_MBX_TX_IDX:
>  		wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AR_CFG,

[Severity: Low]

On the comment wording itself: AW_FLUSH_DONE doesn't appear anywhere in the
tree except in this new comment.  A grep over drivers/net/ethernet/meta/fbnic
turns up only FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH, the newly added
FBNIC_PUL_OB_TLP_HDR_AW_CFG_FLUSH_MODE and FBNIC_PUL_OB_TLP_HDR_AR_CFG_FLUSH,
and this hunk adds no status bit to the AW_CFG bit list.

Since no FLUSH_DONE bit is defined and no code reads one, could the comment
and the commit message text "nothing drains and the flush never completes"
be reworded to describe something the driver can actually observe?  As
written a reader will go looking for the completion check.

Could the commit message also say what the user-visible symptom is, given
the Fixes: tag makes this a stable candidate?

This behaviour is unchanged at the end of the series (6798d6588c7d): no
later patch adds a flush completion wait or a FLUSH_DONE definition.

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

[net PATCH 3/4] 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 capabilities request from
fbnic_mbx_poll_tx_ready() completed with FW_ERR 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 in fbnic_fw_mbx.resp_error,
which is also shown in debugfs, warn, and drop the Rx page instead of
parsing it. In fbnic_mbx_poll_tx_ready() re-issue the capabilities
request when the Tx resp_error counter advances so a FW_ERR completion
triggers a retry rather than a 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      |   27 ++++++++++++++++++++++-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h      |    1 +
 4 files changed, 33 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..4581fd8c8edf 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: %lld\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 ace6685df039..8f4a195bb8ed 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -291,6 +291,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(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++;
@@ -1672,6 +1678,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(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,6 +1753,7 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
 {
 	struct fbnic_fw_mbx *tx_mbx = &fbd->mbx[FBNIC_IPC_MBX_TX_IDX];
 	unsigned long timeout = jiffies + 10 * HZ + 1;
+	u64 resp_error;
 	int err, i;
 
 	do {
@@ -1770,6 +1784,8 @@ 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:
+	resp_error = tx_mbx->resp_error;
 	err = fbnic_fw_xmit_simple_msg(fbd, FBNIC_TLV_MSG_ID_HOST_CAP_REQ);
 	if (err)
 		goto clean_mbx;
@@ -1788,8 +1804,17 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
 		fbnic_mbx_poll(fbd);
 
 		/* 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;
+		}
+
+		/* If the FW completed our capabilities request with an error
+		 * (FW_ERR) it produced no response; the ring is not wedged, so
+		 * re-issue the request instead of timing out.
+		 */
+		if (tx_mbx->resp_error != 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 3/4] fbnic: Handle FW mailbox completions flagged with an error

[netdev-bot+sashiko]

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

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

- [High] The Rx half of the new FW_ERR handling has no consumer.
- [Medium] fbnic_mbx_process_tx_msgs() detects that the FW completed a Tx
  mailbox request with FBNIC_IPC_MBX_DESC_FW_ERR — i.e. the request was…
- [Medium] In fbnic_mbx_poll_tx_ready() the new retry test 'if
  (tx_mbx->resp_error != resp_error) goto send_cap_req;'…
- [Low] Both new FW_ERR branches emit an unratelimited dev_warn() on a
  path whose trigger bit is entirely device/firmware controlled:…
- [Low] The new debugfs output formats the unsigned counter with a signed
  conversion: struct fbnic_fw_mbx declares 'u64 resp_error'…

--

fbnic: Handle FW mailbox completions flagged with an error

This adds a FBNIC_IPC_MBX_DESC_FW_ERR definition and checks it on both
mailboxes, counting occurrences in a new fbnic_fw_mbx.resp_error field that
is also printed in debugfs.  The Rx path drops the page instead of parsing
it, and fbnic_mbx_poll_tx_ready() re-issues the capabilities request when
the Tx counter advances.

A few questions below.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c b/drivers/net/ethernet/meta/fbnic/fbnic_debugfs.c
> index 3c4563c8f403..4581fd8c8edf 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: %lld\n",
> +		   mbx->ready, mbx->head, mbx->tail, mbx->resp_error);

[Severity: Low]
Should this be %llu?  resp_error is declared as u64 in struct fbnic_fw_mbx,
and Documentation/core-api/printk-formats.rst asks for %llu for u64 and
%lld for s64.  The kernel does not build with -Wformat-signedness, so the
mismatch is not diagnosed at compile time, and values above S64_MAX would
be shown as negative here.

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index ace6685df039..8f4a195bb8ed 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -291,6 +291,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(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]
This is the only place that learns the request was not processed, so should
it also fail the completion registered for that request?

Requests sent through fbnic_mbx_map_req_w_cmpl() install a struct
fbnic_fw_completion in fbd->cmpl_data[] via fbnic_mbx_set_cmpl_slot().
Completions are only signalled from the Rx TLV parsers through
fbnic_fw_get_cmpl_by_type(), or on teardown by __fbnic_fw_evict_cmpl():

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

If the FW completes the Tx descriptor with FW_ERR, no response will ever
arrive, but the slot stays registered.  Does the waiter then sleep out its
full timeout?  fbnic_mbx_wait_for_cmpl() waits FBNIC_MBX_RX_TO_SEC * HZ for
the devlink flash/coredump paths, the ethtool module EEPROM read and the
mailbox self test, and fbnic_mac_get_sensor_asic() has its own 10 s
wait_for_completion_timeout().

There is a second effect during that window, in fbnic_mbx_set_cmpl_slot():

		else if (fbd->cmpl_data[i]->msg_type == cmpl_data->msg_type)
			return -EEXIST;

so every further request of the same type is rejected until the stale
waiter times out, and the caller reports a timeout rather than the error the
FW already told us about.  Since __fbnic_fw_evict_cmpl() already exists,
could this branch use it for the slot belonging to the failed descriptor
instead of only special-casing the polling capabilities request?

[Severity: Low]
Would dev_warn_ratelimited() or dev_warn_once() be better here and in the
new Rx branch below?  The trigger bit is firmware controlled, and
fbnic_mbx_poll() runs from fbnic_fw_msix_intr() once per firmware
interrupt, walking up to FBNIC_IPC_MBX_DESC_LEN descriptors each time, with
Rx slots recycled at the end of every iteration.  A firmware that sets
FW_ERR on every completion produces one line per descriptor per interrupt.
The new retry loop also polls every 20 ms for up to 10 s, so the benign
case can already print hundreds of identical lines per probe or resume.
The resp_error counters exported in debugfs keep the observability either
way.

> @@ -1672,6 +1678,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(fbd->dev,
> +				 "FW reported an error on an Rx mailbox message; dropping\n");
> +			goto next_page;
> +		}
> +

[Severity: High]
Does anything consume rx_mbx->resp_error?  Grepping the tree, the only
reader is the debugfs line added by this patch; fbnic_mbx_poll_tx_ready()
snapshots and compares tx_mbx->resp_error only.

So what happens when the FW flags FW_ERR on the Rx descriptor carrying the
HOST_CAP_RESP?  The message is dropped without fbnic_tlv_msg_parse(), so
fbnic_fw_parse_cap_resp() never runs and fbd->fw_cap.running.mgmt.version
stays at 1.  The polling loop then spins until:

		if (!time_is_after_jiffies(timeout)) {
			err = -ETIMEDOUT;
			continue;
		}

and falls out to 'goto clean_mbx', which disables the mailbox.  That error
propagates through __fbnic_fw_enable_mbx() and fbnic_fw_request_mbx() and
aborts probe or __fbnic_pm_resume() on an otherwise healthy ring, which is
the failure mode the commit message describes as being fixed:

    "If the capabilities request from fbnic_mbx_poll_tx_ready() completed
    with FW_ERR no response was parsed and the poll spun until it timed out
    even though the ring was healthy."

Also, before this patch that descriptor was at least parsed.  Should the
retry in fbnic_mbx_poll_tx_ready() be driven by the Rx counter as well, so
an Rx-side FW_ERR re-issues the request instead of ending in -ETIMEDOUT?

>  		dma_sync_single_for_cpu(fbd->dev, rx_mbx->buf_info[head].addr,
>  					FBNIC_RX_PAGE_SIZE, DMA_FROM_DEVICE);
>  
> @@ -1740,6 +1753,7 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
>  {
>  	struct fbnic_fw_mbx *tx_mbx = &fbd->mbx[FBNIC_IPC_MBX_TX_IDX];
>  	unsigned long timeout = jiffies + 10 * HZ + 1;
> +	u64 resp_error;
>  	int err, i;
>  
>  	do {
> @@ -1770,6 +1784,8 @@ 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:
> +	resp_error = tx_mbx->resp_error;
>  	err = fbnic_fw_xmit_simple_msg(fbd, FBNIC_TLV_MSG_ID_HOST_CAP_REQ);
>  	if (err)
>  		goto clean_mbx;
> @@ -1788,8 +1804,17 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
>  		fbnic_mbx_poll(fbd);
>  
>  		/* 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;
> +		}
> +
> +		/* If the FW completed our capabilities request with an error
> +		 * (FW_ERR) it produced no response; the ring is not wedged, so
> +		 * re-issue the request instead of timing out.
> +		 */
> +		if (tx_mbx->resp_error != resp_error)
> +			goto send_cap_req;

[Severity: Medium]
Can this goto throw away a capability response that was just parsed?

The test sits at the end of the loop body, so it runs before the for
condition re-evaluates mgmt.version, and fbnic_mbx_poll() does both halves
in one call:

fbnic_mbx_poll()
    fbnic_mbx_process_tx_msgs()   /* bumps tx_mbx->resp_error */
    fbnic_mbx_process_rx_msgs()   /* parses cap resp, sets mgmt.version */

If both happen in the same poll, the goto re-enters the for statement whose
init clause runs unconditionally:

	for (fbd->fw_cap.running.mgmt.version = 1;
	     fbd->fw_cap.running.mgmt.version < MIN_FW_VER_CODE;) {

which resets the version that was just parsed and sends a redundant
HOST_CAP_REQ.

Note tx_mbx->resp_error advances for any Tx descriptor completed with
FW_ERR, not just the capabilities request, so is the trigger too broad?
fbnic_mbx_init_desc_ring() sets tx_mbx->ready = true before this loop, so
fbnic_mbx_map_msg() no longer returns -ENODEV, and this function also runs
at runtime from __fbnic_pm_resume() while hwmon and devlink stay registered
(fbnic_pm_suspend() does not unregister hwmon).  A concurrent sysfs sensor
read can queue a TSENE request that the FW completes with FW_ERR in the
same poll that delivers the capabilities response.  Should the retry be
keyed to the specific request rather than a mailbox-wide counter?

>  	}
>  
>  	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;

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

[net PATCH 4/4] net: ethtool: keep rtnl_lock for the ioctl self test

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

fbnic's offline self test brings the interface down and back up with
netif_close() / netif_open(), both of which require rtnl_lock. 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 (fbnic is ops-locked via queue_mgmt_ops) the self test
now tears the device down without rtnl_lock.

With lockdep this reproduces deterministically on every offline self
test; 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.

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 fbnic.

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

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 12683b5d125e..97a1adbd9eae 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;
 }

Re: [net PATCH 4/4] net: ethtool: keep rtnl_lock for the ioctl self test

[netdev-bot+sashiko]

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

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

- [High] Incomplete fix: the rtnl requirement for ETHTOOL_TEST is made a
  per-driver opt-in (ETHTOOL_OP_NEEDS_RTNL_TEST) and set only on fbnic,…

--

net: ethtool: keep rtnl_lock for the ioctl self test

The ETHTOOL_TEST ioctl on an ops-locked driver runs with only the netdev
instance lock, so fbnic's offline self test tears the device down through
netif_close() without rtnl_lock. The patch adds an opt-in bit,
ETHTOOL_OP_NEEDS_RTNL_TEST, handled in ethtool_ioctl_needs_rtnl(), and
sets it in fbnic_ethtool_ops.

> 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/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;
>  }

[Severity: High]
Making the rtnl requirement for ETHTOOL_TEST a per-driver opt-in leaves the
same condition reachable on bnxt, doesn't it?

bnxt is ops-locked, since bnxt_init_one() sets dev->queue_mgmt_ops
unconditionally:

	dev->queue_mgmt_ops = &bnxt_queue_mgmt_ops_unsupp;
	if (BNXT_SUPPORTS_QUEUE_API(bp))
		dev->queue_mgmt_ops = &bnxt_queue_mgmt_ops;

But bnxt_ethtool_ops does not carry the new bit:

	.op_needs_rtnl			= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
					  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
					  ETHTOOL_OP_NEEDS_RTNL_SCOALESCE |
					  ETHTOOL_OP_NEEDS_RTNL_RSS,

so with this patch applied, __dev_ethtool() still computes need_rtnl == false
for ETHTOOL_TEST on bnxt, and bnxt_self_test() runs under
netdev_lock_ops() only.

The offline branch of bnxt_self_test() then does:

		bnxt_close_nic(bp, true, false);
		...
		rc = bnxt_half_open_nic(bp);
		...
		bnxt_half_close_nic(bp);
		rc = bnxt_open_nic(bp, true, true);

and both bnxt_half_open_nic() and bnxt_open_nic() call netif_close() on
failure:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_half_open_nic() {
	...
half_open_err:
	bnxt_free_skbs(bp);
	bnxt_free_mem(bp, true);
	netif_close(bp->dev);
	return rc;
}

IFF_UP is still set at that point, since __bnxt_close_nic() never clears it,
so netif_close() goes into netif_close_many() -> __dev_close_many() with its
ASSERT_RTNL() and the rtnl-protected rcu_dereference_protected() in
netpoll_poll_disable(), which is the same call chain quoted in the commit
message, only on bnxt.

Would it make sense to either set ETHTOOL_OP_NEEDS_RTNL_TEST on bnxt as well,
or to keep rtnl_lock unconditionally for ETHTOOL_TEST on the ioctl path?
Given self_test is an ioctl-only and rarely used command, an unconditional
case here would also cover any future ops-locked driver that forgets the
opt-in.

I checked the other ops-locked drivers for the same exposure: mlx5's
mlx5e_self_test() only runs loopback under priv->state_lock and never calls
netif_close()/netif_open(), and gve, netdevsim, netkit, dummy, bnge and
mlxsw do not implement .self_test.

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

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

[Paolo Abeni]

On 9/3/26 12:31 AM, Alexander Duyck 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 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 next 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.
> 
> The last 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. An opt-in flag restores rtnl_lock for
> drivers that need it.
I think some of the comments from sashiko (the high ones on patch 2 and
3) should be addressed in the same series.

Alex, please note that the current expectation is for the submitter to
treat sashiko comments alike to human reviewer's one, see commit
c82ff94592fb68f529afe63ca7f5ddb7dae4ba83.

/P

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

[Paolo Abeni]

On 9/3/26 12:31 AM, Alexander Duyck 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 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 next 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.
> 
> The last 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. An opt-in flag restores rtnl_lock for
> drivers that need it.
I think some of the comments from sashiko (the high ones on patch 2 and
3) should be addressed in the same series.

Alex, please note that the current expectation is for the submitter to
treat sashiko comments alike to human reviewer's one, see commit
c82ff94592fb68f529afe63ca7f5ddb7dae4ba83.

/P

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

[Alexander Duyck]

On Tue, Sep 8, 2026 at 2:41 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 9/3/26 12:31 AM, Alexander Duyck 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 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 next 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.
> >
> > The last 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. An opt-in flag restores rtnl_lock for
> > drivers that need it.
> I think some of the comments from sashiko (the high ones on patch 2 and
> 3) should be addressed in the same series.
>
> Alex, please note that the current expectation is for the submitter to
> treat sashiko comments alike to human reviewer's one, see commit
> c82ff94592fb68f529afe63ca7f5ddb7dae4ba83.

Thanks. I will go through and address the comments. I assume that in
the cases where the AI is recommending something due to lacking
context I just need to add a comment or update the patch description
to provide the needed context?

Case in point being the AW_FLUSH_MODE and it asking about how we are
verifying the flush has occurred since it doesn't seem to grok that in
the same write that set it we cleared the BME so the device itself
cannot issue any more writes and the read completion essentially
clears the PCIe pipe.

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

[Paolo Abeni]

On 9/8/26 7:01 PM, Alexander Duyck wrote:
> On Tue, Sep 8, 2026 at 2:41 AM Paolo Abeni <pabeni@redhat.com> wrote:
>> I think some of the comments from sashiko (the high ones on patch 2 and
>> 3) should be addressed in the same series.
>>
>> Alex, please note that the current expectation is for the submitter to
>> treat sashiko comments alike to human reviewer's one, see commit
>> c82ff94592fb68f529afe63ca7f5ddb7dae4ba83.
> 
> Thanks. I will go through and address the comments. I assume that in
> the cases where the AI is recommending something due to lacking
> context I just need to add a comment or update the patch description
> to provide the needed context?
> 
> Case in point being the AW_FLUSH_MODE and it asking about how we are
> verifying the flush has occurred since it doesn't seem to grok that in
> the same write that set it we cleared the BME so the device itself
> cannot issue any more writes and the read completion essentially
> clears the PCIe pipe.

I think in that case an additional code comment would be helpful. In
general an AI comment could be addressed with just a reply explaining
why the comment is off or is better addressed separately (quite alike
good old reviews :)

/P