Re: [PATCH net-next 03/12] net: airoha: Add airoha_ppe_get_num_stats_entries() and airoha_ppe_get_num_total_stats_entries()

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3678 bytes --]

> On Wed, Oct 15, 2025 at 09:15:03AM +0200, Lorenzo Bianconi wrote:
> > Introduce airoha_ppe_get_num_stats_entries and
> > airoha_ppe_get_num_total_stats_entries routines in order to make the
> > code more readable controlling if CONFIG_NET_AIROHA_FLOW_STATS is
> > enabled or disabled.
> > Modify airoha_ppe_foe_get_flow_stats_index routine signature relying on
> > airoha_ppe_get_num_total_stats_entries().
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/net/ethernet/airoha/airoha_eth.h |  10 +--
> >  drivers/net/ethernet/airoha/airoha_ppe.c | 103 +++++++++++++++++++++++++------
> >  2 files changed, 86 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> > index 4330b672d99e1e190efa5ad75d13fb35e77d070e..1f7e34a5f457ca2200e9c81dd05dc03cd7c5eb77 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.h
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> > @@ -50,15 +50,9 @@
> >  
> >  #define PPE_NUM				2
> >  #define PPE1_SRAM_NUM_ENTRIES		(8 * 1024)
> > -#define PPE_SRAM_NUM_ENTRIES		(2 * PPE1_SRAM_NUM_ENTRIES)
> > -#ifdef CONFIG_NET_AIROHA_FLOW_STATS
> > +#define PPE_SRAM_NUM_ENTRIES		(PPE_NUM * PPE1_SRAM_NUM_ENTRIES)
> >  #define PPE1_STATS_NUM_ENTRIES		(4 * 1024)
> > -#else
> > -#define PPE1_STATS_NUM_ENTRIES		0
> > -#endif /* CONFIG_NET_AIROHA_FLOW_STATS */
> > -#define PPE_STATS_NUM_ENTRIES		(2 * PPE1_STATS_NUM_ENTRIES)
> > -#define PPE1_SRAM_NUM_DATA_ENTRIES	(PPE1_SRAM_NUM_ENTRIES - PPE1_STATS_NUM_ENTRIES)
> > -#define PPE_SRAM_NUM_DATA_ENTRIES	(2 * PPE1_SRAM_NUM_DATA_ENTRIES)
> > +#define PPE_STATS_NUM_ENTRIES		(PPE_NUM * PPE1_STATS_NUM_ENTRIES)
> >  #define PPE_DRAM_NUM_ENTRIES		(16 * 1024)
> >  #define PPE_NUM_ENTRIES			(PPE_SRAM_NUM_ENTRIES + PPE_DRAM_NUM_ENTRIES)
> >  #define PPE_HASH_MASK			(PPE_NUM_ENTRIES - 1)
> > diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
> > index 8d1dceadce0becb2b1ce656d64ab77bd3c2f914a..303d31e1da4b723023ee0cc1ca5f6038c16966cd 100644
> > --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> > +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> > @@ -32,6 +32,30 @@ static const struct rhashtable_params airoha_l2_flow_table_params = {
> >  	.automatic_shrinking = true,
> >  };
> >  
> > +static int airoha_ppe_get_num_stats_entries(struct airoha_ppe *ppe,
> > +					    u32 *num_stats)
> > +{
> > +#ifdef CONFIG_NET_AIROHA_FLOW_STATS
> > +	*num_stats = PPE1_STATS_NUM_ENTRIES;
> > +	return 0;
> > +#else
> > +	return -EOPNOTSUPP;
> > +#endif /* CONFIG_NET_AIROHA_FLOW_STATS */
> > +}
> 
> Hi Lorenzo,

Hi Simon,

> 
> I think that in general using IS_ENABLED is preferred over #ifdef
> in cases where the former can be used. For one thing it improves compile
> coverage.
> 
> That does seem applicable here, so I'm wondering if
> we can do something like the following.
> (Compile tested only!)
> 
> static int airoha_ppe_get_num_stats_entries(struct airoha_ppe *ppe,
>                                             u32 *num_stats)
> {
>         if (!IS_ENABLED(CONFIG_NET_AIROHA_FLOW_STATS))
>                 return -EOPNOTSUPP;
> 
>         *num_stats = PPE1_STATS_NUM_ENTRIES;
>         return 0;
> }

ack, fine. I will fix it in v2.

> 
> Also, very subjectively, I might have returned num_stats as
> a positive return value. I'm assuming it's value will never overflow an int.
> Likewise elsewhere.
> 
> But that's just my idea. Feel free to stick with the current scheme.

I guess it is fine. I will fix it in v2.

Regards,
Lorenzo

> 
> ...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]