[net PATCH 0/5] fbnic: Fix FW crash restore, stop path and mailbox teardown bugs

[net PATCH 0/5] fbnic: Fix FW crash restore, stop path and mailbox teardown bugs

[Alexander Duyck]

Fixes for races and ordering problems in the paths that take the datapath
down or rebuild it: the FW crash restore, ndo_stop and PM suspend/resume,
the Tx force-flush, and the DMA flush on mailbox teardown.

Patches 1-3 were found by code review or inspection; patch 3 has not been
seen to trigger. Patches 4 and 5 together appear to resolve a hang with a
PTP Tx timestamp left in flight. The series was tested on fbnic hardware
with firmware 26.09.14-017_h7ab84d2ef68f, cycling the interface through
DPC/AER recoveries for a full day, and under QEMU with the fbnic device
model and live firmware.

These patches were previously posted as part of a larger set:
https://lore.kernel.org/netdev/179322311834.40667.5383039334126572422.stgit@ahduyck-xeon-server.home.arpa/

---

Alexander Duyck (5):
      fbnic: Take the instance lock to restore filters after a FW crash
      fbnic: Fix service task races with ndo_stop and PM suspend
      fbnic: Disable PTP completions during the Tx force-flush
      fbnic: Suspend phylink after taking the datapath down in ndo_stop
      fbnic: Wait for the outbound flush to complete on mailbox teardown


 drivers/net/ethernet/meta/fbnic/fbnic_csr.h   |  5 +++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    | 16 +++++++
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    |  4 +-
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c   | 43 +++++++++++++++----
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  | 10 +++--
 5 files changed, 64 insertions(+), 14 deletions(-)

--

[net PATCH 1/5] fbnic: Take the instance lock to restore filters after a FW crash

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_fw_config_after_crash function resets the MACDA and action
TCAM shadows and rewrites them through the __fbnic_set_rx_mode function
from the service task, holding RTNL only. fbnic is ops-locked, so since
the netdev workqueue moved from RTNL to the ops-compat lock the core
calls the fbnic_set_rx_mode function from it holding only the instance
lock. The two do not exclude each other.

Both sides can then allocate MACDA entries at the same time, one can
zero an entry the other is still filling in, and one can clear hardware
entries the other has just written. The crash path also syncs the live
address lists while the netif_rx_mode_run function works on a snapshot
of them, so the reconcile that follows can count the same delta twice
and leave an address that is never unsynced.

Hold the instance lock across the restore, as the fbnic_bmc_rpc_check
function does. The netif_rx_mode_run function holds it from snapshot
through reconcile, so the crash path now syncs either before the
snapshot or after the reconcile.

Fixes: 5a81c72ef9ad ("net: run netdev work under the ops-compat lock")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index ad234bb0f75d..09eeb4b4ded8 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -181,10 +181,15 @@ static int fbnic_fw_config_after_crash(struct fbnic_dev *fbd)
 		return -1;
 	}
 
+	/* The rx mode work rewrites the same shadow under the instance lock
+	 * alone, and this runs from the service task under RTNL only.
+	 */
+	netdev_lock(fbd->netdev);
 	fbnic_rpc_reset_valid_entries(fbd);
 	netif_addr_lock_bh(fbd->netdev);
 	__fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
 	netif_addr_unlock_bh(fbd->netdev);
+	netdev_unlock(fbd->netdev);
 
 	return 0;
 }

[net PATCH 2/5] fbnic: Fix service task races with ndo_stop and PM suspend

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The service task can run after the datapath has been torn down
underneath it. ndo_stop stops the task with the cancel_delayed_work
function, which does not wait for a pass already executing. It cannot
use the cancel_delayed_work_sync function, as ndo_stop holds RTNL and
the task takes RTNL itself, so a pass blocked on RTNL while ndo_stop
runs carries on once ndo_stop returns.

By then the fbnic_down function has demoted the host MACDA and action
entries to UPDATE and disabled the host queues, and ndo_stop has handed
ownership back to firmware, which clears fw_heartbeat_enabled. The late
pass's health check reports a firmware crash that did not happen, and
the fbnic_fw_config_after_crash function reclaims ownership and writes
the host rules back to hardware. If the BMC raised
FBNIC_FW_CAP_F_BMC_TCAM_REINIT in that window, the fbnic_bmc_rpc_check
function does the same through the __fbnic_set_rx_mode function. Either
way host traffic is steered at queues that are no longer running until
the next open and close.

The fbnic_pm_suspend function, which also serves shutdown and PCIe
error recovery, has the same problem and a worse one. It calls ndo_stop
directly with the device detached but __LINK_STATE_START still set, so
a late pass there sees the interface running, restores the host state,
and keeps re-arming itself every second after the mailbox, IRQs and
MMIO are gone.

Only run the health and BMC checks, and only re-arm, while the
interface is running and the device is present. The __dev_close_many
function clears __LINK_STATE_START under RTNL before it calls ndo_stop,
and the suspend path detaches the device under RTNL, so a late pass now
leaves the host state alone and stops. The BMC flags stay raised, and
the __fbnic_open function reclaims ownership, restarts the heartbeat
and rebuilds the BMC rules on the next open. The fbnic_pm_suspend
function also cancels the task synchronously once it has dropped RTNL,
so a pass already running cannot overlap the mailbox and IRQ teardown.

On resume the __fbnic_pm_attach function called the fbnic_up function,
which schedules the next pass, before attaching the device, all without
RTNL. A pass that ran in between would now see the device detached and
stop for good. Check for an init failure first, then hold RTNL and the
instance lock across the stats reset, the fbnic_up call and the attach,
as the suspend path does, so the pass blocks until the device is
present. This also gives the fbnic_up function the instance lock it
expects for the napi_enable_locked calls and the rx mode rewrite.

