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