[PATCH net 0/2] bnxt_en: Fix MAX_SKB_FRAGS > 30

[PATCH net 0/2] bnxt_en: Fix MAX_SKB_FRAGS > 30

[Michael Chan]

The driver was written with the assumption that MAX_SKB_FRAGS could
not exceed what the NIC can support.  About 2 years ago,
CONFIG_MAX_SKB_FRAGS was added.  The value can exceed what the NIC
can support and it may cause TX timeout.  These 2 patches will fix
the issue.

Michael Chan (2):
  bnxt_en: Mask the bd_cnt field in the TX BD properly
  bnxt_en: Linearize TX SKB if the fragments exceed the max

 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 15 +++++++++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  6 ++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  3 +--
 3 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.30.1

[PATCH net 1/2] bnxt_en: Mask the bd_cnt field in the TX BD properly

[Michael Chan]

The bd_cnt field in the TX BD specifies the total number of BDs for
the TX packet.  The bd_cnt field has 5 bits and the maximum number
supported is 32 with the value 0.

CONFIG_MAX_SKB_FRAGS can be modified and the total number of SKB
fragments can approach or exceed the maximum supported by the chip.
Add a macro to properly mask the bd_cnt field so that the value 32
will be properly masked and set to 0 in the bd_cnd field.

Without this patch, the out-of-range bd_cnt value will corrupt the
TX BD and may cause TX timeout.

The next patch will check for values exceeding 32.

Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 4 ++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     | 2 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 3 +--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 0ddc3d41e2d8..158e9789c1f4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -564,7 +564,7 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 					TX_BD_FLAGS_LHINT_512_AND_SMALLER |
 					TX_BD_FLAGS_COAL_NOW |
 					TX_BD_FLAGS_PACKET_END |
-					(2 << TX_BD_FLAGS_BD_CNT_SHIFT));
+					TX_BD_CNT(2));
 
 		if (skb->ip_summed == CHECKSUM_PARTIAL)
 			tx_push1->tx_bd_hsize_lflags =
@@ -639,7 +639,7 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	dma_unmap_addr_set(tx_buf, mapping, mapping);
 	flags = (len << TX_BD_LEN_SHIFT) | TX_BD_TYPE_LONG_TX_BD |
-		((last_frag + 2) << TX_BD_FLAGS_BD_CNT_SHIFT);
+		TX_BD_CNT(last_frag + 2);
 
 	txbd->tx_bd_haddr = cpu_to_le64(mapping);
 	txbd->tx_bd_opaque = SET_TX_OPAQUE(bp, txr, prod, 2 + last_frag);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 2373f423a523..3b4a044db73e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -82,6 +82,8 @@ struct tx_bd {
 #define TX_OPAQUE_PROD(bp, opq)	((TX_OPAQUE_IDX(opq) + TX_OPAQUE_BDS(opq)) &\
 				 (bp)->tx_ring_mask)
 
