On Thu, Feb 26, 2026 at 06:26:13PM -0800, Jakub Kicinski wrote:
> 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
>
(...)
> > +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.
Yes, that is indeed true. Thanks for catching it!
Ioana
