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

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

Hi Jacob,

On 14/05/26 1:30 am, Jacob Keller wrote:
> On 5/12/2026 2:40 AM, MD Danish Anwar 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@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.
>>
> The way we solved this in the Intel drivers is to use a single array
> which contains both the stat name as well as the offset from the
> structure where the stat resides.
> 
> The stat string code just iterates over the stat list for the strings,
> while the stat value code iterates the array and computes the stat
> address from the offset and size and base structure pointer. Each object
> that has stats has its own stat array structure.
> 
> This is probably overkill, but the advantage is that the strings and
> their values are stored together and adding a new stat is as simple as
> adding a new entry to that list.
> 
> I.e.
> 
> struct ice_stats {
>         char stat_string[ETH_GSTRING_LEN];
>         int sizeof_stat;
>         int stat_offset;
> };
> 
> #define ICE_STAT(_type, _name, _stat) { \
>         .stat_string = _name, \
>         .sizeof_stat = sizeof_field(_type, _stat), \
>         .stat_offset = offsetof(_type, _stat) \
> }
> 
> #define ICE_VSI_STAT(_name, _stat) \
>                 ICE_STAT(struct ice_vsi, _name, _stat)
> #define ICE_PF_STAT(_name, _stat) \
>                 ICE_STAT(struct ice_pf, _name, _stat)
> 
> 
> Then the stats for the individial arrays are defined like this:
> 
> static const struct ice_stats ice_gstrings_vsi_stats[] = {
>         ICE_VSI_STAT(ICE_RX_UNICAST, eth_stats.rx_unicast),
>         ICE_VSI_STAT(ICE_TX_UNICAST, eth_stats.tx_unicast),
>         ICE_VSI_STAT(ICE_RX_MULTICAST, eth_stats.rx_multicast),
>         ICE_VSI_STAT(ICE_TX_MULTICAST, eth_stats.tx_multicast),
>         ICE_VSI_STAT(ICE_RX_BROADCAST, eth_stats.rx_broadcast),
>         ICE_VSI_STAT(ICE_TX_BROADCAST, eth_stats.tx_broadcast),
> 	...
> };
> 
> (Note, ICE_RX_UNICAST is a macro that defines the string value.. I don't
> recall who changed this to macros or why vs just having the strings be
> directly in the definition...)
> 

Thanks for sharing the ice driver pattern — that's a clean design.

> This is probably a lot bigger refactor to make work, and may not be
> exactly suitable for your driver. I've considered "upgrading" these data

Yes, I need to see if refactoring is applicable to ICSSG or not. I will
look into this and send a separate patch / series in future if
applicable. For this series I will stick with what David Carlier suggested.

> structures and logic as helpers to the core ethtool code (or perhaps
> now, to libeth) but never got around to it.


-- 
Thanks and Regards,
Danish