[PATCH net] net: airoha: Fix PPE table access in airoha_ppe_debugfs_foe_show()

[PATCH net] net: airoha: Fix PPE table access in airoha_ppe_debugfs_foe_show()

[Lorenzo Bianconi]

In order to avoid any possible race we need to hold the ppe_lock
spinlock accessing the hw PPE table. airoha_ppe_foe_get_entry routine is
always executed holding ppe_lock except in airoha_ppe_debugfs_foe_show
routine. Fix the problem introducing airoha_ppe_foe_get_entry_locked
routine.

Fixes: 3fe15c640f380 ("net: airoha: Introduce PPE debugfs support")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/ethernet/airoha/airoha_eth.h         |  4 ++--
 drivers/net/ethernet/airoha/airoha_ppe.c         | 18 ++++++++++++++++--
 drivers/net/ethernet/airoha/airoha_ppe_debugfs.c |  2 +-
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
index a970b789cf232c316e5ea27b0146493bf91c3767..cf33d731ad0db43bca8463fde76673b39a4f6796 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -615,8 +615,8 @@ int airoha_ppe_setup_tc_block_cb(struct net_device *dev, void *type_data);
 int airoha_ppe_init(struct airoha_eth *eth);
 void airoha_ppe_deinit(struct airoha_eth *eth);
 void airoha_ppe_init_upd_mem(struct airoha_gdm_port *port);
-struct airoha_foe_entry *airoha_ppe_foe_get_entry(struct airoha_ppe *ppe,
-						  u32 hash);
+struct airoha_foe_entry *
+airoha_ppe_foe_get_entry_locked(struct airoha_ppe *ppe, u32 hash);
 void airoha_ppe_foe_entry_get_stats(struct airoha_ppe *ppe, u32 hash,
 				    struct airoha_foe_stats64 *stats);
 
diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
index 0e217acfc5ef748453b020e5713ace1910abc4a8..4dbf2bf187d02e3e8b9d2b966036c3aa58c867b1 100644
--- a/drivers/net/ethernet/airoha/airoha_ppe.c
+++ b/drivers/net/ethernet/airoha/airoha_ppe.c
@@ -498,9 +498,11 @@ static void airoha_ppe_foe_flow_stats_update(struct airoha_ppe *ppe,
 		FIELD_PREP(AIROHA_FOE_IB2_NBQ, nbq);
 }
 
