Re: [net-next,2/5] net: dpaa2-mac: retrieve MAC statistics in one firmware command

[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
---
net: dpaa2-mac: retrieve MAC statistics in one firmware command

The latest MC firmware version added a new command to retrieve all DPMAC
counters in a single firmware call. Use this new command, when possible,
in dpaa2-mac as well. In order to use the dpmac_get_statistics() API,
two DMA memory areas are used: one to transmit what counters the driver
is requesting and one to receive the values of those counters. These
memory areas are allocated and DMA mapped at probe time so that we don't
waste time at runtime.

> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> index 422ce13a7c94..63dc597dbd7c 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c

[ ... ]

> @@ -15,7 +15,121 @@
>  #define DPMAC_PROTOCOL_CHANGE_VER_MAJOR		4
>  #define DPMAC_PROTOCOL_CHANGE_VER_MINOR		8
>
> +#define DPMAC_STATS_BUNDLE_VER_MAJOR		4
> +#define DPMAC_STATS_BUNDLE_VER_MINOR		10
> +
>  #define DPAA2_MAC_FEATURE_PROTOCOL_CHANGE	BIT(0)
> +#define DPAA2_MAC_FEATURE_STATS_BUNDLE		BIT(1)
> +
> +struct dpmac_counter {
> +	enum dpmac_counter_id id;
> +	const char *name;
> +};
> +
> +#define DPMAC_UNSTRUCTURED_COUNTER(counter_id, counter_name)	\
> +	{							\
> +		.id = counter_id,				\
> +		.name = counter_name,				\
> +	}
> +
> +static const struct dpmac_counter dpaa2_mac_ethtool_stats[] = {
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ALL_FRAME,  "[mac] rx all frames"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_GOOD_FRAME,  "[mac] rx frames ok"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ERR_FRAME, "[mac] rx frame errors"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_DISCARD, "[mac] rx frame discards"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_UCAST_FRAME, "[mac] rx u-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_BCAST_FRAME, "[mac] rx b-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_MCAST_FRAME, "[mac] rx m-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_64, "[mac] rx 64 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_127, "[mac] rx 65-127 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_255, "[mac] rx 128-255 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_511, "[mac] rx 256-511 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1023, "[mac] rx 512-1023 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1518, "[mac] rx 1024-1518 bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAME_1519_MAX, "[mac] rx 1519-max bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_FRAG, "[mac] rx frags"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_JABBER, "[mac] rx jabber"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_ALIGN_ERR, "[mac] rx align errors"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_OVERSIZED, "[mac] rx oversized"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_VALID_PAUSE_FRAME, "[mac] rx pause"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_ING_BYTE, "[mac] rx bytes"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_GOOD_FRAME, "[mac] tx frames ok"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_UCAST_FRAME, "[mac] tx u-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_MCAST_FRAME, "[mac] tx m-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_BCAST_FRAME, "[mac] tx b-cast"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_ERR_FRAME, "[mac] tx frame errors"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_UNDERSIZED, "[mac] tx undersized"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_VALID_PAUSE_FRAME, "[mac] tx b-pause"),
> +	DPMAC_UNSTRUCTURED_COUNTER(DPMAC_CNT_EGR_BYTE, "[mac] tx bytes"),
> +};
> +
> +#define DPAA2_MAC_NUM_ETHTOOL_STATS	ARRAY_SIZE(dpaa2_mac_ethtool_stats)

[ ... ]

