Re: [net-next,v2,2/4] bnxt_en: Implement XDP RSS hash metadata extraction

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
bnxt_en: Implement XDP RSS hash metadata extraction

This commit adds support for extracting RSS hash values and hash types from
hardware completion descriptors in XDP programs for the bnxt_en driver.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> index 85cbeb35681c..1ab3d9f9b3ac 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c

[ ... ]

> @@ -472,3 +472,50 @@ bnxt_xdp_build_skb(struct bnxt *bp, struct sk_buff *skb, u8 num_frags,
>  				  xdp_buff_get_skb_flags(xdp));
>  	return skb;
>  }
> +
> +int bnxt_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash,
> +		     enum xdp_rss_hash_type *rss_type)
> +{
> +	const struct bnxt_xdp_buff *xdp = (void *)ctx;
> +	const struct rx_cmp *rxcmp = xdp->rxcmp;
> +	const struct rx_cmp_ext *rxcmp1 = xdp->rxcmp1;
> +	enum xdp_rss_hash_type hash_type = 0;
> +	u32 itypes;
> +
> +	if (!rxcmp || !RX_CMP_HASH_VALID(rxcmp))
> +		return -ENODATA;
> +
> +	*hash = le32_to_cpu(rxcmp->rx_cmp_rss_hash);
> +
> +	if (!rxcmp1) {
> +		*rss_type = XDP_RSS_TYPE_L2;
> +		return 0;
> +	}
> +
> +	if (xdp->cmp_type == CMP_TYPE_RX_L2_CMP) {
> +		itypes = RX_CMP_ITYPES(rxcmp);
> +		if (rxcmp1->rx_cmp_flags2 &
> +		    cpu_to_le32(RX_CMP_FLAGS2_IP_TYPE)) {
> +			hash_type |= XDP_RSS_TYPE_L3_IPV6;
> +		} else {
> +			hash_type |= XDP_RSS_TYPE_L3_IPV4;
> +		}
> +
> +		switch (itypes) {
> +		case RX_CMP_FLAGS_ITYPE_TCP:
> +			hash_type |= XDP_RSS_L4 | XDP_RSS_L4_TCP;
> +			break;
> +		case RX_CMP_FLAGS_ITYPE_UDP:
> +			hash_type |= XDP_RSS_L4 | XDP_RSS_L4_UDP;
> +			break;
> +		case RX_CMP_FLAGS_ITYPE_ICMP:
> +			hash_type |= XDP_RSS_L4 | XDP_RSS_L4_ICMP;
> +			break;
> +		default:
> +			break;
> +		}
> +	}

What happens when xdp->cmp_type is CMP_TYPE_RX_L2_V3_CMP instead of
CMP_TYPE_RX_L2_CMP? Looking at bnxt_rx_pkt() in bnxt.c, when XDP is attached,
the cmp_type field can be set to CMP_TYPE_RX_L2_V3_CMP on P5+ hardware:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_rx_pkt() {
    ...
    cmp_type = RX_CMP_TYPE(rxcmp);
    ...
    if (bp->xdp_prog) {
        ...
        bnxt_xdp.cmp_type = cmp_type;
        ...
    }
}

In the V3 case, this function returns success with *hash set correctly but
leaves *rss_type as 0 (XDP_RSS_TYPE_L2/XDP_RSS_TYPE_NONE) because the type
classification logic is skipped.

The existing SKB path handles V3 completions differently:

drivers/net/ethernet/broadcom/bnxt/bnxt.c:bnxt_rx_pkt() {
    ...
    if (cmp_type == CMP_TYPE_RX_L2_V3_CMP) {
        bnxt_rss_ext_op(bp, rxcmp1, skb);
    } else {
        skb_set_hash(skb, ...);
    }
}

Where bnxt_rss_ext_op() uses RX_CMP_V3_HASH_TYPE() for proper L3/L4
classification.

Should bnxt_xdp_rx_hash() handle CMP_TYPE_RX_L2_V3_CMP completions with
equivalent V3 hash type decoding, or return -ENODATA for V3 completions to
avoid returning success with misleading type information?

> +
> +	*rss_type = hash_type;
> +	return 0;
> +}
-- 
pw-bot: cr