+#define TX_BD_CNT(n)	(((n) << TX_BD_FLAGS_BD_CNT_SHIFT) & TX_BD_FLAGS_BD_CNT)
+
 struct tx_bd_ext {
 	__le32 tx_bd_hsize_lflags;
 	#define TX_BD_FLAGS_TCP_UDP_CHKSUM			(1 << 0)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 299822cacca4..d71bad3cfd6b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -48,8 +48,7 @@ struct bnxt_sw_tx_bd *bnxt_xmit_bd(struct bnxt *bp,
 		tx_buf->page = virt_to_head_page(xdp->data);
 
 	txbd = &txr->tx_desc_ring[TX_RING(bp, prod)][TX_IDX(prod)];
-	flags = (len << TX_BD_LEN_SHIFT) |
-		((num_frags + 1) << TX_BD_FLAGS_BD_CNT_SHIFT) |
+	flags = (len << TX_BD_LEN_SHIFT) | TX_BD_CNT(num_frags + 1) |
 		bnxt_lhint_arr[len >> 9];
 	txbd->tx_bd_len_flags_type = cpu_to_le32(flags);
 	txbd->tx_bd_opaque = SET_TX_OPAQUE(bp, txr, prod, 1 + num_frags);
-- 
2.30.1

[PATCH net 2/2] bnxt_en: Linearize TX SKB if the fragments exceed the max

[Michael Chan]

If skb_shinfo(skb)->nr_frags excceds what the chip can support,
linearize the SKB and warn once to let the user know.
net.core.max_skb_frags can be lowered, for example, to avoid the
issue.

Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS")
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 11 +++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 158e9789c1f4..2cd79b59cf00 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -485,6 +485,17 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	txr = &bp->tx_ring[bp->tx_ring_map[i]];
 	prod = txr->tx_prod;
 
+#if (MAX_SKB_FRAGS > TX_MAX_FRAGS)
+	if (skb_shinfo(skb)->nr_frags > TX_MAX_FRAGS) {
+		netdev_warn_once(dev, "SKB has too many (%d) fragments, max supported is %d.  SKB will be linearized.\n",
+				 skb_shinfo(skb)->nr_frags, TX_MAX_FRAGS);
+		if (skb_linearize(skb)) {
+			dev_kfree_skb_any(skb);
+			dev_core_stats_tx_dropped_inc(dev);
+			return NETDEV_TX_OK;
+		}
+	}
+#endif
 	free_size = bnxt_tx_avail(bp, txr);
 	if (unlikely(free_size < skb_shinfo(skb)->nr_frags + 2)) {
 		/* We must have raced with NAPI cleanup */
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 3b4a044db73e..d621fb621f30 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -84,6 +84,10 @@ struct tx_bd {
 
 #define TX_BD_CNT(n)	(((n) << TX_BD_FLAGS_BD_CNT_SHIFT) & TX_BD_FLAGS_BD_CNT)
 
+#define TX_MAX_BD_CNT	32
+
+#define TX_MAX_FRAGS		(TX_MAX_BD_CNT - 2)
+
 struct tx_bd_ext {
 	__le32 tx_bd_hsize_lflags;
 	#define TX_BD_FLAGS_TCP_UDP_CHKSUM			(1 << 0)
-- 
2.30.1

Re: [PATCH net 1/2] bnxt_en: Mask the bd_cnt field in the TX BD properly

[Simon Horman]

On Fri, Mar 21, 2025 at 02:16:38PM -0700, Michael Chan wrote:
> The bd_cnt field in the TX BD specifies the total number of BDs for
> the TX packet.  The bd_cnt field has 5 bits and the maximum number
> supported is 32 with the value 0.
> 
> CONFIG_MAX_SKB_FRAGS can be modified and the total number of SKB
> fragments can approach or exceed the maximum supported by the chip.
> Add a macro to properly mask the bd_cnt field so that the value 32
> will be properly masked and set to 0 in the bd_cnd field.
> 
> Without this patch, the out-of-range bd_cnt value will corrupt the
> TX BD and may cause TX timeout.
> 
> The next patch will check for values exceeding 32.
> 
> Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS")
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 2/2] bnxt_en: Linearize TX SKB if the fragments exceed the max

[Simon Horman]

On Fri, Mar 21, 2025 at 02:16:39PM -0700, Michael Chan wrote:
> If skb_shinfo(skb)->nr_frags excceds what the chip can support,
> linearize the SKB and warn once to let the user know.
> net.core.max_skb_frags can be lowered, for example, to avoid the
> issue.
> 
> Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS")
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 1/2] bnxt_en: Mask the bd_cnt field in the TX BD properly

[Jakub Kicinski]

On Fri, 21 Mar 2025 14:16:38 -0700 Michael Chan wrote:
> The bd_cnt field in the TX BD specifies the total number of BDs for
> the TX packet.  The bd_cnt field has 5 bits and the maximum number
> supported is 32 with the value 0.
> 
> CONFIG_MAX_SKB_FRAGS can be modified and the total number of SKB
> fragments can approach or exceed the maximum supported by the chip.
> Add a macro to properly mask the bd_cnt field so that the value 32
> will be properly masked and set to 0 in the bd_cnd field.
> 
> Without this patch, the out-of-range bd_cnt value will corrupt the
> TX BD and may cause TX timeout.
> 
> The next patch will check for values exceeding 32.

Could you clarify how this patch improves things, exactly?
Patch 2/2 looks like the real fix, silently truncating 
the number of frags does not seem to make anything correct..
-- 
pw-bot: cr

Re: [PATCH net 1/2] bnxt_en: Mask the bd_cnt field in the TX BD properly

[Michael Chan]

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

On Mon, Mar 24, 2025 at 2:12 PM Jakub Kicinski <kuba@kernel.org> wrote:

> Could you clarify how this patch improves things, exactly?
> Patch 2/2 looks like the real fix, silently truncating
> the number of frags does not seem to make anything correct..

This patch fixes the value 32 because the hardware treats the value 0
(5 bits of 0) to be 32.

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

Re: [PATCH net 1/2] bnxt_en: Mask the bd_cnt field in the TX BD properly

[Jakub Kicinski]

On Mon, 24 Mar 2025 14:16:12 -0700 Michael Chan wrote:
> > Could you clarify how this patch improves things, exactly?
> > Patch 2/2 looks like the real fix, silently truncating
> > the number of frags does not seem to make anything correct..  
> 
> This patch fixes the value 32 because the hardware treats the value 0
> (5 bits of 0) to be 32.

Sorry, must be jetlag.. But your one sentence would be a much better
commit description than what's currently there ;)
-- 
pw-bot: new

Re: [PATCH net 0/2] bnxt_en: Fix MAX_SKB_FRAGS > 30

[patchwork-bot+netdevbpf]

Hello:

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

On Fri, 21 Mar 2025 14:16:37 -0700 you wrote:
> The driver was written with the assumption that MAX_SKB_FRAGS could
> not exceed what the NIC can support.  About 2 years ago,
> CONFIG_MAX_SKB_FRAGS was added.  The value can exceed what the NIC
> can support and it may cause TX timeout.  These 2 patches will fix
> the issue.
> 
> Michael Chan (2):
>   bnxt_en: Mask the bd_cnt field in the TX BD properly
>   bnxt_en: Linearize TX SKB if the fragments exceed the max
> 
> [...]

Here is the summary with links:
  - [net,1/2] bnxt_en: Mask the bd_cnt field in the TX BD properly
    https://git.kernel.org/netdev/net/c/107b25db6112
  - [net,2/2] bnxt_en: Linearize TX SKB if the fragments exceed the max
    https://git.kernel.org/netdev/net/c/b91e82129400

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

Re: [PATCH net 2/2] bnxt_en: Linearize TX SKB if the fragments exceed the max

[Eric Dumazet]

On Fri, Mar 21, 2025 at 2:17 PM Michael Chan <michael.chan@broadcom.com> wrote:
>
> If skb_shinfo(skb)->nr_frags excceds what the chip can support,
> linearize the SKB and warn once to let the user know.
> net.core.max_skb_frags can be lowered, for example, to avoid the
> issue.
>
> Fixes: 3948b05950fd ("net: introduce a config option to tweak MAX_SKB_FRAGS")
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 11 +++++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h |  4 ++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 158e9789c1f4..2cd79b59cf00 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -485,6 +485,17 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
>         txr = &bp->tx_ring[bp->tx_ring_map[i]];
>         prod = txr->tx_prod;
>
> +#if (MAX_SKB_FRAGS > TX_MAX_FRAGS)
> +       if (skb_shinfo(skb)->nr_frags > TX_MAX_FRAGS) {
> +               netdev_warn_once(dev, "SKB has too many (%d) fragments, max supported is %d.  SKB will be linearized.\n",
> +                                skb_shinfo(skb)->nr_frags, TX_MAX_FRAGS);
> +               if (skb_linearize(skb)) {
> +                       dev_kfree_skb_any(skb);
> +                       dev_core_stats_tx_dropped_inc(dev);
> +                       return NETDEV_TX_OK;
> +               }
> +       }
> +#endif

Hi Michael

Sorry for the delay. I just saw your patch.

GSO would be more efficient and less likely to fail under memory pressure.

Could you test the following patch for me ?

Thanks !

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 3a9ffaaf60ae810bf8772b0cad0da45bc4ec9a05..c2a831efa969742711f5afdc7b24ba2c450cc595
100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -13787,6 +13787,10 @@ static netdev_features_t
bnxt_features_check(struct sk_buff *skb,
        u8 *l4_proto;

        features = vlan_features_check(skb, features);
+#if MAX_SKB_FRAGS > TX_MAX_FRAGS
+       if (skb_shinfo(skb)->nr_frags > TX_MAX_FRAGS)
+               features &= ~NETIF_F_GSO_MASK;
+#endif
        switch (vlan_get_protocol(skb)) {
        case htons(ETH_P_IP):
                if (!skb->encapsulation)

Re: [PATCH net 2/2] bnxt_en: Linearize TX SKB if the fragments exceed the max

[Michael Chan]

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

On Mon, May 5, 2025 at 8:37 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Fri, Mar 21, 2025 at 2:17 PM Michael Chan <michael.chan@broadcom.com> wrote:
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 11 +++++++++++
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.h |  4 ++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 158e9789c1f4..2cd79b59cf00 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -485,6 +485,17 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >         txr = &bp->tx_ring[bp->tx_ring_map[i]];
> >         prod = txr->tx_prod;
> >
> > +#if (MAX_SKB_FRAGS > TX_MAX_FRAGS)
> > +       if (skb_shinfo(skb)->nr_frags > TX_MAX_FRAGS) {
> > +               netdev_warn_once(dev, "SKB has too many (%d) fragments, max supported is %d.  SKB will be linearized.\n",
> > +                                skb_shinfo(skb)->nr_frags, TX_MAX_FRAGS);
> > +               if (skb_linearize(skb)) {
> > +                       dev_kfree_skb_any(skb);
> > +                       dev_core_stats_tx_dropped_inc(dev);
> > +                       return NETDEV_TX_OK;
> > +               }
> > +       }
> > +#endif
>
> Hi Michael
>
> Sorry for the delay. I just saw your patch.
>
> GSO would be more efficient and less likely to fail under memory pressure.
>
> Could you test the following patch for me ?
>
> Thanks !

The patch works.  I forced a smaller TX_MAX_FRAGS to easily test it.
But we now skip the warn_once() above since we intercept it earlier.
 Should we add a warn_once() here also?  Thanks.

>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 3a9ffaaf60ae810bf8772b0cad0da45bc4ec9a05..c2a831efa969742711f5afdc7b24ba2c450cc595
> 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -13787,6 +13787,10 @@ static netdev_features_t
> bnxt_features_check(struct sk_buff *skb,
>         u8 *l4_proto;
>
>         features = vlan_features_check(skb, features);
> +#if MAX_SKB_FRAGS > TX_MAX_FRAGS
> +       if (skb_shinfo(skb)->nr_frags > TX_MAX_FRAGS)
> +               features &= ~NETIF_F_GSO_MASK;
> +#endif
>         switch (vlan_get_protocol(skb)) {
>         case htons(ETH_P_IP):
>                 if (!skb->encapsulation)

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

Re: [PATCH net 2/2] bnxt_en: Linearize TX SKB if the fragments exceed the max

[Eric Dumazet]

On Mon, May 5, 2025 at 11:34 AM Michael Chan <michael.chan@broadcom.com> wrote:

> The patch works.  I forced a smaller TX_MAX_FRAGS to easily test it.
> But we now skip the warn_once() above since we intercept it earlier.
>  Should we add a warn_once() here also?  Thanks.
>

I do not think a warn_once() is needed here, GSO is pretty fast, in
the unlikely case this would be hit.

You could add a per tx queue counter, that ethtool -S could fetch for
curious minds.