[PATCH] net/txgbe: fix AML mailbox lock acquisition

[PATCH] net/txgbe: fix AML mailbox lock acquisition

[Stephen Hemminger]

The try-lock spin loop in txgbe_host_interface_command_aml() has
the condition inverted. rte_atomic32_test_and_set returns non-zero
on successful acquisition (0 -> 1), zero when the lock was already
held. Walk through the two cases:

  swfw_busy was 0 (free): test_and_set returns 1, sets to 1, loop
  body runs and sleeps 1ms. Next iteration finds 1, returns 0, loop
  exits. Lock held, after an unnecessary 1ms sleep.

  swfw_busy was 1 (busy): test_and_set returns 0, loop exits
  immediately. The caller proceeds without holding the lock,
  racing with the in-flight host interface command.

Invert the condition so the loop spins while the lock remains held
and exits only when acquisition succeeds, matching the intent of
the surrounding timeout machinery.

Fixes: 6a139ade82e7 ("net/txgbe: add new SW-FW mailbox interface")
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/txgbe/base/txgbe_mng.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/txgbe/base/txgbe_mng.c b/drivers/net/txgbe/base/txgbe_mng.c
index a1974820b6..dcd8f58a68 100644
--- a/drivers/net/txgbe/base/txgbe_mng.c
+++ b/drivers/net/txgbe/base/txgbe_mng.c
@@ -185,7 +185,7 @@ txgbe_host_interface_command_aml(struct txgbe_hw *hw, u32 *buffer,
 	}
 
 	/* try to get lock */
-	while (rte_atomic32_test_and_set(&hw->swfw_busy)) {
+	while (rte_atomic32_test_and_set(&hw->swfw_busy) == 0) {
 		timeout--;
 		if (!timeout)
 			return TXGBE_ERR_TIMEOUT;
-- 
2.53.0

RE: [PATCH] net/txgbe: fix AML mailbox lock acquisition

[Zaiyu Wang]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, May 22, 2026 1:01 PM
> To: dev@dpdk.org; dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; stable@dpdk.org;
> Jiawen Wu <jiawenwu@trustnetic.com>; Zaiyu Wang
> <zaiyuwang@trustnetic.com>; Stephen Hemminger
> <stephen@networkplumber.org>; stable@dpdk.org; Jiawen Wu
> <jiawenwu@trustnetic.com>; Zaiyu Wang <zaiyuwang@trustnetic.com>
> Subject: [PATCH] net/txgbe: fix AML mailbox lock acquisition
> 
> The try-lock spin loop in txgbe_host_interface_command_aml() has the
condition
> inverted. rte_atomic32_test_and_set returns non-zero on successful
acquisition (0
> -> 1), zero when the lock was already held. Walk through the two cases:
> 
>   swfw_busy was 0 (free): test_and_set returns 1, sets to 1, loop
>   body runs and sleeps 1ms. Next iteration finds 1, returns 0, loop
>   exits. Lock held, after an unnecessary 1ms sleep.
> 
>   swfw_busy was 1 (busy): test_and_set returns 0, loop exits
>   immediately. The caller proceeds without holding the lock,
>   racing with the in-flight host interface command.
> 
> Invert the condition so the loop spins while the lock remains held and
exits only
> when acquisition succeeds, matching the intent of the surrounding timeout
> machinery.
> 
> Fixes: 6a139ade82e7 ("net/txgbe: add new SW-FW mailbox interface")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/txgbe/base/txgbe_mng.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/txgbe/base/txgbe_mng.c
> b/drivers/net/txgbe/base/txgbe_mng.c
> index a1974820b6..dcd8f58a68 100644
> --- a/drivers/net/txgbe/base/txgbe_mng.c
> +++ b/drivers/net/txgbe/base/txgbe_mng.c
> @@ -185,7 +185,7 @@ txgbe_host_interface_command_aml(struct txgbe_hw
> *hw, u32 *buffer,
>  	}
> 
>  	/* try to get lock */
> -	while (rte_atomic32_test_and_set(&hw->swfw_busy)) {
> +	while (rte_atomic32_test_and_set(&hw->swfw_busy) == 0) {
>  		timeout--;
>  		if (!timeout)
>  			return TXGBE_ERR_TIMEOUT;
> --
> 2.53.0

Thanks for the patch and the detailed analysis.

The fix is correct. This code was originally ported from our out-of-tree
Linux kernel driver, where it used a classic test_and_set_bit() that returns
the old bit value (0 on success). When adapting it for DPDK, we overlooked
that rte_atomic32_test_and_set() returns the opposite semantic.

Acked-by: Zaiyu Wang <zaiyuwang@trustnetic.com>