[net PATCH 00/13] fbnic: Fix BMC filter, FW crash restore, teardown and Tx flush bugs

[net PATCH 00/13] fbnic: Fix BMC filter, FW crash restore, teardown and Tx flush bugs

[Alexander Duyck]

Review of the "Rework the BMC state pulled from the capabilities
message" and "Remove BMC routing rules when the BMC channel is disabled"
pair, d261a371e3b7 and db99b6fae21c, turned up a number of bugs in the
surrounding code that the pair did not introduce. This series fixes
them, along with the order of the ndo_stop path, the mailbox teardown
and the Tx force-flush.

Patch 1 rejects a MAC address attribute shorter than ETH_ALEN from
firmware, which was read past the end of the attribute.

Patches 2-3 and 7 fix MACDA allocation: a missing break in the
fbnic_sift_macda function, the all-multi wildcard entry being handed out
to the unicast and multicast lists, and the multicast sync ignoring
entries waiting for deletion.

Patch 4 retries a failed MACDA sync to firmware instead of dropping it.

Patches 5-6 close the races between the service task and ndo_stop and
the rx mode work: a service task pass that runs after ndo_stop no longer
restores host filters for a FW crash or a BMC update, and the crash
restore takes the instance lock.

Patch 8 rebuilds the BMC rules from scratch on every reinit so a changed
BMC MAC set, or a BMC that left while the interface was down, does not
leave stale rules behind.

Patch 9 drops the write-only active_slot field from the FW capabilities.

Patch 10 puts the BMC MAC address array under a seqcount so the service
task does not program a torn set while the mailbox replaces it.

Patch 11 suspends phylink after the datapath is taken down in ndo_stop
rather than before, so the Tx path drains with the link still up and the
stop path mirrors the open path.

Patch 12 waits for the outbound read and write flush armed on mailbox
teardown to complete before returning.

Patch 13 disables PTP completions along with the packet drop during the
Tx force-flush, so a dropped timestamped frame does not leave its
completion to be reported to the host without a matching packet.

---

Alexander Duyck (13):
      fbnic: Reject a short MAC address attribute from firmware
      fbnic: Stop sifting into a BMC slot once it has been filled
      fbnic: Keep the all-multi MACDA entry out of the address lists
      fbnic: Retry the MACDA sync to firmware if it fails
      fbnic: Don't touch host filters from a service task pass after ndo_stop
      fbnic: Take the instance lock to restore filters after a FW crash
      fbnic: Let the multicast sync reuse an entry waiting for deletion
      fbnic: Rebuild the BMC rules from scratch on every reinit
      fbnic: Drop the unused active_slot field from the FW capabilities
      fbnic: Keep the BMC MAC address set whole across a capabilities update
      fbnic: Suspend phylink after taking the datapath down in ndo_stop
      fbnic: Wait for the outbound flush to complete on mailbox teardown
      fbnic: Disable PTP completions during the Tx force-flush


 drivers/net/ethernet/meta/fbnic/fbnic_csr.h   |   5 +
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    |  53 +++--
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |   4 +-
 .../net/ethernet/meta/fbnic/fbnic_netdev.c    |   6 +-
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c   |  20 +-
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c   | 218 +++++++++++-------
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.h   |   6 +-
 drivers/net/ethernet/meta/fbnic/fbnic_tlv.c   |  21 +-
 drivers/net/ethernet/meta/fbnic/fbnic_tlv.h   |   2 +-
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c  |  10 +-
 10 files changed, 225 insertions(+), 120 deletions(-)

--

[net PATCH 01/13] fbnic: Reject a short MAC address attribute from firmware

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The TLV validator accepts an FBNIC_TLV_BINARY attribute of any length
from 1 up to the size in its index entry, so a MAC address attribute
shorter than ETH_ALEN passes it. The fbnic_tlv_attr_addr_copy function
then copies ETH_ALEN bytes regardless, reading past the end of the
attribute and, for the last attribute in a message, past the end of the
message.

Check the length in the fbnic_tlv_attr_addr_copy function and have it
return an error. The fbnic_fw_parse_bmc_addrs function stages the
addresses locally and only copies them out once all of them have parsed,
so a bad address leaves the previous set in place instead of half of a
new one.

Fixes: 20d2e88cc746 ("eth: fbnic: Add initial messaging to notify FW of our presence")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c  |   16 ++++++++++------
 drivers/net/ethernet/meta/fbnic/fbnic_tlv.c |   21 +++++++++++++++++----
 drivers/net/ethernet/meta/fbnic/fbnic_tlv.h |    2 +-
 3 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 49b8813a3564..bcbeb0814b82 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -596,6 +596,7 @@ static int fbnic_fw_parse_bmc_addrs(u8 bmc_mac_addr[][ETH_ALEN],
 {
 	int attr_len = le16_to_cpu(attr->hdr.len) / sizeof(u32) - 1;
 	struct fbnic_tlv_msg *mac_results[8];
+	u8 mac_addr[8][ETH_ALEN] = {};
 	int err, i = 0;
 
 	/* Make sure we have enough room to process all the MAC addresses */
@@ -609,13 +610,16 @@ static int fbnic_fw_parse_bmc_addrs(u8 bmc_mac_addr[][ETH_ALEN],
 	if (err)
 		return err;
 
-	/* Copy results into MAC addr array */
-	for (i = 0; i < len && mac_results[i]; i++)
-		fbnic_tlv_attr_addr_copy(bmc_mac_addr[i], mac_results[i]);
+	/* Stage the addresses so a bad one leaves the old set untouched.
+	 * Unused entries stay zeroed.
+	 */
+	for (i = 0; i < len && mac_results[i]; i++) {
+		err = fbnic_tlv_attr_addr_copy(mac_addr[i], mac_results[i]);
+		if (err)
+			return err;
+	}
 
-	/* Zero remaining unused addresses */
-	while (i < len)
-		eth_zero_addr(bmc_mac_addr[i++]);
+	memcpy(bmc_mac_addr, mac_addr, len * ETH_ALEN);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c b/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
index 3ef3935c0419..e1ef134ddb94 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
@@ -724,12 +724,25 @@ struct fbnic_tlv_msg *fbnic_tlv_test_create(struct fbnic_dev *fbd)
 	return NULL;
 }
 
-void fbnic_tlv_attr_addr_copy(u8 *dest, struct fbnic_tlv_msg *src)
+/**
+ * fbnic_tlv_attr_addr_copy - Copy a MAC address out of an attribute
+ * @dest: Buffer of ETH_ALEN bytes to copy the address into
+ * @src: Attribute holding the address
+ *
+ * The validator accepts a binary attribute of any length up to the size in
+ * its index entry, so a short MAC address gets through it. Check the length
+ * here rather than read past the end of the attribute.
+ *
+ * Return: zero on success, or -EINVAL if the attribute is not ETH_ALEN long.
+ **/
+int fbnic_tlv_attr_addr_copy(u8 *dest, struct fbnic_tlv_msg *src)
 {
-	u8 *mac_addr;
+	if (le16_to_cpu(src->hdr.len) != sizeof(*src) + ETH_ALEN)
+		return -EINVAL;
+
+	memcpy(dest, fbnic_tlv_attr_get_value_ptr(src), ETH_ALEN);
 
-	mac_addr = fbnic_tlv_attr_get_value_ptr(src);
-	memcpy(dest, mac_addr, ETH_ALEN);
+	return 0;
 }
 
 /**
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_tlv.h b/drivers/net/ethernet/meta/fbnic/fbnic_tlv.h
index 9c4e4759394a..baf8e8b40b90 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_tlv.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_tlv.h
@@ -136,7 +136,7 @@ int fbnic_tlv_attr_put_string(struct fbnic_tlv_msg *msg, u16 attr_id,
 struct fbnic_tlv_msg *fbnic_tlv_attr_nest_start(struct fbnic_tlv_msg *msg,
 						u16 attr_id);
 void fbnic_tlv_attr_nest_stop(struct fbnic_tlv_msg *msg);
-void fbnic_tlv_attr_addr_copy(u8 *dest, struct fbnic_tlv_msg *src);
+int fbnic_tlv_attr_addr_copy(u8 *dest, struct fbnic_tlv_msg *src);
 int fbnic_tlv_attr_parse_array(struct fbnic_tlv_msg *attr, int len,
 			       struct fbnic_tlv_msg **results,
 			       const struct fbnic_tlv_index *tlv_index,

[net PATCH 02/13] fbnic: Stop sifting into a BMC slot once it has been filled

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_sift_macda function moves BMC-only multicast addresses down
into the free slots of the BMC region. The inner loop that looks for the
next address to move does not stop after it moves one, so with two or
more BMC-only multicast addresses waiting the second is copied over the
first in the same destination slot. Both sources are marked for
deletion, and the first address is dropped from the MACDA table.

Break out of the inner loop once the destination has been filled so the
outer loop moves on to the next free slot.

Fixes: eb690ef8d1c2 ("eth: fbnic: Add L2 address programming")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index 939d56585b26..dc86b51f3583 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -645,6 +645,9 @@ void fbnic_sift_macda(struct fbnic_dev *fbd)
 			memcpy(dest_addr, src_addr, sizeof(*src_addr));
 			src_addr->state = FBNIC_TCAM_S_DELETE;
 			dest_addr->state = FBNIC_TCAM_S_ADD;
+
+			/* dest_addr is taken, move on to the next free one */
+			break;
 		}
 	}
 }

[net PATCH 03/13] fbnic: Keep the all-multi MACDA entry out of the address lists

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_bmc_rpc_all_multi_config function installs the multicast
wildcard in the entry just below mac_addr_boundary and points the BMC
all-multi action rule at that index. It only installs the wildcard if
the entry is not already valid, and never checks what a valid entry
holds.

Both the unicast and the multicast list can hand that entry out. The
__fbnic_uc_sync function scans up from mac_addr_boundary - 1 and takes
it once the entries above are full, and the __fbnic_mc_sync function
takes it once the entries below are full. Once a host address sits there
the wildcard is never installed, the BMC loses all-multi, and the BMC
all-multi rule, which only delivers to the BMC, matches the host address
instead and steals its traffic. A unicast address there is not even
reachable by the fbnic_uc_unsync function, which starts its scan at
mac_addr_boundary.

Reserve the entry for the wildcard. The unicast scan now starts at
mac_addr_boundary, matching the fbnic_uc_unsync function, and the
multicast sync and unsync scans start one below it. This costs the
multicast list one entry; past that it falls back to all-multi as
before.

Fixes: eb690ef8d1c2 ("eth: fbnic: Add L2 address programming")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_netdev.c |    2 +-
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c    |    8 +++++---
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.h    |    6 +++---
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index 8a6703afca38..a40566029a86 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -170,7 +170,7 @@ static int fbnic_mc_unsync(struct net_device *netdev, const unsigned char *addr)
 	 * Skip over the address reserved for the BMC MAC and
 	 * exclude index 0 as that belongs to the broadcast address
 	 */