A device that detaches itself on a failed register read likewise stops
re-arming the task until recovery attaches it.

The issue was found by code review. The change was tested on fbnic
hardware with firmware 26.09.14-017_h7ab84d2ef68f by cycling the
interface through DPC/AER recoveries for a day.

Fixes: 04a230b27d8f ("fbnic: Add logic to repopulate RPC TCAM if BMC enables channel")
Fixes: 6ae7da8e9e06 ("eth: fbnic: reprogram TCAMs after FW crash")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c |   38 +++++++++++++++++++++------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index 09eeb4b4ded8..09ae797b1f0d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -234,18 +234,25 @@ static void fbnic_service_task(struct work_struct *work)
 
 	fbnic_fw_check_heartbeat(fbd);
 
-	fbnic_health_check(fbd);
-
-	fbnic_bmc_rpc_check(fbd);
-
 	if (netif_carrier_ok(fbd->netdev)) {
 		netdev_lock(fbd->netdev);
 		fbnic_napi_depletion_check(fbd->netdev);
 		netdev_unlock(fbd->netdev);
 	}
 
-	if (netif_running(netdev))
+	/* fbnic_service_task_stop() does not wait for a pass that is already
+	 * blocked on RTNL, so one can run after ndo_stop. Leave host state
+	 * alone in that case, and do not re-arm; __fbnic_open() reclaims and
+	 * rebuilds it. The PM path calls ndo_stop with the device detached but
+	 * still running; fbnic_up() starts the task again on resume.
+	 */
+	if (netif_running(netdev) && netif_device_present(netdev)) {
+		fbnic_health_check(fbd);
+
+		fbnic_bmc_rpc_check(fbd);
+
 		schedule_delayed_work(&fbd->service_task, HZ);
+	}
 
 	rtnl_unlock();
 }
@@ -458,6 +465,12 @@ static int fbnic_pm_suspend(struct device *dev)
 	netdev_unlock(netdev);
 	rtnl_unlock();
 
+	/* ndo_stop could not wait for a pass already blocked on RTNL. That
+	 * pass no longer re-arms, but wait for it to finish before the
+	 * mailbox, IRQs and MMIO go away.
+	 */
+	cancel_delayed_work_sync(&fbd->service_task);
+
 null_uc_addr:
 	fbnic_fw_log_disable(fbd);
 
@@ -558,19 +571,26 @@ static void __fbnic_pm_attach(struct device *dev)
 	struct net_device *netdev = fbd->netdev;
 	struct fbnic_net *fbn;
 
-	rtnl_lock();
-	fbnic_reset_hw_stats(fbd);
-	rtnl_unlock();
-
 	if (fbnic_init_failure(fbd))
 		return;
 
 	fbn = netdev_priv(netdev);
 
+	/* Hold RTNL until the device is attached, so the service task pass
+	 * fbnic_up() schedules cannot run until it will see it present.
+	 */
+	rtnl_lock();
+	netdev_lock(netdev);
+
+	fbnic_reset_hw_stats(fbd);
+
 	if (netif_running(netdev))
 		fbnic_up(fbn);
 
 	netif_device_attach(netdev);
+
+	netdev_unlock(netdev);
+	rtnl_unlock();
 }
 
 static int __maybe_unused fbnic_pm_resume(struct device *dev)

[net PATCH 3/5] fbnic: Disable PTP completions during the Tx force-flush

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

When the Tx queues fail to go idle, fbnic_wait_all_queues_idle() falls
back to fbnic_tx_flush(), which sets FBNIC_TMI_DROP_CTRL_EN. That bit
only drops packets at the Tx MAC Interface start of packet protection FIFO.

The PTP completion path is governed by a separate bit in the same
register. With only the drop enable set, a timestamped frame has its
packet discarded while the matching completion is still forwarded to
the host. fbnic_clean_tsq() then walks the ring with no packet left to
match it against and reports "Tx timestamp without matching packet".
The drop does not retire the completion either, as the completion FIFO
is only drained when the completion disable is set.

Set FBNIC_TMI_DROP_CTRL_CMPL_DIS together with the drop enable so the
completions are drained rather than leaked to the host, and clear both
bits again when the flush is turned off.