>  void dpaa2_mac_close(struct dpaa2_mac *mac)
>  {
>  	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
>
> +	if (mac->features & DPAA2_MAC_FEATURE_STATS_BUNDLE)
> +		dpaa2_mac_clear_stats(mac, &mac->ethtool_stats, DPAA2_MAC_NUM_ETHTOOL_STATS);
> +
>  	dpmac_close(mac->mc_io, 0, dpmac_dev->mc_handle);
>  	if (mac->fw_node)
>  		fwnode_handle_put(mac->fw_node);
>  }
>
> -static char dpaa2_mac_ethtool_stats[][ETH_GSTRING_LEN] = {
> -	[DPMAC_CNT_ING_ALL_FRAME]		= "[mac] rx all frames",
> -	[DPMAC_CNT_ING_GOOD_FRAME]		= "[mac] rx frames ok",
> -	[DPMAC_CNT_ING_ERR_FRAME]		= "[mac] rx frame errors",
> -	[DPMAC_CNT_ING_FRAME_DISCARD]		= "[mac] rx frame discards",
> -	[DPMAC_CNT_ING_UCAST_FRAME]		= "[mac] rx u-cast",
> -	[DPMAC_CNT_ING_BCAST_FRAME]		= "[mac] rx b-cast",
> -	[DPMAC_CNT_ING_MCAST_FRAME]		= "[mac] rx m-cast",
> -	[DPMAC_CNT_ING_FRAME_64]		= "[mac] rx 64 bytes",
> -	[DPMAC_CNT_ING_FRAME_127]		= "[mac] rx 65-127 bytes",
> -	[DPMAC_CNT_ING_FRAME_255]		= "[mac] rx 128-255 bytes",
> -	[DPMAC_CNT_ING_FRAME_511]		= "[mac] rx 256-511 bytes",
> -	[DPMAC_CNT_ING_FRAME_1023]		= "[mac] rx 512-1023 bytes",
> -	[DPMAC_CNT_ING_FRAME_1518]		= "[mac] rx 1024-1518 bytes",
> -	[DPMAC_CNT_ING_FRAME_1519_MAX]		= "[mac] rx 1519-max bytes",
> -	[DPMAC_CNT_ING_FRAG]			= "[mac] rx frags",
> -	[DPMAC_CNT_ING_JABBER]			= "[mac] rx jabber",
> -	[DPMAC_CNT_ING_ALIGN_ERR]		= "[mac] rx align errors",
> -	[DPMAC_CNT_ING_OVERSIZED]		= "[mac] rx oversized",
> -	[DPMAC_CNT_ING_VALID_PAUSE_FRAME]	= "[mac] rx pause",
> -	[DPMAC_CNT_ING_BYTE]			= "[mac] rx bytes",
> -	[DPMAC_CNT_EGR_GOOD_FRAME]		= "[mac] tx frames ok",
> -	[DPMAC_CNT_EGR_UCAST_FRAME]		= "[mac] tx u-cast",
> -	[DPMAC_CNT_EGR_MCAST_FRAME]		= "[mac] tx m-cast",
> -	[DPMAC_CNT_EGR_BCAST_FRAME]		= "[mac] tx b-cast",
> -	[DPMAC_CNT_EGR_ERR_FRAME]		= "[mac] tx frame errors",
> -	[DPMAC_CNT_EGR_UNDERSIZED]		= "[mac] tx undersized",
> -	[DPMAC_CNT_EGR_VALID_PAUSE_FRAME]	= "[mac] tx b-pause",
> -	[DPMAC_CNT_EGR_BYTE]			= "[mac] tx bytes",
> -};
> -
> -#define DPAA2_MAC_NUM_STATS	ARRAY_SIZE(dpaa2_mac_ethtool_stats)
> -
>  int dpaa2_mac_get_sset_count(void)
>  {
> -	return DPAA2_MAC_NUM_STATS;
> +	return DPAA2_MAC_NUM_ETHTOOL_STATS;
>  }
>
>  void dpaa2_mac_get_strings(u8 **data)
>  {
>  	int i;
>
> -	for (i = 0; i < DPAA2_MAC_NUM_STATS; i++)
> -		ethtool_puts(data, dpaa2_mac_ethtool_stats[i]);
> +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++)
> +		ethtool_puts(data, dpaa2_mac_ethtool_stats[i].name);
>  }
>
>  void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data)
>  {
> +	struct device *dev = mac->net_dev->dev.parent;
>  	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
> +	u64 *cnt_values;
>  	int i, err;
>  	u64 value;
>
> -	for (i = 0; i < DPAA2_MAC_NUM_STATS; i++) {
> +	if (!(mac->features & DPAA2_MAC_FEATURE_STATS_BUNDLE))
> +		goto fallback;
> +
> +	if (!mac->ethtool_stats.idx_dma_mem || !mac->ethtool_stats.values_dma_mem)
> +		goto fallback;
> +
> +	err = dpmac_get_statistics(mac->mc_io, 0, dpmac_dev->mc_handle,
> +				   mac->ethtool_stats.idx_iova, mac->ethtool_stats.values_iova,
> +				   DPAA2_MAC_NUM_ETHTOOL_STATS);
> +	if (err)
> +		goto fallback;
> +
> +	dma_sync_single_for_cpu(dev, mac->ethtool_stats.values_iova,
> +				DPAA2_MAC_NUM_ETHTOOL_STATS * sizeof(u64),
> +				DMA_FROM_DEVICE);
> +
> +	cnt_values = mac->ethtool_stats.values_dma_mem;
> +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++)
> +		*(data + i) = le64_to_cpu(*cnt_values++);
> +
> +	return;
> +
> +fallback:
> +
> +	/* Fallback and retrieve each counter one by one */
> +	for (i = 0; i < DPAA2_MAC_NUM_ETHTOOL_STATS; i++) {
>  		err = dpmac_get_counter(mac->mc_io, 0, dpmac_dev->mc_handle,
>  					i, &value);
                                        ^

Does the fallback path need to pass dpaa2_mac_ethtool_stats[i].id instead
of i here?

The old code used a sparse array indexed by enum dpmac_counter_id values:

    static char dpaa2_mac_ethtool_stats[][ETH_GSTRING_LEN] = {
        [DPMAC_CNT_ING_ALL_FRAME] = "[mac] rx all frames",
        ...
    };

where the loop index i directly corresponded to the enum value passed to
dpmac_get_counter(). The new code uses a compact array where array index 0
contains DPMAC_CNT_ING_ALL_FRAME (enum value 18), but the fallback code
still passes i (sequential 0, 1, 2...) instead of the stored enum value.

When i=0, the code requests counter 0 (DPMAC_CNT_ING_FRAME_64) but the
name is "[mac] rx all frames" (DPMAC_CNT_ING_ALL_FRAME). This would cause
all statistics to report wrong values for their labels.

>  		if (err) {
>  			netdev_err(mac->net_dev,
>  				   "dpmac_get_counter(%d) failed\n", i);

[ ... ]
-- 
pw-bot: cr