[PATCH net-next V0.5 0/5] eth: fbnic: Update IPC mailbox support

[PATCH net-next V0.5 0/5] eth: fbnic: Update IPC mailbox support

[Mohsin Bashir]

Update IPC mailbox support for fbnic to cater for several changes.

Mohsin Bashir (5):
  eth: fbnic: Use GFP_KERNEL to allocting mbx pages
  eth: fbnic: Allocate all pages for RX mailbox
  eth: fbnic: Reuse RX mailbox pages
  eth: fbnic: Remove retry support
  eth: fbnic: Update RX mbox timeout value

 .../net/ethernet/meta/fbnic/fbnic_devlink.c   |  8 ++--
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  2 +-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c    | 42 ++++++++++++-------
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h    |  8 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_mac.c   | 24 +++--------
 5 files changed, 44 insertions(+), 40 deletions(-)

-- 
2.47.3

[PATCH net-next V0.5 1/5] eth: fbnic: Use GFP_KERNEL to allocting mbx pages

[Mohsin Bashir]

Replace GFP_ATOMIC with GFP_KERNEL for mailbox RX page allocation. Since
interrupt handler is threaded GFP_KERNEL is a safe option to reduce
allocation failures.

Also remove __GFP_NOWARN so the kernel reports a warning on allocation
failure to aid debugging.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index d8d9b6cfde82..3dfd3f2442ff 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -205,8 +205,7 @@ static int fbnic_mbx_alloc_rx_msgs(struct fbnic_dev *fbd)
 	while (!err && count--) {
 		struct fbnic_tlv_msg *msg;
 
-		msg = (struct fbnic_tlv_msg *)__get_free_page(GFP_ATOMIC |
-							      __GFP_NOWARN);
+		msg = (struct fbnic_tlv_msg *)__get_free_page(GFP_KERNEL);
 		if (!msg) {
 			err = -ENOMEM;
 			break;
-- 
2.47.3

[PATCH net-next V0.5 2/5] eth: fbnic: Allocate all pages for RX mailbox

[Mohsin Bashir]

Now that memory is allocated with GFP_KERNEL, allocation failures
should be extremely rare. Ensure the FW communication ring is
always fully populated with free pages, and hard fail initialization
otherwise. This enables simplifications in next patches.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 3dfd3f2442ff..09252b3e03ca 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -415,7 +415,7 @@ static int fbnic_fw_xmit_simple_msg(struct fbnic_dev *fbd, u32 msg_type)
 	return err;
 }
 
-static void fbnic_mbx_init_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
+static int fbnic_mbx_init_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
 {
 	struct fbnic_fw_mbx *mbx = &fbd->mbx[mbx_idx];
 
@@ -428,14 +428,15 @@ static void fbnic_mbx_init_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
 		     FBNIC_PUL_OB_TLP_HDR_AW_CFG_BME);
 
 		/* Make sure we have a page for the FW to write to */
-		fbnic_mbx_alloc_rx_msgs(fbd);
-		break;
+		return fbnic_mbx_alloc_rx_msgs(fbd);
 	case FBNIC_IPC_MBX_TX_IDX:
 		/* Enable DMA reads from the device */
 		wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AR_CFG,
 		     FBNIC_PUL_OB_TLP_HDR_AR_CFG_BME);
 		break;
 	}
+
+	return 0;
 }
 
 static bool fbnic_mbx_event(struct fbnic_dev *fbd)
@@ -1683,8 +1684,11 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
 	} while (!fbnic_mbx_event(fbd));
 
 	/* FW has shown signs of life. Enable DMA and start Tx/Rx */