-	for (i = fbd->mac_addr_boundary, ret = -ENOENT;
+	for (i = fbd->mac_addr_boundary - 1, ret = -ENOENT;
 	     --i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX && ret;) {
 		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[i];
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index dc86b51f3583..0be71e6fd3d4 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -477,8 +477,9 @@ struct fbnic_mac_addr *__fbnic_uc_sync(struct fbnic_dev *fbd,
 	/* Scan from middle of list to bottom, filling bottom up.
 	 * Skip the first entry which is reserved for dev_addr and
 	 * leave the last entry to use for promiscuous filtering.
+	 * The entry below the boundary is reserved for all-multi.
 	 */
-	for (i = fbd->mac_addr_boundary - 1;
+	for (i = fbd->mac_addr_boundary;
 	     i < FBNIC_RPC_TCAM_MACDA_HOST_ADDR_IDX; i++) {
 		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[i];
 
@@ -507,9 +508,10 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
 
 	/* Scan from middle of list to top, filling top down.
 	 * Skip over the address reserved for the BMC MAC and
-	 * exclude index 0 as that belongs to the broadcast address
+	 * exclude index 0 as that belongs to the broadcast address.
+	 * The entry below the boundary is reserved for all-multi.
 	 */
-	for (i = fbd->mac_addr_boundary;
+	for (i = fbd->mac_addr_boundary - 1;
 	     --i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX;) {
 		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[i];
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
index 3d4925b2ac75..679ba0a1058d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
@@ -156,9 +156,9 @@ enum {
  * always go top-down with unicast, and bottom-up with multicast so that
  * there should be free-space in the middle between the two.
  *
- * The entry at MADCA_DEFAULT_BOUNDARY is a special case as it can be used
- * for the ALL MULTI address if the list is full, or the BMC has requested
- * it.
+ * The entry just below the boundary is reserved for the ALL MULTI address,
+ * used if the list is full, or the BMC has requested it. Neither the unicast
+ * nor the multicast list allocates it.
  */
 #define FBNIC_RPC_TCAM_MACDA_MULTICAST_IDX	5
 #define FBNIC_RPC_TCAM_MACDA_DEFAULT_BOUNDARY	24

[net PATCH 04/13] fbnic: Retry the MACDA sync to firmware if it fails

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_bmc_rpc_check function clears FBNIC_FW_CAP_F_BMC_MACDA_SYNC
before it sends the MACDA table to firmware and does not set it again
when the send fails. A transient failure, -ENOMEM on the message page,
-EBUSY from a full mailbox, or a DMA mapping error, leaves firmware with
a stale address list and stale receive flags until some unrelated change
happens to raise the flag again.

Set the flag again on failure so the next service task pass retries, and
rate limit the warning now that it can repeat.

Fixes: cee8d21d8091 ("fbnic: Push local unicast MAC addresses to FW to populate TCAMs")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index 0be71e6fd3d4..d69c91066508 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -345,9 +345,14 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
 	if (test_and_clear_bit(FBNIC_FW_CAP_F_BMC_MACDA_SYNC,
 			       &fbd->fw_cap.state)) {
 		err = fbnic_fw_xmit_rpc_macda_sync(fbd);
-		if (err)
-			dev_warn(fbd->dev,
-				 "Writing MACDA table to FW failed, err: %d\n", err);
+		if (err) {
+			dev_warn_ratelimited(fbd->dev,
+					     "Writing MACDA table to FW failed, err: %d\n",
+					     err);
+			/* Leave FW to be synced on the next pass */
+			set_bit(FBNIC_FW_CAP_F_BMC_MACDA_SYNC,
+				&fbd->fw_cap.state);
+		}
 	}
 
 	netdev_unlock(fbd->netdev);

[net PATCH 05/13] fbnic: Don't touch host filters from a service task pass after ndo_stop

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_down_noidle function stops the service task by calling the
cancel_delayed_work function, which does not wait for a pass that is
already executing. It cannot use the cancel_delayed_work_sync function,
as ndo_stop holds RTNL and the task takes RTNL itself. A pass that is
blocked on RTNL while ndo_stop runs carries on once ndo_stop returns,
and only the re-arm at the end of it checks that the interface is still
up.

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. The fbnic_fw_xmit_ownership_msg function
clears fw_heartbeat_enabled when it does, so the fbnic_health_check
function 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. In both
cases host traffic is steered at queues that are no longer running,
until the next open and close.

Move both calls to the end of the service task, into the netif_running
check that re-arms it. The __dev_close_many function clears
__LINK_STATE_START under RTNL before it calls ndo_stop, so the late pass
now skips them. The BMC flags stay raised for the first pass after the
next open, and the __fbnic_open function reclaims ownership, restarts
the heartbeat and rebuilds the BMC rules.

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 |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index ad234bb0f75d..e7744915d001 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -229,18 +229,23 @@ 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; __fbnic_open() reclaims and rebuilds it.
+	 */
+	if (netif_running(netdev)) {
+		fbnic_health_check(fbd);
+
+		fbnic_bmc_rpc_check(fbd);
+
 		schedule_delayed_work(&fbd->service_task, HZ);
+	}
 
 	rtnl_unlock();
 }

[net PATCH 06/13] 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 e7744915d001..37cb3bedb0d7 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 07/13] fbnic: Let the multicast sync reuse an entry waiting for deletion

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The __fbnic_mc_sync function only hands out MACDA entries in the
DISABLED state. The __hw_addr_sync_dev function unsyncs the addresses
that went away before it syncs the new ones, and the fbnic_mc_unsync
function only marks the old entries DELETE; they are not freed until the
fbnic_write_macda function runs at the end of the pass. With the
multicast region full, swapping one address for another in a single rx
mode update finds no free entry, the sync fails with -ENOSPC and the
device drops to all-multi. The new address keeps a sync_cnt of zero, so
it is only retried the next time the address lists change, and the
device can sit in all-multi with room to spare until then.

Treat an entry in DELETE as free. It may still carry tags, as the
fbnic_sift_macda function leaves the BMC tag on the entry it moves out
of, so clear them along with the address and mask before handing it out.

An entry in DELETE that still holds the address being synced was already
matched by address, but it was handed back still in DELETE, and the
fbnic_write_macda function then wiped it along with the tag the caller
had just set. Reset that one the same way, so it comes back as a fresh
entry.

Fixes: eb690ef8d1c2 ("eth: fbnic: Add L2 address programming")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index d69c91066508..ee781a3e1528 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -520,12 +520,16 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
 	     --i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX;) {
 		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[i];
 
-		if (mac_addr->state == FBNIC_TCAM_S_DISABLED) {
-			avail_addr = mac_addr;
-		} else if (ether_addr_equal(mac_addr->value.addr8, addr)) {
+		if (mac_addr->state != FBNIC_TCAM_S_DISABLED &&
+		    ether_addr_equal(mac_addr->value.addr8, addr)) {
 			avail_addr = mac_addr;
 			break;
 		}
+
+		/* An entry waiting to be deleted is as good as a free one */
+		if (mac_addr->state == FBNIC_TCAM_S_DISABLED ||
+		    mac_addr->state == FBNIC_TCAM_S_DELETE)
+			avail_addr = mac_addr;
 	}
 
 	/* Scan the BMC addresses to see if it may have already
@@ -551,9 +555,12 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
 		break;
 	}
 
-	if (avail_addr && avail_addr->state == FBNIC_TCAM_S_DISABLED) {
+	if (avail_addr && (avail_addr->state == FBNIC_TCAM_S_DISABLED ||
+			   avail_addr->state == FBNIC_TCAM_S_DELETE)) {
 		ether_addr_copy(avail_addr->value.addr8, addr);
 		eth_zero_addr(avail_addr->mask.addr8);
+		bitmap_zero(avail_addr->act_tcam,
+			    FBNIC_RPC_TCAM_ACT_NUM_ENTRIES);
 		avail_addr->state = FBNIC_TCAM_S_ADD;
 	}
 

[net PATCH 08/13] fbnic: Rebuild the BMC rules from scratch on every reinit

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_bmc_rpc_init function tags the MACDA entries for the MAC
addresses the BMC currently reports, but never untags the ones it
reported before. If the BMC changes its MAC set while staying present,
or drops its channel and comes back with different addresses before the
service task gets to the first message, the addresses it no longer owns
stay BMC-tagged and valid. Action rule 0 keeps steering their Rx traffic
to the BMC and the fbnic_write_tce_tcam function keeps looping host Tx
for them to it.

The fbnic_bmc_rpc_check function only tore the rules down when the BMC
was absent and the fbnic_bmc_rules_present function found a BMC-tagged
MACDA entry. That misses the host-unicast RSS entries when the
fbnic_rss_reinit function programmed them from ethtool or SIOCSHWTSTAMP
while the BMC was briefly present, before any MACDA entry was tagged. It
also does nothing at open: if the BMC went away while the interface was
down, the __fbnic_open function writes the entries left from the last
ifdown back to hardware and they stay there until the first service task
pass.

Have the fbnic_bmc_rpc_init function start by calling the
fbnic_bmc_rpc_disable function and then rebuild from the current
capabilities, and drop the conditional and the fbnic_bmc_rules_present
function. The teardown only marks shadow state, so an entry rebuilt
before the next hardware write is rewritten in place, not cleared and
re-added. Both callers already follow it with the fbnic_rss_reinit
function, which puts the host-unicast RSS entries back when a BMC is
present. A BMC multicast address that is still reported is found again
by the __fbnic_mc_sync function in the DELETE entry the teardown left,
and one that changed can take a freed slot, now that the multicast sync
reuses entries waiting for deletion. One that the fbnic_sift_macda
function moved into the BMC region is only pulled back out by the
__fbnic_mc_sync function when a shared slot is free, so with the shared
region full it is taken back in place by the new fbnic_bmc_mc_reserved
function instead of being lost.

The all-multi wildcard is rebuilt by the fbnic_bmc_rpc_all_multi_config
function instead, which only tagged it when it installed it. Tag it on
every pass, as an entry the host kept for its own all-multi stays valid
through the teardown.

Fixes: 04a230b27d8f ("fbnic: Add logic to repopulate RPC TCAM if BMC enables channel")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c |  167 +++++++++++++++------------
 1 file changed, 91 insertions(+), 76 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index ee781a3e1528..e6b9b9d6511f 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -123,9 +123,12 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
 			eth_broadcast_addr(mac_addr->mask.addr8);
 			mac_addr->value.addr8[0] ^= 1;
 			mac_addr->mask.addr8[0] ^= 1;
-			set_bit(FBNIC_MAC_ADDR_T_BMC, mac_addr->act_tcam);
 			mac_addr->state = FBNIC_TCAM_S_ADD;
 		}
+		/* Tag it even if valid, the host may have held on to it while
+		 * fbnic_bmc_rpc_disable() stripped the BMC tag.
+		 */
+		set_bit(FBNIC_MAC_ADDR_T_BMC, mac_addr->act_tcam);
 		assign_bit(FBNIC_MAC_ADDR_T_ALLMULTI, mac_addr->act_tcam,
 			   enable_host);
 	} else {
@@ -172,6 +175,77 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
 	act_tcam->state = FBNIC_TCAM_S_UPDATE;
 }
 
+/**
+ * fbnic_bmc_rpc_disable - remove the BMC MAC and action rules
+ * @fbd: Pointer to fbnic device struct
+ *
+ * Strip everything a previous fbnic_bmc_rpc_init() programmed. Idempotent,
+ * and only marks shadow state; entries that are rebuilt before the next
+ * hardware write are rewritten in place rather than cleared and re-added.
+ */
+static void fbnic_bmc_rpc_disable(struct fbnic_dev *fbd)
+{
+	struct fbnic_act_tcam *act_tcam;
+	int idx;
+
+	/* Drop the BMC's claim; entries the host also uses survive */
+	for (idx = ARRAY_SIZE(fbd->mac_addr); idx--;) {
+		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[idx];
+
+		if (mac_addr->state == FBNIC_TCAM_S_DISABLED)
+			continue;
+
+		__fbnic_xc_unsync(mac_addr, FBNIC_MAC_ADDR_T_BMC);
+	}
+
+	/* Delete in any live state, not just VALID: init leaves it UPDATE and
+	 * ifdown demotes VALID to UPDATE.
+	 */
+	act_tcam = &fbd->act_tcam[FBNIC_RPC_ACT_TBL_BMC_OFFSET];
+	if (act_tcam->state != FBNIC_TCAM_S_DISABLED)
+		act_tcam->state = FBNIC_TCAM_S_DELETE;
+
+	/* fbnic_rss_reinit() only programs the host-unicast entries while a BMC
+	 * is present, and does not clear them otherwise. Delete them here and
+	 * let it put them back if they are still needed.
+	 */
+	for (idx = 0; idx < FBNIC_RSS_EN_NUM_UNICAST; idx++) {
+		act_tcam = &fbd->act_tcam[FBNIC_RPC_ACT_TBL_RSS_OFFSET + idx];
+		if (act_tcam->state != FBNIC_TCAM_S_DISABLED)
+			act_tcam->state = FBNIC_TCAM_S_DELETE;
+	}
+}
+
+/**
+ * fbnic_bmc_mc_reserved - find a BMC multicast entry in the BMC region
+ * @fbd: Pointer to fbnic device struct
+ * @addr: Multicast address to look for
+ *
+ * fbnic_sift_macda() moves a BMC-only multicast entry into a free BMC slot,
+ * and __fbnic_mc_sync() only pulls it back out when a shared slot is free.
+ *
+ * Return: the entry holding @addr, or NULL if there is none.
+ */
+static struct fbnic_mac_addr *fbnic_bmc_mc_reserved(struct fbnic_dev *fbd,
+						    const u8 *addr)
+{
+	int idx;
+
+	for (idx = FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX + 1;
+	     idx < FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX; idx++) {
+		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[idx];
+
+		if (mac_addr->state == FBNIC_TCAM_S_DISABLED)
+			continue;
+
+		if (is_zero_ether_addr(mac_addr->mask.addr8) &&
+		    ether_addr_equal(mac_addr->value.addr8, addr))
+			return mac_addr;
+	}
+
+	return NULL;
+}
+
 void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
 {
 	int i = FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX;
@@ -179,6 +253,11 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
 	struct fbnic_mac_addr *mac_addr;
 	int j;
 
+	/* Start from nothing so a MAC the BMC no longer reports does not
+	 * keep its tag, then rebuild from what firmware last sent.
+	 */
+	fbnic_bmc_rpc_disable(fbd);
+
 	/* Check if BMC is present */
 	if (!fbnic_bmc_present(fbd))
 		return;
@@ -191,10 +270,16 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
 		if (is_zero_ether_addr(bmc_mac))
 			continue;
 
-		if (is_multicast_ether_addr(bmc_mac))
+		if (is_multicast_ether_addr(bmc_mac)) {
 			mac_addr = __fbnic_mc_sync(fbd, bmc_mac);
-		else
+			/* With the shared region full, keep the copy the
+			 * teardown left in the BMC region in place.
+			 */
+			if (!mac_addr)
+				mac_addr = fbnic_bmc_mc_reserved(fbd, bmc_mac);
+		} else {
 			mac_addr = &fbd->mac_addr[i++];
+		}
 
 		if (!mac_addr) {
 			netdev_err(fbd->netdev,
@@ -236,73 +321,6 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
 	act_tcam->state = FBNIC_TCAM_S_UPDATE;
 }
 
-/**
- * fbnic_bmc_rules_present - is the BMC currently programmed into the filters?
- * @fbd: Pointer to fbnic device struct
- *
- * The BMC tag is only set while a BMC is present, so it doubles as the state.
- *
- * Return: true if any MACDA entry carries the BMC tag, false otherwise.
- */
-static bool fbnic_bmc_rules_present(struct fbnic_dev *fbd)
-{
-	int idx;
-
-	for (idx = ARRAY_SIZE(fbd->mac_addr); idx--;) {
-		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[idx];
-
-		if (mac_addr->state == FBNIC_TCAM_S_DISABLED)
-			continue;
-
-		if (test_bit(FBNIC_MAC_ADDR_T_BMC, mac_addr->act_tcam))
-			return true;
-	}
-
-	return false;
-}
-
-/**
- * fbnic_bmc_rpc_disable - remove the BMC MAC and action rules
- * @fbd: Pointer to fbnic device struct
- *
- * Undo fbnic_bmc_rpc_init() when the BMC drops its NC-SI channel while the
- * host interface stays up. Only marks shadow state; the caller's
- * __fbnic_set_rx_mode() pushes it to hardware, so ordering here is moot.
- */
-static void fbnic_bmc_rpc_disable(struct fbnic_dev *fbd)
-{
-	struct fbnic_act_tcam *act_tcam;
-	int idx;
-
-	/* Drop the BMC's claim; entries the host also uses survive */
-	for (idx = ARRAY_SIZE(fbd->mac_addr); idx--;) {
-		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[idx];
-
-		if (mac_addr->state == FBNIC_TCAM_S_DISABLED)
-			continue;
-
-		__fbnic_xc_unsync(mac_addr, FBNIC_MAC_ADDR_T_BMC);
-	}
-
-	/* Delete in any live state, not just VALID: init leaves it UPDATE and
-	 * ifdown demotes VALID to UPDATE. A miss leaks the rule for good, as
-	 * fbnic_bmc_rules_present() keys off the MACDA tag just cleared above.
-	 */
-	act_tcam = &fbd->act_tcam[FBNIC_RPC_ACT_TBL_BMC_OFFSET];
-	if (act_tcam->state != FBNIC_TCAM_S_DISABLED)
-		act_tcam->state = FBNIC_TCAM_S_DELETE;
-
-	/* fbnic_rss_reinit() only programs the host-unicast entries while a BMC
-	 * is present, so the reinit after this would leave them stale and valid
-	 * in hardware. Delete them to converge on the no-BMC layout.
-	 */
-	for (idx = 0; idx < FBNIC_RSS_EN_NUM_UNICAST; idx++) {
-		act_tcam = &fbd->act_tcam[FBNIC_RPC_ACT_TBL_RSS_OFFSET + idx];
-		if (act_tcam->state != FBNIC_TCAM_S_DISABLED)
-			act_tcam->state = FBNIC_TCAM_S_DELETE;
-	}
-}
-
 void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
 {
 	struct fbnic_net *fbn = netdev_priv(fbd->netdev);
@@ -328,13 +346,10 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
 	 */
 	if (test_and_clear_bit(FBNIC_FW_CAP_F_BMC_TCAM_REINIT,
 			       &fbd->fw_cap.state)) {
-		if (!fbnic_bmc_present(fbd) && fbnic_bmc_rules_present(fbd))
-			fbnic_bmc_rpc_disable(fbd);
-		else
-			fbnic_bmc_rpc_init(fbd);
+		fbnic_bmc_rpc_init(fbd);
 
-		/* Neither path touches the RSS actions, which carry the mc/bc
-		 * copy to the BMC, so recompute them alongside.
+		/* The RSS actions carry the mc/bc copy to the BMC and depend on
+		 * its presence, so recompute them alongside.
 		 */
 		fbnic_rss_reinit(fbd, fbn);
 		netif_addr_lock_bh(fbd->netdev);

[net PATCH 09/13] fbnic: Drop the unused active_slot field from the FW capabilities

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic_fw_parse_cap_resp function stores
FBNIC_FW_CAP_RESP_ACTIVE_FW_SLOT in fw_cap.active_slot, and nothing has
ever read it back. Drop the field and the store as it is just a waste of
space.

Keep the attribute in the capabilities response index. The parser
rejects an attribute it has no index entry for if firmware marks it
cannot_ignore, so removing the entry could make it refuse the whole
response.

Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c |    2 --
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h |    1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index bcbeb0814b82..7964ce814b8e 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -725,8 +725,6 @@ static int fbnic_fw_parse_cap_resp(void *opaque, struct fbnic_tlv_msg **results)
 		    fbd->fw_cap.stored.undi.commit,
 		    FBNIC_FW_CAP_RESP_COMMIT_MAX_SIZE);
 
-	fbd->fw_cap.active_slot =
-		fta_get_uint(results, FBNIC_FW_CAP_RESP_ACTIVE_FW_SLOT);
 	fbd->fw_cap.link_speed =
 		fta_get_uint(results, FBNIC_FW_CAP_RESP_FW_LINK_SPEED);
 	fbd->fw_cap.link_fec =
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index f47669129c9d..7790ea0badea 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -67,7 +67,6 @@ struct fbnic_fw_cap {
 	struct {
 		struct fbnic_fw_ver mgmt, bootloader, undi;
 	} stored;
-	u8	active_slot;
 	u8	bmc_mac_addr[4][ETH_ALEN];
 	u8	link_speed;
 	u8	link_fec;

[net PATCH 10/13] fbnic: Keep the BMC MAC address set whole across a capabilities update

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The firmware can send a capabilities message at any time, and the
mailbox handler writes the BMC MAC addresses into fw_cap.bmc_mac_addr
from the mailbox interrupt thread. The fbnic_bmc_rpc_init function reads
the same array from the service task and from ndo_open with no lock in
common. A read that overlaps the update can see part of the old set and
part of the new one, or a single address with bytes from both, and
program it into the MACDA TCAM as a BMC address, steering that address's
Rx traffic to the BMC and looping host Tx for it.

The update always raises FBNIC_FW_CAP_F_BMC_TCAM_REINIT after it writes
the array, so the next service task pass rebuilds from the full set.
Until then, up to a second later, the torn entry stays in hardware and
has been pushed to firmware with the MACDA sync.

Guard the array with a seqcount in struct fbnic_fw_cap. The mailbox
parses the new set into a local copy and swaps it in inside a write
section, and the fbnic_bmc_rpc_init function takes a snapshot, retrying
if an update overlapped it, and works from that. Updates are rare and
the mailbox Rx path is the only writer, so a seqcount is enough and the
readers never wait on a lock. The write section runs with preemption
disabled so a reader cannot spin on a preempted update.

Fixes: 04a230b27d8f ("fbnic: Add logic to repopulate RPC TCAM if BMC enables channel")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c  |   19 ++++++++++++++-----
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h  |    3 +++
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c |   14 +++++++++++++-
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 7964ce814b8e..09d03274be08 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -104,6 +104,7 @@ void fbnic_mbx_init(struct fbnic_dev *fbd)
 
 	/* Reset FW Capabilities */
 	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));
+	seqcount_init(&fbd->fw_cap.bmc_mac_seq);
 
 	/* Reinitialize mailbox memory */
 	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
@@ -627,6 +628,7 @@ static int fbnic_fw_parse_bmc_addrs(u8 bmc_mac_addr[][ETH_ALEN],
 static int fbnic_fw_parse_bmc_cap(struct fbnic_dev *fbd,
 				  struct fbnic_tlv_msg **results)
 {
+	u8 bmc_mac_addr[4][ETH_ALEN] = {};
 	struct fbnic_tlv_msg *attr;
 	bool bmc_present = false;
 	u32 all_multi = 0;
@@ -639,19 +641,26 @@ static int fbnic_fw_parse_bmc_cap(struct fbnic_dev *fbd,
 	attr = results[FBNIC_FW_CAP_RESP_BMC_PRESENT] ?
 	       results[FBNIC_FW_CAP_RESP_BMC_MAC_ARRAY] : NULL;
 	if (attr) {
-		err = fbnic_fw_parse_bmc_addrs(fbd->fw_cap.bmc_mac_addr,
-					       attr, 4);
+		err = fbnic_fw_parse_bmc_addrs(bmc_mac_addr, attr, 4);
 		if (err)
 			return err;
 
 		all_multi = fta_get_uint(results,
 					 FBNIC_FW_CAP_RESP_BMC_ALL_MULTI);
 		bmc_present = true;
-	} else {
-		memset(fbd->fw_cap.bmc_mac_addr, 0,
-		       sizeof(fbd->fw_cap.bmc_mac_addr));
 	}
 
+	/* The service task can be reading the set in fbnic_bmc_rpc_init(),
+	 * so replace it whole. The mailbox Rx path is the only writer, so a
+	 * seqcount is enough; keep preemption off so a reader cannot spin on
+	 * a preempted update.
+	 */
+	preempt_disable();
+	write_seqcount_begin(&fbd->fw_cap.bmc_mac_seq);
+	memcpy(fbd->fw_cap.bmc_mac_addr, bmc_mac_addr, sizeof(bmc_mac_addr));
+	write_seqcount_end(&fbd->fw_cap.bmc_mac_seq);
+	preempt_enable();
+
 	/* all_multi is only read out of the message when a BMC is present, so
 	 * it is never set on its own and callers can test it without pairing
 	 * it with the presence check.
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index 7790ea0badea..f380291a5ad6 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -6,6 +6,7 @@
 
 #include <linux/completion.h>
 #include <linux/if_ether.h>
+#include <linux/seqlock.h>
 #include <linux/types.h>
 
 struct fbnic_dev;
@@ -67,6 +68,8 @@ struct fbnic_fw_cap {
 	struct {
 		struct fbnic_fw_ver mgmt, bootloader, undi;
 	} stored;
+	/* Keeps bmc_mac_addr whole for readers, the mailbox is the writer */
+	seqcount_t bmc_mac_seq;
 	u8	bmc_mac_addr[4][ETH_ALEN];
 	u8	link_speed;
 	u8	link_fec;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index e6b9b9d6511f..d64bd622e4a3 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -251,6 +251,8 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
 	int i = FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX;
 	struct fbnic_act_tcam *act_tcam;
 	struct fbnic_mac_addr *mac_addr;
+	u8 bmc_mac_addr[4][ETH_ALEN];
+	unsigned int seq;
 	int j;
 
 	/* Start from nothing so a MAC the BMC no longer reports does not
@@ -262,9 +264,19 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
 	if (!fbnic_bmc_present(fbd))
 		return;
 
+	/* Snapshot the set, the mailbox can replace it under us. A message
+	 * that lands after this raises FBNIC_FW_CAP_F_BMC_TCAM_REINIT again,
+	 * so the next pass rebuilds from it.
+	 */
+	do {
+		seq = read_seqcount_begin(&fbd->fw_cap.bmc_mac_seq);
+		memcpy(bmc_mac_addr, fbd->fw_cap.bmc_mac_addr,
+		       sizeof(bmc_mac_addr));
+	} while (read_seqcount_retry(&fbd->fw_cap.bmc_mac_seq, seq));
+
 	/* Fetch BMC MAC addresses from firmware capabilities */
 	for (j = 0; j < 4; j++) {
-		u8 *bmc_mac = fbd->fw_cap.bmc_mac_addr[j];
+		u8 *bmc_mac = bmc_mac_addr[j];
 
 		/* Validate BMC MAC addresses */
 		if (is_zero_ether_addr(bmc_mac))

[net PATCH 11/13] 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 a40566029a86..36ef17dec0f4 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 12/13] 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 read and write flush in a single register write, but nothing
waits for that flush to finish. The fbnic_mbx_clean function returns as
soon as the rings are torn down, so the caller is free to let the device
reset while the outbound path still owns read requests it has not
retired. A completion landing after that point, or a tag handed back out
while the old one is still live, has nowhere valid to go.

The PUL link reset status register reports when the flush has drained in
both directions, so poll it before returning. The flush only has to
retire what is already in flight, so it settles well inside a
microsecond; the 1ms cap is there so a wedged device cannot spin.

Every caller of the fbnic_mbx_clean function runs with the datapath
already down, which matters because the flush covers all outstanding
reads, not just the mailbox ones. A device that has lost its link reads
back as all ones, so both done bits appear 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 cbb25d11638c..d7583243e5cd 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_csr.h
@@ -955,6 +955,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 09d03274be08..32a18cb8990c 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"
@@ -187,10 +188,25 @@ static void fbnic_mbx_clean_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
 
 void fbnic_mbx_clean(struct fbnic_dev *fbd)
 {
+	u32 flush_done = FBNIC_PUL_USER_IP_LINK_RST_STATUS_AR_FLUSH_DONE |
+			 FBNIC_PUL_USER_IP_LINK_RST_STATUS_AW_FLUSH_DONE;
+	u32 val;
 	int i;
 
 	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
 		fbnic_mbx_clean_desc_ring(fbd, i);
+
+	/* Arming the flush above only starts it. Wait for the outbound path
+	 * to retire the requests it still owns so none of them can complete
+	 * after the reset that follows. A lost link reads back as all ones,
+	 * which ends the poll on the first pass.
+	 */
+	if (read_poll_timeout_atomic(fbnic_rd32, val,
+				     (val & flush_done) == 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);
 }
 
 #define FBNIC_MBX_MAX_PAGE_SIZE	FIELD_MAX(FBNIC_IPC_MBX_DESC_LEN_MASK)

[net PATCH 13/13] 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: 40bf06a160a1 ("eth: fbnic: Implement Tx queue alloc/start/stop/free")
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 d7583243e5cd..c371ac9f78c1 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 c572a0062e77..9bc934c6771d 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 {

Re: [net PATCH 00/13] fbnic: Fix BMC filter, FW crash restore, teardown and Tx flush bugs

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [net PATCH 00/13] fbnic: Fix BMC filter, FW crash restore, teardown and Tx flush bugs

[Alexander Duyck]

On Wed, Oct 28, 2026 at 2:42 PM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.
>
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.

Patches 1-10 were found by code inspection while reviewing
d261a371e3b7 ("fbnic: Rework the BMC state pulled from the
capabilities message") and db99b6fae21c ("fbnic: Remove BMC routing
rules when the BMC channel is disabled"). They were not triggered in
practice.

Patches 11 and 12 come from a hang we hit on hardware. A test loop
injects a PCIe error, which triggers DPC containment and driver
recovery, while timestamped Tx traffic is running. Over many
iterations the device's PTP in-flight counter climbed until the PTP
request FIFO filled. After that, Tx timestamp requests never completed.
There was no kernel splat; the symptom is the stuck counter and Tx
timestamping stopping. With both patches applied the hang has not
reproduced in over 10,000 iterations of the same loop.

Patch 13 was found by inspection. It targets the "Tx timestamp without
matching packet" message from fbnic_clean_tsq(). We do see that
message on hardware after the recovery cycles above, but we have not
seen the Tx force-flush path it changes actually run, and the message
still shows up with the patch applied. So this fixes a real gap in the
flush, but it is not the whole story for that message.

Testing: the full series was run on an fbnic NIC with a BMC attached
(firmware 26.09.14-017_h7ab84d2ef68f), using the recovery loop above.
It was also tested in QEMU, using an emulated fbnic paired with a
second QEMU instance running the device firmware. That covered link
bring-up, BMC NC-SI channel enable, disable and re-enable, and a
reproducer for the multicast table exhaustion case handled in patch 8.

Re: [net PATCH 01/13] fbnic: Reject a short MAC address attribute from firmware

[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] This patch changes fbnic_tlv_attr_addr_copy() to return int:
  -EINVAL when the attribute is not exactly ETH_ALEN long, and in that…

Pre-existing issues:
- [High] fbnic_fw_parse_qsfp_read_resp() in
  drivers/net/ethernet/meta/fbnic/fbnic_fw.c has the same short-BINARY-…
- [Medium] (Baseline decision: not reported.

--

fbnic: Reject a short MAC address attribute from firmware

This patch makes fbnic_tlv_attr_addr_copy() reject a MAC address attribute
that is not exactly ETH_ALEN long and return -EINVAL. It also changes
fbnic_fw_parse_bmc_addrs() to stage the parsed addresses in a local array.
They are copied out only once all of them have parsed.

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

[ ... ]

> @@ -609,13 +610,16 @@ static int fbnic_fw_parse_bmc_addrs(u8 bmc_mac_addr[][ETH_ALEN],
>  	if (err)
>  		return err;
>  
> -	/* Copy results into MAC addr array */
> -	for (i = 0; i < len && mac_results[i]; i++)
> -		fbnic_tlv_attr_addr_copy(bmc_mac_addr[i], mac_results[i]);
> +	/* Stage the addresses so a bad one leaves the old set untouched.
> +	 * Unused entries stay zeroed.
> +	 */
> +	for (i = 0; i < len && mac_results[i]; i++) {
> +		err = fbnic_tlv_attr_addr_copy(mac_addr[i], mac_results[i]);
> +		if (err)
> +			return err;
> +	}
>  
> -	/* Zero remaining unused addresses */
> -	while (i < len)
> -		eth_zero_addr(bmc_mac_addr[i++]);
> +	memcpy(bmc_mac_addr, mac_addr, len * ETH_ALEN);

[Severity: Medium]
This is a pre-existing issue, but is anything serializing this write
against the reader of fbd->fw_cap.bmc_mac_addr?

This memcpy() runs from the threaded FW mailbox IRQ path and takes no
lock. fbnic_bmc_rpc_init() walks the same array from the service task
while holding only netdev_lock, which the writer never takes.

The TCAM_REINIT bit handshake in fbnic_bmc_rpc_check() orders the flag
against the data. It does not provide mutual exclusion, though.

Suppose a second CAP_RESP is processed while fbnic_bmc_rpc_init() is
partway through its loop. Can the reader then see a mix of old and new
addresses and program a bogus MACDA TCAM entry?

The baseline had the same race, because it wrote the live array one entry
at a time.

This looks resolved later in the series by "fbnic: Keep the BMC MAC
address set whole across a capabilities update". That commit publishes the
array under write_seqcount_begin()/write_seqcount_end(), and
fbnic_bmc_rpc_init() takes a snapshot in a read_seqcount_retry() loop.

>  
>  	return 0;
>  }
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c b/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
> index 3ef3935c0419f..e1ef134ddb944 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_tlv.c
> @@ -724,12 +724,25 @@ struct fbnic_tlv_msg *fbnic_tlv_test_create(struct fbnic_dev *fbd)
>  	return NULL;
>  }
>  
> -void fbnic_tlv_attr_addr_copy(u8 *dest, struct fbnic_tlv_msg *src)

[ ... ]

> +int fbnic_tlv_attr_addr_copy(u8 *dest, struct fbnic_tlv_msg *src)
>  {
> -	u8 *mac_addr;
> +	if (le16_to_cpu(src->hdr.len) != sizeof(*src) + ETH_ALEN)
> +		return -EINVAL;

[Severity: Low]
Should the two callers in fbnic_tlv_parser_test_attr() also handle the
new return value? They still treat the function as void:

fbnic_tlv_parser_test_attr() {
    ...
	attr = results[FBNIC_TLV_TEST_MSG_MAC_ADDR];
	if (attr)
		fbnic_tlv_attr_addr_copy(str->test_mac, attr);
    ...
			for (i = 0; i < 4 && array_results[i]; i++)
				fbnic_tlv_attr_addr_copy(str->test_mac_array[i],
							 array_results[i]);
    ...
}

If a short MAC attribute comes back in the self-test response, the
destination keeps the zeros from the memset() in fbnic_tlv_parser_test().
The failure then shows up only as a generic memcmp() mismatch, not as
-EINVAL.

If the random expected MAC happens to be all zeros, the test would pass
even though the parse failed.

The prototype is not marked __must_check, so the build won't flag these
callers.

> +
> +	memcpy(dest, fbnic_tlv_attr_get_value_ptr(src), ETH_ALEN);

[Severity: High]
This isn't a bug introduced by this patch, but
fbnic_fw_parse_qsfp_read_resp() in fbnic_fw.c seems to have the same
over-read on a short BINARY attribute that this patch fixes for MAC
addresses. Could it get a similar check?

FBNIC_FW_QSFP_DATA is declared with FBNIC_TLV_ATTR_RAW_DATA. As a result,
fbnic_tlv_attr_validate() accepts any payload from 1 up to TLV_MAX_DATA
bytes. The handler only checks length against the host request:

fbnic_fw_parse_qsfp_read_resp() {
    ...
	if (length != cmpl_data->u.qsfp.length ||
	    offset != cmpl_data->u.qsfp.offset) {
    ...
	data = fbnic_tlv_attr_get_value_ptr(data_hdr);
	memcpy(cmpl_data->u.qsfp.data, data, length);
    ...
}

Say firmware reports a length of 128 but sends a truncated data
attribute. Can this read up to 127 bytes past the end of the attribute
and of the message?

The mailbox Rx pages come from __get_free_page(GFP_KERNEL) in
fbnic_mbx_alloc_rx_msgs() and are not zeroed. If the attribute is near
the end of the page, the read could also run into the next page.

fbnic_get_module_eeprom_by_page() then copies those bytes into the
ethtool buffer that goes back to userspace:

	memcpy(page_data->data, fw_cmpl->u.qsfp.data, page_data->length);

The coredump handler already has the equivalent check:

fbnic_fw_parse_coredump_resp() {
    ...
	if (length > le16_to_cpu(data_hdr->hdr.len) - sizeof(u32)) {
    ...
}

RAW_DATA is variable length, so a check in the validator wouldn't cover
this case. The QSFP handler would need its own payload length check.

>  
> -	mac_addr = fbnic_tlv_attr_get_value_ptr(src);
> -	memcpy(dest, mac_addr, ETH_ALEN);
> +	return 0;
>  }

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

Re: [net PATCH 03/13] fbnic: Keep the all-multi MACDA entry out of the address lists

[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 rewritten layout comment in
  drivers/net/ethernet/meta/fbnic/fbnic_rpc.h says the reserved entry…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but a BMC
  multicast address can be silently dropped when the host fills the…
- [Low] This problem wasn't introduced by this patch, but the comments on
  the multicast scan loops in __fbnic_mc_sync…

--

fbnic: Keep the all-multi MACDA entry out of the address lists

The MACDA entry just below mac_addr_boundary is now kept for the BMC
all-multi wildcard. Before this, either the unicast or the multicast list
could take that entry, and the BMC rule then matched a host address. The
unicast scan now starts at mac_addr_boundary, and the multicast sync and
unsync scans start one entry below it.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index dc86b51f35830..0be71e6fd3d4e 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c

[ ... ]

> @@ -507,9 +508,10 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
>  
>  	/* Scan from middle of list to top, filling top down.
>  	 * Skip over the address reserved for the BMC MAC and
> -	 * exclude index 0 as that belongs to the broadcast address
> +	 * exclude index 0 as that belongs to the broadcast address.
> +	 * The entry below the boundary is reserved for all-multi.
>  	 */

[Severity: Low]
This is a pre-existing issue, but the comment says the scan excludes
"index 0 as that belongs to the broadcast address". Broadcast is at
FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX (4), and index 0 is
FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX. The loop stops at:

	--i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX

Since this comment is being edited anyway, could the index be fixed too?

fbnic_mc_unsync() in fbnic_netdev.c has the same text. This patch changes
the start of that loop, but its comment does not get the new reservation
sentence.

> -	for (i = fbd->mac_addr_boundary;
> +	for (i = fbd->mac_addr_boundary - 1;
>  	     --i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX;) {
>  		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[i];
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but the shared multicast
region is now 5..22. If the host fills it, can a BMC multicast address
get dropped?

When the first scan finds no free entry, the second scan in
__fbnic_mc_sync() walks BMC indices 3..1 and only matches entries that
already exist:

	while (--i) {
		...
		/* Only move on if we find a match */
		if (!ether_addr_equal(mac_addr->value.addr8, addr))
			continue;

So __fbnic_mc_sync() returns NULL even when BMC slots 1-3 are free.
fbnic_bmc_rpc_init() then logs the failure and skips the address:

		if (!mac_addr) {
			netdev_err(fbd->netdev,
				   "No slot for BMC MAC address[%d]\n", j);
			continue;
		}

By this point fbnic_bmc_rpc_check() has already cleared
FBNIC_FW_CAP_F_BMC_TCAM_REINIT, and nothing schedules a retry. If the
host addresses are already synced, __fbnic_set_rx_mode() never sees
-ENOSPC, so neither the host nor the BMC falls back to all-multi. With BMC
all-multi off, the BMC address stays unprogrammed until the next firmware
reinit.

This patch lowers the trigger from 19 host multicast entries to 18.

Later in the series, "fbnic: Rebuild the BMC rules from scratch on every
reinit" adds fbnic_bmc_mc_reserved(). That only keeps a copy that is
already in the BMC region, so a new, distinct BMC multicast address would
still fail.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
> index 3d4925b2ac75d..679ba0a1058d9 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.h
> @@ -156,9 +156,9 @@ enum {
>   * always go top-down with unicast, and bottom-up with multicast so that
>   * there should be free-space in the middle between the two.
>   *
> - * The entry at MADCA_DEFAULT_BOUNDARY is a special case as it can be used
> - * for the ALL MULTI address if the list is full, or the BMC has requested
> - * it.
> + * The entry just below the boundary is reserved for the ALL MULTI address,
> + * used if the list is full, or the BMC has requested it. Neither the unicast
> + * nor the multicast list allocates it.
>   */

[Severity: Low]
Is "used if the list is full, or the BMC has requested it" accurate? The
only place that fills this entry is fbnic_bmc_rpc_all_multi_config(), and
it does so only when BMC all-multi is on:

	mac_addr = &fbd->mac_addr[fbd->mac_addr_boundary - 1];
	if (fbnic_bmc_all_multi(fbd)) {

If the host multicast list is full and BMC all-multi is off,
fbnic_promisc_sync() puts the wildcard at PROMISC_IDX instead:

	mac_addr = &fbd->mac_addr[FBNIC_RPC_TCAM_MACDA_PROMISC_IDX];
	...
	} else if (mc_promisc && !fbnic_bmc_all_multi(fbd)) {

That agrees with the comment a few lines further down:

/* Reserved for use to record Multicast promisc, or Promiscuous */
#define FBNIC_RPC_TCAM_MACDA_PROMISC_IDX	31

The host only shares this entry, through FBNIC_MAC_ADDR_T_ALLMULTI, when
the BMC has already asked for all-multi. Otherwise the entry is unused.

Could the comment say "and" instead of "or", so it does not contradict
the PROMISC_IDX comment?

>  #define FBNIC_RPC_TCAM_MACDA_MULTICAST_IDX	5
>  #define FBNIC_RPC_TCAM_MACDA_DEFAULT_BOUNDARY	24

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

Re: [net PATCH 04/13] fbnic: Retry the MACDA sync to firmware if it fails

[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] The patch makes fbnic_bmc_rpc_check()
  (drivers/net/ethernet/meta/fbnic/fbnic_rpc.c:345-356) set…

--

fbnic: Retry the MACDA sync to firmware if it fails

When fbnic_fw_xmit_rpc_macda_sync() fails, fbnic_bmc_rpc_check() now sets
FBNIC_FW_CAP_F_BMC_MACDA_SYNC again, so the next service task pass retries
the MACDA table sync. The failure warning is switched to
dev_warn_ratelimited() because it can now repeat.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index 0be71e6fd3d4e..d69c91066508f 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> @@ -345,9 +345,14 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
>  	if (test_and_clear_bit(FBNIC_FW_CAP_F_BMC_MACDA_SYNC,
>  			       &fbd->fw_cap.state)) {
>  		err = fbnic_fw_xmit_rpc_macda_sync(fbd);
> -		if (err)
> -			dev_warn(fbd->dev,
> -				 "Writing MACDA table to FW failed, err: %d\n", err);
> +		if (err) {
> +			dev_warn_ratelimited(fbd->dev,
> +					     "Writing MACDA table to FW failed, err: %d\n",
> +					     err);
> +			/* Leave FW to be synced on the next pass */
> +			set_bit(FBNIC_FW_CAP_F_BMC_MACDA_SYNC,
> +				&fbd->fw_cap.state);

[Severity: Medium]
Will this retry ever stop if the error is permanent rather than transient?

The bit is set again for any non-zero return, and that includes -ENODEV
after the device goes away. When fbnic_rd32() or fbnic_fw_rd32() sees
all-ones reads, they do:

    WRITE_ONCE(fbd->uc_addr0, NULL);
    WRITE_ONCE(fbd->uc_addr4, NULL);
    ...
    netif_device_detach(fbd->netdev);

This leaves mbx[FBNIC_IPC_MBX_TX_IDX].ready set and netif_running() true.
fbnic_fw_xmit_rpc_macda_sync() only bails out early on:

    if (!fbd->mbx[FBNIC_IPC_MBX_TX_IDX].ready)
        return 0;

So on every call it allocates a page, builds the TLV message, and gets
-ENODEV back from fbnic_mbx_map_tlv_msg()->fbnic_mbx_map_msg():

    if (!mbx->ready || !fbnic_fw_present(fbd))
        return -ENODEV;

It then frees the page.

fbnic_service_task() calls fbnic_bmc_rpc_check() and reschedules itself
with schedule_delayed_work(&fbd->service_task, HZ) while netif_running().
If MACDA_SYNC is pending when the device is lost, this loop runs about once
a second until the interface is downed, the device is removed, or AER
recovery runs.

The same thing happens with -EBUSY if firmware hangs with the TX mailbox
full.

About the commit message, which says:

    rate limit the warning now that it can repeat.

Does the rate limit actually suppress anything here?
dev_warn_ratelimited() uses DEFAULT_RATELIMIT_INTERVAL (5 * HZ) and
DEFAULT_RATELIMIT_BURST (10), which allows 2 messages per second. The
service task fires once per second, so every warning would be printed.

Before this patch, this condition logged one warning. With the patch, it
logs one warning per second for as long as the condition lasts.

For comparison, fbnic_fw_check_heartbeat() warns once, clears
fw_heartbeat_enabled, and stops. Could the retry skip setting the bit on
-ENODEV (or check fbnic_fw_present() first), and/or back off between
attempts?

> +		}
>  	}
>  
>  	netdev_unlock(fbd->netdev);

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

Re: [net PATCH 05/13] fbnic: Don't touch host filters from a service task pass after ndo_stop

[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

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

- [High] The patch is incomplete.

--

fbnic: Don't touch host filters from a service task pass after ndo_stop

This commit moves fbnic_health_check() and fbnic_bmc_rpc_check() in
fbnic_service_task() under the netif_running() check that re-arms the
task. A pass that was blocked on RTNL during ndo_stop should then skip
the firmware recovery and BMC TCAM rebuild. The fix relies on
__dev_close_many() clearing __LINK_STATE_START before it calls ndo_stop.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index ad234bb0f75d0..e7744915d001c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -229,18 +229,23 @@ static void fbnic_service_task(struct work_struct *work)

[ ... ]

> -	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; __fbnic_open() reclaims and rebuilds it.
> +	 */
> +	if (netif_running(netdev)) {
> +		fbnic_health_check(fbd);
> +
> +		fbnic_bmc_rpc_check(fbd);
> +
>  		schedule_delayed_work(&fbd->service_task, HZ);
> +	}

[Severity: High]
Does this gate cover the ndo_stop call in fbnic_pm_suspend()?

That path calls ndo_stop directly and does not go through
__dev_close_many(). Before calling it, it only does
netif_device_detach():

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_pm_suspend() {
    ...
	rtnl_lock();
	netdev_lock(netdev);
    ...
	netif_device_detach(netdev);

	if (netif_running(netdev))
		netdev->netdev_ops->ndo_stop(netdev);
    ...
	netdev_unlock(netdev);
	rtnl_unlock();
    ...
}

netif_device_detach() clears __LINK_STATE_PRESENT and leaves
__LINK_STATE_START set. So netif_running() is still true after
fbnic_pm_suspend() drops RTNL.

fbnic_pm_suspend() is reached from system suspend, fbnic_shutdown() and
fbnic_err_error_detected(). On those paths, can the same sequence the
commit message describes still happen?

CPU1 (fbnic_pm_suspend)              CPU2 (fbnic_service_task)
rtnl_lock()
                                     rtnl_lock()  <- blocks
ndo_stop()
  fbnic_stop()
    fbnic_down()
      fbnic_service_task_stop()
        cancel_delayed_work()
    fbnic_fw_xmit_ownership_msg(fbd, false)
      fw_heartbeat_enabled = false
rtnl_unlock()
                                     netif_running() is still true
                                     fbnic_health_check()
                                       fbnic_fw_config_after_crash()
                                     fbnic_bmc_rpc_check()
                                     schedule_delayed_work()

In that pass, fbnic_health_check() sees fw_heartbeat_enabled == false
with an empty Tx mailbox. It reports a firmware crash through devlink
that did not happen. fbnic_fw_config_after_crash() then reclaims
ownership and writes the host rules back with __fbnic_set_rx_mode()
while the host queues are disabled.

If FBNIC_FW_CAP_F_BMC_TCAM_REINIT is set, fbnic_bmc_rpc_check() also
runs fbnic_bmc_rpc_init(), fbnic_rss_reinit() and __fbnic_set_rx_mode().

The pass also re-arms itself through schedule_delayed_work(). After
fbnic_pm_suspend() drops RTNL, it goes on to call fbnic_fw_free_mbx()
and fbnic_free_irqs() and to set uc_addr0/uc_addr4 to NULL. It never
does a synchronous cancel of service_task. Could the task keep running
every HZ after that teardown, until reboot on shutdown or until
__fbnic_open() on resume?

Would it be more robust to use a driver-owned stop indication? It could
be set under RTNL in fbnic_down() or fbnic_stop() and checked by the
service task, or the work could be drained synchronously, instead of
keying on netif_running().

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

Re: [net PATCH 07/13] fbnic: Let the multicast sync reuse an entry waiting for deletion

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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).

- [High] The patch fixes two defects in __fbnic_mc_sync().
- [Medium] A full-region multicast swap can still drop the device to all-
  multi with room to spare when the removed host address is also tagged…

--

fbnic: Let the multicast sync reuse an entry waiting for deletion

This patch lets __fbnic_mc_sync() treat MACDA entries in the DELETE state
as free. This lets a full multicast region swap one address for another
in a single rx mode pass. It also resets a matched DELETE entry, clearing
its tags, so fbnic_write_macda() no longer wipes the tag the caller has
just set.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index d69c91066508f..ee781a3e15285 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> @@ -520,12 +520,16 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
>  	     --i > FBNIC_RPC_TCAM_MACDA_BROADCAST_IDX;) {
>  		struct fbnic_mac_addr *mac_addr = &fbd->mac_addr[i];
>  
> -		if (mac_addr->state == FBNIC_TCAM_S_DISABLED) {
> -			avail_addr = mac_addr;
> -		} else if (ether_addr_equal(mac_addr->value.addr8, addr)) {
> +		if (mac_addr->state != FBNIC_TCAM_S_DISABLED &&
> +		    ether_addr_equal(mac_addr->value.addr8, addr)) {
>  			avail_addr = mac_addr;
>  			break;
>  		}
> +
> +		/* An entry waiting to be deleted is as good as a free one */
> +		if (mac_addr->state == FBNIC_TCAM_S_DISABLED ||
> +		    mac_addr->state == FBNIC_TCAM_S_DELETE)
> +			avail_addr = mac_addr;
>  	}

[Severity: Medium]
Can a full-region swap still end in all-multi with a slot free, when the
address being removed is also tagged for the BMC?

Take this setup:

- every shared multicast slot is in use
- one of those slots holds address A with both the MULTICAST and BMC tags
- one of the BMC-only slots below it is DISABLED

Now the host leaves A and joins B in one rx mode pass:

__hw_addr_sync_dev()
  fbnic_mc_unsync(A)
    __fbnic_mc_unsync()
      clears the MULTICAST bit. The BMC bit stays set, so
      __fbnic_xc_unsync() leaves the entry VALID, not DELETE.
  fbnic_mc_sync(B)
    __fbnic_mc_sync()
      finds no match and no DISABLED or DELETE slot, and B is not
      in the BMC region, so it returns NULL
    returns -ENOSPC

__fbnic_set_rx_mode() then turns on all-multi:

	err = __hw_addr_sync_dev(mc, netdev, fbnic_mc_sync, fbnic_mc_unsync);
	if (err == -ENOSPC)
		mc_promisc = true;

Later in the same pass, fbnic_sift_macda() moves the now BMC-only A into
the free BMC slot and marks its shared slot DELETE. fbnic_write_macda()
then frees that slot.

Nothing retries the sync of B, so its sync_cnt stays at zero. Wouldn't the
device then stay in all-multi with a free slot until the address lists
next change? That is the symptom the commit message describes as "the
device can sit in all-multi with room to spare until then".

[ ... ]

> @@ -551,9 +555,12 @@ struct fbnic_mac_addr *__fbnic_mc_sync(struct fbnic_dev *fbd,
>  		break;
>  	}
>  
> -	if (avail_addr && avail_addr->state == FBNIC_TCAM_S_DISABLED) {
> +	if (avail_addr && (avail_addr->state == FBNIC_TCAM_S_DISABLED ||
> +			   avail_addr->state == FBNIC_TCAM_S_DELETE)) {
>  		ether_addr_copy(avail_addr->value.addr8, addr);
>  		eth_zero_addr(avail_addr->mask.addr8);
> +		bitmap_zero(avail_addr->act_tcam,
> +			    FBNIC_RPC_TCAM_ACT_NUM_ENTRIES);
>  		avail_addr->state = FBNIC_TCAM_S_ADD;
>  	}

[Severity: High]
Does __fbnic_uc_sync() need the same fix? It has both of the patterns this
patch fixes for multicast, and it is left unchanged:

drivers/net/ethernet/meta/fbnic/fbnic_rpc.c:__fbnic_uc_sync() {
	...
		if (mac_addr->state == FBNIC_TCAM_S_DISABLED) {
			avail_addr = mac_addr;
		} else if (ether_addr_equal(mac_addr->value.addr8, addr)) {
			avail_addr = mac_addr;
			break;
		}
	}

	if (avail_addr && avail_addr->state == FBNIC_TCAM_S_DISABLED) {
	...
}

The first pattern is the swap. Suppose the unicast region is full and one
secondary unicast address (a macvlan, for example) is swapped for another
in a single rx mode pass. That hits the same unsync-before-sync ordering:

- fbnic_uc_unsync() only marks the old entry DELETE
- __fbnic_uc_sync() returns NULL
- fbnic_uc_sync() returns -ENOSPC

In __fbnic_set_rx_mode() that sets uc_promisc, which also forces
mc_promisc:

	if (err == -ENOSPC)
		uc_promisc = true;
	...
	mc_promisc |= !!(netdev->flags & IFF_ALLMULTI) || uc_promisc;

The new address keeps a sync_cnt of zero. Won't the device then stay fully
promiscuous, with a free slot, until the address lists change again?

The second pattern is the matched DELETE entry. The else-if branch can
match a DELETE entry by address and return it still in DELETE. The caller
then sets its tag, and fbnic_write_macda() wipes the entry, tag included:

		if (mac_addr->state == FBNIC_TCAM_S_DELETE) {
			/* Invalidate entry and clear addr state info */
			fbnic_clear_macda_entry(fbd, idx);
			memset(mac_addr, 0, sizeof(*mac_addr));

There seem to be two ways to reach this for unicast.

In the first, the interface is down and an ethtool ETHER_FLOW rule for
unicast DA X is deleted:

- fbnic_clear_nfc_macda() skips fbnic_write_macda() when !netif_running(),
  so the entry stays in DELETE.
- A new rule for X then goes through
  fbnic_set_cls_rule_ins()->__fbnic_uc_sync(), gets the DELETE entry, and
  sets the idx bit.
- On ifup, fbnic_write_macda() wipes the entry.

The act TCAM rule then points at an empty MACDA index. Can that index
later be handed out for a different address, so the rule steers the wrong
traffic?

In the second, host secondary unicast X shares an entry with an NFC rule:

- On ifdown, fbnic_clear_rx_mode() and fbnic_clear_macda() leave the
  entry NFC-only.
- Deleting the NFC rule while the interface is down moves it to DELETE.
- On ifup, fbnic_uc_sync(X) matches the DELETE entry and sets
  FBNIC_MAC_ADDR_T_UNICAST.
- fbnic_write_macda() then wipes the entry.

Wouldn't traffic to X be dropped, while the core counts X as synced and
never retries it?

fbnic_set_cls_rule_ins() picks __fbnic_mc_sync() or __fbnic_uc_sync() by
address type and handles both results the same way. After this patch, only
the multicast branch is sure not to return an entry still in DELETE.

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

Re: [net PATCH 08/13] fbnic: Rebuild the BMC rules from scratch on every reinit

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 (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] In fbnic_bmc_rpc_init()
  (drivers/net/ethernet/meta/fbnic/fbnic_rpc.c), two allocators hand out…
- [Medium] Because of the new teardown, a host multicast sync in the same
  pass can wipe the BMC tag that fbnic_bmc_rpc_init() just restored.
- [Medium] fbnic: BMC multicast slot loss and overwrite in
  fbnic_bmc_mc_reserved()

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch's
  argument that 'an entry rebuilt before the next hardware write is…

--

fbnic: Rebuild the BMC rules from scratch on every reinit

This commit makes fbnic_bmc_rpc_init() first tear down the BMC MACDA and
action rules with fbnic_bmc_rpc_disable() and then rebuild them from the
current firmware capabilities. It drops fbnic_bmc_rules_present() and adds
fbnic_bmc_mc_reserved() as a fallback for BMC multicast entries that
fbnic_sift_macda() moved into the BMC region. It also tags the all-multi
wildcard entry for the BMC on every pass.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index ee781a3e15285..e6b9b9d6511fa 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c

[ ... ]

> @@ -172,6 +175,77 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
>  	act_tcam->state = FBNIC_TCAM_S_UPDATE;
>  }
>  
> +/**
> + * fbnic_bmc_rpc_disable - remove the BMC MAC and action rules
> + * @fbd: Pointer to fbnic device struct
> + *
> + * Strip everything a previous fbnic_bmc_rpc_init() programmed. Idempotent,
> + * and only marks shadow state; entries that are rebuilt before the next
> + * hardware write are rewritten in place rather than cleared and re-added.
> + */
> +static void fbnic_bmc_rpc_disable(struct fbnic_dev *fbd)

[Severity: Medium]
This isn't a bug introduced by this patch, but this comment assumes nothing
writes the MACDA and action TCAMs to hardware between the teardown and the
rebuild. Does that hold for every writer?

fbnic_bmc_rpc_check() uses netdev_lock() to serialize against the rx mode
and ethtool paths. __fbnic_pm_attach() in fbnic_pci.c, however, calls
fbnic_up() without RTNL or the instance lock:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:__fbnic_pm_attach() {
	rtnl_lock();
	fbnic_reset_hw_stats(fbd);
	rtnl_unlock();
	...
	if (netif_running(netdev))
		fbnic_up(fbn);
	...
}

fbnic_up() then goes on to __fbnic_set_rx_mode()->fbnic_write_macda().

The service task can still be alive at this point. On suspend,
fbnic_down()->fbnic_service_task_stop() uses cancel_delayed_work() rather
than the _sync variant. A pass that was blocked on RTNL then continues after
suspend drops it. It still sees netif_running() as true, so it calls
fbnic_bmc_rpc_check() and re-arms itself.

Suppose that pass runs fbnic_bmc_rpc_disable() on resume while the unlocked
fbnic_write_macda() is walking the table. fbnic_write_macda() would see the
transient DELETE state and do:

	if (mac_addr->state == FBNIC_TCAM_S_DELETE) {
		fbnic_clear_macda_entry(fbd, idx);
		memset(mac_addr, 0, sizeof(*mac_addr));
		continue;
	}

Could this wipe a BMC MACDA entry, or BMC rule 0, that fbnic_bmc_rpc_init()
is about to rebuild? The BMC would then have no Rx path until the next
REINIT.

The unlocked fbnic_up() and the non-sync cancel are older than this patch,
and later patches in the series don't change __fbnic_pm_attach(). This patch
adds one more transient shadow state that the race can expose.

[ ... ]

> @@ -179,6 +253,11 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
>  	struct fbnic_mac_addr *mac_addr;
>  	int j;
>  
> +	/* Start from nothing so a MAC the BMC no longer reports does not
> +	 * keep its tag, then rebuild from what firmware last sent.
> +	 */
> +	fbnic_bmc_rpc_disable(fbd);

[Severity: Medium]
Can this teardown let a host multicast sync later in the same open strip the
BMC tag that this function just restored?

Take a BMC-only multicast address M that fbnic_sift_macda() moved into BMC
slot 1, with a free shared slot S and M pending in the host multicast list.
At open, M is always pending because fbnic_clear_rx_mode() calls
__dev_mc_unsync(netdev, NULL) at ifdown. The sequence is:

__fbnic_open()
  fbnic_bmc_rpc_init()
    fbnic_bmc_rpc_disable()
      slot 1: BMC bit cleared, state DELETE, value M kept
    __fbnic_mc_sync(M)
      memcpy() slot 1 into S, slot 1 left in DELETE
    set_bit(FBNIC_MAC_ADDR_T_BMC, ...) on S
  fbnic_rss_reinit()
fbnic_up()
  __fbnic_set_rx_mode()
    fbnic_mc_sync(M)
      __fbnic_mc_sync(M)
        for loop matches S and breaks
        while (--i) loop matches the stale slot 1 copy
      set_bit(FBNIC_MAC_ADDR_T_MULTICAST, avail_addr->act_tcam)

No fbnic_write_macda() runs in between to clear slot 1. The second
__fbnic_mc_sync() therefore repeats the copy:

		if (avail_addr) {
			memcpy(avail_addr, mac_addr, sizeof(*mac_addr));
			mac_addr->state = FBNIC_TCAM_S_DELETE;
			avail_addr->state = FBNIC_TCAM_S_ADD;
		}

S now has slot 1's act_tcam bitmap, which the teardown emptied. Before this
patch slot 1 still had the BMC bit here, so the second copy kept the tag.

With only FBNIC_MAC_ADDR_T_MULTICAST left on S, __fbnic_write_tce_tcam()
would stop looping host Tx for M to the BMC. The firmware MACDA sync would
also stop marking M as a BMC address.

When the host later leaves M, __fbnic_xc_unsync() empties the bitmap and
deletes the entry. The BMC would then lose Rx for M until the next REINIT.

The same thing can happen if M was added while the interface was down, or
just before a service task REINIT. This also looks to be still present at
the end of the series.

> +
>  	/* Check if BMC is present */
>  	if (!fbnic_bmc_present(fbd))
>  		return;
> @@ -191,10 +270,16 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
>  		if (is_zero_ether_addr(bmc_mac))
>  			continue;
>  
> -		if (is_multicast_ether_addr(bmc_mac))
> +		if (is_multicast_ether_addr(bmc_mac)) {
>  			mac_addr = __fbnic_mc_sync(fbd, bmc_mac);
> -		else
> +			/* With the shared region full, keep the copy the
> +			 * teardown left in the BMC region in place.
> +			 */
> +			if (!mac_addr)
> +				mac_addr = fbnic_bmc_mc_reserved(fbd, bmc_mac);
> +		} else {
>  			mac_addr = &fbd->mac_addr[i++];
> +		}
>  
>  		if (!mac_addr) {
>  			netdev_err(fbd->netdev,

[Severity: Medium]
Does this fallback handle the case where the BMC changes or adds a
multicast address while the shared region is full?

fbnic_bmc_mc_reserved() only returns an entry in slots 1..3 whose value
already equals addr, and it skips DISABLED entries. Suppose
fbnic_sift_macda() earlier moved a BMC-only multicast address M1 into
slot 1, the host has filled every shared multicast slot, and the BMC
now reports M2 in place of M1:

fbnic_bmc_rpc_init()
  fbnic_bmc_rpc_disable()
    slot 1: BMC bit cleared, state DELETE, value M1 kept
  __fbnic_mc_sync(M2)
    no DISABLED or DELETE entry in the shared region, returns NULL
  fbnic_bmc_mc_reserved(M2)
    slot 1 holds M1, slots 2 and 3 are DISABLED, returns NULL
  logs "No slot for BMC MAC address"

The next fbnic_write_macda() then clears slot 1. The BMC is left with
no MACDA entry for either address, even though slots 1..3 are unused.

Because M2 never gets an entry, fbnic_sift_macda() has nothing to move
into the BMC region either. Three things stay missing until another
REINIT happens to run with a shared slot free:

  - Rx for M2 to the BMC
  - the TCE loopback of host Tx for M2
  - the firmware MACDA sync entry for M2

Dropping M2 here is not new. However, the commit message says an
address that changed "can take a freed slot, now that the multicast
sync reuses entries waiting for deletion". In this case the freed slot
is in the BMC region, which __fbnic_mc_sync() never hands out. So that
claim does not seem to hold once the address had been sifted.

Letting the fallback claim a free slot in 1..3 would also need to be
coordinated with the unicast branch. That branch takes slots by
position via fbd->mac_addr[i++] and does not check what this helper
has returned.

As it stands, take mcA sifted into slot 1:

  - With the order [uc0, mcA, uc1], the fallback hands back slot 1
    and uc1 then overwrites it.
  - With [uc0, uc1, mcA], slot 1 is overwritten before the lookup,
    and mcA is dropped even though slots 2 and 3 are free.

Would it work to place the unicast addresses first? The multicast
fallback could then take any slot in 1..3 that the unicast addresses
did not claim and that is DISABLED, in DELETE, or already holds the
address. Or is there a reason to keep the BMC region out of the
allocation for new multicast addresses?

[Severity: Medium]
Can the unicast branch overwrite the slot that fbnic_bmc_mc_reserved() just
handed back?

fbnic_bmc_mc_reserved() searches BMC slots 1..3, and the unicast addresses
take those same slots by position:

			mac_addr = &fbd->mac_addr[i++];

Neither path checks what the other has already taken in this pass.

Suppose the BMC previously reported one unicast address (slot 0) and a
multicast address mcA. fbnic_sift_macda() moved mcA into slot 1, and host
entries fill the shared multicast region. The BMC then reports two unicast
addresses plus mcA, in the order [mcA, uc0, uc1]:

  __fbnic_mc_sync(mcA) returns NULL: no shared entry is free or in DELETE,
  and the while (--i) loop finds mcA in slot 1 but breaks because avail_addr
  is NULL
  fbnic_bmc_mc_reserved(mcA) returns slot 1, which gets tagged
  uc0 takes slot 0
  uc1 takes slot 1 via i++ and overwrites mcA's value and mask

mcA then disappears from the MACDA and TCE tables, and nothing is logged.

With the order [uc0, uc1, mcA], slot 1 is overwritten first. Both lookups
then miss, and mcA is dropped with "No slot for BMC MAC address" even though
slots 2 and 3 are free.

The commit message says:

    One that the fbnic_sift_macda
    function moved into the BMC region is only pulled back out by the
    __fbnic_mc_sync function when a shared slot is free, so with the shared
    region full it is taken back in place by the new fbnic_bmc_mc_reserved
    function instead of being lost.

Doesn't that fail whenever the BMC reports more unicast addresses than
before?

Once the entry and its BMC tag are lost, __fbnic_write_tce_tcam() stops
looping host Tx for mcA to the BMC, and the firmware MACDA sync no longer
lists mcA. If the shared region is exactly full, BMC Rx for mcA is lost as
well. This lasts until a later REINIT succeeds.

Would it help to place the unicast addresses first? Another option is for
the multicast fallback to skip, or move out of, the slots the unicast
addresses need. This also looks to be still present at the end of the
series.

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

Re: [net PATCH 10/13] fbnic: Keep the BMC MAC address set whole across a capabilities update

[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] fbnic_mbx_init() is a second, unsequenced writer of
  fw_cap.bmc_mac_addr, and it also resets the live seqcount.

--

fbnic: Keep the BMC MAC address set whole across a capabilities update

This patch puts a seqcount around fw_cap.bmc_mac_addr. With it,
fbnic_bmc_rpc_init() can take a consistent snapshot while the mailbox
Rx path replaces the BMC MAC address set. The writer parses the new set
into a local copy and swaps it in inside a write section, with
preemption disabled.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 7964ce814b8ea..09d03274be08c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -104,6 +104,7 @@ void fbnic_mbx_init(struct fbnic_dev *fbd)
>  
>  	/* Reset FW Capabilities */
>  	memset(&fbd->fw_cap, 0, sizeof(fbd->fw_cap));
> +	seqcount_init(&fbd->fw_cap.bmc_mac_seq);

[Severity: Medium]
Doesn't this make fbnic_mbx_init() a second writer of bmc_mac_addr?

The commit message and the new comments in struct fbnic_fw_cap and
fbnic_fw_parse_bmc_cap() say there is only one writer:

    Updates are rare and the mailbox Rx path is the only writer, so a
    seqcount is enough and the readers never wait on a lock.

However, the memset() here clears bmc_mac_addr outside any write
section. seqcount_init() then sets the live counter back to 0 instead
of making it odd.

fbnic_mbx_init() is reached on system resume and PCI slot_reset with
only devl_lock held:

__fbnic_pm_resume()
  fbnic_fw_request_mbx()
    __fbnic_fw_enable_mbx()
      fbnic_mbx_init()

The fbnic_bmc_rpc_init() reader in the service task holds RTNL and the
netdev instance lock. Nothing makes these two paths exclude each other.

Can a service task pass still be running at that point?
fbnic_pm_suspend() calls ndo_stop directly, so netif_running() stays
true. fbnic_service_task_stop() also does not wait for a running pass:

fbnic_service_task_stop() {
	...
	cancel_delayed_work(&fbd->service_task);
}

Take a pass that is already blocked on rtnl_lock() during suspend. It
runs once suspend drops RTNL, passes this check in
fbnic_service_task(), and re-arms itself every HZ into resume:

	if (netif_running(netdev)) {
		fbnic_health_check(fbd);

		fbnic_bmc_rpc_check(fbd);

		schedule_delayed_work(&fbd->service_task, HZ);
	}

If that pass is inside the new snapshot loop in fbnic_bmc_rpc_init(),
it looks like it can accept a torn copy in two ways.

First, bmc_mac_seq sits just before bmc_mac_addr in struct fbnic_fw_cap,
so the forward memset() clears the counter before the addresses. A
reader that samples 0 during the memset() can copy a half-cleared
address that is not all zeros. read_seqcount_retry() still sees 0, so
the copy is accepted.

Second, a reader samples seq == 2 and is preempted partway through the
24-byte copy. 2 is the usual value after one caps response since the
last init. The reset sets the counter to 0, and the next caps response
parsed via fbnic_mbx_poll_tx_ready() takes it 0 -> 1 -> 2. The reader
then copies the rest from the new set, and read_seqcount_retry() sees
2 == 2.

Either way, the torn address is tagged FBNIC_MAC_ADDR_T_BMC. The same
pass's __fbnic_set_rx_mode() then writes it to the MACDA TCAM, which is
the failure the commit message describes.

__fbnic_pm_resume() later rebuilds through __fbnic_open() ->
fbnic_bmc_rpc_init(), so the bad entry should normally be short-lived.

Would it work to init the seqcount once at probe and do the
bmc_mac_addr reset inside a write section? Another option would be to
quiesce the service task with cancel_delayed_work_sync() before the
mailbox is reinitialized.

[ ... ]

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

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

[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] The PM suspend path does not protect against a service-task
  pass that runs late.

--

fbnic: Suspend phylink after taking the datapath down in ndo_stop

This patch reorders fbnic_stop() so that fbnic_down() runs before
fbnic_mac_free_irq() and phylink_suspend(). Tx then drains while the link
is still up, and the shutdown order becomes the reverse of the open path.
The commit message says the PM suspend path picks up the same order
because it goes through ndo_stop.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> index a40566029a863..36ef17dec0f4a 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);

[Severity: Medium]

The commit message says:

  The PM suspend path goes through ndo_stop and picks up the same order.

Does the PM path also get the service task protection that this ordering
relies on after ndo_stop?

fbnic_service_task() uses netif_running() to decide whether it may touch
host state:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_service_task() {
    ...
	/* 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; __fbnic_open() reclaims and rebuilds it.
	 */
	if (netif_running(netdev)) {
		fbnic_health_check(fbd);

		fbnic_bmc_rpc_check(fbd);
    ...
}

That works for __dev_close_many(), which clears __LINK_STATE_START before
it calls ndo_stop. fbnic_pm_suspend() calls ndo_stop directly instead:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_pm_suspend() {
    ...
	netif_device_detach(netdev);

	if (netif_running(netdev))
		netdev->netdev_ops->ndo_stop(netdev);
    ...
}

netif_device_detach() only clears __LINK_STATE_PRESENT, so netif_running()
is still true after this. Also, fbnic_stop()->fbnic_down()->
fbnic_down_noidle()->fbnic_service_task_stop() uses cancel_delayed_work().
That call does not wait for a pass that is already blocked in rtnl_lock().

Suppose a pass is waiting on RTNL during the PM ndo_stop. When
fbnic_pm_suspend() drops RTNL, that pass sees netif_running() == true.

At that point fbnic_health_check() runs right after
fbnic_fw_xmit_ownership_msg(fbd, false) has cleared fw_heartbeat_enabled.
If the Tx mailbox has drained, can it report "Firmware crash detected!"?
After that, fbnic_fw_config_after_crash() would reclaim ownership and write
the host MACDA/action rules through __fbnic_set_rx_mode() while the queues
are disabled.

fbnic_fw_present() only checks uc_addr4, and uc_addr4 stays set until the
end of fbnic_pm_suspend(). The work can also run at the same time as
fbnic_fw_free_mbx(), which is serialized by devl_lock and not RTNL.

fbnic_bmc_rpc_check() can reprogram the BMC/host rules in the same way.

The pass then calls schedule_delayed_work(&fbd->service_task, HZ). Would
the service task then keep re-arming every HZ across suspend, after the
mailbox and IRQs are freed? If so, it would repeat the false crash report
and the ownership reclaim until resume calls ndo_open.

The window is not trivially small, because ndo_stop includes
fbnic_wait_all_queues_idle(). The effect looks transient: on resume,
__fbnic_open() reclaims ownership and rebuilds the rules.

The netif_running() gate was added by the earlier series commit "fbnic:
Don't touch host filters from a service task pass after ndo_stop". It only
covers the __dev_close_many() caller, and the code is unchanged at the end
of the series.

Should fbnic_pm_suspend() keep a late service task pass out in a way that
does not depend on __LINK_STATE_START?

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

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

[netdev-bot+sashiko]

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

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

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] The commit makes the new flush-done poll in fbnic_mbx_clean()
  (drivers/net/ethernet/meta/fbnic/fbnic_fw.c:204-209) wait for the…
- [Low] The commit message says fbnic_mbx_reset_desc_ring 'drops bus
  mastering and arms the outbound read and write flush in a single…

--

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

This patch adds a poll on the PUL link reset status register at the end
of fbnic_mbx_clean(). The poll waits for the outbound read and write
flushes armed during mailbox teardown to report done before returning.
It has a 1ms timeout and logs a warning if the flush does not complete.

> The fbnic_mbx_reset_desc_ring function drops bus mastering and arms the
> outbound read and write flush in a single register write, but nothing
> waits for that flush to finish.

[Severity: Low]
Is this description accurate? In fbnic_mbx_reset_desc_ring(), two
separate writes to two different registers arm the write flush and the
read flush. mbx_idx selects which one runs:

    case FBNIC_IPC_MBX_RX_IDX:
        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_MODE);
        break;
    case FBNIC_IPC_MBX_TX_IDX:
        wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AR_CFG,
             FBNIC_PUL_OB_TLP_HDR_AR_CFG_FLUSH);
        break;

fbnic_mbx_clean() calls it once per ring, and the RX buffers are freed
in between. So the AW flush is armed before the AR flush, and no single
write arms both directions.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 09d03274be08c..32a18cb8990cb 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c

[ ... ]

> @@ -187,10 +188,25 @@ static void fbnic_mbx_clean_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
>  
>  void fbnic_mbx_clean(struct fbnic_dev *fbd)
>  {
> +	u32 flush_done = FBNIC_PUL_USER_IP_LINK_RST_STATUS_AR_FLUSH_DONE |
> +			 FBNIC_PUL_USER_IP_LINK_RST_STATUS_AW_FLUSH_DONE;
> +	u32 val;
>  	int i;
>  
>  	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
>  		fbnic_mbx_clean_desc_ring(fbd, i);
> +
> +	/* Arming the flush above only starts it. Wait for the outbound path
> +	 * to retire the requests it still owns so none of them can complete
> +	 * after the reset that follows. A lost link reads back as all ones,
> +	 * which ends the poll on the first pass.
> +	 */
> +	if (read_poll_timeout_atomic(fbnic_rd32, val,
> +				     (val & flush_done) == 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);
>  }

[Severity: Medium]
Does this wait come too late to protect the TX mailbox pages?

By the time the poll runs, fbnic_mbx_clean_desc_ring() has already
unmapped and freed every buffer on both rings:

fbnic_mbx_clean()
  fbnic_mbx_clean_desc_ring(fbd, FBNIC_IPC_MBX_TX_IDX)
    fbnic_mbx_reset_desc_ring()
      wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AR_CFG, ..._AR_CFG_FLUSH);
    fbnic_mbx_unmap_and_free_msg()
      dma_unmap_single(...);
      free_page(...);
  read_poll_timeout_atomic(...)

The new comment says arming the flush "only starts it" and that the
outbound path still owns requests at that point. If so, could a device
read of a TX page still be outstanding after dma_unmap_single() and
free_page() have run? With an IOMMU, it could also target an IOVA that
has already been unmapped or reused.

TX pages can still be on the ring at clean time if firmware has not set
FW_CMPL. One case is fbnic_mbx_flush_tx() timing out in
fbnic_fw_disable_mbx(). Another is the clean_mbx error path in
fbnic_mbx_poll_tx_ready() after the cap request has been posted.

The RX side sets FLUSH_MODE so parked writes do not land on the host.
The AR side has no equivalent before the pages are freed.

Would it be better to poll for the flush to complete in
fbnic_mbx_clean_desc_ring(), between fbnic_mbx_reset_desc_ring() and
the unmap/free loop?

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

Re: [net PATCH 13/13] 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 Fixes: tag names the wrong commit.

--

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. Timestamped frames are then dropped while their
PTP completions are still forwarded 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().

> Fixes: 40bf06a160a1 ("eth: fbnic: Implement Tx queue alloc/start/stop/free")

[Severity: Low]
Is this the right commit for the Fixes tag?

40bf06a160a1 added fbnic_tx_flush(), fbnic_tx_flush_off() and
fbnic_wait_all_queues_idle(). Its commit message says it has "No actual
datapath / packet handling here, yet". At that point the driver never
asked the hardware for Tx timestamp completions.

The symptom in the commit message comes from fbnic_clean_tsq() in
fbnic_txrx.c:

	if (head == ring->tail) {
		if (unlikely(net_ratelimit()))
			netdev_err(nv->napi.dev,
				   "Tx timestamp without matching packet\n");
		return;
	}

git blame shows that this code was added later by:

  ad3d9f8bc66c ("eth: fbnic: add TX packets timestamping support")

That commit also added the request for Tx timestamp completions.

So the leaked completion can't happen on a tree that has 40bf06a160a1 but
not ad3d9f8bc66c. ad3d9f8bc66c is not in v6.12, but v6.12.y already has
the flush code.

Stable tooling that relies on the current tag could pick this patch for
6.12.y, where the problem can't occur.

Should the tag name ad3d9f8bc66c instead, or include it as a second
Fixes tag?

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