Fixes: ad3d9f8bc66c ("eth: fbnic: add TX packets timestamping support")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_csr.h  |    1 +
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c |   10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
index cbb25d11638c..414871a94104 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
@@ -441,6 +441,7 @@ enum {
 #define FBNIC_TMI_SOP_PROT_CTRL		0x04400		/* 0x11000 */
 #define FBNIC_TMI_DROP_CTRL		0x04401		/* 0x11004 */
 #define FBNIC_TMI_DROP_CTRL_EN			CSR_BIT(0)
+#define FBNIC_TMI_DROP_CTRL_CMPL_DIS		CSR_BIT(1)
 #define FBNIC_TMI_DROP_PKTS		0x04402		/* 0x11008 */
 #define FBNIC_TMI_DROP_BYTE_L		0x04403		/* 0x1100c */
 #define FBNIC_TMI_ILLEGAL_PTP_REQS	0x04409		/* 0x11024 */
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 0bcee152d47c..07d4b2e92362 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -2349,15 +2349,19 @@ void fbnic_disable(struct fbnic_net *fbn)
 
 static void fbnic_tx_flush(struct fbnic_dev *fbd)
 {
+	u32 drop_ctrl = FBNIC_TMI_DROP_CTRL_EN | FBNIC_TMI_DROP_CTRL_CMPL_DIS;
+
 	netdev_warn(fbd->netdev, "triggering Tx flush\n");
 
-	fbnic_rmw32(fbd, FBNIC_TMI_DROP_CTRL, FBNIC_TMI_DROP_CTRL_EN,
-		    FBNIC_TMI_DROP_CTRL_EN);
+	/* Drop packets and retire their PTP completions */
+	fbnic_rmw32(fbd, FBNIC_TMI_DROP_CTRL, drop_ctrl, drop_ctrl);
 }
 
 static void fbnic_tx_flush_off(struct fbnic_dev *fbd)
 {
-	fbnic_rmw32(fbd, FBNIC_TMI_DROP_CTRL, FBNIC_TMI_DROP_CTRL_EN, 0);
+	u32 drop_ctrl = FBNIC_TMI_DROP_CTRL_EN | FBNIC_TMI_DROP_CTRL_CMPL_DIS;
+
+	fbnic_rmw32(fbd, FBNIC_TMI_DROP_CTRL, drop_ctrl, 0);
 }
 
 struct fbnic_idle_regs {

[net PATCH 4/5] fbnic: Suspend phylink after taking the datapath down in ndo_stop

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_stop function calls the phylink_suspend function before the
fbnic_down function. With no BMC present that stops phylink, and the
fbnic_phylink_mac_link_down function disables the MAC and holds its Tx
and Rx clocks and FIFOs in reset. Only then does the fbnic_down function
stop the queues and wait for the Tx path to go idle, so anything still in
flight has to drain into a MAC that is no longer passing traffic, and the
fbnic_wait_all_queues_idle function can be left to time out and force a
flush. With a BMC present phylink leaves the link up but turns the carrier
off, which the phylink_suspend function notes assumes all packets have
already been sent.

It is also the reverse of how the interface comes up: the __fbnic_open
function resumes phylink last, and the fbnic_up function then enables the
queues and installs the host Rx filters.

Call the fbnic_down function first. The Tx path then drains with the link
still up, and Rx is still stopped first, as the fbnic_clear_rx_mode and
fbnic_clear_rules functions remove the host as a destination for its MAC
addresses in the RPC before the queues are disabled. The MAC IRQ is still
freed before phylink is suspended, so no link interrupt runs while the
link is taken down. The PM suspend path goes through ndo_stop and picks
up the same order.

Fixes: 6ede14a2c636 ("fbnic: Move phylink resume out of service_task and into open/close")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_netdev.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index 8a6703afca38..52244219d166 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -86,11 +86,11 @@ static int fbnic_stop(struct net_device *netdev)
 {
 	struct fbnic_net *fbn = netdev_priv(netdev);
 
+	fbnic_down(fbn);
+
 	fbnic_mac_free_irq(fbn->fbd);
 	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
 
-	fbnic_down(fbn);
-
 	fbnic_time_stop(fbn);
 	fbnic_fw_xmit_ownership_msg(fbn->fbd, false);
 

[net PATCH 5/5] fbnic: Wait for the outbound flush to complete on mailbox teardown

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_mbx_reset_desc_ring function drops bus mastering and arms the
outbound flush for its ring: the write flush for the Rx ring and the
read flush for the Tx ring. Nothing waits for that flush to finish.
The fbnic_mbx_clean_desc_ring function goes straight on to unmap and
free the ring's buffers, so a device read of a Tx buffer, or a write to
an Rx buffer, can still be outstanding after the buffer is gone. With
an IOMMU it can also target an IOVA that has been unmapped or reused.

The PUL link reset status register reports when each direction's flush
has drained. Poll the done bit for the ring's direction after arming
the flush and before freeing its buffers. The flush only has to retire
what the outbound path already holds; the 1ms cap keeps a wedged device
from spinning.

Every caller of the fbnic_mbx_clean function runs with the datapath
already down, which matters because the read flush covers all
outstanding reads, not just the mailbox ones. A device that has lost
its link reads back as all ones, so the done bit appears set and the
poll ends on the first pass.

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 |    4 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c  |   16 ++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
index 414871a94104..c371ac9f78c1 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
@@ -956,6 +956,10 @@ enum {
 
 /* PUL User Registers */
 #define FBNIC_CSR_START_PUL_USER	0x31000	/* CSR section delimiter */
+#define FBNIC_PUL_USER_IP_LINK_RST_STATUS \
+					0x31030		/* 0xc40c0 */
+#define FBNIC_PUL_USER_IP_LINK_RST_STATUS_AW_FLUSH_DONE	CSR_BIT(7)
+#define FBNIC_PUL_USER_IP_LINK_RST_STATUS_AR_FLUSH_DONE	CSR_BIT(8)
 #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)
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 49b8813a3564..e00d626f6438 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -7,6 +7,7 @@
 #include <linux/dev_printk.h>
 #include <linux/dma-mapping.h>
 #include <linux/gfp.h>
+#include <linux/iopoll.h>
 #include <linux/types.h>
 
 #include "fbnic.h"
@@ -176,10 +177,25 @@ static void fbnic_mbx_unmap_and_free_msg(struct fbnic_dev *fbd, int mbx_idx,
 
 static void fbnic_mbx_clean_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
 {
+	u32 flush_done, val;
 	int i;
 
 	fbnic_mbx_reset_desc_ring(fbd, mbx_idx);
 
+	/* Arming the flush only starts it. Wait for the outbound path to
+	 * retire the requests it still holds before the buffers they may
+	 * target are unmapped and freed. A lost link reads back as all ones,
+	 * which ends the poll on the first pass.
+	 */
+	flush_done = mbx_idx == FBNIC_IPC_MBX_RX_IDX ?
+		     FBNIC_PUL_USER_IP_LINK_RST_STATUS_AW_FLUSH_DONE :
+		     FBNIC_PUL_USER_IP_LINK_RST_STATUS_AR_FLUSH_DONE;
+	if (read_poll_timeout_atomic(fbnic_rd32, val, val & flush_done, 2,
+				     1000, false, fbd,
+				     FBNIC_PUL_USER_IP_LINK_RST_STATUS))
+		dev_warn(fbd->dev, "Outbound flush did not complete: %08x\n",
+			 val);
+
 	for (i = FBNIC_IPC_MBX_DESC_LEN; i--;)
 		fbnic_mbx_unmap_and_free_msg(fbd, mbx_idx, i);
 }

Re: [net PATCH 2/5] fbnic: Fix service task races with ndo_stop and PM suspend

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Regression in __fbnic_pm_attach()
  (drivers/net/ethernet/meta/fbnic/fbnic_pci.c): the patch moves…
- [Medium] The Fixes tags are incomplete.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is an
  unbounded hang across the RTNL drop between the two PCI error-recovery…

--

fbnic: Fix service task races with ndo_stop and PM suspend

This commit runs the service task's health check, BMC check and re-arm
only while the interface is running and the device is present. It also
makes fbnic_pm_suspend() cancel the service task synchronously after
dropping RTNL. On resume, __fbnic_pm_attach() now holds RTNL and the
instance lock across the stats reset, fbnic_up() and the device attach.

> Fixes: 04a230b27d8f ("fbnic: Add logic to repopulate RPC TCAM if BMC enables channel")
> Fixes: 6ae7da8e9e06 ("eth: fbnic: reprogram TCAMs after FW crash")

[Severity: Medium]
Are these Fixes tags complete? The commit message calls the suspend
problem "a worse one":

> It calls ndo_stop
> directly with the device detached but __LINK_STATE_START still set, so
> a late pass there sees the interface running, restores the host state,
> and keeps re-arming itself every second after the mailbox, IRQs and
> MMIO are gone.

This behaviour seems older than both cited commits. In bc6107771bb4
("eth: fbnic: Allocate a netdevice and napi vectors with queues"):

- fbnic_service_task() already re-armed on netif_running() alone.
- fbnic_service_task_stop() already used the non-sync
  cancel_delayed_work().
- fbnic_pm_suspend() already called netif_device_detach() and then
  ndo_stop(), with no cancel_delayed_work_sync().

The instance lock added around fbnic_up() in __fbnic_pm_attach() also
fixes an older problem. fbnic_up()->fbnic_napi_enable()->
napi_enable_locked() expects the instance lock. That requirement came in
with 4ddb17c1a2c2 ("eth: fbnic: request ops lock").

Should bc6107771bb4 and 4ddb17c1a2c2 also get Fixes tags? Without them,
stable trees that have the fbnic PM and service task code but not the two
TCAM commits won't pick up the suspend fix.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index 09eeb4b4ded8f..09ae797b1f0dc 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c

[ ... ]

> @@ -558,19 +571,26 @@ static void __fbnic_pm_attach(struct device *dev)
>  	struct net_device *netdev = fbd->netdev;
>  	struct fbnic_net *fbn;
>  
> -	rtnl_lock();
> -	fbnic_reset_hw_stats(fbd);
> -	rtnl_unlock();
> -
>  	if (fbnic_init_failure(fbd))
>  		return;

[Severity: Medium]
fbnic_reset_hw_stats() now sits below the init failure check, so it no
longer runs when fbd->netdev is NULL. Was that intended?

Commit b1161b1863c5 ("eth: fbnic: Reset hw stats upon PCI error") put the
reset ahead of this check on purpose. fbnic_reset_hw_stats() also handles
a NULL netdev:

	if (fbd->netdev)
		ASSERT_RTNL();

A device in init failure mode stays bound so devlink still works. It also
keeps the debugfs pcie_stats file created by fbnic_dbg_fbd_init(), and
reading that file calls fbnic_get_hw_stats().

Resume or AER/DPC recovery (fbnic_err_resume()->__fbnic_pm_attach()) can
reset the hardware counters. The next read then goes through
fbnic_hw_stat_rd64():

	new_reg_value = fbnic_stat_rd64(fbd, reg, offset);
	stat->value += new_reg_value - stat->u.old_reg_value_64;

Won't this subtraction against the stale old_reg_value_64 wrap? That would
permanently add a huge value to ob_rd_tlp, ob_wr_tlp, ob_cpl_tlp and the
other PCIe counters. The 32-bit counters would get wrong deltas the same
way.

The new RTNL and attach ordering doesn't seem to need the reset skipped
here, and the commit message doesn't mention this change.

> +	/* Hold RTNL until the device is attached, so the service task pass
> +	 * fbnic_up() schedules cannot run until it will see it present.
> +	 */
> +	rtnl_lock();
> +	netdev_lock(netdev);
> +
> +	fbnic_reset_hw_stats(fbd);
> +
>  	if (netif_running(netdev))
>  		fbnic_up(fbn);
>  
>  	netif_device_attach(netdev);
> +
> +	netdev_unlock(netdev);
> +	rtnl_unlock();
>  }

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Error
recovery adds the NAPI vectors under one RTNL hold and enables them under
a later one. Can a close between the two hang?

fbnic_err_slot_reset()->__fbnic_pm_resume() calls __fbnic_open() under
rtnl_lock() and netdev_lock(). That reaches netif_napi_add_config_locked(),
which leaves NAPI_STATE_SCHED and NAPI_STATE_NPSVC set. Then both locks
are dropped:

	if (netif_running(netdev)) {
		err = __fbnic_open(fbn);
	...
	netdev_unlock(netdev);
	rtnl_unlock();

The NAPIs are only enabled later, in
fbnic_err_resume()->__fbnic_pm_attach()->fbnic_up()->fbnic_napi_enable().

In between, __LINK_STATE_START is still set, and __dev_close_many() calls
ndo_stop even on a detached device. So "ip link set dev X down" in that
window would run:

fbnic_stop()
  fbnic_down()
    fbnic_down_noidle()
      fbnic_napi_disable()
        napi_disable_locked()

napi_disable_locked() then waits for bits that only napi_enable_locked()
clears:

	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}

Wouldn't that loop spin forever while holding RTNL and the instance lock?
fbnic_err_resume()->__fbnic_pm_attach() would then block on rtnl_lock().

This patch now holds RTNL across fbnic_up() and the attach. The gap
between __fbnic_open() and fbnic_up() is still open, though.

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

Re: [net PATCH 3/5] fbnic: Disable PTP completions during the Tx force-flush

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message understates what a leaked PTP completion can
  do.

--

fbnic: Disable PTP completions during the Tx force-flush

When the Tx queues fail to go idle, fbnic_tx_flush() sets only
FBNIC_TMI_DROP_CTRL_EN. That drops the packets but still forwards their
PTP completions to the host. This patch also sets
FBNIC_TMI_DROP_CTRL_CMPL_DIS during the flush, so those completions are
drained, and clears both bits in fbnic_tx_flush_off().

> The PTP completion path is governed by a separate bit in the same
> register. With only the drop enable set, a timestamped frame has its
> packet discarded while the matching completion is still forwarded to
> the host. fbnic_clean_tsq() then walks the ring with no packet left to
> match it against and reports "Tx timestamp without matching packet".

[Severity: Low]
The code change looks right. Is the impact described here complete,
though?

fbnic_clean_tsq() never checks which packet a type 1 completion belongs
to. It starts at ring->head (or *ts_head) and gives the timestamp to the
first skb that has FBNIC_XMIT_CB_TS set:

drivers/net/ethernet/meta/fbnic/fbnic_txrx.c:fbnic_clean_tsq() {
    ...
	do {
		...
		if (head == ring->tail) {
			if (unlikely(net_ratelimit()))
				netdev_err(nv->napi.dev,
					   "Tx timestamp without matching packet\n");
			return;
		}
		...
	} while (!(FBNIC_XMIT_CB(skb)->flags & FBNIC_XMIT_CB_TS));
    ...
	skb_tstamp_tx(skb, &hwtstamp);
    ...
}

The head == ring->tail check is the only safeguard, and it only covers
an empty ring.

Before this patch, a completion left over from the flush could be
handled while a new timestamped skb was already pending. One possible
path:

fbnic_down()
  fbnic_wait_all_queues_idle() times out
    fbnic_tx_flush()   /* only DROP_CTRL_EN set */
  fbnic_flush()        /* Tx skbs freed */
fbnic_up()
  fbnic_enable()       /* TCQ enabled */
  fbnic_napi_enable()
  netif_tx_wake_all_queues()
    new TS skb posted
fbnic_clean_tcq()
  fbnic_clean_tsq()    /* stale completion */

In that case the new skb would get the stale hardware timestamp with no
warning, and ts_packets would still be incremented. Its real completion
would then take the empty-ring path and print the error.

Whether this happens depends on when the TMI completion FIFO releases
the stale entry, and the driver code doesn't show that.

Could the commit message also mention that SO_TIMESTAMPING users might
get a wrong Tx timestamp, not just a ratelimited error message? That
would help anyone judging this fix for stable backports.

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

Re: [net PATCH 4/5] fbnic: Suspend phylink after taking the datapath down in ndo_stop

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The Fixes tag points at the wrong commit.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the kernel-doc
  for fbnic_mac_free_irq()…

--

fbnic: Suspend phylink after taking the datapath down in ndo_stop

This commit reorders fbnic_stop() so that fbnic_down() runs before
fbnic_mac_free_irq() and phylink_suspend(). Tx can then drain while the
link is still up, instead of draining into a MAC that is disabled and held
in reset. The PM suspend path goes through ndo_stop, so it gets the same
ordering.

> Fixes: 6ede14a2c636 ("fbnic: Move phylink resume out of service_task and into open/close")

[Severity: Low]
Should this tag point at an earlier commit?

Before 6ede14a2c636, phylink_suspend() was already the first thing that
ran on the down path. fbnic_down_noidle() first called
fbnic_service_task_stop(), and that function started with:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_service_task_stop() {
	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbd));
	cancel_delayed_work(&fbd->service_task);
    ...
}

fbnic_napi_disable(), netif_tx_disable(), the RPC/rx-mode clearing,
fbnic_disable() and the fbnic_wait_all_queues_idle() call in fbnic_down()
all came after it.

At 69684376eed5, fbnic_mac_link_down_asic() already disabled the MAC and
held it in reset:

	mac_ctrl |= FBNIC_SIG_MAC_IN0_RESET_FF_TX_CLK |
		    FBNIC_SIG_MAC_IN0_RESET_TX_CLK |
		    FBNIC_SIG_MAC_IN0_RESET_FF_RX_CLK |
		    FBNIC_SIG_MAC_IN0_RESET_RX_CLK;

6ede14a2c636 moved phylink_suspend() from the top of fbnic_down() to just
before fbnic_down() in fbnic_stop(). It was still ahead of the queue
teardown in both places.

It looks like the ordering problem described here dates from
69684376eed5 ("eth: fbnic: Add link detection"). Would this be more
accurate?

Fixes: 69684376eed5 ("eth: fbnic: Add link detection")

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> index 8a6703afca384..52244219d1668 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> @@ -86,11 +86,11 @@ static int fbnic_stop(struct net_device *netdev)
>  {
>  	struct fbnic_net *fbn = netdev_priv(netdev);
>  
> +	fbnic_down(fbn);
> +
>  	fbnic_mac_free_irq(fbn->fbd);

[Severity: Low]
This isn't a bug introduced by this patch, but the kernel-doc for
fbnic_mac_free_irq() in fbnic_irq.c says:

 * This function undoes the work done in fbnic_mac_request_irq and prepares
 * the device to no longer receive traffic on the host interface.

The function only disables the PCS MSI-X control entry, calls
synchronize_irq(), masks the vector, frees the IRQ and clears
fbd->mac_msix_vector.

Host Rx is actually stopped in fbnic_down_noidle(), through
fbnic_clear_rx_mode(), fbnic_clear_rules(), fbnic_rss_disable_hw() and
fbnic_disable(). With this reordering, fbnic_down() has already stopped
host traffic before fbnic_mac_free_irq() is called.

Could the comment be updated to say that this only tears down the MAC/PCS
link interrupt?

>  	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
>  
> -	fbnic_down(fbn);
> -
>  	fbnic_time_stop(fbn);
>  	fbnic_fw_xmit_ownership_msg(fbn->fbd, false);

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

Re: [net PATCH 5/5] fbnic: Wait for the outbound flush to complete on mailbox teardown

[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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] If the new poll times out, fbnic_mbx_clean_desc_ring()
  (drivers/net/ethernet/meta/fbnic/fbnic_fw.c) still frees the mailbox…

--

fbnic: Wait for the outbound flush to complete on mailbox teardown

After fbnic_mbx_reset_desc_ring() arms the flush, this patch polls the AW
or AR flush-done bit in FBNIC_PUL_USER_IP_LINK_RST_STATUS. The poll runs
before the mailbox ring buffers are unmapped and freed, and it gives up
after 1ms and warns.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 49b8813a35643..e00d626f64384 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -176,10 +177,25 @@ static void fbnic_mbx_unmap_and_free_msg(struct fbnic_dev *fbd, int mbx_idx,
>  
>  static void fbnic_mbx_clean_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
>  {
> +	u32 flush_done, val;
>  	int i;
>  
>  	fbnic_mbx_reset_desc_ring(fbd, mbx_idx);
>  
> +	/* Arming the flush only starts it. Wait for the outbound path to
> +	 * retire the requests it still holds before the buffers they may
> +	 * target are unmapped and freed. A lost link reads back as all ones,
> +	 * which ends the poll on the first pass.
> +	 */
> +	flush_done = mbx_idx == FBNIC_IPC_MBX_RX_IDX ?
> +		     FBNIC_PUL_USER_IP_LINK_RST_STATUS_AW_FLUSH_DONE :
> +		     FBNIC_PUL_USER_IP_LINK_RST_STATUS_AR_FLUSH_DONE;
> +	if (read_poll_timeout_atomic(fbnic_rd32, val, val & flush_done, 2,
> +				     1000, false, fbd,
> +				     FBNIC_PUL_USER_IP_LINK_RST_STATUS))
> +		dev_warn(fbd->dev, "Outbound flush did not complete: %08x\n",
> +			 val);
> +
>  	for (i = FBNIC_IPC_MBX_DESC_LEN; i--;)
>  		fbnic_mbx_unmap_and_free_msg(fbd, mbx_idx, i);
>  }

[Severity: Medium]
If read_poll_timeout_atomic() returns -ETIMEDOUT, does the original
DMA-after-free problem still happen?

On timeout the code only calls dev_warn(). It then falls through to the
same loop as before, and fbnic_mbx_unmap_and_free_msg() does this for
every descriptor:

    dma_unmap_single(fbd->dev, mbx->buf_info[desc_idx].addr,
                     PAGE_SIZE, direction);

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

As the comment says, a lost link reads back as all ones, so that case
exits on the first read. That leaves a timeout to mean a live device whose
outbound path has not drained, which is the wedged-device case named in
the commit message.

In that case the page can still be freed and reused, or its IOVA unmapped,
while a Tx read or an Rx write is still pending against it. Both paths
reach this branch:

  fbnic_fw_disable_mbx()->fbnic_mbx_clean()->fbnic_mbx_clean_desc_ring()

  fbnic_mbx_poll_tx_ready() clean_mbx error path->fbnic_mbx_clean()

The Rx ring has one more case. fbnic_mbx_init_desc_ring() later sets BME
and clears FLUSH and FLUSH_MODE, and nothing checks that the earlier AW
flush finished:

    wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AW_CFG,
         FBNIC_PUL_OB_TLP_HDR_AW_CFG_BME |
         ...

Could writes still parked in the pipeline then land on freed or reused DMA
addresses?

On timeout, would it be safer to skip the unmap and free and leak the
mailbox pages, instead of giving them back to the page allocator?

Also, the 1ms limit doesn't seem to be based on the PCIe completion
timeout, and that timeout is allowed to be longer. Is 1ms enough for
outstanding reads to complete?

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

Re: [net PATCH 2/5] fbnic: Fix service task races with ndo_stop and PM suspend

[Alexander Duyck]

On Sun, Nov 8, 2026 at 10:32 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 (1) · Medium: 2 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] Regression in __fbnic_pm_attach()
>   (drivers/net/ethernet/meta/fbnic/fbnic_pci.c): the patch moves…
> - [Medium] The Fixes tags are incomplete.
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but there is an
>   unbounded hang across the RTNL drop between the two PCI error-recovery…
>
> --
>
> fbnic: Fix service task races with ndo_stop and PM suspend
>
> This commit runs the service task's health check, BMC check and re-arm
> only while the interface is running and the device is present. It also
> makes fbnic_pm_suspend() cancel the service task synchronously after
> dropping RTNL. On resume, __fbnic_pm_attach() now holds RTNL and the
> instance lock across the stats reset, fbnic_up() and the device attach.
>
> > Fixes: 04a230b27d8f ("fbnic: Add logic to repopulate RPC TCAM if BMC enables channel")
> > Fixes: 6ae7da8e9e06 ("eth: fbnic: reprogram TCAMs after FW crash")
>
> [Severity: Medium]
> Are these Fixes tags complete? The commit message calls the suspend
> problem "a worse one":

It may have been a fix that was picked up along the way. The general
"Fixes" tags are correct. Having the service task itself run isn't an
issue until those two bits of logic were added that introduced the
races.

> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> > index 09eeb4b4ded8f..09ae797b1f0dc 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
>
> [ ... ]
>
> > @@ -558,19 +571,26 @@ static void __fbnic_pm_attach(struct device *dev)
> >       struct net_device *netdev = fbd->netdev;
> >       struct fbnic_net *fbn;
> >
> > -     rtnl_lock();
> > -     fbnic_reset_hw_stats(fbd);
> > -     rtnl_unlock();
> > -
> >       if (fbnic_init_failure(fbd))
> >               return;
>
> [Severity: Medium]
> fbnic_reset_hw_stats() now sits below the init failure check, so it no
> longer runs when fbd->netdev is NULL. Was that intended?

It was intended. I had actually recommended the change. However that
said I had overlooked the PCIe stats referenced below.

> Commit b1161b1863c5 ("eth: fbnic: Reset hw stats upon PCI error") put the
> reset ahead of this check on purpose. fbnic_reset_hw_stats() also handles
> a NULL netdev:
>
>         if (fbd->netdev)
>                 ASSERT_RTNL();
>
> A device in init failure mode stays bound so devlink still works. It also
> keeps the debugfs pcie_stats file created by fbnic_dbg_fbd_init(), and
> reading that file calls fbnic_get_hw_stats().

Now that you mentioned it there also appears to be a bug there in the
driver as the stats are initialized *after* the PCIe stats debugfs is
created, not before so there is a brief period where things can get
collected and then lost.

> Resume or AER/DPC recovery (fbnic_err_resume()->__fbnic_pm_attach()) can
> reset the hardware counters. The next read then goes through
> fbnic_hw_stat_rd64():
>
>         new_reg_value = fbnic_stat_rd64(fbd, reg, offset);
>         stat->value += new_reg_value - stat->u.old_reg_value_64;
>
> Won't this subtraction against the stale old_reg_value_64 wrap? That would
> permanently add a huge value to ob_rd_tlp, ob_wr_tlp, ob_cpl_tlp and the
> other PCIe counters. The 32-bit counters would get wrong deltas the same
> way.
>
> The new RTNL and attach ordering doesn't seem to need the reset skipped
> here, and the commit message doesn't mention this change.

This will probably need to be addressed. I will look at updating the
patch or creating a new one to address.

That said the issue itself should be very minor. It would present
itself as a set of corrupted stats on an already disabled device.

> > +     /* Hold RTNL until the device is attached, so the service task pass
> > +      * fbnic_up() schedules cannot run until it will see it present.
> > +      */
> > +     rtnl_lock();
> > +     netdev_lock(netdev);
> > +
> > +     fbnic_reset_hw_stats(fbd);
> > +
> >       if (netif_running(netdev))
> >               fbnic_up(fbn);
> >
> >       netif_device_attach(netdev);
> > +
> > +     netdev_unlock(netdev);
> > +     rtnl_unlock();
> >  }
>
> [Severity: High]
> This is a pre-existing issue, not one introduced by this patch. Error
> recovery adds the NAPI vectors under one RTNL hold and enables them under
> a later one. Can a close between the two hang?
>
> fbnic_err_slot_reset()->__fbnic_pm_resume() calls __fbnic_open() under
> rtnl_lock() and netdev_lock(). That reaches netif_napi_add_config_locked(),
> which leaves NAPI_STATE_SCHED and NAPI_STATE_NPSVC set. Then both locks
> are dropped:
>
>         if (netif_running(netdev)) {
>                 err = __fbnic_open(fbn);
>         ...
>         netdev_unlock(netdev);
>         rtnl_unlock();
>
> The NAPIs are only enabled later, in
> fbnic_err_resume()->__fbnic_pm_attach()->fbnic_up()->fbnic_napi_enable().
>
> In between, __LINK_STATE_START is still set, and __dev_close_many() calls
> ndo_stop even on a detached device. So "ip link set dev X down" in that
> window would run:
>
> fbnic_stop()
>   fbnic_down()
>     fbnic_down_noidle()
>       fbnic_napi_disable()
>         napi_disable_locked()
>
> napi_disable_locked() then waits for bits that only napi_enable_locked()
> clears:
>
>         while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
>                 usleep_range(20, 200);
>                 val = READ_ONCE(n->state);
>         }
>
> Wouldn't that loop spin forever while holding RTNL and the instance lock?
> fbnic_err_resume()->__fbnic_pm_attach() would then block on rtnl_lock().
>
> This patch now holds RTNL across fbnic_up() and the attach. The gap
> between __fbnic_open() and fbnic_up() is still open, though.

I will have to look into this. It will likely require some significant
changes to address. I am not sure if this is the correct patch to
address it in.

Re: [net PATCH 5/5] fbnic: Wait for the outbound flush to complete on mailbox teardown

[Alexander Duyck]

On Sun, Nov 8, 2026 at 10:32 AM <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
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] If the new poll times out, fbnic_mbx_clean_desc_ring()
>   (drivers/net/ethernet/meta/fbnic/fbnic_fw.c) still frees the mailbox…
>
> --
>
> fbnic: Wait for the outbound flush to complete on mailbox teardown
>
> After fbnic_mbx_reset_desc_ring() arms the flush, this patch polls the AW
> or AR flush-done bit in FBNIC_PUL_USER_IP_LINK_RST_STATUS. The poll runs
> before the mailbox ring buffers are unmapped and freed, and it gives up
> after 1ms and warns.
>
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> > index 49b8813a35643..e00d626f64384 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
>
> [ ... ]
>
> > @@ -176,10 +177,25 @@ static void fbnic_mbx_unmap_and_free_msg(struct fbnic_dev *fbd, int mbx_idx,
> >
> >  static void fbnic_mbx_clean_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
> >  {
> > +     u32 flush_done, val;
> >       int i;
> >
> >       fbnic_mbx_reset_desc_ring(fbd, mbx_idx);
> >
> > +     /* Arming the flush only starts it. Wait for the outbound path to
> > +      * retire the requests it still holds before the buffers they may
> > +      * target are unmapped and freed. A lost link reads back as all ones,
> > +      * which ends the poll on the first pass.
> > +      */
> > +     flush_done = mbx_idx == FBNIC_IPC_MBX_RX_IDX ?
> > +                  FBNIC_PUL_USER_IP_LINK_RST_STATUS_AW_FLUSH_DONE :
> > +                  FBNIC_PUL_USER_IP_LINK_RST_STATUS_AR_FLUSH_DONE;
> > +     if (read_poll_timeout_atomic(fbnic_rd32, val, val & flush_done, 2,
> > +                                  1000, false, fbd,
> > +                                  FBNIC_PUL_USER_IP_LINK_RST_STATUS))
> > +             dev_warn(fbd->dev, "Outbound flush did not complete: %08x\n",
> > +                      val);
> > +
> >       for (i = FBNIC_IPC_MBX_DESC_LEN; i--;)
> >               fbnic_mbx_unmap_and_free_msg(fbd, mbx_idx, i);
> >  }
>
> [Severity: Medium]
> If read_poll_timeout_atomic() returns -ETIMEDOUT, does the original
> DMA-after-free problem still happen?

Short answer. No.

> On timeout the code only calls dev_warn(). It then falls through to the
> same loop as before, and fbnic_mbx_unmap_and_free_msg() does this for
> every descriptor:
>
>     dma_unmap_single(fbd->dev, mbx->buf_info[desc_idx].addr,
>                      PAGE_SIZE, direction);
>
>     free_page((unsigned long)mbx->buf_info[desc_idx].msg);
>
> As the comment says, a lost link reads back as all ones, so that case
> exits on the first read. That leaves a timeout to mean a live device whose
> outbound path has not drained, which is the wedged-device case named in
> the commit message.
>
> In that case the page can still be freed and reused, or its IOVA unmapped,
> while a Tx read or an Rx write is still pending against it. Both paths
> reach this branch:
>
>   fbnic_fw_disable_mbx()->fbnic_mbx_clean()->fbnic_mbx_clean_desc_ring()
>
>   fbnic_mbx_poll_tx_ready() clean_mbx error path->fbnic_mbx_clean()

You have already called out your fallacy. If the reads are returning
~0 that means the PCIe link is down. If the PCIe link is down the
device is unable to communicate over the bus until it is reset and
reconfigured.

> The Rx ring has one more case. fbnic_mbx_init_desc_ring() later sets BME
> and clears FLUSH and FLUSH_MODE, and nothing checks that the earlier AW
> flush finished:
>
>     wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AW_CFG,
>          FBNIC_PUL_OB_TLP_HDR_AW_CFG_BME |
>          ...
>
> Could writes still parked in the pipeline then land on freed or reused DMA
> addresses?

This is why we reset the flush bits as a part of the initialization
path in the first place. Setting the flush and flush mode bits cause
the DMA writes that are stuck in the pipeline waiting on BME to all be
cleared so that they don't write to the host when the BME bit is set.

> On timeout, would it be safer to skip the unmap and free and leak the
> mailbox pages, instead of giving them back to the page allocator?

On clearing the BME the writes are essentially all terminated. This is
clearing the path between the PCIe user logic and the AXI fabric. The
general idea is we want to clear this path before the PCIe link is
dropped in the case of a warm or hot reset.

> Also, the 1ms limit doesn't seem to be based on the PCIe completion
> timeout, and that timeout is allowed to be longer. Is 1ms enough for
> outstanding reads to complete?

This is more about synchronizing the PCIe user logic to the AXI bus,
not the PCIe to the Host path. The PCIe itself is normally reset as
part of a hot or cold reset. We just need to clear the data that is
internal to the NIC that is being routed to the PCIe interface before
the PCIe is reset.

Re: [net PATCH 0/5] fbnic: Fix FW crash restore, stop path and mailbox teardown bugs

[patchwork-bot+netdevbpf]

Hello:

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

On Wed, 04 Nov 2026 09:58:59 -0800 you wrote:
> Fixes for races and ordering problems in the paths that take the datapath
> down or rebuild it: the FW crash restore, ndo_stop and PM suspend/resume,
> the Tx force-flush, and the DMA flush on mailbox teardown.
> 
> Patches 1-3 were found by code review or inspection; patch 3 has not been
> seen to trigger. Patches 4 and 5 together appear to resolve a hang with a
> PTP Tx timestamp left in flight. The series was tested on fbnic hardware
> with firmware 26.09.14-017_h7ab84d2ef68f, cycling the interface through
> DPC/AER recoveries for a full day, and under QEMU with the fbnic device
> model and live firmware.
> 
> [...]

Here is the summary with links:
  - [net,1/5] fbnic: Take the instance lock to restore filters after a FW crash
    https://git.kernel.org/netdev/net/c/999431e6153d
  - [net,2/5] fbnic: Fix service task races with ndo_stop and PM suspend
    https://git.kernel.org/netdev/net/c/05ec5504ff69
  - [net,3/5] fbnic: Disable PTP completions during the Tx force-flush
    https://git.kernel.org/netdev/net/c/7d4568edce74
  - [net,4/5] fbnic: Suspend phylink after taking the datapath down in ndo_stop
    https://git.kernel.org/netdev/net/c/b45b9c6b83e7
  - [net,5/5] fbnic: Wait for the outbound flush to complete on mailbox teardown
    https://git.kernel.org/netdev/net/c/c5820a8cf59b

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