-	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++)
-		fbnic_mbx_init_desc_ring(fbd, i);
+	for (i = 0; i < FBNIC_IPC_MBX_INDICES; i++) {
+		err = fbnic_mbx_init_desc_ring(fbd, i);
+		if (err)
+			goto clean_mbx;
+	}
 
 	/* Request an update from the firmware. This should overwrite
 	 * mgmt.version once we get the actual version from the firmware
-- 
2.47.3

[PATCH net-next V0.5 3/5] eth: fbnic: Reuse RX mailbox pages

[Mohsin Bashir]

Currently, the RX mailbox frees and reallocates a page for each received
message. Since FW Rx messages are processed synchronously, and nothing
hold these pages (unlike skbs which we hand over to the stack), reuse
the pages and put them back on the Rx ring. Now that we ensure the ring
is always fully populated we don't have to worry about filling it up
after partial population during init, either. Update
fbnic_mbx_process_rx_msgs() to recycle pages after message processing.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_fw.c | 25 ++++++++++++++--------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
index 09252b3e03ca..66c9412f4057 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
@@ -1592,7 +1592,7 @@ static const struct fbnic_tlv_parser fbnic_fw_tlv_parser[] = {
 static void fbnic_mbx_process_rx_msgs(struct fbnic_dev *fbd)
 {
 	struct fbnic_fw_mbx *rx_mbx = &fbd->mbx[FBNIC_IPC_MBX_RX_IDX];
-	u8 head = rx_mbx->head;
+	u8 head = rx_mbx->head, tail = rx_mbx->tail;
 	u64 desc, length;
 
 	while (head != rx_mbx->tail) {
@@ -1603,8 +1603,8 @@ static void fbnic_mbx_process_rx_msgs(struct fbnic_dev *fbd)
 		if (!(desc & FBNIC_IPC_MBX_DESC_FW_CMPL))
 			break;
 
-		dma_unmap_single(fbd->dev, rx_mbx->buf_info[head].addr,
-				 PAGE_SIZE, DMA_FROM_DEVICE);
+		dma_sync_single_for_cpu(fbd->dev, rx_mbx->buf_info[head].addr,
+					FBNIC_RX_PAGE_SIZE, DMA_FROM_DEVICE);
 
 		msg = rx_mbx->buf_info[head].msg;
 
@@ -1637,19 +1637,26 @@ static void fbnic_mbx_process_rx_msgs(struct fbnic_dev *fbd)
 
 		dev_dbg(fbd->dev, "Parsed msg type %d\n", msg->hdr.type);
 next_page:
+		fw_wr32(fbd, FBNIC_IPC_MBX(FBNIC_IPC_MBX_RX_IDX, head), 0);
 
-		free_page((unsigned long)rx_mbx->buf_info[head].msg);
+		rx_mbx->buf_info[tail] = rx_mbx->buf_info[head];
 		rx_mbx->buf_info[head].msg = NULL;
+		rx_mbx->buf_info[head].addr = 0;
 
-		head++;
-		head %= FBNIC_IPC_MBX_DESC_LEN;
+		__fbnic_mbx_wr_desc(fbd, FBNIC_IPC_MBX_RX_IDX, tail,
+				    FIELD_PREP(FBNIC_IPC_MBX_DESC_LEN_MASK,
+					       FBNIC_RX_PAGE_SIZE) |
+				    (rx_mbx->buf_info[tail].addr &
+				     FBNIC_IPC_MBX_DESC_ADDR_MASK) |
+				    FBNIC_IPC_MBX_DESC_HOST_CMPL);
+
+		head = (head + 1) & (FBNIC_IPC_MBX_DESC_LEN - 1);
+		tail = (tail + 1) & (FBNIC_IPC_MBX_DESC_LEN - 1);
 	}
 
 	/* Record head for next interrupt */
 	rx_mbx->head = head;
-
-	/* Make sure we have at least one page for the FW to write to */
-	fbnic_mbx_alloc_rx_msgs(fbd);
+	rx_mbx->tail = tail;
 }
 
 void fbnic_mbx_poll(struct fbnic_dev *fbd)
-- 
2.47.3

[PATCH net-next V0.5 4/5] eth: fbnic: Remove retry support

[Mohsin Bashir]

