Re: [PATCH net-next] net: airoha: bind WLAN-bound flows on PPE driver L2 cache miss

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> The Linux bridge FDB can resolve a destination station to WDMA even when
> the Airoha PPE driver's L2 offload cache has no entry for that MAC pair.
> The normal bind path only checks the PPE driver's L2 offload cache, so an
> unbound PPE hit for WLAN egress can stay unbound even though the bridge
> already knows the right output path, unless a later offload event fills
> that PPE driver cache.
> 
> This matters for bridge-visible WLAN egress, such as wired-to-WLAN
> forwarding or WLAN peer forwarding across another BSS, radio or MLO link.
> Same-link or same-radio intra-BSS forwarding can stay inside the WLAN
> datapath and is not covered.

Hi Jihong,

In order to offload L2 flows, I assume you are using the OpenWrt bridger
package, right?
IIUC the issue you want to resolve is we are not adding PPE L2 entries for
the specified cases (same-link or same-radio intra-BSS forwarding), correct?
Using this approach, we are breaking the assumption PPE flow-table and hw
flow-table are in sync. If the issue is the one described above, why not
fixing the problem directly in the bridger package?
Moreover, I see you developed the patch using Codex:gpt-5.5. Have you tested it
on a real hw?

Some comments inline.

Regards,
Lorenzo

