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

[MD Danish Anwar <danishanwar@ti.com>]

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@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@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