[next-next PATCH v2 0/2] eth: fbnic: Update BMC MAC address handling

[next-next PATCH v2 0/2] eth: fbnic: Update BMC MAC address handling

[Alexander Duyck]

Update the handling of BMC MAC addresses on fbnic, and in the process also
address several issues identified with the changes. This patch series
consists of 2 patches.

The first patch addresses issues found in the bmc_present and
bmc_tcam_reinit handling. It started out addressing the fact that present
w/o a MAC address was flagged as an invalid message. However, we ended up
also addressing a potential race in that setting bmc_present and
need_bmc_tcam_reinit could potentially race against the bmc_rpc_check. To
address that the bit flags were moved to a state bitmap and are now handled
via the test_and_clear_bit and assign_bit operations.

The second patch cleans up the BMC MAC addresses in the TCAMs if the BMC
opts to disable the channel and the FW notifies us of the BMC removal. By
doing this we can terminate the now defunct paths through our NIC data path
to the BMC. Similarly with this a potential race was found due to recent
changes that had moved the handling for the TCAMs out of the RTNL mutex and
into the netdev instance lock. To maintain sync additional locking was
added.

---

Alexander Duyck (2):
      fbnic: Rework the BMC state pulled from the capabilities message
      fbnic: Remove BMC routing rules when the BMC channel is disabled


 drivers/net/ethernet/meta/fbnic/fbnic.h     |   7 +-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c  |  78 ++++++++-----
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h  |  19 ++-
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c | 121 ++++++++++++++++++--
 4 files changed, 179 insertions(+), 46 deletions(-)

--

[next-next PATCH v2 1/2] fbnic: Rework the BMC state pulled from the capabilities message

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The BMC side of the RPC configuration is driven entirely out of the
capabilities message: it carries the BMC presence, its MAC addresses and
the all-multi request, and the reinit flag raised off it is what gets the
service task to reprogram the MACDA and action TCAMs. Pull that out of
fbnic_fw_parse_cap_resp() into a helper and rework the state it publishes.

The firmware reports the BMC present as soon as its NC-SI channel is
enabled, before the BMC has been assigned a MAC address. Such a message
carries no BMC MAC array and is rejected with -EINVAL, discarding the rest
of it. Treat a present BMC with no MAC as absent instead. There is no entry
to steer traffic to without one, so the BMC paths stay disabled either way,
and the link speed, FEC and anti-rollback version in the same message are
no longer thrown out with it.

bmc_present, need_bmc_tcam_reinit, need_bmc_macda_sync and all_multi are
single bit members of one bitfield, so every write to one is a
read-modify-write of all four. The mailbox writes three of them and the
service task writes the other two. Move them into an unsigned long state
field driven by the atomic bitops. Putting it at the top of the struct
keeps the cost to the word itself: 152 bytes grows to 160, still three
cachelines, with a 1 byte hole left ahead of anti_rollback_version.

Order the reinit handoff while there. The mailbox publishes the BMC state
before raising the flag and the service task tests the flag before
consuming it, so pair an smp_mb__before_atomic() on the raise with a
test_and_clear_bit() on the consume. Claiming the flag atomically also
keeps a raise that lands mid-pass instead of dropping it.

Commit all_multi unconditionally rather than only when the firmware
supplied the attribute. An absent attribute means the BMC is not asking for
all-multi, so holding the previous value just kept stale state. It is read
out of the message only when a BMC is present, so it is never set on its
own and the three callers that paired it with the presence check can test
it alone.

fbnic_write_macda() tests the reinit flag to force a MACDA sync and now
always sees it clear when reached from fbnic_bmc_rpc_check(). That path is
unaffected, as fbnic_bmc_rpc_init() unconditionally marks the broadcast
entry, so the update count is never zero while a BMC is present.

Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic.h     |    7 ++
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c  |   78 +++++++++++++++++----------
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h  |   15 ++++-
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c |   24 +++++---
 4 files changed, 80 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic.h b/drivers/net/ethernet/meta/fbnic/fbnic.h
index 0e7ae1def5bf..1f9fbd71447f 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic.h
@@ -161,7 +161,12 @@ void fbnic_fw_wr32(struct fbnic_dev *fbd, u32 reg, u32 val);
 
 static inline bool fbnic_bmc_present(struct fbnic_dev *fbd)
 {
-	return fbd->fw_cap.bmc_present;
+	return test_bit(FBNIC_FW_CAP_F_BMC_PRESENT, &fbd->fw_cap.state);
+}
+
+static inline bool fbnic_bmc_all_multi(struct fbnic_dev *fbd)
+{
+	return test_bit(FBNIC_FW_CAP_F_BMC_ALL_MULTI, &fbd->fw_cap.state);
 }
 
 static inline bool fbnic_init_failure(struct fbnic_dev *fbd)
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 47894b45676b..49b8813a3564 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -620,11 +620,55 @@ static int fbnic_fw_parse_bmc_addrs(u8 bmc_mac_addr[][ETH_ALEN],
 	return 0;
 }
 
