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

[MD Danish Anwar]

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.

>   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