[PATCH net] net: airoha: Keep UPDMEM egress MAC in sync for offloaded IPv6 flows

[PATCH net] net: airoha: Keep UPDMEM egress MAC in sync for offloaded IPv6 flows

[陈耀明]

PPE routed IPv6 FOE entries do not carry the egress source MAC inline;
they store an SMAC id and the hardware resolves the address through the
UPDMEM table, which airoha_ppe_init_upd_mem() only programs with the GDM
port own dev_addr at probe time.

The netdev the flow actually egresses can legally carry a different
address: on PON setups the authentication MAC belongs to the pon device
while each service VLAN routes with its own address, so offloaded IPv6
frames left the port with a stale source MAC and were silently dropped
upstream. IPv4 was unaffected since HNAPT entries embed the MAC inline,
which made the failure hard to diagnose: TCP handshake and TLS completed
over the slow path and the transfer stalled exactly when the flow got
offloaded.

Before writing smac_id for routed IPv6 entries, program the UPDMEM slot
with the source address of this flow.

airoha_set_macaddr() writes the same slot from the ndo_open and
ndo_set_mac_address paths: take flow_offload_mutex there too so the two
UPDMEM writers cannot interleave their two-step DATA/CTRL sequence and
leave a stale address behind.

Tested on a Nokia XG-040G-MD (AN7581, GPON WAN, PPPoE over a service
VLAN carrying its own MAC address): IPv6 HTTPS transfers went from
stalling at the 15 s curl cap with partial bytes to completing at line
rate with hardware offloading enabled, matching IPv4 throughput.

Fixes: 00a7678310fe3 ("net: airoha: Introduce flowtable offload support")
Signed-off-by: Yaoming Chen <cc201008@gmail.com>
---
 drivers/net/ethernet/airoha/airoha_eth.c  |  8 ++++++++
 drivers/net/ethernet/airoha/airoha_ppe.c | 13 +++++++++++++
 2 files changed, 21 insertions(+)

--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -138,7 +138,15 @@ airoha_set_macaddr(struct airoha_gdm_dev *dev,
  airoha_fe_wr(eth, REG_FE_MAC_LMIN(reg), lmin);
  airoha_fe_wr(eth, REG_FE_MAC_LMAX(reg), lmax);

+ /*
+ * Serialize with the flowtable offload path: both writers program the
+ * same UPDMEM slot with a two-step DATA/CTRL sequence and concurrent
+ * updates would interleave, leaving a stale source MAC under already
+ * bound IPv6 entries.
+ */
+ mutex_lock(&flow_offload_mutex);
  airoha_ppe_init_upd_mem(dev, addr);
+ mutex_unlock(&flow_offload_mutex);

  return 0;
 }

--- a/drivers/net/ethernet/airoha/airoha_ppe.c
+++ b/drivers/net/ethernet/airoha/airoha_ppe.c
@@ -414,6 +414,19 @@ airoha_ppe_foe_entry_prepare(struct airoha_eth *eth,
   dsa_port);

  smac_id = port->id;
+ /*
+ * Routed IPv6 entries resolve the egress source MAC
+ * through the UPDMEM table indexed by smac_id. The
+ * egress netdev can legally carry a different address
+ * than the GDM port itself: on PON setups the
+ * authentication MAC lives on the pon device while
+ * each service VLAN routes with its own address. Keep
+ * the table entry in sync with the source address this
+ * flow is actually going to use.
+ */
+ if (type >= PPE_PKT_TYPE_IPV6_ROUTE_3T)
+ airoha_ppe_init_upd_mem(dev,
+ data->eth.h_source);
  }
  }

Re: [PATCH net] net: airoha: Keep UPDMEM egress MAC in sync for offloaded IPv6 flows

[Lorenzo Bianconi]

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

