Re: [PATCH net-next] octeontx2-af: Show DMAC filter drop count

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] New debugfs label contradicts the driver's own name for the
  same CGX register.

--

octeontx2-af: Show DMAC filter drop count

This adds a new mac_ops callback, get_dmacflt_dropped_pktcnt, implemented
for CGX (reading CGXX_CMRX_RX_STAT4) and RPM/RPM2 (reading RX_STAT2), and
prints the value in the mac_filter debugfs entry.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> index f5fd6138c352f..3696bec7a8fc1 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> @@ -752,6 +752,16 @@ u64 cgx_features_get(void *cgxd)
>  	return ((struct cgx *)cgxd)->hw_features;
>  }
>  
> +u64 cgx_get_dmacflt_dropped_pktcnt(void *cgxd, int lmac_id)
> +{
> +	struct cgx *cgx = cgxd;
> +
> +	if (!is_lmac_valid(cgx, lmac_id))
> +		return 0;
> +
> +	return cgx_read(cgx, lmac_id, CGXX_CMRX_RX_STAT4);
> +}
> +

[Severity: Medium]
Is CGXX_CMRX_RX_STAT4 really the DMAC filter drop counter?  This driver
already exposes that same register under a different name.

cgx_get_rx_stats() reads:

	*rx_stat =  cgx_read(cgx, lmac_id, CGXX_CMRX_RX_STAT0 + (idx * 8));

For idx == 4 that is 0x070 + 32 == 0x090, i.e. the newly added
CGXX_CMRX_RX_STAT4.  The name printed for it in the cgx_stat debugfs file
is:

drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c:
	[CGX_STAT4]	= "Filtered DMAC0 (NIX-bound) packets",
	[CGX_STAT5]	= "Filtered DMAC0 (NIX-bound) octets",
	[CGX_STAT6]	= "Packets dropped due to RX FIFO full",
	[CGX_STAT7]	= "Octets dropped due to RX FIFO full",
	...
	[CGX_STAT11]	= "NCSI-bound packets dropped",

That table uses "dropped" wording for STAT6/7 and STAT11/12, and the
"Filtered DMAC<n> (<channel>-bound)" wording for STAT4/5 and STAT9/10,
which reads as packets that passed the DMAC filter towards NIX rather than
packets discarded by it.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.h b/drivers/net/ethernet/marvell/octeontx2/af/cgx.h
> index 8411a75dd723f..cd8858c4e41ce 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.h
> @@ -39,6 +39,7 @@
>  #define CGXX_CMRX_INT_ENA_W1S		0x058
>  #define CGXX_CMRX_RX_ID_MAP		0x060
>  #define CGXX_CMRX_RX_STAT0		0x070
> +#define CGXX_CMRX_RX_STAT4		0x090
>  #define CGXX_CMRX_RX_LOGL_XON		0x100

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> index 22ee996768796..f6320d6b959f0 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> @@ -3003,6 +3005,12 @@ static int cgx_print_dmac_flt(struct seq_file *s, int lmac_id)
>  		}
>  	}
>  
> +	mac_ops = get_mac_ops(cgxd);
> +	if (mac_ops && mac_ops->get_dmacflt_dropped_pktcnt) {
> +		drop_cnt = rvu_cgx_get_dmacflt_dropped_pktcnt(cgxd, lmac_id);
> +		seq_printf(s, "\nDMAC filter drop count: %llu\n", drop_cnt);
> +	}
> +

[Severity: Medium]
After this change, the same CGX register is presented to users with two
opposite meanings in two debugfs files of the same driver:
cgx_print_stats() prints it via cgx_rx_stats_fields[CGX_STAT4] as
"Filtered DMAC0 (NIX-bound) packets", while cgx_print_dmac_flt() prints it
as "DMAC filter drop count".

One of the two descriptions has to be wrong.  If the existing table is the
correct one, then any interface passing traffic will show a large and
continuously growing "DMAC filter drop count" that is really the accepted
packet count.

Could the two labels be reconciled, i.e. either correct
cgx_rx_stats_fields[CGX_STAT4] or use a different register for the drop
count here?

The commit message says:

    CGX uses RX_STAT4 for this counter, while RPM/RPM2 use RX_STAT2.

Can a reference to the CGX/RPM register descriptions be added so it is
clear which of the two in-tree descriptions of RX_STAT4 matches the
hardware?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260901042503.1182121-1-nshettyj%40marvell.com