Re: [v4, net-next 2/7] bng_en: Add RX support

[Bhargava Chenna Marreddy]

On Tue, Jan 13, 2026 at 2:41 AM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> On Tue, Jan 13, 2026 at 01:14:33AM +0530, Bhargava Chenna Marreddy wrote:
> > On Thu, Jan 8, 2026 at 3:15 PM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
> > >
> > > On 1/5/26 8:21 AM, Bhargava Marreddy wrote:
> > > > diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_hw_def.h b/drivers/net/ethernet/broadcom/bnge/bnge_hw_def.h
> > > > new file mode 100644
> > > > index 000000000000..4da4259095fa
> > > > --- /dev/null
> > > > +++ b/drivers/net/ethernet/broadcom/bnge/bnge_hw_def.h
> > > > @@ -0,0 +1,198 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/* Copyright (c) 2025 Broadcom */
> > > > +
> > > > +#ifndef _BNGE_HW_DEF_H_
> > > > +#define _BNGE_HW_DEF_H_
> > > > +
> > > > +struct tx_bd_ext {
> > > > +     __le32 tx_bd_hsize_lflags;
> > > > +     #define TX_BD_FLAGS_TCP_UDP_CHKSUM                      (1 << 0)
> > >
> > > Please use BIT()
> >
> > Simon Horman raised a similar point. However, some hardware BD values
> > use non-contiguous bits that make BIT() and GENMASK() overly complex.
> > We believe the current definitions better reflect the hardware spec.
> > Please let us know if you’d still prefer a different approach.
>
> You probably want to use BIT() for all fields which are
> contiguous. Doing something different then marks the other fields are
> somehow special and need treating with care.

Thanks, Andrew. Will address in the next version.

>
> > > > +     #define TX_BD_FLAGS_IP_CKSUM                            (1 << 1)
> > > > +     #define TX_BD_FLAGS_NO_CRC                              (1 << 2)
> > > > +     #define TX_BD_FLAGS_STAMP                               (1 << 3)
> > > > +     #define TX_BD_FLAGS_T_IP_CHKSUM                         (1 << 4)
> > > > +     #define TX_BD_FLAGS_LSO                                 (1 << 5)
> > > > +     #define TX_BD_FLAGS_IPID_FMT                            (1 << 6)
> > > > +     #define TX_BD_FLAGS_T_IPID                              (1 << 7)
> > > > +     #define TX_BD_HSIZE                                     (0xff << 16)
> > > > +      #define TX_BD_HSIZE_SHIFT                               16
> > >
> > > I'm quite suprised checkpatch does not complain, but the above
> > > indentation is IMHO quite messy.
>
> > > please move the macro definition before the struct and avoid mixing
> > > whitespaces and tabs.
> >
> > Since these are hardware-defined structs, we kept the #defines with
> > their members to make the mapping clear.
> > Any concerns with this?

Ack. Will fix it in the next spin.

>
> The names should make it clear. The structure member can be called
> tx_bd_flags, and the bits are TX_BD_FLAGS_IP_CKSUM etc.
>
> > > > +static struct sk_buff *bnge_copy_skb(struct bnge_napi *bnapi, u8 *data,
> > > > +                                  unsigned int len, dma_addr_t mapping)
> > > > +{
> > > > +     struct bnge_net *bn = bnapi->bn;
> > > > +     struct bnge_dev *bd = bn->bd;
> > > > +     struct sk_buff *skb;
> > > > +
> > > > +     skb = napi_alloc_skb(&bnapi->napi, len);
> > > > +     if (!skb)
> > > > +             return NULL;
> > > > +
> > > > +     dma_sync_single_for_cpu(bd->dev, mapping, bn->rx_copybreak,
> > > > +                             bn->rx_dir);
> > > > +
> > > > +     memcpy(skb->data - NET_IP_ALIGN, data - NET_IP_ALIGN,
> > > > +            len + NET_IP_ALIGN);
> > >
> > > This works under the assumption that len <=  bn->rx_copybreak; why
> > > syncing the whole 'rx_copybreak' instead of 'len' ?
> >
> > Good point. syncing the actual packet length is more precise and
> > avoids unnecessary cache maintenance.
> > Let us test this on our hardware and get back to you.
>
> When i did this for the FEC, i got a nice performance boost.

Thanks for sharing the results, will fix it in next spin.

Thanks,
Bhargava Marreddy

>
>      Andrew

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature