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

[Andrew Lunn <andrew@lunn.ch>]

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@redhat.com> 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.

> > > +     #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?

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.

     Andrew