> 
> Before touching the PPE table, resolve the destination MAC through the
> bridge device above the ingress netdev. If the PPE driver's L2 offload
> cache lookup misses, bind the hardware flow to the resolved CDM4/WDMA
> path.
> 
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Jihong Min <hurryman2212@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_ppe.c | 138 +++++++++++++++++++----
>  1 file changed, 119 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_ppe.c b/drivers/net/ethernet/airoha/airoha_ppe.c
> index 26da519236bf..ea932e6d87f6 100644
> --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> @@ -803,65 +803,163 @@ static void airoha_ppe_foe_flow_remove_entry(struct airoha_ppe *ppe,
>  }
>  
>  static int
> -airoha_ppe_foe_commit_subflow_entry(struct airoha_ppe *ppe,
> -				    struct airoha_flow_table_entry *e,
> -				    u32 hash, bool rx_wlan)
> +airoha_ppe_foe_commit_subflow(struct airoha_ppe *ppe,
> +			      const struct airoha_foe_entry *bridge,

maybe l2_hwe instead of bridge?

> +			      u32 hash, bool rx_wlan)
>  {
>  	u32 mask = AIROHA_FOE_IB1_BIND_PACKET_TYPE | AIROHA_FOE_IB1_BIND_UDP;
>  	struct airoha_foe_entry *hwe_p, hwe;
> -	struct airoha_flow_table_entry *f;
>  	int type;
>  
>  	hwe_p = airoha_ppe_foe_get_entry_locked(ppe, hash);
>  	if (!hwe_p)
>  		return -EINVAL;
>  
> -	f = kzalloc_obj(*f, GFP_ATOMIC);
> -	if (!f)
> -		return -ENOMEM;
> -
> -	hlist_add_head(&f->l2_subflow_node, &e->l2_flows);
> -	f->type = FLOW_TYPE_L2_SUBFLOW;
> -	f->hash = hash;
> -
>  	memcpy(&hwe, hwe_p, sizeof(*hwe_p));
> -	hwe.ib1 = (hwe.ib1 & mask) | (e->data.ib1 & ~mask);
> +	hwe.ib1 = (hwe.ib1 & mask) | (bridge->ib1 & ~mask);
>  
>  	type = FIELD_GET(AIROHA_FOE_IB1_BIND_PACKET_TYPE, hwe.ib1);
>  	if (type >= PPE_PKT_TYPE_IPV6_ROUTE_3T) {
> -		memcpy(&hwe.ipv6.l2, &e->data.bridge.l2, sizeof(hwe.ipv6.l2));
> -		hwe.ipv6.ib2 = e->data.bridge.ib2;
> +		memcpy(&hwe.ipv6.l2, &bridge->bridge.l2,
> +		       sizeof(hwe.ipv6.l2));
> +		hwe.ipv6.ib2 = bridge->bridge.ib2;
>  		/* setting smac_id to 0xf instruct the hw to keep original
>  		 * source mac address
>  		 */
>  		hwe.ipv6.l2.src_mac_hi = FIELD_PREP(AIROHA_FOE_MAC_SMAC_ID,
>  						    0xf);
>  	} else {
> -		memcpy(&hwe.bridge.l2, &e->data.bridge.l2,
> +		memcpy(&hwe.bridge.l2, &bridge->bridge.l2,
>  		       sizeof(hwe.bridge.l2));
> -		hwe.bridge.ib2 = e->data.bridge.ib2;
> +		hwe.bridge.ib2 = bridge->bridge.ib2;
>  		if (type == PPE_PKT_TYPE_IPV4_HNAPT)
>  			memcpy(&hwe.ipv4.new_tuple, &hwe.ipv4.orig_tuple,
>  			       sizeof(hwe.ipv4.new_tuple));
>  	}
>  
> -	hwe.bridge.data = e->data.bridge.data;
> -	airoha_ppe_foe_commit_entry(ppe, &hwe, hash, rx_wlan);
> +	hwe.bridge.data = bridge->bridge.data;
> +
> +	return airoha_ppe_foe_commit_entry(ppe, &hwe, hash, rx_wlan);
> +}
> +
> +static int
> +airoha_ppe_foe_commit_subflow_entry(struct airoha_ppe *ppe,
> +				    struct airoha_flow_table_entry *e,
> +				    u32 hash, bool rx_wlan)
> +{
> +	struct airoha_flow_table_entry *f;
> +	int err;
> +
> +	f = kzalloc_obj(*f, GFP_ATOMIC);
> +	if (!f)
> +		return -ENOMEM;
> +
> +	err = airoha_ppe_foe_commit_subflow(ppe, &e->data, hash, rx_wlan);
> +	if (err) {
> +		kfree(f);
> +		return err;
> +	}
> +
> +	hlist_add_head(&f->l2_subflow_node, &e->l2_flows);
> +	f->type = FLOW_TYPE_L2_SUBFLOW;
> +	f->hash = hash;
>  
>  	return 0;
>  }
>  
> +static bool
> +airoha_ppe_foe_prepare_wdma_subflow_dev(struct airoha_ppe *ppe,
> +					struct net_device *dev,
> +					struct airoha_flow_data *data,
> +					struct airoha_foe_entry *hwe)
> +{
> +	u32 pse_port;
> +	int err;
> +
> +	err = airoha_ppe_foe_entry_prepare(ppe->eth, hwe, dev,
> +					   PPE_PKT_TYPE_BRIDGE, data, 0);
> +	if (err)
> +		return false;
> +
> +	pse_port = FIELD_GET(AIROHA_FOE_IB2_PSE_PORT, hwe->bridge.ib2);
> +	if (pse_port != FE_PSE_PORT_CDM4)
> +		return false;
> +
> +	return true;

	return pse_port == FE_PSE_PORT_CDM4;

> +}
> +
> +static struct net_device *
> +airoha_ppe_foe_get_bridge_master(struct net_device *dev)
> +{
> +	struct net_device *master = NULL;
> +
> +	rcu_read_lock();
> +	master = netdev_master_upper_dev_get_rcu(dev);
> +	if (master && netif_is_bridge_master(master))
> +		dev_hold(master);
> +	else
> +		master = NULL;
> +	rcu_read_unlock();
> +
> +	return master;
> +}
> +
> +static bool
> +airoha_ppe_foe_prepare_wdma_subflow(struct airoha_ppe *ppe,
> +				    struct sk_buff *skb,
> +				    struct airoha_foe_entry *hwe)
> +{
> +	struct ethhdr *eh = eth_hdr(skb);
> +	struct airoha_flow_data data = {};
> +	struct net_device *master;
> +
> +	if (!is_valid_ether_addr(eh->h_source) ||
> +	    !is_valid_ether_addr(eh->h_dest))
> +		return false;
> +
> +	ether_addr_copy(data.eth.h_dest, eh->h_dest);
> +	ether_addr_copy(data.eth.h_source, eh->h_source);
> +
> +	if (!skb->dev)
> +		return false;
> +
> +	/* WLAN egress unbound hits can arrive before flowtable creates the
> +	 * L2 master flow normally used for subflow binding. Resolve only
> +	 * through the bridge master so dev_fill_forward_path() must use the
> +	 * bridge FDB for the destination MAC. Calling the ingress AP netdev
> +	 * directly can describe the source station's WDMA path and would
> +	 * corrupt Wi-Fi-to-wired flows whose real egress is not WDMA.
> +	 */
> +	master = airoha_ppe_foe_get_bridge_master(skb->dev);
> +	if (!master)
> +		return false;
> +
> +	if (airoha_ppe_foe_prepare_wdma_subflow_dev(ppe, master, &data,
> +						    hwe)) {
> +		dev_put(master);
> +		return true;
> +	}
> +
> +	dev_put(master);
> +	return false;

maybe something like:

	ret = airoha_ppe_foe_prepare_wdma_subflow_dev();
	dev_put(master);

	return ret;

> +}
> +
>  static void airoha_ppe_foe_insert_entry(struct airoha_ppe *ppe,
>  					struct sk_buff *skb,
>  					u32 hash, bool rx_wlan)
>  {
> +	struct airoha_foe_entry wdma_hwe = {};
>  	struct airoha_flow_table_entry *e;
>  	struct airoha_foe_bridge br = {};
>  	struct airoha_foe_entry *hwe;
>  	bool commit_done = false;
> +	bool wdma_ready = false;
>  	struct hlist_node *n;
>  	u32 index, state;
>  
> +	wdma_ready = airoha_ppe_foe_prepare_wdma_subflow(ppe, skb,
> +							 &wdma_hwe);
> +
>  	spin_lock_bh(&ppe_lock);
>  
>  	hwe = airoha_ppe_foe_get_entry_locked(ppe, hash);
> @@ -899,6 +997,8 @@ static void airoha_ppe_foe_insert_entry(struct airoha_ppe *ppe,
>  				   airoha_l2_flow_table_params);
>  	if (e)
>  		airoha_ppe_foe_commit_subflow_entry(ppe, e, hash, rx_wlan);
> +	else if (wdma_ready)
> +		airoha_ppe_foe_commit_subflow(ppe, &wdma_hwe, hash, rx_wlan);
>  unlock:
>  	spin_unlock_bh(&ppe_lock);
>  }
> -- 
> 2.53.0
> 

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