> PPE routed IPv6 FOE entries do not carry the egress source MAC inline;
> they store an SMAC id and the hardware resolves the address through the
> UPDMEM table, which airoha_ppe_init_upd_mem() only programs with the GDM
> port own dev_addr at probe time.
> 
> The netdev the flow actually egresses can legally carry a different
> address: on PON setups the authentication MAC belongs to the pon device
> while each service VLAN routes with its own address, so offloaded IPv6
> frames left the port with a stale source MAC and were silently dropped
> upstream. IPv4 was unaffected since HNAPT entries embed the MAC inline,
> which made the failure hard to diagnose: TCP handshake and TLS completed
> over the slow path and the transfer stalled exactly when the flow got
> offloaded.
> 
> Before writing smac_id for routed IPv6 entries, program the UPDMEM slot
> with the source address of this flow.
> 
> airoha_set_macaddr() writes the same slot from the ndo_open and
> ndo_set_mac_address paths: take flow_offload_mutex there too so the two
> UPDMEM writers cannot interleave their two-step DATA/CTRL sequence and
> leave a stale address behind.
> 
> Tested on a Nokia XG-040G-MD (AN7581, GPON WAN, PPPoE over a service
> VLAN carrying its own MAC address): IPv6 HTTPS transfers went from
> stalling at the 15 s curl cap with partial bytes to completing at line
> rate with hardware offloading enabled, matching IPv4 throughput.
> 
> Fixes: 00a7678310fe3 ("net: airoha: Introduce flowtable offload support")
> Signed-off-by: Yaoming Chen <cc201008@gmail.com>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c  |  8 ++++++++
>  drivers/net/ethernet/airoha/airoha_ppe.c | 13 +++++++++++++
>  2 files changed, 21 insertions(+)
> 
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -138,7 +138,15 @@ airoha_set_macaddr(struct airoha_gdm_dev *dev,
>   airoha_fe_wr(eth, REG_FE_MAC_LMIN(reg), lmin);
>   airoha_fe_wr(eth, REG_FE_MAC_LMAX(reg), lmax);
> 
> + /*
> + * Serialize with the flowtable offload path: both writers program the
> + * same UPDMEM slot with a two-step DATA/CTRL sequence and concurrent
> + * updates would interleave, leaving a stale source MAC under already
> + * bound IPv6 entries.
> + */
> + mutex_lock(&flow_offload_mutex);
>   airoha_ppe_init_upd_mem(dev, addr);
> + mutex_unlock(&flow_offload_mutex);

This seems a deadlock to me since airoha_set_macaddr() is running holding
flow_offload_mutex mutex in airoha_enable_qos_for_gdm34().

> 
>   return 0;
>  }
> 
> --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> @@ -414,6 +414,19 @@ airoha_ppe_foe_entry_prepare(struct airoha_eth *eth,
>    dsa_port);
> 
>   smac_id = port->id;
> + /*
> + * Routed IPv6 entries resolve the egress source MAC
> + * through the UPDMEM table indexed by smac_id. The
> + * egress netdev can legally carry a different address
> + * than the GDM port itself: on PON setups the
> + * authentication MAC lives on the pon device while
> + * each service VLAN routes with its own address. Keep
> + * the table entry in sync with the source address this
> + * flow is actually going to use.
> + */
> + if (type >= PPE_PKT_TYPE_IPV6_ROUTE_3T)
> + airoha_ppe_init_upd_mem(dev,
> + data->eth.h_source);

I do not think you used checkpatch here.

Regards,
Lorenzo

>   }
>   }

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

Re: [PATCH net] net: airoha: Keep UPDMEM egress MAC in sync for offloaded IPv6 flows

[陈耀明]

Ooo,  the new patch should be a dedicated leaf lock inside
airoha_ppe_init_upd_mem().
--- a/drivers/net/ethernet/airoha/airoha_ppe.c
+++ b/drivers/net/ethernet/airoha/airoha_ppe.c
@@ -416,6 +416,19 @@
   dsa_port);

  smac_id = port->id;
+ /*
+ * Routed IPv6 entries resolve the egress source MAC
+ * through the UPDMEM table indexed by smac_id. The
+ * egress netdev can legally carry a different address
+ * than the GDM port itself: on PON setups the
+ * authentication MAC lives on the pon device while
+ * each service VLAN routes with its own address. Keep
+ * the table entry in sync with the source address this
+ * flow is actually going to use.
+ */
+ if (type >= PPE_PKT_TYPE_IPV6_ROUTE_3T)
+ airoha_ppe_init_upd_mem(dev,
+ data->eth.h_source);
  }
  }