-struct airoha_foe_entry *airoha_ppe_foe_get_entry(struct airoha_ppe *ppe,
-						  u32 hash)
+static struct airoha_foe_entry *
+airoha_ppe_foe_get_entry(struct airoha_ppe *ppe, u32 hash)
 {
+	lockdep_assert_held(&ppe_lock);
+
 	if (hash < PPE_SRAM_NUM_ENTRIES) {
 		u32 *hwe = ppe->foe + hash * sizeof(struct airoha_foe_entry);
 		struct airoha_eth *eth = ppe->eth;
@@ -527,6 +529,18 @@ struct airoha_foe_entry *airoha_ppe_foe_get_entry(struct airoha_ppe *ppe,
 	return ppe->foe + hash * sizeof(struct airoha_foe_entry);
 }
 
+struct airoha_foe_entry *
+airoha_ppe_foe_get_entry_locked(struct airoha_ppe *ppe, u32 hash)
+{
+	struct airoha_foe_entry *hwe;
+
+	spin_lock_bh(&ppe_lock);
+	hwe = airoha_ppe_foe_get_entry(ppe, hash);
+	spin_unlock_bh(&ppe_lock);
+
+	return hwe;
+}
+
 static bool airoha_ppe_foe_compare_entry(struct airoha_flow_table_entry *e,
 					 struct airoha_foe_entry *hwe)
 {
diff --git a/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c b/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c
index 05a756233f6a44fa51d1c57dd39d89c8ea488054..992bf2e9598414ee3f1f126be3f451e486b26640 100644
--- a/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c
+++ b/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c
@@ -67,7 +67,7 @@ static int airoha_ppe_debugfs_foe_show(struct seq_file *m, void *private,
 		u32 type, state, ib2, data;
 		bool ipv6 = false;
 
-		hwe = airoha_ppe_foe_get_entry(ppe, i);
+		hwe = airoha_ppe_foe_get_entry_locked(ppe, i);
 		if (!hwe)
 			continue;
 

---
base-commit: afd8c2c9e2e29c6c7705635bea2960593976dacc
change-id: 20250728-airoha_ppe_foe_get_entry_locked-70e4ebbee984

Best regards,
-- 
Lorenzo Bianconi <lorenzo@kernel.org>

Re: [PATCH net] net: airoha: Fix PPE table access in airoha_ppe_debugfs_foe_show()

[Dawid Osuchowski]

On 2025-07-28 1:58 PM, Lorenzo Bianconi wrote:
> In order to avoid any possible race we need to hold the ppe_lock
> spinlock accessing the hw PPE table. airoha_ppe_foe_get_entry routine is
> always executed holding ppe_lock except in airoha_ppe_debugfs_foe_show
> routine. Fix the problem introducing airoha_ppe_foe_get_entry_locked
> routine.
> 
> Fixes: 3fe15c640f380 ("net: airoha: Introduce PPE debugfs support")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Reviewed-by: Dawid Osuchowski <dawid.osuchowski@linux.intel.com>

Thanks,
Dawid

> ---
>   drivers/net/ethernet/airoha/airoha_eth.h         |  4 ++--
>   drivers/net/ethernet/airoha/airoha_ppe.c         | 18 ++++++++++++++++--
>   drivers/net/ethernet/airoha/airoha_ppe_debugfs.c |  2 +-
>   3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index a970b789cf232c316e5ea27b0146493bf91c3767..cf33d731ad0db43bca8463fde76673b39a4f6796 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -615,8 +615,8 @@ int airoha_ppe_setup_tc_block_cb(struct net_device *dev, void *type_data);
>   int airoha_ppe_init(struct airoha_eth *eth);
>   void airoha_ppe_deinit(struct airoha_eth *eth);
>   void airoha_ppe_init_upd_mem(struct airoha_gdm_port *port);
> -struct airoha_foe_entry *airoha_ppe_foe_get_entry(struct airoha_ppe *ppe,
> -						  u32 hash);
> +struct airoha_foe_entry *
> +airoha_ppe_foe_get_entry_locked(struct airoha_ppe *ppe, u32 hash);
>   void airoha_ppe_foe_entry_get_stats(struct airoha_ppe *ppe, u32 hash,
>   				    struct airoha_foe_stats64 *stats);
>   
> diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
> index 0e217acfc5ef748453b020e5713ace1910abc4a8..4dbf2bf187d02e3e8b9d2b966036c3aa58c867b1 100644
> --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> @@ -498,9 +498,11 @@ static void airoha_ppe_foe_flow_stats_update(struct airoha_ppe *ppe,
>   		FIELD_PREP(AIROHA_FOE_IB2_NBQ, nbq);
>   }
>   
> -struct airoha_foe_entry *airoha_ppe_foe_get_entry(struct airoha_ppe *ppe,
> -						  u32 hash)
> +static struct airoha_foe_entry *
> +airoha_ppe_foe_get_entry(struct airoha_ppe *ppe, u32 hash)
>   {
> +	lockdep_assert_held(&ppe_lock);
> +
>   	if (hash < PPE_SRAM_NUM_ENTRIES) {
>   		u32 *hwe = ppe->foe + hash * sizeof(struct airoha_foe_entry);
>   		struct airoha_eth *eth = ppe->eth;
> @@ -527,6 +529,18 @@ struct airoha_foe_entry *airoha_ppe_foe_get_entry(struct airoha_ppe *ppe,
>   	return ppe->foe + hash * sizeof(struct airoha_foe_entry);
>   }
>   
> +struct airoha_foe_entry *
> +airoha_ppe_foe_get_entry_locked(struct airoha_ppe *ppe, u32 hash)
> +{
> +	struct airoha_foe_entry *hwe;
> +
> +	spin_lock_bh(&ppe_lock);
> +	hwe = airoha_ppe_foe_get_entry(ppe, hash);
> +	spin_unlock_bh(&ppe_lock);
> +
> +	return hwe;
> +}
> +
>   static bool airoha_ppe_foe_compare_entry(struct airoha_flow_table_entry *e,
>   					 struct airoha_foe_entry *hwe)
>   {
> diff --git a/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c b/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c
> index 05a756233f6a44fa51d1c57dd39d89c8ea488054..992bf2e9598414ee3f1f126be3f451e486b26640 100644
> --- a/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c
> +++ b/drivers/net/ethernet/airoha/airoha_ppe_debugfs.c
> @@ -67,7 +67,7 @@ static int airoha_ppe_debugfs_foe_show(struct seq_file *m, void *private,
>   		u32 type, state, ib2, data;
>   		bool ipv6 = false;
>   
> -		hwe = airoha_ppe_foe_get_entry(ppe, i);
> +		hwe = airoha_ppe_foe_get_entry_locked(ppe, i);
>   		if (!hwe)
>   			continue;
>   
> 
> ---
> base-commit: afd8c2c9e2e29c6c7705635bea2960593976dacc
> change-id: 20250728-airoha_ppe_foe_get_entry_locked-70e4ebbee984
> 
> Best regards,

Re: [PATCH net] net: airoha: Fix PPE table access in airoha_ppe_debugfs_foe_show()

[Jakub Kicinski]

On Mon, 28 Jul 2025 13:58:08 +0200 Lorenzo Bianconi wrote:
> +struct airoha_foe_entry *
> +airoha_ppe_foe_get_entry_locked(struct airoha_ppe *ppe, u32 hash)

Hm, could be just me, but the way we/I used _locked in the core was 
the opposite. _locked means the caller's already taken the lock.
Here you seem to be saying that the "callee is locked"..
Can we stick to core's interpretation?

> +	struct airoha_foe_entry *hwe;
> +
> +	spin_lock_bh(&ppe_lock);
> +	hwe = airoha_ppe_foe_get_entry(ppe, hash);
> +	spin_unlock_bh(&ppe_lock);
> +
> +	return hwe;

Is the lifetime of the hwe object somehow guaranteed in the debugfs
code? Looks questionable..
-- 
pw-bot: cr

Re: [PATCH net] net: airoha: Fix PPE table access in airoha_ppe_debugfs_foe_show()

[Lorenzo Bianconi]

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

> On Mon, 28 Jul 2025 13:58:08 +0200 Lorenzo Bianconi wrote:
> > +struct airoha_foe_entry *
> > +airoha_ppe_foe_get_entry_locked(struct airoha_ppe *ppe, u32 hash)
> 
> Hm, could be just me, but the way we/I used _locked in the core was 
> the opposite. _locked means the caller's already taken the lock.
> Here you seem to be saying that the "callee is locked"..
> Can we stick to core's interpretation?

sure, that's fine.

> 
> > +	struct airoha_foe_entry *hwe;
> > +
> > +	spin_lock_bh(&ppe_lock);
> > +	hwe = airoha_ppe_foe_get_entry(ppe, hash);
> > +	spin_unlock_bh(&ppe_lock);
> > +
> > +	return hwe;
> 
> Is the lifetime of the hwe object somehow guaranteed in the debugfs
> code? Looks questionable..

PPE table entries are allocated at driver load and never freed, the hw is just
writing into this DMA area when the entry is binded.

Regards,
Lorenzo

> -- 
> pw-bot: cr

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