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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aPCodE0_u94s-w73@lore-desk>
Date: Thu, 16 Oct 2025 10:10:28 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Simon Horman <horms@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 03/12] net: airoha: Add
 airoha_ppe_get_num_stats_entries() and
 airoha_ppe_get_num_total_stats_entries()

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

> 
> ...

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)