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

[Jacob Keller <jacob.e.keller@intel.com>]

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...)

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
structures and logic as helpers to the core ethtool code (or perhaps
now, to libeth) but never got around to it.