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

[Paolo Abeni <pabeni@redhat.com>]

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()

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

[...]
> @@ -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.

> +
> +		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
> @@ -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.

[...]
> +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' ?

> +
> +	dma_sync_single_for_device(bd->dev, mapping, bn->rx_copybreak,
> +				   bn->rx_dir);

Why is the above needed?

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

> +	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.

/P