+static int fbnic_fw_parse_bmc_cap(struct fbnic_dev *fbd,
+				  struct fbnic_tlv_msg **results)
+{
+	struct fbnic_tlv_msg *attr;
+	bool bmc_present = false;
+	u32 all_multi = 0;
+	int err;
+
+	/* FW reports the BMC present once its NC-SI channel is enabled, before
+	 * it has a MAC. No MAC array means nothing to program, so treat it as
+	 * absent rather than rejecting the message.
+	 */
+	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);
+		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));
+	}
+
+	/* 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.
+	 */
+	assign_bit(FBNIC_FW_CAP_F_BMC_PRESENT, &fbd->fw_cap.state, bmc_present);
+	assign_bit(FBNIC_FW_CAP_F_BMC_ALL_MULTI, &fbd->fw_cap.state, all_multi);
+
+	/* Always assume we need a BMC reinit. The barrier orders the BMC state
+	 * published above ahead of the flag, pairing with the ordering implied
+	 * by the test_and_clear_bit() in fbnic_bmc_rpc_check().
+	 */
+	smp_mb__before_atomic();
+	set_bit(FBNIC_FW_CAP_F_BMC_TCAM_REINIT, &fbd->fw_cap.state);
+
+	return 0;
+}
+
 static int fbnic_fw_parse_cap_resp(void *opaque, struct fbnic_tlv_msg **results)
 {
-	u32 all_multi = 0, version = 0;
 	struct fbnic_dev *fbd = opaque;
-	bool bmc_present;
+	u32 version = 0;
 	int err;
 
 	version = fta_get_uint(results, FBNIC_FW_CAP_RESP_VERSION);
@@ -684,37 +728,13 @@ static int fbnic_fw_parse_cap_resp(void *opaque, struct fbnic_tlv_msg **results)
 	fbd->fw_cap.link_fec =
 		fta_get_uint(results, FBNIC_FW_CAP_RESP_FW_LINK_FEC);
 
-	bmc_present = !!results[FBNIC_FW_CAP_RESP_BMC_PRESENT];
-	if (bmc_present) {
-		struct fbnic_tlv_msg *attr;
-
-		attr = results[FBNIC_FW_CAP_RESP_BMC_MAC_ARRAY];
-		if (!attr)
-			return -EINVAL;
-
-		err = fbnic_fw_parse_bmc_addrs(fbd->fw_cap.bmc_mac_addr,
-					       attr, 4);
-		if (err)
-			return err;
-
-		all_multi =
-			fta_get_uint(results, FBNIC_FW_CAP_RESP_BMC_ALL_MULTI);
-	} else {
-		memset(fbd->fw_cap.bmc_mac_addr, 0,
-		       sizeof(fbd->fw_cap.bmc_mac_addr));
-	}
-
-	fbd->fw_cap.bmc_present = bmc_present;
-
-	if (results[FBNIC_FW_CAP_RESP_BMC_ALL_MULTI] || !bmc_present)
-		fbd->fw_cap.all_multi = all_multi;
+	err = fbnic_fw_parse_bmc_cap(fbd, results);
+	if (err)
+		return err;
 
 	fbd->fw_cap.anti_rollback_version =
 		fta_get_uint(results, FBNIC_FW_CAP_RESP_ANTI_ROLLBACK_VERSION);
 
-	/* Always assume we need a BMC reinit */
-	fbd->fw_cap.need_bmc_tcam_reinit = true;
-
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index 5f9969247e30..e542b17bdac7 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -45,7 +45,18 @@ struct fbnic_fw_ver {
 	char commit[FBNIC_FW_CAP_RESP_COMMIT_MAX_SIZE];
 };
 
+/* Bits in fbnic_fw_cap.state. The firmware mailbox publishes the BMC state
+ * and the service task consumes it, so they are updated with atomic bitops.
+ */
+enum {
+	FBNIC_FW_CAP_F_BMC_PRESENT,
+	FBNIC_FW_CAP_F_BMC_ALL_MULTI,
+	FBNIC_FW_CAP_F_BMC_TCAM_REINIT,
+	FBNIC_FW_CAP_F_BMC_MACDA_SYNC,
+};
+
 struct fbnic_fw_cap {
+	unsigned long state;
 	struct {
 		struct fbnic_fw_ver mgmt, bootloader;
 	} running;
@@ -54,10 +65,6 @@ struct fbnic_fw_cap {
 	} stored;
 	u8	active_slot;
 	u8	bmc_mac_addr[4][ETH_ALEN];
-	u8	bmc_present		: 1;
-	u8	need_bmc_tcam_reinit	: 1;
-	u8	need_bmc_macda_sync	: 1;
-	u8	all_multi		: 1;
 	u8	link_speed;
 	u8	link_fec;
 	u32	anti_rollback_version;
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index ab6e342ec3fa..51fd0564d33f 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -117,7 +117,7 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
 	 * BMC.
 	 */
 	mac_addr = &fbd->mac_addr[fbd->mac_addr_boundary - 1];
-	if (fbnic_bmc_present(fbd) && fbd->fw_cap.all_multi) {
+	if (fbnic_bmc_all_multi(fbd)) {
 		if (mac_addr->state != FBNIC_TCAM_S_VALID) {
 			eth_zero_addr(mac_addr->value.addr8);
 			eth_broadcast_addr(mac_addr->mask.addr8);
@@ -144,7 +144,7 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
 	/* If we are not enabling the rule just delete it. We will fall
 	 * back to the RSS rules that support the multicast addresses.
 	 */
-	if (!fbnic_bmc_present(fbd) || !fbd->fw_cap.all_multi || enable_host) {
+	if (!fbnic_bmc_all_multi(fbd) || enable_host) {
 		if (act_tcam->state == FBNIC_TCAM_S_VALID)
 			act_tcam->state = FBNIC_TCAM_S_DELETE;
 		return;
@@ -238,20 +238,25 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
 {
 	int err;
 
-	if (fbd->fw_cap.need_bmc_tcam_reinit) {
+	/* Consume the flag before the state it advertises. The ordering
+	 * implied by test_and_clear_bit() pairs with the barrier in
+	 * fbnic_fw_parse_bmc_cap(), and claiming it atomically means a set
+	 * racing with us is kept and retried rather than overwritten.
+	 */
+	if (test_and_clear_bit(FBNIC_FW_CAP_F_BMC_TCAM_REINIT,
+			       &fbd->fw_cap.state)) {
 		fbnic_bmc_rpc_init(fbd);
 		netif_addr_lock_bh(fbd->netdev);
 		__fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
 		netif_addr_unlock_bh(fbd->netdev);
-		fbd->fw_cap.need_bmc_tcam_reinit = false;
 	}
 
-	if (fbd->fw_cap.need_bmc_macda_sync) {
+	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);
-		fbd->fw_cap.need_bmc_macda_sync = false;
 	}
 }
 
@@ -484,8 +489,7 @@ void fbnic_promisc_sync(struct fbnic_dev *fbd,
 				mac_addr->act_tcam);
 			mac_addr->state = FBNIC_TCAM_S_ADD;
 		}
-	} else if (mc_promisc &&
-		   (!fbnic_bmc_present(fbd) || !fbd->fw_cap.all_multi)) {
+	} else if (mc_promisc && !fbnic_bmc_all_multi(fbd)) {
 		/* We have to add a special handler for multicast as the
 		 * BMC may have an all-multi rule already in place. As such
 		 * adding a rule ourselves won't do any good so we will have
@@ -656,12 +660,12 @@ void fbnic_write_macda(struct fbnic_dev *fbd)
 	}
 
 	/* If reinitializing the BMC TCAM we are doing an initial update */
-	if (fbd->fw_cap.need_bmc_tcam_reinit)
+	if (test_bit(FBNIC_FW_CAP_F_BMC_TCAM_REINIT, &fbd->fw_cap.state))
 		updates++;
 
 	/* If needed notify firmware of changes to MACDA TCAM */
 	if (updates != 0 && fbnic_bmc_present(fbd))
-		fbd->fw_cap.need_bmc_macda_sync = true;
+		set_bit(FBNIC_FW_CAP_F_BMC_MACDA_SYNC, &fbd->fw_cap.state);
 }
 
 static void fbnic_clear_act_tcam(struct fbnic_dev *fbd, unsigned int idx)

Re: [next-next PATCH v2 1/2] fbnic: Rework the BMC state pulled from the capabilities message

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 (7) · Low: 2 (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] In this patch, a present-with-MAC to present-without-MAC
  capabilities message clears FBNIC_FW_CAP_F_BMC_PRESENT and raises…
- [Medium] fbnic_fw_parse_bmc_cap() now clears FBNIC_FW_CAP_F_BMC_PRESENT
  whenever firmware reports BMC_PRESENT without a MAC array.
- [Low] The commit is titled as a rework, but it fixes at least two real
  bugs and has no Fixes: tags.
- [Low] The commit message says that with the old -EINVAL, "the link
  speed, FEC and anti-rollback version in the same message are no longer…

Pre-existing issues:
- [Medium] When a BMC becomes present while the interface is up, the RSS
  mc/bc action entries are not rebuilt with DEST_BMC, because…
- [Medium] The disable branch in fbnic_bmc_rpc_all_multi_config() moves
  act_tcam[FBNIC_RPC_ACT_TBL_BMC_ALL_MULTI_OFFSET] to DELETE only when…
- [Medium] fbnic_bmc_rpc_all_multi_config() treats
  mac_addr[mac_addr_boundary - 1] being VALID as proof that it already…
- [Medium] In fbnic_sift_macda(), the inner while loop does not break
  after it moves a qualifying BMC-only entry into dest_addr.
- [Medium] fbnic_bmc_rpc_check() consumes FBNIC_FW_CAP_F_BMC_MACDA_SYNC
  before it calls fbnic_fw_xmit_rpc_macda_sync(), and does not raise the…
- [Medium] fbnic_bmc_rpc_check() runs from the service task under RTNL
  only.
- [Medium] FBNIC_FW_CAP_RESP_BMC_MAC_ADDR is validated as
  FBNIC_TLV_BINARY, which accepts any payload length from 1 to ETH_ALEN…
- [Low] The mailbox parser can overwrite fbd->fw_cap.bmc_mac_addr for
  response B while the service task is still validating and copying it…

--

fbnic: Rework the BMC state pulled from the capabilities message

This moves the BMC parsing out of fbnic_fw_parse_cap_resp() into a new
fbnic_fw_parse_bmc_cap() helper. A BMC reported present without a MAC
array is now treated as absent instead of rejecting the whole message.

The BMC bitfield members are replaced by atomic bitops on a new
fw_cap.state word. The reinit handoff is ordered with
smp_mb__before_atomic() and test_and_clear_bit(), and all_multi is now
always taken from the message.

> The firmware reports the BMC present as soon as its NC-SI channel is
> enabled, before the BMC has been assigned a MAC address. Such a message
> carries no BMC MAC array and is rejected with -EINVAL, discarding the rest
> of it. Treat a present BMC with no MAC as absent instead. There is no entry
> to steer traffic to without one, so the BMC paths stay disabled either way,
> and the link speed, FEC and anti-rollback version in the same message are
> no longer thrown out with it.

[Severity: Low]
Is the part about link speed and FEC accurate? The baseline
fbnic_fw_parse_cap_resp() stores both of them before it reaches the BMC
block that returned -EINVAL:

	fbd->fw_cap.link_speed =
		fta_get_uint(results, FBNIC_FW_CAP_RESP_FW_LINK_SPEED);
	fbd->fw_cap.link_fec =
		fta_get_uint(results, FBNIC_FW_CAP_RESP_FW_LINK_FEC);

	bmc_present = !!results[FBNIC_FW_CAP_RESP_BMC_PRESENT];
	if (bmc_present) {
		...
		if (!attr)
			return -EINVAL;

The only caller, fbnic_mbx_process_rx_msgs(), logs "Unable to process
message" and does not undo fields that were already written.

The old code actually lost anti_rollback_version and the
need_bmc_tcam_reinit raise. It also left bmc_present, bmc_mac_addr and
all_multi at their previous values.

Could the commit message describe that impact instead?

> bmc_present, need_bmc_tcam_reinit, need_bmc_macda_sync and all_multi are
> single bit members of one bitfield, so every write to one is a
> read-modify-write of all four. The mailbox writes three of them and the
> service task writes the other two.

[ ... ]

> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>

[Severity: Low]
The subject calls this a rework, but it seems to fix at least two real
bugs. Should it have Fixes: tags?

The -EINVAL rejection of a BMC_PRESENT message without a BMC_MAC_ARRAY
comes from:

Fixes: 20d2e88cc746 ("eth: fbnic: Add initial messaging to notify FW of our presence")

The shared u8 bitfield comes from:

Fixes: 04a230b27d8f ("fbnic: Add logic to repopulate RPC TCAM if BMC enables channel")
Fixes: cee8d21d8091 ("fbnic: Push local unicast MAC addresses to FW to populate TCAMs")

Three contexts write that bitfield: the mailbox threaded IRQ, the service
task, and the rx-mode path through fbnic_write_macda(). Each write is a
non-atomic read-modify-write of the whole byte, so updates can be lost.

These fixes are also mixed in with the helper extraction and the
all_multi semantics change. Would splitting them out make a stable
backport easier?

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> index 47894b45676b..49b8813a3564 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> @@ -620,11 +620,55 @@ static int fbnic_fw_parse_bmc_addrs(u8 bmc_mac_addr[][ETH_ALEN],
>  	return 0;
>  }
>  
> +static int fbnic_fw_parse_bmc_cap(struct fbnic_dev *fbd,
> +				  struct fbnic_tlv_msg **results)
> +{
> +	struct fbnic_tlv_msg *attr;
> +	bool bmc_present = false;
> +	u32 all_multi = 0;
> +	int err;
> +
> +	/* FW reports the BMC present once its NC-SI channel is enabled, before
> +	 * it has a MAC. No MAC array means nothing to program, so treat it as
> +	 * absent rather than rejecting the message.
> +	 */
> +	attr = results[FBNIC_FW_CAP_RESP_BMC_PRESENT] ?
> +	       results[FBNIC_FW_CAP_RESP_BMC_MAC_ARRAY] : NULL;

[Severity: Medium]
Does treating present-without-MAC as absent affect more than MAC
steering? fbnic_bmc_present() also decides what happens to the link and
the RX parser on ifdown:

fbnic_stop()
	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));

fbnic_rss_disable_hw()
	if (!fbnic_bmc_present(fbd))
		wr32(fbd, FBNIC_RPC_RMI_CONFIG,
		     FIELD_PREP(FBNIC_RPC_RMI_CONFIG_OH_BYTES, 20));

fbnic_clear_rules() also uses it to decide whether to keep the last
BMC-routing action rule.

Before this patch, a present-without-MAC message that followed a
present-with-MAC message was rejected. That could happen during firmware
recovery, or when the BMC clears its MAC filters but keeps the channel
enabled. bmc_present stayed set, so link and RPC stayed up on ifdown.

If the host now does an ifdown in that window, can the link and the RX
parser be shut off while the NC-SI channel is still active? The service
task is not rescheduled while the interface is down. A later message that
restores the MAC would not bring the link back until the host brings the
interface up again.

The commit message only says "the BMC paths stay disabled either way" and
does not mention these link and RPC effects. Can firmware send
present-without-MAC after present-with-MAC with no absent message in
between?

> +	if (attr) {
> +		err = fbnic_fw_parse_bmc_addrs(fbd->fw_cap.bmc_mac_addr,
> +					       attr, 4);

[Severity: Medium]
This isn't a bug introduced by this patch, but can a short MAC attribute
cause an out-of-bounds read here? FBNIC_FW_CAP_RESP_BMC_MAC_ADDR is
validated as FBNIC_TLV_BINARY, which accepts any length from 1 to
ETH_ALEN:

fbnic_tlv_attr_validate()
	case FBNIC_TLV_BINARY:
		if (!len || len > tlv_index->len)
			return -EINVAL;

However, fbnic_tlv_attr_addr_copy() always copies ETH_ALEN bytes:

	mac_addr = fbnic_tlv_attr_get_value_ptr(src);
	memcpy(dest, mac_addr, ETH_ALEN);

If a truncated MAC attribute sits at the end of a firmware message, the
copy reads past the attribute, and possibly past the receive page, into
bmc_mac_addr[]. Those bytes are then programmed into the TCAM.

[Severity: Low]
This is a pre-existing issue, but what stops this write to
fbd->fw_cap.bmc_mac_addr from racing with the reader in
fbnic_bmc_rpc_init()?

fbnic_bmc_rpc_init()
	for (j = 0; j < 4; j++) {
		u8 *bmc_mac = fbd->fw_cap.bmc_mac_addr[j];
		...
		ether_addr_copy(mac_addr->value.addr8, bmc_mac);

The mailbox can parse response B while the service task is still copying
the addresses from response A. One pass could then program a MAC made of
bytes from both responses.

The new ordering does place B's set_bit() after B's writes, so another
reinit pass follows. The snapshot taken in that first pass is still not
coherent.

> +		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));
> +	}
> +
> +	/* 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.
> +	 */
> +	assign_bit(FBNIC_FW_CAP_F_BMC_PRESENT, &fbd->fw_cap.state, bmc_present);

[Severity: Medium]
What happens to the BMC rules already in hardware when a
present-with-MAC message is followed by a present-without-MAC one? This
clears FBNIC_FW_CAP_F_BMC_PRESENT and raises TCAM_REINIT, but
fbnic_bmc_rpc_init() then returns early:

fbnic_bmc_rpc_check()
    fbnic_bmc_rpc_init()
	if (!fbnic_bmc_present(fbd))
		return;

Nothing removes the BMC-tagged MACDA entries, action rule 0, or the
DEST_BMC bit on the RSS multicast and broadcast entries. Does hardware
keep steering traffic to a BMC that the driver now considers absent,
until the interface is bounced?

The next patch in the series, "fbnic: Remove BMC routing rules when the
BMC channel is disabled", adds fbnic_bmc_rpc_disable() and calls
fbnic_rss_reinit() from fbnic_bmc_rpc_check(). That resolves this by the
end of the series.

> +	assign_bit(FBNIC_FW_CAP_F_BMC_ALL_MULTI, &fbd->fw_cap.state, all_multi);

[ ... ]

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index ab6e342ec3fa..51fd0564d33f 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> @@ -117,7 +117,7 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
>  	 * BMC.
>  	 */
>  	mac_addr = &fbd->mac_addr[fbd->mac_addr_boundary - 1];
> -	if (fbnic_bmc_present(fbd) && fbd->fw_cap.all_multi) {
> +	if (fbnic_bmc_all_multi(fbd)) {
>  		if (mac_addr->state != FBNIC_TCAM_S_VALID) {

[Severity: Medium]
This isn't a bug introduced by this patch, but does a VALID state prove
that mac_addr[mac_addr_boundary - 1] holds the multicast wildcard? The
value, mask and owner are not checked. __fbnic_uc_sync() can also give
that index to a host unicast address once the higher entries are full:

__fbnic_uc_sync()
	for (i = fbd->mac_addr_boundary - 1;
	     i < FBNIC_RPC_TCAM_MACDA_HOST_ADDR_IDX; i++) {

In that case the wildcard is never installed, and the BMC all-multi
action rule (DEST_BMC) matches the MACDA index of the host's unicast
address. Wouldn't that address's traffic then go only to the BMC?

This needs BMC all-multi on, host all-multi off, and enough extra host
unicast addresses to fill the entries above the boundary.

>  			eth_zero_addr(mac_addr->value.addr8);
>  			eth_broadcast_addr(mac_addr->mask.addr8);
> @@ -144,7 +144,7 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
>  	/* If we are not enabling the rule just delete it. We will fall
>  	 * back to the RSS rules that support the multicast addresses.
>  	 */
> -	if (!fbnic_bmc_present(fbd) || !fbd->fw_cap.all_multi || enable_host) {
> +	if (!fbnic_bmc_all_multi(fbd) || enable_host) {
>  		if (act_tcam->state == FBNIC_TCAM_S_VALID)
>  			act_tcam->state = FBNIC_TCAM_S_DELETE;

[Severity: Medium]
This is a pre-existing issue, but does this check skip entries in the
UPDATE state? After ifdown, fbnic_clear_valid_act_tcam() leaves
act_tcam[FBNIC_RPC_ACT_TBL_BMC_ALL_MULTI_OFFSET] in UPDATE. It is then
not moved to DELETE here, and fbnic_write_rules() writes it back as a
BMC-only rule even when host all-multi is requested. The BMC-only rule
would then take over host all-multicast delivery.

The next patch in the series, "fbnic: Remove BMC routing rules when the
BMC channel is disabled", changes this test to
act_tcam->state != FBNIC_TCAM_S_DISABLED.

>  		return;
> @@ -238,20 +238,25 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
>  {
>  	int err;
>  
> -	if (fbd->fw_cap.need_bmc_tcam_reinit) {
> +	/* Consume the flag before the state it advertises. The ordering
> +	 * implied by test_and_clear_bit() pairs with the barrier in
> +	 * fbnic_fw_parse_bmc_cap(), and claiming it atomically means a set
> +	 * racing with us is kept and retried rather than overwritten.
> +	 */
> +	if (test_and_clear_bit(FBNIC_FW_CAP_F_BMC_TCAM_REINIT,
> +			       &fbd->fw_cap.state)) {
>  		fbnic_bmc_rpc_init(fbd);

[Severity: Medium]
This isn't a bug introduced by this patch, but when a BMC becomes present
while the interface is up, do the RSS multicast and broadcast action
entries ever get rebuilt with DEST_BMC? That bit is only computed in
fbnic_rss_reinit():

	if (i >= FBNIC_RSS_EN_NUM_UNICAST && fbnic_bmc_present(fbd))
		dest |= FIELD_PREP(FBNIC_RPC_ACT_TBL0_DEST_MASK,
				   FBNIC_RPC_ACT_TBL0_DEST_BMC);

This reinit path does not call fbnic_rss_reinit(), so the BMC would miss
broadcast and shared multicast traffic until the interface is bounced.

This has been the case since 04a230b27d8f ("fbnic: Add logic to
repopulate RPC TCAM if BMC enables channel"). The next patch in the
series, "fbnic: Remove BMC routing rules when the BMC channel is
disabled", adds fbnic_rss_reinit(fbd, fbn) to this path.

[Severity: Medium]
This is also pre-existing, but what serializes this path against the
async rx-mode worker? fbnic_bmc_rpc_check() runs from the service task
holding only RTNL. It modifies fbd->mac_addr[] and fbd->act_tcam[] here,
then walks the MACDA table in fbnic_fw_xmit_rpc_macda_sync().

For ops-locked devices, the async rx-mode callback modifies the same
arrays while holding only the netdev instance lock:

netdev_work_proc()
	netdev_lock_ops_compat(dev);
	...
	netdev_work_run(dev, events, core);

The two lock sets do not overlap. Can both sides update the TCAM shadow
state at the same time and send firmware an inconsistent snapshot?

The next patch in the series, "fbnic: Remove BMC routing rules when the
BMC channel is disabled", adds netdev_lock()/netdev_unlock() around
fbnic_bmc_rpc_check().

>  		netif_addr_lock_bh(fbd->netdev);
>  		__fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);

[Severity: Medium]
This isn't a bug introduced by this patch, but __fbnic_set_rx_mode()
calls fbnic_sift_macda(), and the inner loop there keeps going after it
moves an entry:

fbnic_sift_macda()
		while (src < fbd->mac_addr_boundary) {
			struct fbnic_mac_addr *src_addr = &fbd->mac_addr[src++];
			...
			memcpy(dest_addr, src_addr, sizeof(*src_addr));
			src_addr->state = FBNIC_TCAM_S_DELETE;
			dest_addr->state = FBNIC_TCAM_S_ADD;
		}

With two or more BMC-only multicast addresses, won't the second matching
source be copied over the same dest_addr, with both sources marked
DELETE? The first BMC-only multicast filter would then be lost. Is a
break missing after the move?

>  		netif_addr_unlock_bh(fbd->netdev);
> -		fbd->fw_cap.need_bmc_tcam_reinit = false;
>  	}
>  
> -	if (fbd->fw_cap.need_bmc_macda_sync) {
> +	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);

[Severity: Medium]
This is a pre-existing issue, since the old code also cleared
need_bmc_macda_sync unconditionally. Should the flag be set again when
fbnic_fw_xmit_rpc_macda_sync() fails?

A transient -ENOMEM or -ENOSPC, or -EBUSY from a full mailbox, leaves
firmware with a stale MACDA list and stale receive-mode flags. That lasts
until some other change sets FBNIC_FW_CAP_F_BMC_MACDA_SYNC again.

> -		fbd->fw_cap.need_bmc_macda_sync = false;
>  	}
>  }

[ ... ]

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

Re: [next-next PATCH v2 1/2] fbnic: Rework the BMC state pulled from the capabilities message

[Alexander Duyck]

On Wed, Sep 30, 2026 at 3:12 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 2 (7) · Low: 2 (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] In this patch, a present-with-MAC to present-without-MAC
>   capabilities message clears FBNIC_FW_CAP_F_BMC_PRESENT and raises…
> - [Medium] fbnic_fw_parse_bmc_cap() now clears FBNIC_FW_CAP_F_BMC_PRESENT
>   whenever firmware reports BMC_PRESENT without a MAC array.
> - [Low] The commit is titled as a rework, but it fixes at least two real
>   bugs and has no Fixes: tags.
> - [Low] The commit message says that with the old -EINVAL, "the link
>   speed, FEC and anti-rollback version in the same message are no longer…
>
> Pre-existing issues:
> - [Medium] When a BMC becomes present while the interface is up, the RSS
>   mc/bc action entries are not rebuilt with DEST_BMC, because…
> - [Medium] The disable branch in fbnic_bmc_rpc_all_multi_config() moves
>   act_tcam[FBNIC_RPC_ACT_TBL_BMC_ALL_MULTI_OFFSET] to DELETE only when…
> - [Medium] fbnic_bmc_rpc_all_multi_config() treats
>   mac_addr[mac_addr_boundary - 1] being VALID as proof that it already…
> - [Medium] In fbnic_sift_macda(), the inner while loop does not break
>   after it moves a qualifying BMC-only entry into dest_addr.
> - [Medium] fbnic_bmc_rpc_check() consumes FBNIC_FW_CAP_F_BMC_MACDA_SYNC
>   before it calls fbnic_fw_xmit_rpc_macda_sync(), and does not raise the…
> - [Medium] fbnic_bmc_rpc_check() runs from the service task under RTNL
>   only.
> - [Medium] FBNIC_FW_CAP_RESP_BMC_MAC_ADDR is validated as
>   FBNIC_TLV_BINARY, which accepts any payload length from 1 to ETH_ALEN…
> - [Low] The mailbox parser can overwrite fbd->fw_cap.bmc_mac_addr for
>   response B while the service task is still validating and copying it…
>
> --
>
> fbnic: Rework the BMC state pulled from the capabilities message
>
> This moves the BMC parsing out of fbnic_fw_parse_cap_resp() into a new
> fbnic_fw_parse_bmc_cap() helper. A BMC reported present without a MAC
> array is now treated as absent instead of rejecting the whole message.
>
> The BMC bitfield members are replaced by atomic bitops on a new
> fw_cap.state word. The reinit handoff is ordered with
> smp_mb__before_atomic() and test_and_clear_bit(), and all_multi is now
> always taken from the message.
>
> > The firmware reports the BMC present as soon as its NC-SI channel is
> > enabled, before the BMC has been assigned a MAC address. Such a message
> > carries no BMC MAC array and is rejected with -EINVAL, discarding the rest
> > of it. Treat a present BMC with no MAC as absent instead. There is no entry
> > to steer traffic to without one, so the BMC paths stay disabled either way,
> > and the link speed, FEC and anti-rollback version in the same message are
> > no longer thrown out with it.
>
> [Severity: Low]
> Is the part about link speed and FEC accurate? The baseline
> fbnic_fw_parse_cap_resp() stores both of them before it reaches the BMC
> block that returned -EINVAL:
>
>         fbd->fw_cap.link_speed =
>                 fta_get_uint(results, FBNIC_FW_CAP_RESP_FW_LINK_SPEED);
>         fbd->fw_cap.link_fec =
>                 fta_get_uint(results, FBNIC_FW_CAP_RESP_FW_LINK_FEC);
>
>         bmc_present = !!results[FBNIC_FW_CAP_RESP_BMC_PRESENT];
>         if (bmc_present) {
>                 ...
>                 if (!attr)
>                         return -EINVAL;
>
> The only caller, fbnic_mbx_process_rx_msgs(), logs "Unable to process
> message" and does not undo fields that were already written.
>
> The old code actually lost anti_rollback_version and the
> need_bmc_tcam_reinit raise. It also left bmc_present, bmc_mac_addr and
> all_multi at their previous values.
>
> Could the commit message describe that impact instead?

This reads like a bit of a word soup to me. I assume this is asking
for an update to the commit message or is this asking for additional
changes so that we force all the error returns to be processed before
we start storing the values.

To me it sounds like we may need to do some additional follow-up work.

> > bmc_present, need_bmc_tcam_reinit, need_bmc_macda_sync and all_multi are
> > single bit members of one bitfield, so every write to one is a
> > read-modify-write of all four. The mailbox writes three of them and the
> > service task writes the other two.
>
> [ ... ]
>
> > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
>
> [Severity: Low]
> The subject calls this a rework, but it seems to fix at least two real
> bugs. Should it have Fixes: tags?
>
> The -EINVAL rejection of a BMC_PRESENT message without a BMC_MAC_ARRAY
> comes from:
>
> Fixes: 20d2e88cc746 ("eth: fbnic: Add initial messaging to notify FW of our presence")
>
> The shared u8 bitfield comes from:
>
> Fixes: 04a230b27d8f ("fbnic: Add logic to repopulate RPC TCAM if BMC enables channel")
> Fixes: cee8d21d8091 ("fbnic: Push local unicast MAC addresses to FW to populate TCAMs")
>
> Three contexts write that bitfield: the mailbox threaded IRQ, the service
> task, and the rx-mode path through fbnic_write_macda(). Each write is a
> non-atomic read-modify-write of the whole byte, so updates can be lost.
>
> These fixes are also mixed in with the helper extraction and the
> all_multi semantics change. Would splitting them out make a stable
> backport easier?

I considered it, however none of these rise to the level of being
something that really needs a backport. The FW sends these messages
quite seldomly and they are almost always at the start of init. The
code itself is mostly there for exception handling for a BMC that
decides to jump channels which is a very rare event so this likely
doesn't jump to the point of needing a "fixes" tag.

> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> > index 47894b45676b..49b8813a3564 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> > @@ -620,11 +620,55 @@ static int fbnic_fw_parse_bmc_addrs(u8 bmc_mac_addr[][ETH_ALEN],
> >       return 0;
> >  }
> >
> > +static int fbnic_fw_parse_bmc_cap(struct fbnic_dev *fbd,
> > +                               struct fbnic_tlv_msg **results)
> > +{
> > +     struct fbnic_tlv_msg *attr;
> > +     bool bmc_present = false;
> > +     u32 all_multi = 0;
> > +     int err;
> > +
> > +     /* FW reports the BMC present once its NC-SI channel is enabled, before
> > +      * it has a MAC. No MAC array means nothing to program, so treat it as
> > +      * absent rather than rejecting the message.
> > +      */
> > +     attr = results[FBNIC_FW_CAP_RESP_BMC_PRESENT] ?
> > +            results[FBNIC_FW_CAP_RESP_BMC_MAC_ARRAY] : NULL;
>
> [Severity: Medium]
> Does treating present-without-MAC as absent affect more than MAC
> steering? fbnic_bmc_present() also decides what happens to the link and
> the RX parser on ifdown:
>
> fbnic_stop()
>         phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
>
> fbnic_rss_disable_hw()
>         if (!fbnic_bmc_present(fbd))
>                 wr32(fbd, FBNIC_RPC_RMI_CONFIG,
>                      FIELD_PREP(FBNIC_RPC_RMI_CONFIG_OH_BYTES, 20));
>
> fbnic_clear_rules() also uses it to decide whether to keep the last
> BMC-routing action rule.
>
> Before this patch, a present-without-MAC message that followed a
> present-with-MAC message was rejected. That could happen during firmware
> recovery, or when the BMC clears its MAC filters but keeps the channel
> enabled. bmc_present stayed set, so link and RPC stayed up on ifdown.
>
> If the host now does an ifdown in that window, can the link and the RX
> parser be shut off while the NC-SI channel is still active? The service
> task is not rescheduled while the interface is down. A later message that
> restores the MAC would not bring the link back until the host brings the
> interface up again.
>
> The commit message only says "the BMC paths stay disabled either way" and
> does not mention these link and RPC effects. Can firmware send
> present-without-MAC after present-with-MAC with no absent message in
> between?
>
> > +     if (attr) {
> > +             err = fbnic_fw_parse_bmc_addrs(fbd->fw_cap.bmc_mac_addr,
> > +                                            attr, 4);
>

If the link is brought down by the host driver, then the driver sends
a message to the FW letting it know it is releasing ownership. Even if
the host driver crashes the heartbeat will timeout and the FW will
seize ownership. One side effect of that is the FW then becomes
responsible for managing the RPC and link so this becomes moot.

> [Severity: Medium]
> This isn't a bug introduced by this patch, but can a short MAC attribute
> cause an out-of-bounds read here? FBNIC_FW_CAP_RESP_BMC_MAC_ADDR is
> validated as FBNIC_TLV_BINARY, which accepts any length from 1 to
> ETH_ALEN:
>
> fbnic_tlv_attr_validate()
>         case FBNIC_TLV_BINARY:
>                 if (!len || len > tlv_index->len)
>                         return -EINVAL;
>
> However, fbnic_tlv_attr_addr_copy() always copies ETH_ALEN bytes:
>
>         mac_addr = fbnic_tlv_attr_get_value_ptr(src);
>         memcpy(dest, mac_addr, ETH_ALEN);
>
> If a truncated MAC attribute sits at the end of a firmware message, the
> copy reads past the attribute, and possibly past the receive page, into
> bmc_mac_addr[]. Those bytes are then programmed into the TCAM.
>

I suppose it might be possible. We can probably look at adding an
additional verification to enforce the length in a follow-on patch.

> [Severity: Low]
> This is a pre-existing issue, but what stops this write to
> fbd->fw_cap.bmc_mac_addr from racing with the reader in
> fbnic_bmc_rpc_init()?
>
> fbnic_bmc_rpc_init()
>         for (j = 0; j < 4; j++) {
>                 u8 *bmc_mac = fbd->fw_cap.bmc_mac_addr[j];
>                 ...
>                 ether_addr_copy(mac_addr->value.addr8, bmc_mac);
>
> The mailbox can parse response B while the service task is still copying
> the addresses from response A. One pass could then program a MAC made of
> bytes from both responses.
>
> The new ordering does place B's set_bit() after B's writes, so another
> reinit pass follows. The snapshot taken in that first pass is still not
> coherent.

In general these events should not be firing that quickly. The BMC
changing a channel and triggering this sort of event should be a rare
event so we shouldn't have two updates coming in at the same time. In
addition normally we would go from channel disabled, to channel
enabled and then MAC being set. As such we would go from !bmc_present
to bmc_present only after the MAC address has been assigned.

> > +             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));
> > +     }
> > +
> > +     /* 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.
> > +      */
> > +     assign_bit(FBNIC_FW_CAP_F_BMC_PRESENT, &fbd->fw_cap.state, bmc_present);
>
> [Severity: Medium]
> What happens to the BMC rules already in hardware when a
> present-with-MAC message is followed by a present-without-MAC one? This
> clears FBNIC_FW_CAP_F_BMC_PRESENT and raises TCAM_REINIT, but
> fbnic_bmc_rpc_init() then returns early:
>
> fbnic_bmc_rpc_check()
>     fbnic_bmc_rpc_init()
>         if (!fbnic_bmc_present(fbd))
>                 return;
>
> Nothing removes the BMC-tagged MACDA entries, action rule 0, or the
> DEST_BMC bit on the RSS multicast and broadcast entries. Does hardware
> keep steering traffic to a BMC that the driver now considers absent,
> until the interface is bounced?
>
> The next patch in the series, "fbnic: Remove BMC routing rules when the
> BMC channel is disabled", adds fbnic_bmc_rpc_disable() and calls
> fbnic_rss_reinit() from fbnic_bmc_rpc_check(). That resolves this by the
> end of the series.

Yes, that is the point of the second patch. The first one didn't
remove the MAC rules it was only handling adding them. We needed to
add the logic to clear them because the rules were being left behind
and triggered a warning when the interfaces was stopped for BMC rules
being there but no BMC present.

> > +     assign_bit(FBNIC_FW_CAP_F_BMC_ALL_MULTI, &fbd->fw_cap.state, all_multi);
>
> [ ... ]
>
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > index ab6e342ec3fa..51fd0564d33f 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > @@ -117,7 +117,7 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
> >        * BMC.
> >        */
> >       mac_addr = &fbd->mac_addr[fbd->mac_addr_boundary - 1];
> > -     if (fbnic_bmc_present(fbd) && fbd->fw_cap.all_multi) {
> > +     if (fbnic_bmc_all_multi(fbd)) {
> >               if (mac_addr->state != FBNIC_TCAM_S_VALID) {
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but does a VALID state prove
> that mac_addr[mac_addr_boundary - 1] holds the multicast wildcard? The
> value, mask and owner are not checked. __fbnic_uc_sync() can also give
> that index to a host unicast address once the higher entries are full:
>
> __fbnic_uc_sync()
>         for (i = fbd->mac_addr_boundary - 1;
>              i < FBNIC_RPC_TCAM_MACDA_HOST_ADDR_IDX; i++) {
>
> In that case the wildcard is never installed, and the BMC all-multi
> action rule (DEST_BMC) matches the MACDA index of the host's unicast
> address. Wouldn't that address's traffic then go only to the BMC?
>
> This needs BMC all-multi on, host all-multi off, and enough extra host
> unicast addresses to fill the entries above the boundary.
>

The VALID flag means the rule was written to the HW and is in use.

I will have to recheck the logic on this. The "- 1" may be incorrect
as I believe this logic is supposed to start at the mac_addr_boundary,
not before. Likely material for yet another follow-on patch.

> >                       eth_zero_addr(mac_addr->value.addr8);
> >                       eth_broadcast_addr(mac_addr->mask.addr8);
> > @@ -144,7 +144,7 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
> >       /* If we are not enabling the rule just delete it. We will fall
> >        * back to the RSS rules that support the multicast addresses.
> >        */
> > -     if (!fbnic_bmc_present(fbd) || !fbd->fw_cap.all_multi || enable_host) {
> > +     if (!fbnic_bmc_all_multi(fbd) || enable_host) {
> >               if (act_tcam->state == FBNIC_TCAM_S_VALID)
> >                       act_tcam->state = FBNIC_TCAM_S_DELETE;
>
> [Severity: Medium]
> This is a pre-existing issue, but does this check skip entries in the
> UPDATE state? After ifdown, fbnic_clear_valid_act_tcam() leaves
> act_tcam[FBNIC_RPC_ACT_TBL_BMC_ALL_MULTI_OFFSET] in UPDATE. It is then
> not moved to DELETE here, and fbnic_write_rules() writes it back as a
> BMC-only rule even when host all-multi is requested. The BMC-only rule
> would then take over host all-multicast delivery.
>
> The next patch in the series, "fbnic: Remove BMC routing rules when the
> BMC channel is disabled", changes this test to
> act_tcam->state != FBNIC_TCAM_S_DISABLED.

This is pointing out something fixed in the next diff. Not sure why it
felt the need to call this out here.

>
> >               return;
> > @@ -238,20 +238,25 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
> >  {
> >       int err;
> >
> > -     if (fbd->fw_cap.need_bmc_tcam_reinit) {
> > +     /* Consume the flag before the state it advertises. The ordering
> > +      * implied by test_and_clear_bit() pairs with the barrier in
> > +      * fbnic_fw_parse_bmc_cap(), and claiming it atomically means a set
> > +      * racing with us is kept and retried rather than overwritten.
> > +      */
> > +     if (test_and_clear_bit(FBNIC_FW_CAP_F_BMC_TCAM_REINIT,
> > +                            &fbd->fw_cap.state)) {
> >               fbnic_bmc_rpc_init(fbd);
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but when a BMC becomes present
> while the interface is up, do the RSS multicast and broadcast action
> entries ever get rebuilt with DEST_BMC? That bit is only computed in
> fbnic_rss_reinit():
>
>         if (i >= FBNIC_RSS_EN_NUM_UNICAST && fbnic_bmc_present(fbd))
>                 dest |= FIELD_PREP(FBNIC_RPC_ACT_TBL0_DEST_MASK,
>                                    FBNIC_RPC_ACT_TBL0_DEST_BMC);
>
> This reinit path does not call fbnic_rss_reinit(), so the BMC would miss
> broadcast and shared multicast traffic until the interface is bounced.
>
> This has been the case since 04a230b27d8f ("fbnic: Add logic to
> repopulate RPC TCAM if BMC enables channel"). The next patch in the
> series, "fbnic: Remove BMC routing rules when the BMC channel is
> disabled", adds fbnic_rss_reinit(fbd, fbn) to this path.

Again, why is it calling this out if it already acknowledged it gets
fixed in the next patch?

> [Severity: Medium]
> This is also pre-existing, but what serializes this path against the
> async rx-mode worker? fbnic_bmc_rpc_check() runs from the service task
> holding only RTNL. It modifies fbd->mac_addr[] and fbd->act_tcam[] here,
> then walks the MACDA table in fbnic_fw_xmit_rpc_macda_sync().
>
> For ops-locked devices, the async rx-mode callback modifies the same
> arrays while holding only the netdev instance lock:
>
> netdev_work_proc()
>         netdev_lock_ops_compat(dev);
>         ...
>         netdev_work_run(dev, events, core);
>
> The two lock sets do not overlap. Can both sides update the TCAM shadow
> state at the same time and send firmware an inconsistent snapshot?
>
> The next patch in the series, "fbnic: Remove BMC routing rules when the
> BMC channel is disabled", adds netdev_lock()/netdev_unlock() around
> fbnic_bmc_rpc_check().
>
> >               netif_addr_lock_bh(fbd->netdev);
> >               __fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);

Again, fixed in next patch.

> [Severity: Medium]
> This isn't a bug introduced by this patch, but __fbnic_set_rx_mode()
> calls fbnic_sift_macda(), and the inner loop there keeps going after it
> moves an entry:
>
> fbnic_sift_macda()
>                 while (src < fbd->mac_addr_boundary) {
>                         struct fbnic_mac_addr *src_addr = &fbd->mac_addr[src++];
>                         ...
>                         memcpy(dest_addr, src_addr, sizeof(*src_addr));
>                         src_addr->state = FBNIC_TCAM_S_DELETE;
>                         dest_addr->state = FBNIC_TCAM_S_ADD;
>                 }
>
> With two or more BMC-only multicast addresses, won't the second matching
> source be copied over the same dest_addr, with both sources marked
> DELETE? The first BMC-only multicast filter would then be lost. Is a
> break missing after the move?

I will have to look into that. If it is an issue it is material for a
follow-on patch.

> >               netif_addr_unlock_bh(fbd->netdev);
> > -             fbd->fw_cap.need_bmc_tcam_reinit = false;
> >       }
> >
> > -     if (fbd->fw_cap.need_bmc_macda_sync) {
> > +     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);
>
> [Severity: Medium]
> This is a pre-existing issue, since the old code also cleared
> need_bmc_macda_sync unconditionally. Should the flag be set again when
> fbnic_fw_xmit_rpc_macda_sync() fails?
>
> A transient -ENOMEM or -ENOSPC, or -EBUSY from a full mailbox, leaves
> firmware with a stale MACDA list and stale receive-mode flags. That lasts
> until some other change sets FBNIC_FW_CAP_F_BMC_MACDA_SYNC again.
>

This would likely lead to us just hammering the mailbox resulting in a
stream of errors rather than the error presenting as a warning and
likely getting resolved after the issues with the mailbox get
resolved. Resolving the mailbox issues would likely require a resync
of things with the FW anyway as dropping messages would result in a
loss of state sync between the FW and the host.

> > -             fbd->fw_cap.need_bmc_macda_sync = false;
> >       }
> >  }
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179061185153.414394.13396934246780284806.stgit%40ahduyck-xeon-server.home.arpa

[next-next PATCH v2 2/2] fbnic: Remove BMC routing rules when the BMC channel is disabled

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

While a BMC is present the driver programs MACDA entries for the BMC
addresses, action rules steering matching traffic to it, the DEST_BMC copy
bit on the multicast/broadcast RSS actions, and a host-unicast copy of the
RSS actions. All of these are only torn down when the host interface goes
down.

A BMC can disable its NC-SI channel while the host interface stays up. The
filters are then left in place, so the host keeps steering traffic to a BMC
that is no longer there, and the driver logs "Found BMC MAC address w/ BMC
not present" at the next interface down.

Detect that bmc_present dropped while BMC tagged rules are still programmed
in fbnic_bmc_rpc_check(), and remove the BMC MAC entries, the action rule
and the host-unicast RSS entries. That needs three supporting changes:

1. Action rules must be marked for deletion in any live state rather than
   only VALID, as a missed deletion leaks the rule for good. The same
   applies to the BMC all-multi rule, which ifdown leaves in UPDATE.
2. The host-unicast entries have to go because fbnic_rss_reinit() only
   programs them while a BMC is present and would otherwise leave them
   stale and valid in hardware.
3. Recompute the RSS actions for both directions; without that a disable
   and re-enable cycle leaves the BMC without its multicast/broadcast
   copies until the interface bounces.

Take the instance lock over the whole of fbnic_bmc_rpc_check() while here.
fbnic_fw_xmit_rpc_macda_sync() walks the entire MACDA shadow and the
teardown above rewrites it, but the service task they both run from holds
only RTNL. RTNL used to cover that, back when ndo_set_rx_mode() was called
inline from dev_set_rx_mode(). fbnic is ops-locked, so netif_rx_mode_run()
now calls ndo_set_rx_mode_async() under the instance lock alone, having
dropped netif_addr_lock and handed over a snapshot of the address lists.
The ethtool NFC paths are in the same position. The instance lock is the
one lock every writer of the shadow holds. The flags are tested once
without it first so the lock stays off the common path, where the service
task has nothing to do.

Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h  |    4 +
 drivers/net/ethernet/meta/fbnic/fbnic_rpc.c |   97 ++++++++++++++++++++++++++-
 2 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index e542b17bdac7..f47669129c9d 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -55,6 +55,10 @@ enum {
 	FBNIC_FW_CAP_F_BMC_MACDA_SYNC,
 };
 
+#define FBNIC_FW_CAP_BMC_PENDING	\
+	(BIT(FBNIC_FW_CAP_F_BMC_TCAM_REINIT) |	\
+	 BIT(FBNIC_FW_CAP_F_BMC_MACDA_SYNC))
+
 struct fbnic_fw_cap {
 	unsigned long state;
 	struct {
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
index 51fd0564d33f..939d56585b26 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
@@ -143,9 +143,11 @@ void fbnic_bmc_rpc_all_multi_config(struct fbnic_dev *fbd,
 
 	/* If we are not enabling the rule just delete it. We will fall
 	 * back to the RSS rules that support the multicast addresses.
+	 * Delete in any live state, not just VALID, as ifdown demotes it to
+	 * UPDATE and a miss there leaves it to be rewritten to hardware.
 	 */
 	if (!fbnic_bmc_all_multi(fbd) || enable_host) {
-		if (act_tcam->state == FBNIC_TCAM_S_VALID)
+		if (act_tcam->state != FBNIC_TCAM_S_DISABLED)
 			act_tcam->state = FBNIC_TCAM_S_DELETE;
 		return;
 	}
@@ -234,10 +236,91 @@ 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);
 	int err;
 
+	/* Nothing to do unless the firmware raised one of the flags. Test
+	 * before taking the lock to keep it off the common path; one raised
+	 * after this is picked up on the next pass.
+	 */
+	if (!(READ_ONCE(fbd->fw_cap.state) & FBNIC_FW_CAP_BMC_PENDING))
+		return;
+
+	/* The rx mode work and the ethtool paths rewrite the MACDA and action
+	 * TCAM shadows under the instance lock, and this runs from the service
+	 * task under RTNL only, so the two do not exclude each other.
+	 */
+	netdev_lock(fbd->netdev);
+
 	/* Consume the flag before the state it advertises. The ordering
 	 * implied by test_and_clear_bit() pairs with the barrier in
 	 * fbnic_fw_parse_bmc_cap(), and claiming it atomically means a set
@@ -245,7 +328,15 @@ 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)) {
-		fbnic_bmc_rpc_init(fbd);
+		if (!fbnic_bmc_present(fbd) && fbnic_bmc_rules_present(fbd))
+			fbnic_bmc_rpc_disable(fbd);
+		else
+			fbnic_bmc_rpc_init(fbd);
+
+		/* Neither path touches the RSS actions, which carry the mc/bc
+		 * copy to the BMC, so recompute them alongside.
+		 */
+		fbnic_rss_reinit(fbd, fbn);
 		netif_addr_lock_bh(fbd->netdev);
 		__fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
 		netif_addr_unlock_bh(fbd->netdev);
@@ -258,6 +349,8 @@ void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
 			dev_warn(fbd->dev,
 				 "Writing MACDA table to FW failed, err: %d\n", err);
 	}
+
+	netdev_unlock(fbd->netdev);
 }
 
 #define FBNIC_ACT1_INIT(_l4, _udp, _ip, _v6)		\

Re: [next-next PATCH v2 2/2] fbnic: Remove BMC routing rules when the BMC channel is disabled

[netdev-bot+sashiko]

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

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

- [High] The commit message says "The instance lock is the one lock every
  writer of the shadow holds", and fbnic_bmc_rpc_check() now takes…
- [Medium] fbnic_bmc_rpc_check() picks the new teardown only when
  fbnic_bmc_rules_present() finds a MACDA entry carrying…
- [Medium] The new teardown runs only from fbnic_bmc_rpc_check(), which
  only the service task calls.
- [Low] The patch fixes user-visible bugs but has no Fixes: tag, and it
  combines several fixes that could be split out and that come from…

Pre-existing issues:
- [Medium] fbnic_bmc_rpc_check() now calls fbnic_rss_reinit() and then
  __fbnic_set_rx_mode(), which runs…
- [Medium] fbnic_bmc_rpc_init() (fbnic_rpc.c:175-236) writes BMC unicast
  MACs into slots starting at FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX, and…

--

fbnic: Remove BMC routing rules when the BMC channel is disabled

When the firmware reports that the BMC is gone but BMC-tagged rules are
still programmed, fbnic_bmc_rpc_check() now removes the BMC MACDA
entries, the BMC action rule and the host-unicast RSS entries. The patch
also recomputes the RSS actions on every TCAM reinit and deletes action
rules in any live state. It also takes the netdev instance lock across
fbnic_bmc_rpc_check().

> Take the instance lock over the whole of fbnic_bmc_rpc_check() while here.

[Severity: Low]

This isn't a bug, but the patch has no Fixes: tag. It also combines
several fixes that come from different commits:

- the new BMC teardown when NC-SI is disabled while the host is up

- the fbnic_bmc_rpc_all_multi_config() change from "== VALID" to
  "!= DISABLED". This fixes a separate pre-existing leak: ifdown demotes
  the all-multi rule to UPDATE, and fbnic_write_rules() writes it back
  at the next ifup.

- the locking fix in fbnic_bmc_rpc_check(). The commit message traces
  this to commit 60dd9781e9b8 ("fbnic: convert to
  ndo_set_rx_mode_async"). It can be hit on every
  FBNIC_FW_CAP_F_BMC_MACDA_SYNC pass, even if the BMC never disables.
  git blame points at commit 04a230b27d8f3 for the service task reinit
  path.

Could these be split out, each with its own Fixes: tag? Stable could then
take the locking fix without also taking the new teardown logic.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> index 51fd0564d33f..939d56585b26 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c

[ ... ]

> @@ -234,10 +236,91 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)

[ ... ]

> +/**
> + * 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.
> + */

[Severity: Medium]

What happens if the BMC disables its NC-SI channel while the host
interface is down?

Only the service task calls fbnic_bmc_rpc_check(), and the service task
re-arms itself only while the netdev is running:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_service_task() {
    ...
	if (netif_running(netdev))
		schedule_delayed_work(&fbd->service_task, HZ);
}

The last ifdown ran with the BMC present, so it left three things behind:

- fbnic_clear_rules() left rule 63 as a DEST_BMC catch-all
- fbnic_clear_macda() left the BMC-tagged MACDA entries valid
- fbnic_rss_disable_hw() left RMI_CONFIG enabled

On the next ifup, __fbnic_open() calls fbnic_bmc_rpc_init(), which
returns early. It then calls fbnic_rss_reinit(), which only touches the
mc/bc entries.

After that, fbnic_up()->__fbnic_set_rx_mode() writes the stale state to
hardware:

- fbnic_write_rules() writes act rule 0 back, since ifdown demoted it to
  UPDATE
- fbnic_write_tce_tcam() programs TCE loopback again for the BMC MACs
  that are still tagged
- the leftover host-unicast RSS entries are written as well

This only converges at the first service pass, at least HZ after ifup.

If the interface goes down again inside that window, won't
fbnic_clear_macda() still print "Found BMC MAC address w/ BMC not
present"? The commit message says that warning is what this patch
removes.

[ ... ]

>  void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
>  {
> +	struct fbnic_net *fbn = netdev_priv(fbd->netdev);
>  	int err;
>  
> +	/* Nothing to do unless the firmware raised one of the flags. Test
> +	 * before taking the lock to keep it off the common path; one raised
> +	 * after this is picked up on the next pass.
> +	 */
> +	if (!(READ_ONCE(fbd->fw_cap.state) & FBNIC_FW_CAP_BMC_PENDING))
> +		return;
> +
> +	/* The rx mode work and the ethtool paths rewrite the MACDA and action
> +	 * TCAM shadows under the instance lock, and this runs from the service
> +	 * task under RTNL only, so the two do not exclude each other.
> +	 */
> +	netdev_lock(fbd->netdev);

[Severity: High]

The commit message says "The instance lock is the one lock every writer
of the shadow holds." Is that true across the whole service task?

fbnic_service_task() in fbnic_pci.c holds only rtnl_lock(). It calls
fbnic_health_check() just before fbnic_bmc_rpc_check(). After a heartbeat
loss, fbnic_health_check() calls fbnic_fw_config_after_crash(). That
function rewrites the same shadow and the hardware without taking the
instance lock:

drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_fw_config_after_crash() {
    ...
	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);
    ...
}

fbnic is ops-locked, so netdev_work_proc() runs netif_rx_mode_run()
under the instance lock alone. netif_rx_mode_run() drops addr_list_lock
before calling fbnic_set_rx_mode()->__fbnic_set_rx_mode() on the uc/mc
snapshots.

Can these two paths run at the same time on different CPUs? If so:

- both could allocate MACDA slots through
  __fbnic_uc_sync()/__fbnic_mc_sync()
- fbnic_write_macda() could memset() an entry that the other side is
  still filling in
- one side could clear hardware entries the other side has just written

There may also be a sync_cnt problem. The crash path syncs the live
lists with __hw_addr_sync_dev(). __hw_addr_list_reconcile() then adds
the snapshot delta on top:

net/core/dev_addr_lists.c:__hw_addr_list_reconcile() {
    ...
		real_ha->sync_cnt += delta;
		real_ha->refcount += delta;
    ...
}

Would that leave sync_cnt/refcount inflated, so that a removed address
is never unsynced from hardware?

Should fbnic_fw_config_after_crash() also run under netdev_lock()? The
other option is to take the lock around every part of the service task
that writes the shadow.

> +
>  	/* Consume the flag before the state it advertises. The ordering
>  	 * implied by test_and_clear_bit() pairs with the barrier in
>  	 * fbnic_fw_parse_bmc_cap(), and claiming it atomically means a set
> @@ -245,7 +328,15 @@ 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)) {
> -		fbnic_bmc_rpc_init(fbd);
> +		if (!fbnic_bmc_present(fbd) && fbnic_bmc_rules_present(fbd))

[Severity: Medium]

Does fbnic_bmc_rules_present() reliably tell us whether the host-unicast
RSS entries are programmed?

fbnic_rss_reinit() programs the host-unicast entries based only on
fbnic_bmc_present(). It is also called from fbnic_set_rss_hash_opts()
and fbnic_hwtstamp_set(), and neither of those sets a MACDA BMC tag.

Take this sequence:

1. The BMC goes from absent to present. FW sets PRESENT and
   TCAM_REINIT.
2. Before the next service pass, ethtool rx-flow-hash or SIOCSHWTSTAMP
   calls fbnic_rss_reinit(). It sees the BMC present and programs the
   host-unicast entries.
3. The BMC goes absent again.
4. The service pass sees !present && !rules_present and takes the else
   branch. fbnic_bmc_rpc_init() returns early, and fbnic_rss_reinit()
   skips the unicast entries:

	for (i = fbnic_bmc_present(fbd) ? 0 : FBNIC_RSS_EN_NUM_UNICAST;
	     i < FBNIC_RSS_EN_NUM_ENTRIES; i++) {

The host-unicast entries are then never deleted.

The same result looks possible within a single pass.
fbnic_fw_parse_bmc_cap() sets PRESENT from mailbox context without the
instance lock. It can land after fbnic_bmc_rpc_disable() has marked the
entries DELETE and before fbnic_rss_reinit() runs. fbnic_rss_reinit()
then turns them back into UPDATE, with no BMC tags left.

These entries match ahead of the mc/bc RSS entries. Unicast traffic would
keep the old rss_en_mask, RXH_DISCARD and TS_ENA settings.
fbnic_clear_valid_act_tcam() also demotes them to UPDATE, so they get
written again at every ifup.

Doesn't this leave the "stale and valid in hardware" case from the
commit message still reachable?

> +			fbnic_bmc_rpc_disable(fbd);
> +		else
> +			fbnic_bmc_rpc_init(fbd);

[Severity: Medium]

This is a pre-existing issue, but what happens here if the BMC MAC set
changes between two TCAM_REINIT passes while fbnic_bmc_present() stays
true?

fbnic_bmc_rpc_init() writes the new BMC unicast MACs starting at
FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX and adds the multicast ones via
__fbnic_mc_sync(). It never clears FBNIC_MAC_ADDR_T_BMC from entries
left over from the previous set:

		if (is_multicast_ether_addr(bmc_mac))
			mac_addr = __fbnic_mc_sync(fbd, bmc_mac);
		else
			mac_addr = &fbd->mac_addr[i++];

The same happens if a disable and a re-enable with different MACs both
arrive before one service pass. The new disable branch is skipped in
that case.

Would the MACs the BMC no longer reports stay BMC-tagged and valid? Act
rule 0 would keep steering their Rx traffic to the BMC, and
fbnic_write_tce_tcam() would keep looping host Tx for them to the BMC.

> +
> +		/* Neither path touches the RSS actions, which carry the mc/bc
> +		 * copy to the BMC, so recompute them alongside.
> +		 */
> +		fbnic_rss_reinit(fbd, fbn);
>  		netif_addr_lock_bh(fbd->netdev);
>  		__fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
>  		netif_addr_unlock_bh(fbd->netdev);

[Severity: Medium]

This isn't a bug introduced by this patch, but can this pass run after
the interface has been brought down?

fbnic_service_task_stop() uses the non-sync cancel:

	cancel_delayed_work(&fbd->service_task);

A service task that has already started and is waiting in rtnl_lock()
while fbnic_stop() runs will carry on afterwards. The other
fbnic_rss_reinit() callers check netif_running(), but this path does
not:

drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c:fbnic_set_rss_hash_opts() {
    ...
	if (netif_running(fbn->netdev)) {
		fbnic_rss_reinit(fbn->fbd, fbn);
		fbnic_write_rules(fbn->fbd);
	}
    ...
}

By that point fbnic_down_noidle() has demoted the host entries to UPDATE,
and fbnic_disable() has shut down the host queues. If TCAM_REINIT is
pending, __fbnic_set_rx_mode() writes the host RSS actions and MACDA
entries back to hardware. Host traffic is then steered to disabled Rx
queues until the next open/close.

The hole existed before this patch, because __fbnic_set_rx_mode() on
this path already rewrote the demoted entries. The added
fbnic_rss_reinit() now also writes the host-unicast entries when the BMC
has just become present.

Should fbnic_bmc_rpc_check() return early when the netdev is not
running?

[ ... ]

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

Re: [next-next PATCH v2 2/2] fbnic: Remove BMC routing rules when the BMC channel is disabled

[Alexander Duyck]

On Wed, Sep 30, 2026 at 3:12 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 2 (2) · 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).
>
> - [High] The commit message says "The instance lock is the one lock every
>   writer of the shadow holds", and fbnic_bmc_rpc_check() now takes…
> - [Medium] fbnic_bmc_rpc_check() picks the new teardown only when
>   fbnic_bmc_rules_present() finds a MACDA entry carrying…
> - [Medium] The new teardown runs only from fbnic_bmc_rpc_check(), which
>   only the service task calls.
> - [Low] The patch fixes user-visible bugs but has no Fixes: tag, and it
>   combines several fixes that could be split out and that come from…
>
> Pre-existing issues:
> - [Medium] fbnic_bmc_rpc_check() now calls fbnic_rss_reinit() and then
>   __fbnic_set_rx_mode(), which runs…
> - [Medium] fbnic_bmc_rpc_init() (fbnic_rpc.c:175-236) writes BMC unicast
>   MACs into slots starting at FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX, and…
>
> --
>
> fbnic: Remove BMC routing rules when the BMC channel is disabled
>
> When the firmware reports that the BMC is gone but BMC-tagged rules are
> still programmed, fbnic_bmc_rpc_check() now removes the BMC MACDA
> entries, the BMC action rule and the host-unicast RSS entries. The patch
> also recomputes the RSS actions on every TCAM reinit and deletes action
> rules in any live state. It also takes the netdev instance lock across
> fbnic_bmc_rpc_check().
>
> > Take the instance lock over the whole of fbnic_bmc_rpc_check() while here.
>
> [Severity: Low]
>
> This isn't a bug, but the patch has no Fixes: tag. It also combines
> several fixes that come from different commits:
>
> - the new BMC teardown when NC-SI is disabled while the host is up
>
> - the fbnic_bmc_rpc_all_multi_config() change from "== VALID" to
>   "!= DISABLED". This fixes a separate pre-existing leak: ifdown demotes
>   the all-multi rule to UPDATE, and fbnic_write_rules() writes it back
>   at the next ifup.
>
> - the locking fix in fbnic_bmc_rpc_check(). The commit message traces
>   this to commit 60dd9781e9b8 ("fbnic: convert to
>   ndo_set_rx_mode_async"). It can be hit on every
>   FBNIC_FW_CAP_F_BMC_MACDA_SYNC pass, even if the BMC never disables.
>   git blame points at commit 04a230b27d8f3 for the service task reinit
>   path.
>
> Could these be split out, each with its own Fixes: tag? Stable could then
> take the locking fix without also taking the new teardown logic.

These are all low risk issues and this change is mostly just to clean
up a warning that was being triggered when the interface was stopped.
In addition due to the ordering of things backporting these would be a
pain since the code that introduces the original issue has been
changed several times so it is likely easier to just go forward with
this as a refactor rather than fixes to be backported.

> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > index 51fd0564d33f..939d56585b26 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_rpc.c
>
> [ ... ]
>
> > @@ -234,10 +236,91 @@ void fbnic_bmc_rpc_init(struct fbnic_dev *fbd)
>
> [ ... ]
>
> > +/**
> > + * 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.
> > + */
>
> [Severity: Medium]
>
> What happens if the BMC disables its NC-SI channel while the host
> interface is down?
>
> Only the service task calls fbnic_bmc_rpc_check(), and the service task
> re-arms itself only while the netdev is running:
>
> drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_service_task() {
>     ...
>         if (netif_running(netdev))
>                 schedule_delayed_work(&fbd->service_task, HZ);
> }
>

If the interface is down the FW owns the RPC TCAMS and MAC/PCS/PHY so
it is responsible for doing the reconfiguration and setting up the
link. The ifup should be clearing out any bad state in the fw
capabilities structure and when it writes the rules it should be using
those to populate things.

> The last ifdown ran with the BMC present, so it left three things behind:
>
> - fbnic_clear_rules() left rule 63 as a DEST_BMC catch-all
> - fbnic_clear_macda() left the BMC-tagged MACDA entries valid
> - fbnic_rss_disable_hw() left RMI_CONFIG enabled
>
> On the next ifup, __fbnic_open() calls fbnic_bmc_rpc_init(), which
> returns early. It then calls fbnic_rss_reinit(), which only touches the
> mc/bc entries.
>
> After that, fbnic_up()->__fbnic_set_rx_mode() writes the stale state to
> hardware:
>
> - fbnic_write_rules() writes act rule 0 back, since ifdown demoted it to
>   UPDATE
> - fbnic_write_tce_tcam() programs TCE loopback again for the BMC MACs
>   that are still tagged
> - the leftover host-unicast RSS entries are written as well
>
> This only converges at the first service pass, at least HZ after ifup.
>
> If the interface goes down again inside that window, won't
> fbnic_clear_macda() still print "Found BMC MAC address w/ BMC not
> present"? The commit message says that warning is what this patch
> removes.

The FW is responsible for clearing the MACDA in that situation. Not the driver.

> [ ... ]
>
> >  void fbnic_bmc_rpc_check(struct fbnic_dev *fbd)
> >  {
> > +     struct fbnic_net *fbn = netdev_priv(fbd->netdev);
> >       int err;
> >
> > +     /* Nothing to do unless the firmware raised one of the flags. Test
> > +      * before taking the lock to keep it off the common path; one raised
> > +      * after this is picked up on the next pass.
> > +      */
> > +     if (!(READ_ONCE(fbd->fw_cap.state) & FBNIC_FW_CAP_BMC_PENDING))
> > +             return;
> > +
> > +     /* The rx mode work and the ethtool paths rewrite the MACDA and action
> > +      * TCAM shadows under the instance lock, and this runs from the service
> > +      * task under RTNL only, so the two do not exclude each other.
> > +      */
> > +     netdev_lock(fbd->netdev);
>
> [Severity: High]
>
> The commit message says "The instance lock is the one lock every writer
> of the shadow holds." Is that true across the whole service task?
>
> fbnic_service_task() in fbnic_pci.c holds only rtnl_lock(). It calls
> fbnic_health_check() just before fbnic_bmc_rpc_check(). After a heartbeat
> loss, fbnic_health_check() calls fbnic_fw_config_after_crash(). That
> function rewrites the same shadow and the hardware without taking the
> instance lock:
>
> drivers/net/ethernet/meta/fbnic/fbnic_pci.c:fbnic_fw_config_after_crash() {
>     ...
>         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);
>     ...
> }
>
> fbnic is ops-locked, so netdev_work_proc() runs netif_rx_mode_run()
> under the instance lock alone. netif_rx_mode_run() drops addr_list_lock
> before calling fbnic_set_rx_mode()->__fbnic_set_rx_mode() on the uc/mc
> snapshots.
>
> Can these two paths run at the same time on different CPUs? If so:
>
> - both could allocate MACDA slots through
>   __fbnic_uc_sync()/__fbnic_mc_sync()
> - fbnic_write_macda() could memset() an entry that the other side is
>   still filling in
> - one side could clear hardware entries the other side has just written
>
> There may also be a sync_cnt problem. The crash path syncs the live
> lists with __hw_addr_sync_dev(). __hw_addr_list_reconcile() then adds
> the snapshot delta on top:
>
> net/core/dev_addr_lists.c:__hw_addr_list_reconcile() {
>     ...
>                 real_ha->sync_cnt += delta;
>                 real_ha->refcount += delta;
>     ...
> }
>
> Would that leave sync_cnt/refcount inflated, so that a removed address
> is never unsynced from hardware?
>
> Should fbnic_fw_config_after_crash() also run under netdev_lock()? The
> other option is to take the lock around every part of the service task
> that writes the shadow.

Yeah, it probably should. That will likely require a follow-on patch.

> > +
> >       /* Consume the flag before the state it advertises. The ordering
> >        * implied by test_and_clear_bit() pairs with the barrier in
> >        * fbnic_fw_parse_bmc_cap(), and claiming it atomically means a set
> > @@ -245,7 +328,15 @@ 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)) {
> > -             fbnic_bmc_rpc_init(fbd);
> > +             if (!fbnic_bmc_present(fbd) && fbnic_bmc_rules_present(fbd))
>
> [Severity: Medium]
>
> Does fbnic_bmc_rules_present() reliably tell us whether the host-unicast
> RSS entries are programmed?
>
> fbnic_rss_reinit() programs the host-unicast entries based only on
> fbnic_bmc_present(). It is also called from fbnic_set_rss_hash_opts()
> and fbnic_hwtstamp_set(), and neither of those sets a MACDA BMC tag.
>
> Take this sequence:
>
> 1. The BMC goes from absent to present. FW sets PRESENT and
>    TCAM_REINIT.
> 2. Before the next service pass, ethtool rx-flow-hash or SIOCSHWTSTAMP
>    calls fbnic_rss_reinit(). It sees the BMC present and programs the
>    host-unicast entries.
> 3. The BMC goes absent again.
> 4. The service pass sees !present && !rules_present and takes the else
>    branch. fbnic_bmc_rpc_init() returns early, and fbnic_rss_reinit()
>    skips the unicast entries:
>
>         for (i = fbnic_bmc_present(fbd) ? 0 : FBNIC_RSS_EN_NUM_UNICAST;
>              i < FBNIC_RSS_EN_NUM_ENTRIES; i++) {
>
> The host-unicast entries are then never deleted.
>
> The same result looks possible within a single pass.
> fbnic_fw_parse_bmc_cap() sets PRESENT from mailbox context without the
> instance lock. It can land after fbnic_bmc_rpc_disable() has marked the
> entries DELETE and before fbnic_rss_reinit() runs. fbnic_rss_reinit()
> then turns them back into UPDATE, with no BMC tags left.
>
> These entries match ahead of the mc/bc RSS entries. Unicast traffic would
> keep the old rss_en_mask, RXH_DISCARD and TS_ENA settings.
> fbnic_clear_valid_act_tcam() also demotes them to UPDATE, so they get
> written again at every ifup.
>
> Doesn't this leave the "stale and valid in hardware" case from the
> commit message still reachable?
>
> > +                     fbnic_bmc_rpc_disable(fbd);
> > +             else
> > +                     fbnic_bmc_rpc_init(fbd);
>
> [Severity: Medium]
>
> This is a pre-existing issue, but what happens here if the BMC MAC set
> changes between two TCAM_REINIT passes while fbnic_bmc_present() stays
> true?
>
> fbnic_bmc_rpc_init() writes the new BMC unicast MACs starting at
> FBNIC_RPC_TCAM_MACDA_BMC_ADDR_IDX and adds the multicast ones via
> __fbnic_mc_sync(). It never clears FBNIC_MAC_ADDR_T_BMC from entries
> left over from the previous set:
>
>                 if (is_multicast_ether_addr(bmc_mac))
>                         mac_addr = __fbnic_mc_sync(fbd, bmc_mac);
>                 else
>                         mac_addr = &fbd->mac_addr[i++];
>
> The same happens if a disable and a re-enable with different MACs both
> arrive before one service pass. The new disable branch is skipped in
> that case.
>
> Would the MACs the BMC no longer reports stay BMC-tagged and valid? Act
> rule 0 would keep steering their Rx traffic to the BMC, and
> fbnic_write_tce_tcam() would keep looping host Tx for them to the BMC.

I will have to follow up on this as it is not a scenario we have seen.
For now I would likely address in a follow-on patch.

> > +
> > +             /* Neither path touches the RSS actions, which carry the mc/bc
> > +              * copy to the BMC, so recompute them alongside.
> > +              */
> > +             fbnic_rss_reinit(fbd, fbn);
> >               netif_addr_lock_bh(fbd->netdev);
> >               __fbnic_set_rx_mode(fbd, &fbd->netdev->uc, &fbd->netdev->mc);
> >               netif_addr_unlock_bh(fbd->netdev);
>
> [Severity: Medium]
>
> This isn't a bug introduced by this patch, but can this pass run after
> the interface has been brought down?
>
> fbnic_service_task_stop() uses the non-sync cancel:
>
>         cancel_delayed_work(&fbd->service_task);
>
> A service task that has already started and is waiting in rtnl_lock()
> while fbnic_stop() runs will carry on afterwards. The other
> fbnic_rss_reinit() callers check netif_running(), but this path does
> not:
>
> drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c:fbnic_set_rss_hash_opts() {
>     ...
>         if (netif_running(fbn->netdev)) {
>                 fbnic_rss_reinit(fbn->fbd, fbn);
>                 fbnic_write_rules(fbn->fbd);
>         }
>     ...
> }
>
> By that point fbnic_down_noidle() has demoted the host entries to UPDATE,
> and fbnic_disable() has shut down the host queues. If TCAM_REINIT is
> pending, __fbnic_set_rx_mode() writes the host RSS actions and MACDA
> entries back to hardware. Host traffic is then steered to disabled Rx
> queues until the next open/close.
>
> The hole existed before this patch, because __fbnic_set_rx_mode() on
> this path already rewrote the demoted entries. The added
> fbnic_rss_reinit() now also writes the host-unicast entries when the BMC
> has just become present.
>
> Should fbnic_bmc_rpc_check() return early when the netdev is not
> running?

Since this was a pre-existing issue we can probably look at addressing
it in a follow-on patch. Generally we shouldn't be touching the RPC
rules if we have stopped the interfaces. I will follow up on that and
verify as the __fbnic_set_rx_mode calls when the interface is not in
the running state could be problematic as it would route traffic to
the host which would just drop it at disabled queues. While it isn't
good behavior it would lead to some stats noise for us dropping
packets in the disabled HW queues.

Re: [next-next PATCH v2 0/2] eth: fbnic: Update BMC MAC address handling

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

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

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

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: [next-next PATCH v2 0/2] eth: fbnic: Update BMC MAC address handling

[Alexander Duyck]

On Mon, Sep 28, 2026 at 9:15 AM <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:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
>
>  - 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.
>
> 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.

This is sort of a fix but it isn't for anything that is high priority.
Basically we had cases where there were noise messages seen when the
BMC would transition between enabling and/or disabling a NIC datapath
which would seldom occur on an active port, and the issue at the time
was very transitory. As such my thought was that this didn't rise to
the level of being something to submit for net. In addition with all
the updates for items found by the AI agents it became more of a
refactor/update to behavior rather than a fix.

So if anything most of this was found via AI analysis of the two diffs
to fix the original minor issues seen.

Re: [next-next PATCH v2 0/2] eth: fbnic: Update BMC MAC address handling

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 28 Sep 2026 09:12:00 -0700 you wrote:
> Update the handling of BMC MAC addresses on fbnic, and in the process also
> address several issues identified with the changes. This patch series
> consists of 2 patches.
> 
> The first patch addresses issues found in the bmc_present and
> bmc_tcam_reinit handling. It started out addressing the fact that present
> w/o a MAC address was flagged as an invalid message. However, we ended up
> also addressing a potential race in that setting bmc_present and
> need_bmc_tcam_reinit could potentially race against the bmc_rpc_check. To
> address that the bit flags were moved to a state bitmap and are now handled
> via the test_and_clear_bit and assign_bit operations.
> 
> [...]

Here is the summary with links:
  - [next-next,v2,1/2] fbnic: Rework the BMC state pulled from the capabilities message
    https://git.kernel.org/netdev/net-next/c/d261a371e3b7
  - [next-next,v2,2/2] fbnic: Remove BMC routing rules when the BMC channel is disabled
    https://git.kernel.org/netdev/net-next/c/db99b6fae21c

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