The driver retries sensor read requests from firmware, but this is
unnecessary. A functioning firmware should respond to each request
within the timeout period. Remove the retry logic and set the timeout
to the sum of all retry timeouts.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_mac.c | 24 +++++----------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
index fc7abea4ef5b..9d0e4b2cc9ac 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_mac.c
@@ -835,7 +835,7 @@ static int fbnic_mac_get_sensor_asic(struct fbnic_dev *fbd, int id,
 				     long *val)
 {
 	struct fbnic_fw_completion *fw_cmpl;
-	int err = 0, retries = 5;
+	int err = 0;
 	s32 *sensor;
 
 	fw_cmpl = fbnic_fw_alloc_cmpl(FBNIC_TLV_MSG_ID_TSENE_READ_RESP);
@@ -862,24 +862,10 @@ static int fbnic_mac_get_sensor_asic(struct fbnic_dev *fbd, int id,
 		goto exit_free;
 	}
 
-	/* Allow 2 seconds for reply, resend and try up to 5 times */
-	while (!wait_for_completion_timeout(&fw_cmpl->done, 2 * HZ)) {
-		retries--;
-
-		if (retries == 0) {
-			dev_err(fbd->dev,
-				"Timed out waiting for TSENE read\n");
-			err = -ETIMEDOUT;
-			goto exit_cleanup;
-		}
-
-		err = fbnic_fw_xmit_tsene_read_msg(fbd, NULL);
-		if (err) {
-			dev_err(fbd->dev,
-				"Failed to transmit TSENE read msg, err %d\n",
-				err);
-			goto exit_cleanup;
-		}
+	if (!wait_for_completion_timeout(&fw_cmpl->done, 10 * HZ)) {
+		dev_err(fbd->dev, "Timed out waiting for TSENE read\n");
+		err = -ETIMEDOUT;
+		goto exit_cleanup;
 	}
 
 	/* Handle error returned by firmware */
-- 
2.47.3

[PATCH net-next V0.5 5/5] eth: fbnic: Update RX mbox timeout value

[Mohsin Bashir]

While waiting for completions on read requests, driver is using
different timeout values for different messages. Make use of a single
timeout value.

Introduce a wrapper function to handle the wait, which also simplify
maintaining the 80 char line limit.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Mohsin Bashir <mohsin.bashr@gmail.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_devlink.c | 8 ++++----
 drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c | 2 +-
 drivers/net/ethernet/meta/fbnic/fbnic_fw.h      | 8 ++++++++
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
index b62b1d5b1453..193f554717b3 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
@@ -178,7 +178,7 @@ fbnic_flash_start(struct fbnic_dev *fbd, struct pldmfw_component *component)
 		goto cmpl_free;
 
 	/* Wait for firmware to ack firmware upgrade start */
-	if (wait_for_completion_timeout(&cmpl->done, 10 * HZ))
+	if (!fbnic_mbx_wait_for_cmpl(cmpl))
 		err = cmpl->result;
 	else
 		err = -ETIMEDOUT;
@@ -252,7 +252,7 @@ fbnic_flash_component(struct pldmfw *context,
 		goto err_no_msg;
 
 	while (offset < size) {
-		if (!wait_for_completion_timeout(&cmpl->done, 15 * HZ)) {
+		if (!fbnic_mbx_wait_for_cmpl(cmpl)) {
 			err = -ETIMEDOUT;
 			break;
 		}
@@ -390,7 +390,7 @@ static int fbnic_fw_reporter_dump(struct devlink_health_reporter *reporter,
 				   "Failed to transmit core dump info msg");
 		goto cmpl_free;
 	}
-	if (!wait_for_completion_timeout(&fw_cmpl->done, 2 * HZ)) {
+	if (!fbnic_mbx_wait_for_cmpl(fw_cmpl)) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "Timed out waiting on core dump info");
 		err = -ETIMEDOUT;
@@ -447,7 +447,7 @@ static int fbnic_fw_reporter_dump(struct devlink_health_reporter *reporter,
 				goto cmpl_cleanup;
 		}
 
-		if (wait_for_completion_timeout(&fw_cmpl->done, 2 * HZ)) {
+		if (!fbnic_mbx_wait_for_cmpl(fw_cmpl)) {
 			reinit_completion(&fw_cmpl->done);
 		} else {
 			NL_SET_ERR_MSG_FMT_MOD(extack,
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index 693ebdf38705..61b8005a0db5 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -1671,7 +1671,7 @@ fbnic_get_module_eeprom_by_page(struct net_device *netdev,
 		goto exit_free;
 	}
 
-	if (!wait_for_completion_timeout(&fw_cmpl->done, 2 * HZ)) {
+	if (!fbnic_mbx_wait_for_cmpl(fw_cmpl)) {
 		err = -ETIMEDOUT;
 		NL_SET_ERR_MSG_MOD(extack,
 				   "Timed out waiting for firmware response");
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
index 1ecd777aaada..6b3fb163d381 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
@@ -4,6 +4,7 @@
 #ifndef _FBNIC_FW_H_
 #define _FBNIC_FW_H_
 
+#include <linux/completion.h>
 #include <linux/if_ether.h>
 #include <linux/types.h>
 
@@ -36,6 +37,7 @@ struct fbnic_fw_mbx {
  *                       + INDEX_SZ))
  */
 #define FBNIC_FW_MAX_LOG_HISTORY		14
+#define FBNIC_MBX_RX_TO_SEC			10
 
 struct fbnic_fw_ver {
 	u32 version;
@@ -129,6 +131,12 @@ struct fbnic_fw_completion *__fbnic_fw_alloc_cmpl(u32 msg_type,
 struct fbnic_fw_completion *fbnic_fw_alloc_cmpl(u32 msg_type);
 void fbnic_fw_put_cmpl(struct fbnic_fw_completion *cmpl_data);
 
+static inline bool fbnic_mbx_wait_for_cmpl(struct fbnic_fw_completion *cmpl)
+{
+	return wait_for_completion_timeout(&cmpl->done,
+					   FBNIC_MBX_RX_TO_SEC * HZ);
+}
+
 #define fbnic_mk_full_fw_ver_str(_rev_id, _delim, _commit, _str, _str_sz) \
 do {									\
 	const u32 __rev_id = _rev_id;					\
-- 
2.47.3

Re: [PATCH net-next V0.5 0/5] eth: fbnic: Update IPC mailbox support

[Mohsin Bashir]

> Update IPC mailbox support for fbnic to cater for several changes.
> 
> Mohsin Bashir (5):
>    eth: fbnic: Use GFP_KERNEL to allocting mbx pages
>    eth: fbnic: Allocate all pages for RX mailbox
>    eth: fbnic: Reuse RX mailbox pages
>    eth: fbnic: Remove retry support
>    eth: fbnic: Update RX mbox timeout value


The version information in the patches is leftover from internal. Please 
ignore the version information. This is V1 of this series.

Re: [net-next,V0.5,4/5] eth: fbnic: Remove retry support

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least pausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
    eth: fbnic: Update RX mbox timeout value

This commit unifies timeout values for firmware mailbox completion waits
by introducing a wrapper function fbnic_mbx_wait_for_cmpl(). However,
there appear to be two regressions where the condition logic was
accidentally inverted.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> index 1ecd777aaada..6b3fb163d381 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h

[ ... ]

> +static inline bool fbnic_mbx_wait_for_cmpl(struct fbnic_fw_completion *cmpl)
> +{
> +	return wait_for_completion_timeout(&cmpl->done,
> +					   FBNIC_MBX_RX_TO_SEC * HZ);
> +}

The wrapper returns the same value as wait_for_completion_timeout():
non-zero on success, zero on timeout.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> index b62b1d5b1453..193f554717b3 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> @@ -178,7 +178,7 @@ fbnic_flash_start(struct fbnic_dev *fbd, struct pldmfw_component *component)
>  		goto cmpl_free;
>
>  	/* Wait for firmware to ack firmware upgrade start */
> -	if (wait_for_completion_timeout(&cmpl->done, 10 * HZ))
> +	if (!fbnic_mbx_wait_for_cmpl(cmpl))
>  		err = cmpl->result;
>  	else
>  		err = -ETIMEDOUT;

Was the negation here intentional? The original code sets err to
cmpl->result when the completion succeeds (wait returns non-zero). With
the added negation, it now sets err to cmpl->result when the wait times
out (returns zero), and sets err to -ETIMEDOUT when the wait succeeds.
This appears to invert the success and timeout handling.

[ ... ]

> @@ -447,7 +447,7 @@ static int fbnic_fw_reporter_dump(struct devlink_health_reporter *reporter,
>  				goto cmpl_cleanup;
>  		}
>
> -		if (wait_for_completion_timeout(&fw_cmpl->done, 2 * HZ)) {
> +		if (!fbnic_mbx_wait_for_cmpl(fw_cmpl)) {
>  			reinit_completion(&fw_cmpl->done);
>  		} else {
>  			NL_SET_ERR_MSG_FMT_MOD(extack,
>  					       "Timed out waiting on core dump (%d/%d)",

Similarly here, the original code calls reinit_completion() when the wait
succeeds (to prepare for the next iteration), and reports "Timed out" in
the else branch when it fails. With the negation added, reinit_completion()
is now called on timeout, and the "Timed out" error message is reported
when the wait actually succeeds. Should the negation be removed from this
call site as well?
-- 
pw-bot: cr

Re: [net-next,V0.5,5/5] eth: fbnic: Update RX mbox timeout value

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least pausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
eth: fbnic: Update RX mbox timeout value

This commit unifies timeout values for mailbox RX read requests and
introduces a wrapper function fbnic_mbx_wait_for_cmpl() to simplify the
completion wait calls.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> index 1ecd777aaada..6b3fb163d381 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.h
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.h

[ ... ]

> +static inline bool fbnic_mbx_wait_for_cmpl(struct fbnic_fw_completion *cmpl)
> +{
> +	return wait_for_completion_timeout(&cmpl->done,
> +					   FBNIC_MBX_RX_TO_SEC * HZ);
> +}

The wrapper returns the result of wait_for_completion_timeout() which is
non-zero (remaining jiffies) on success and zero on timeout.

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> index b62b1d5b1453..193f554717b3 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_devlink.c
> @@ -178,7 +178,7 @@ fbnic_flash_start(struct fbnic_dev *fbd, struct pldmfw_component *component)
>  		goto cmpl_free;
>
>  	/* Wait for firmware to ack firmware upgrade start */
> -	if (wait_for_completion_timeout(&cmpl->done, 10 * HZ))
> +	if (!fbnic_mbx_wait_for_cmpl(cmpl))
>  		err = cmpl->result;
>  	else
>  		err = -ETIMEDOUT;

Is the condition inverted here? The original code enters the first branch
when wait_for_completion_timeout() returns non-zero (success). The new
code enters the first branch when fbnic_mbx_wait_for_cmpl() returns zero
(timeout) due to the added negation. This would cause successful
completions to return -ETIMEDOUT.

[ ... ]

> @@ -447,7 +447,7 @@ static int fbnic_fw_reporter_dump(struct devlink_health_reporter *reporter,
>  				goto cmpl_cleanup;
>  		}
>
> -		if (wait_for_completion_timeout(&fw_cmpl->done, 2 * HZ)) {
> +		if (!fbnic_mbx_wait_for_cmpl(fw_cmpl)) {
>  			reinit_completion(&fw_cmpl->done);
>  		} else {
>  			NL_SET_ERR_MSG_FMT_MOD(extack,

Similar issue in this loop - the original code calls reinit_completion()
on success (non-zero return) and goes to error handling on timeout (zero
return). With the added negation, success now triggers the error path
setting err = -ETIMEDOUT, while timeout triggers reinit_completion().