@@ -1529,10 +1542,21 @@

 void airoha_ppe_init_upd_mem(struct airoha_gdm_dev *dev, const u8 *addr)
 {
+ static DEFINE_MUTEX(updmem_lock);
+
  struct airoha_gdm_port *port = dev->port;
  struct airoha_eth *eth = dev->eth;
  u32 val;

+ /*
+ * Routed IPv6 entries resolve the egress source MAC through the
+ * UPDMEM table indexed by smac_id; the ndo_open/ndo_set_mac_address
+ * writers hold flow_offload_mutex (QoS paths re-lock via
+ * airoha_set_macaddr()), so this lock also serializes the leaf
+ * against them.
+ */
+ mutex_lock(&updmem_lock);
+
  val = (addr[2] << 24) | (addr[3] << 16) | (addr[4] << 8) | addr[5];
  airoha_fe_wr(eth, REG_UPDMEM_DATA(0), val);
  airoha_fe_wr(eth, REG_UPDMEM_CTRL(0),
@@ -1545,6 +1569,8 @@
      FIELD_PREP(PPE_UPDMEM_ADDR_MASK, port->id) |
      FIELD_PREP(PPE_UPDMEM_OFFSET_MASK, 1) |
      PPE_UPDMEM_WR_MASK | PPE_UPDMEM_REQ_MASK);
+
+ mutex_unlock(&updmem_lock);
 }

 struct airoha_ppe_dev *airoha_ppe_get_dev(struct device *dev)

Re: [PATCH net] net: airoha: Keep UPDMEM egress MAC in sync for offloaded IPv6 flows

[Lorenzo Bianconi]

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

> Ooo,  the new patch should be a dedicated leaf lock inside
> airoha_ppe_init_upd_mem().
> --- a/drivers/net/ethernet/airoha/airoha_ppe.c
> +++ b/drivers/net/ethernet/airoha/airoha_ppe.c
> @@ -416,6 +416,19 @@
>    dsa_port);
> 
>   smac_id = port->id;
> + /*
> + * Routed IPv6 entries resolve the egress source MAC
> + * through the UPDMEM table indexed by smac_id. The
> + * egress netdev can legally carry a different address
> + * than the GDM port itself: on PON setups the
> + * authentication MAC lives on the pon device while
> + * each service VLAN routes with its own address. Keep
> + * the table entry in sync with the source address this
> + * flow is actually going to use.

Is the issue occurring just with PON or are you able to trigger the issue even
in the upstream codebase? I guess you should not post a fix for some code that
is not upstream yet. Moreover, in the PON setup, I guess the approach is still
broken if the user change the device mac address.

> + */
> + if (type >= PPE_PKT_TYPE_IPV6_ROUTE_3T)
> + airoha_ppe_init_upd_mem(dev,
> + data->eth.h_source);
>   }
>   }
> 
> @@ -1529,10 +1542,21 @@
> 
>  void airoha_ppe_init_upd_mem(struct airoha_gdm_dev *dev, const u8 *addr)
>  {
> + static DEFINE_MUTEX(updmem_lock);
> +
>   struct airoha_gdm_port *port = dev->port;
>   struct airoha_eth *eth = dev->eth;
>   u32 val;
> 
> + /*
> + * Routed IPv6 entries resolve the egress source MAC through the
> + * UPDMEM table indexed by smac_id; the ndo_open/ndo_set_mac_address
> + * writers hold flow_offload_mutex (QoS paths re-lock via
> + * airoha_set_macaddr()), so this lock also serializes the leaf
> + * against them.
> + */
> + mutex_lock(&updmem_lock);

This is only required adding the above configuration since in the current
codebase airoha_ppe_init_upd_mem() is always running holding RTNL.

Regards,
Lorenzo

> +
>   val = (addr[2] << 24) | (addr[3] << 16) | (addr[4] << 8) | addr[5];
>   airoha_fe_wr(eth, REG_UPDMEM_DATA(0), val);
>   airoha_fe_wr(eth, REG_UPDMEM_CTRL(0),
> @@ -1545,6 +1569,8 @@
>       FIELD_PREP(PPE_UPDMEM_ADDR_MASK, port->id) |
>       FIELD_PREP(PPE_UPDMEM_OFFSET_MASK, 1) |
>       PPE_UPDMEM_WR_MASK | PPE_UPDMEM_REQ_MASK);
> +
> + mutex_unlock(&updmem_lock);
>  }
> 
>  struct airoha_ppe_dev *airoha_ppe_get_dev(struct device *dev)

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