[PATCH net 1/2] eth: bnxt: fix the wake condition

[PATCH net 1/2] eth: bnxt: fix the wake condition

[Jakub Kicinski]

The down condition should be the negation of the wake condition,
IOW when I moved it from:

 if (cond && wake())
to
 if (__netif_txq_completed_wake(cond))

Cond should have been negated. Flip it now.

This bug leads to occasional crashes with netconsole.
It may also lead to queue never waking up in case BQL is not enabled.

Reported-by: David Wei <davidhwei@meta.com>
Fixes: 08a096780d92 ("bnxt: use new queue try_stop/try_wake macros")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: michael.chan@broadcom.com
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index dcd9367f05af..1f04cd4cfab9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -692,7 +692,7 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
 
 	__netif_txq_completed_wake(txq, nr_pkts, tx_bytes,
 				   bnxt_tx_avail(bp, txr), bp->tx_wake_thresh,
-				   READ_ONCE(txr->dev_state) != BNXT_DEV_STATE_CLOSING);
+				   READ_ONCE(txr->dev_state) == BNXT_DEV_STATE_CLOSING);
 }
 
 static struct page *__bnxt_alloc_rx_page(struct bnxt *bp, dma_addr_t *mapping,
-- 
2.40.1

[PATCH net 2/2] eth: ixgbe: fix the wake condition

[Jakub Kicinski]

Flip the netif_carrier_ok() condition in queue wake logic.
When I moved it to inside __netif_txq_completed_wake()
I missed negating it.

This made the condition ineffective and could probably
lead to crashes.

Fixes: 301f227fc860 ("net: piggy back on the memory barrier in bql when waking queues")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jesse.brandeburg@intel.com
CC: anthony.l.nguyen@intel.com
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 5d83c887a3fc..1726297f2e0d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1256,7 +1256,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
 	if (!__netif_txq_completed_wake(txq, total_packets, total_bytes,
 					ixgbe_desc_unused(tx_ring),
 					TX_WAKE_THRESHOLD,
-					netif_carrier_ok(tx_ring->netdev) &&
+					!netif_carrier_ok(tx_ring->netdev) ||
 					test_bit(__IXGBE_DOWN, &adapter->state)))
 		++tx_ring->tx_stats.restart_queue;
 
-- 
2.40.1

Re: [PATCH net 2/2] eth: ixgbe: fix the wake condition

[Tony Nguyen]

On 6/6/2023 6:08 PM, Jakub Kicinski wrote:
> Flip the netif_carrier_ok() condition in queue wake logic.
> When I moved it to inside __netif_txq_completed_wake()
> I missed negating it.
> 
> This made the condition ineffective and could probably
> lead to crashes.
> 
> Fixes: 301f227fc860 ("net: piggy back on the memory barrier in bql when waking queues")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

LGTM

Reviewed-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Re: [PATCH net 1/2] eth: bnxt: fix the wake condition

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On Tue, Jun 6, 2023 at 6:08 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> The down condition should be the negation of the wake condition,
> IOW when I moved it from:
>
>  if (cond && wake())
> to
>  if (__netif_txq_completed_wake(cond))
>
> Cond should have been negated. Flip it now.
>
> This bug leads to occasional crashes with netconsole.
> It may also lead to queue never waking up in case BQL is not enabled.
>
> Reported-by: David Wei <davidhwei@meta.com>
> Fixes: 08a096780d92 ("bnxt: use new queue try_stop/try_wake macros")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Thanks.
Reviewed-by: Michael Chan <michael.chan@broadcom.com>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net 1/2] eth: bnxt: fix the wake condition

[patchwork-bot+netdevbpf]

Hello:

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

On Tue,  6 Jun 2023 18:08:25 -0700 you wrote:
> The down condition should be the negation of the wake condition,
> IOW when I moved it from:
> 
>  if (cond && wake())
> to
>  if (__netif_txq_completed_wake(cond))
> 
> [...]

Here is the summary with links:
  - [net,1/2] eth: bnxt: fix the wake condition
    https://git.kernel.org/netdev/net/c/649c3fed3673
  - [net,2/2] eth: ixgbe: fix the wake condition
    https://git.kernel.org/netdev/net/c/f0d751973f73

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