| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANXQDtYR6P9+oHXpAzxPk4cE1jSYCFoCbELcWad25h1c6wfmQQ@mail.gmail.com>
Date: Tue, 13 Jan 2026 01:14:33 +0530
From: Bhargava Chenna Marreddy <bhargava.marreddy@...adcom.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
andrew+netdev@...n.ch, horms@...nel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, michael.chan@...adcom.com,
pavan.chebbi@...adcom.com, vsrama-krishna.nemani@...adcom.com,
vikas.gupta@...adcom.com,
Rajashekar Hudumula <rajashekar.hudumula@...adcom.com>
Subject: Re: [v4, net-next 2/7] bng_en: Add RX support
On Thu, Jan 8, 2026 at 3:15 PM Paolo Abeni <pabeni@...hat.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.
>
> > + #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.
Thanks, I’ll fix the indentation (mixing whitespaces and tabs) in the
next version.
>
> 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?
>
> [...]
> > @@ -1756,6 +1757,78 @@ static int bnge_cfg_def_vnic(struct bnge_net *bn)
> > return rc;
> > }
> >
> > +static void bnge_disable_int(struct bnge_net *bn)
> > +{
> > + struct bnge_dev *bd = bn->bd;
> > + int i;
> > +
> > + if (!bn->bnapi)
> > + return;
> > +
> > + for (i = 0; i < bd->nq_nr_rings; i++) {
> > + struct bnge_napi *bnapi = bn->bnapi[i];
> > + struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;
> > + struct bnge_ring_struct *ring = &nqr->ring_struct;
>
> Please respect the reverse christmas tree above.
Thanks! I’ll fix it in the next revision.
>
> > +
> > + if (ring->fw_ring_id != INVALID_HW_RING_ID)
> > + bnge_db_nq(bn, &nqr->nq_db, nqr->nq_raw_cons);
> > + }
> > +}
> > +
> > +static void bnge_disable_int_sync(struct bnge_net *bn)
> > +{
> > + struct bnge_dev *bd = bn->bd;
> > + int i;
> > +
> > + bnge_disable_int(bn);
> > + for (i = 0; i < bd->nq_nr_rings; i++) {
> > + int map_idx = bnge_cp_num_to_irq_num(bn, i);
> > +
> > + synchronize_irq(bd->irq_tbl[map_idx].vector);
> > + }
> > +}
> > +
> > +static void bnge_enable_int(struct bnge_net *bn)
> > +{
> > + struct bnge_dev *bd = bn->bd;
> > + int i;
> > +
> > + for (i = 0; i < bd->nq_nr_rings; i++) {
> > + struct bnge_napi *bnapi = bn->bnapi[i];
> > + struct bnge_nq_ring_info *nqr = &bnapi->nq_ring;
>
> Same here
Thanks! I’ll fix it in the next revision.
> > @@ -298,6 +343,10 @@ struct bnge_cp_ring_info {
> > u8 cp_idx;
> > u32 cp_raw_cons;
> > struct bnge_db_info cp_db;
> > + u8 had_work_done:1;
> > + u8 has_more_work:1;
> > + u8 had_nqe_notify:1;
>
> Any special reasons to use bitfields here? `bool` will generate better
> code, and will not change the struct size.
Thanks, I’ll switch these to bool as suggested.
>
> [...]
> > +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.
>
> > +
> > + dma_sync_single_for_device(bd->dev, mapping, bn->rx_copybreak,
> > + bn->rx_dir);
>
> Why is the above needed?
Since the buffer is being reused, the for_device sync hands ownership
back to the hardware.
This ensures the memory is ready for the next DMA transfer and
maintains consistency on non-coherent architectures.
Any concerns with this?
>
> > +
> > + skb_put(skb, len);
> > +
> > + return skb;
> > +}
> > +
> > +static enum pkt_hash_types bnge_rss_ext_op(struct bnge_net *bn,
> > + struct rx_cmp *rxcmp)
> > +{
> > + u8 ext_op = RX_CMP_V3_HASH_TYPE(bn->bd, rxcmp);
> > +
> > + switch (ext_op) {
> > + case EXT_OP_INNER_4:
> > + case EXT_OP_OUTER_4:
> > + case EXT_OP_INNFL_3:
> > + case EXT_OP_OUTFL_3:
> > + return PKT_HASH_TYPE_L4;
> > + default:
> > + return PKT_HASH_TYPE_L3;
> > + }
> > +}
> > +
> > +static struct sk_buff *bnge_rx_vlan(struct sk_buff *skb, u8 cmp_type,
> > + struct rx_cmp *rxcmp,
> > + struct rx_cmp_ext *rxcmp1)
> > +{
> > + __be16 vlan_proto;
> > + u16 vtag;
> > +
> > + if (cmp_type == CMP_TYPE_RX_L2_CMP) {
> > + __le32 flags2 = rxcmp1->rx_cmp_flags2;
> > + u32 meta_data;
> > +
> > + if (!(flags2 & cpu_to_le32(RX_CMP_FLAGS2_META_FORMAT_VLAN)))
> > + return skb;
> > +
> > + meta_data = le32_to_cpu(rxcmp1->rx_cmp_meta_data);
> > + vtag = meta_data & RX_CMP_FLAGS2_METADATA_TCI_MASK;
> > + vlan_proto =
> > + htons(meta_data >> RX_CMP_FLAGS2_METADATA_TPID_SFT);
> > + if (eth_type_vlan(vlan_proto))
> > + __vlan_hwaccel_put_tag(skb, vlan_proto, vtag);
> > + else
> > + goto vlan_err;
> > + } else if (cmp_type == CMP_TYPE_RX_L2_V3_CMP) {
> > + if (RX_CMP_VLAN_VALID(rxcmp)) {
> > + u32 tpid_sel = RX_CMP_VLAN_TPID_SEL(rxcmp);
> > +
> > + if (tpid_sel == RX_CMP_METADATA1_TPID_8021Q)
> > + vlan_proto = htons(ETH_P_8021Q);
> > + else if (tpid_sel == RX_CMP_METADATA1_TPID_8021AD)
> > + vlan_proto = htons(ETH_P_8021AD);
> > + else
> > + goto vlan_err;
> > + vtag = RX_CMP_METADATA0_TCI(rxcmp1);
> > + __vlan_hwaccel_put_tag(skb, vlan_proto, vtag);
> > + }
> > + }
> > + return skb;
> > +
> > +vlan_err:
> > + skb_mark_for_recycle(skb);
> > + dev_kfree_skb(skb);
> > + return NULL;
> > +}
> > +
> > +static struct sk_buff *bnge_rx_skb(struct bnge_net *bn,
> > + struct bnge_rx_ring_info *rxr, u16 cons,
> > + void *data, u8 *data_ptr,
> > + dma_addr_t dma_addr,
> > + unsigned int offset_and_len)
> > +{
> > + struct bnge_dev *bd = bn->bd;
> > + u16 prod = rxr->rx_prod;
> > + struct sk_buff *skb;
> > + int err;
> > +
> > + err = bnge_alloc_rx_data(bn, rxr, prod, GFP_ATOMIC);
> > + if (unlikely(err)) {
> > + bnge_reuse_rx_data(rxr, cons, data);
> > + return NULL;
> > + }
> > +
> > + skb = napi_build_skb(data, bn->rx_buf_size);
> > + dma_sync_single_for_cpu(bd->dev, dma_addr, bn->rx_buf_use_size,
> > + bn->rx_dir);
>
> Why you need to sync the whole `rx_buf_use_size` instead of the actual
> packet len?
Good point. Let us test this on our hardware and get back to you.
>
> > + if (!skb) {
> > + page_pool_free_va(rxr->head_pool, data, true);
> > + return NULL;
> > + }
> > +
> > + skb_mark_for_recycle(skb);
> > + skb_reserve(skb, bn->rx_offset);
> > + skb_put(skb, offset_and_len & 0xffff);
>
> It's unclear why you pass 2 different values mangled together and than
> extract only one of them.
Thanks! I’ll fix it in the next revision.
Thanks,
Bhargava Marreddy
>
> /P
>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (5496 bytes)
Powered by blists - more mailing lists