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 for Android: free password hash cracker in your pocket

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

Message-ID: <aO-ZCqsvPQ6Pqjpg@horms.kernel.org>
Date: Wed, 15 Oct 2025 13:52:26 +0100
From: Simon Horman <horms@...nel.org>
To: Lorenzo Bianconi <lorenzo@...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,

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;
}

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.

...