[PATCH net-next 1/2] net: ti: icssg: Derive stats array lengths from ARRAY_SIZE

[David CARLIER]

Hi Danish,


On Tue, 12 May 2026 at 10:40, MD Danish Anwar <danishanwar at ti.com> wrote:
>
> Hi David,
>
> On 12/05/26 1:28 pm, David CARLIER wrote:
> > Hi MD,
> >
> > On Tue, 12 May 2026 at 07:06, MD Danish Anwar <danishanwar at ti.com> wrote:
> >>
> >> Replace the manually maintained ICSSG_NUM_MIIG_STATS and
> >> ICSSG_NUM_PA_STATS constants with ARRAY_SIZE() expressions derived
> >> directly from the corresponding stat descriptor arrays, so that adding
> >> new entries to icssg_all_miig_stats[] or icssg_all_pa_stats[] no longer
> >> requires a separate update to a numeric constant.
> >>
> >> To make this self-contained, break the circular include dependency
> >> between icssg_stats.h and icssg_prueth.h:
> >>
> >>   - icssg_stats.h previously included icssg_prueth.h (transitively
> >>     pulling in icssg_switch_map.h and ETH_GSTRING_LEN).  Replace that
> >>     with direct includes of <linux/ethtool.h>, <linux/kernel.h> and
> >>     "icssg_switch_map.h".
> >>
> >>   - icssg_prueth.h now includes icssg_stats.h, giving it access to
> >>     the ARRAY_SIZE-based ICSSG_NUM_MIIG_STATS and ICSSG_NUM_PA_STATS
> >>     before they are used in the prueth_emac struct and ICSSG_NUM_STATS.
> >>
> >> Signed-off-by: MD Danish Anwar <danishanwar at ti.com>
> >> ---
> >>  drivers/net/ethernet/ti/icssg/icssg_prueth.h | 3 +--
> >>  drivers/net/ethernet/ti/icssg/icssg_stats.h  | 7 ++++++-
> >>  2 files changed, 7 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> index df93d15c5b78..e2ccecb0a0dd 100644
> >> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
> >> @@ -43,6 +43,7 @@
> >>
> >>  #include "icssg_config.h"
> >>  #include "icss_iep.h"
> >> +#include "icssg_stats.h"
> >>  #include "icssg_switch_map.h"
> >>
> >>  #define PRUETH_MAX_MTU          (2000 - ETH_HLEN - ETH_FCS_LEN)
> >> @@ -57,8 +58,6 @@
> >>
> >>  #define ICSSG_MAX_RFLOWS       8       /* per slice */
> >>
> >> -#define ICSSG_NUM_PA_STATS     32
> >> -#define ICSSG_NUM_MIIG_STATS   60
> >>  /* Number of ICSSG related stats */
> >>  #define ICSSG_NUM_STATS (ICSSG_NUM_MIIG_STATS + ICSSG_NUM_PA_STATS)
> >>  #define ICSSG_NUM_STANDARD_STATS 31
> >> diff --git a/drivers/net/ethernet/ti/icssg/icssg_stats.h b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> >> index 5ec0b38e0c67..b854eb587c1e 100644
> >> --- a/drivers/net/ethernet/ti/icssg/icssg_stats.h
> >> +++ b/drivers/net/ethernet/ti/icssg/icssg_stats.h
> >> @@ -8,10 +8,15 @@
> >>  #ifndef __NET_TI_ICSSG_STATS_H
> >>  #define __NET_TI_ICSSG_STATS_H
> >>
> >> -#include "icssg_prueth.h"
> >> +#include <linux/ethtool.h>
> >> +#include <linux/kernel.h>
> >> +#include "icssg_switch_map.h"
> >>
> >>  #define STATS_TIME_LIMIT_1G_MS    25000    /* 25 seconds @ 1G */
> >>
> >> +#define ICSSG_NUM_MIIG_STATS   ARRAY_SIZE(icssg_all_miig_stats)
> >> +#define ICSSG_NUM_PA_STATS     ARRAY_SIZE(icssg_all_pa_stats)
> >> +
> >>  struct miig_stats_regs {
> >>         /* Rx */
> >>         u32 rx_packets;
> >> --
> >> 2.34.1
> >>
> >
> > One thing that caught my eye: icssg_all_miig_stats[] and
> >   icssg_all_pa_stats[] are 'static const' arrays in icssg_stats.h with
> >   ETH_GSTRING_LEN name buffers per entry. Right now only icssg_stats.c
> >   and icssg_ethtool.c pull them in. After this patch icssg_prueth.h
> >   includes icssg_stats.h, so every .c in the driver (classifier,
> >   common, config, mii_cfg, queues, switchdev, ...) ends up with its own
> >   static-const copy of both tables.
> >
> >   Would a static_assert() work for what you're after? Something like:
> >
>
> While adding more stats manually, The ARRAY_SIZE() approach was
> explicitly requested by maintainer [1]:
>
> This patch is a direct response to that feedback. static_assert() would
> still require updating the numeric constant on every array change. The
> goal here is to eliminate the need of manually incrementing stats count
> whenever new stats are added
>
> Your concern about multiple copies of table is noted and valid. Could
> you advise on the preferred way to reconcile these two requirements? I
> am happy to restructure if there is an approach that satisfies both.
>
> [1]
> https://lore.kernel.org/all/20260112181436.4s5ceywwembn674r@skbuf/#:~:text=Can%27t%20this%20be%20expressed%20as%20ARRAY_SIZE(icssg_all_pa_stats)%3F%20It%20is%20very%0Afragile%20to%20have%20to%20count%20and%20update%20this%20manually.
>
>
> >     static const struct icssg_miig_stats icssg_all_miig_stats[] = {
> >         ...
> >     };
> >     static_assert(ARRAY_SIZE(icssg_all_miig_stats) == ICSSG_NUM_MIIG_STATS);
> >
> >   next to each array, keeping the numeric #defines as-is. Then 2/2 fails
> >   to build the moment a new entry is added without bumping the count,
> >   which is the case you're guarding against — without touching the
> >   include graph.
> >
> > What do you think ?
> >
> > Cheers.
>
> --
> Thanks and Regards,
> Danish
>


  Thanks for digging up the context — fair point, I'd missed Vladimir's
  earlier ask. Reading it again though, what he calls fragile is the
  silent miscount, not the keystroke of typing a number. A static_assert
  turns "forgot to bump" into a build error, which I think gets you
  there.

  What about moving the two arrays into icssg_stats.c, declaring them
  extern in the header, and dropping a static_assert next to each
  definition? Numeric #defines stay where they are, icssg_prueth.h
  doesn't need to know about icssg_stats.h, and the tables live in one
  TU instead of every .o in the driver. If the count and the array
  disagree, you get a compile error on the spot.

  Probably worth keeping Vladimir on Cc for v2 in case he had something
  else in mind.

  Cheers,