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

[David CARLIER]

On Wed, 13 May 2026 at 07:29, MD Danish Anwar <danishanwar@xxxxxx> wrote:
>
> Hi David
>
> On 12/05/26 3:33 pm, David CARLIER wrote:
> > Hi Danish,
> >
> >
> > On Tue, 12 May 2026 at 10:40, MD Danish Anwar <danishanwar@xxxxxx> 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@xxxxxx> 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@xxxxxx>
> >>>> ---
> >>>>  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.
> >
>
> Thank you for the suggestion. I think your previous suggestion fits
> better. I believe keeping the arrays in icssg_stats.h is preferable to
> moving them to icssg_stats.c. Here is my reasoning:
>
>
> Your binary-bloat concern was about icssg_prueth.h including
> icssg_stats.h, which would drag the static const tables into every .c
> that includes icssg_prueth.h (~11 translation units). That concern is
> valid, but it is specific to the include direction of the previous
> patch. If we simply revert to the original include graph —
> icssg_stats.h includes icssg_prueth.h, not the other way around —
> only the two files that have always included icssg_stats.h directly
> (icssg_stats.c and icssg_ethtool.c) get a copy of the arrays. No
> regression in binary size compared to the baseline.
>
> >   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.
> >
>
> Moving the arrays to icssg_stats.c (approach #2) adds extern
> declarations, splits the definition from the static_assert, and is a
> larger restructuring for the same safety guarantee. Keeping the arrays
> in the header with a static_assert immediately after each one is a
> 2-line diff and leaves the code easy to read in one place.
>
> Please let me know if this sounds okay to you. I will send out a v2 soon
> if this approach is fine with you.

Sounds fine by me, note that I am not a maintainer ;  I was just "chiming in" ;)

Cheers !

>
> >   Probably worth keeping Vladimir on Cc for v2 in case he had something
> >   else in mind.
> >
>
> I will CC Vladimir in v2.
>
> --
> Thanks and Regards,
> Danish
>