[RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[DENG Qingfang]

When a client moves from a DSA user port to a software port in a bridge,
it cannot reach any other clients that connected to the DSA user ports.
That is because SA learning on the CPU port is disabled, so the switch
ignores the client's frames from the CPU port and still thinks it is at
the user port.

Fix it by enabling SA learning on the CPU port.

To prevent the switch from learning from flooding frames from the CPU
port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
and let the switch flood them instead of trapping to the CPU port.
Multicast frames still need to be trapped to the CPU port for snooping,
so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
to disable SA learning.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 drivers/net/dsa/mt7530.c |  9 ++-------
 drivers/net/dsa/mt7530.h |  1 +
 net/dsa/tag_mtk.c        | 15 +++++++++++++++
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 5c444cd722bd..34e4aadfa705 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -628,11 +628,8 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
 	mt7530_write(priv, MT7530_PVC_P(port),
 		     PORT_SPEC_TAG);
 
-	/* Disable auto learning on the cpu port */
-	mt7530_set(priv, MT7530_PSC_P(port), SA_DIS);
-
-	/* Unknown unicast frame fordwarding to the cpu port */
-	mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(port)));
+	/* Unknown multicast frame forwarding to the cpu port */
+	mt7530_rmw(priv, MT7530_MFC, UNM_FFP_MASK, UNM_FFP(BIT(port)));
 
 	/* Set CPU port number */
 	if (priv->id == ID_MT7621)
@@ -1294,8 +1291,6 @@ mt7530_setup(struct dsa_switch *ds)
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);
 
-	mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
-
 	for (i = 0; i < MT7530_NUM_PORTS; i++) {
 		/* Disable forwarding by default on all ports */
 		mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 979bb6374678..82af4d2d406e 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -31,6 +31,7 @@ enum {
 #define MT7530_MFC			0x10
 #define  BC_FFP(x)			(((x) & 0xff) << 24)
 #define  UNM_FFP(x)			(((x) & 0xff) << 16)
+#define  UNM_FFP_MASK			UNM_FFP(~0)
 #define  UNU_FFP(x)			(((x) & 0xff) << 8)
 #define  UNU_FFP_MASK			UNU_FFP(~0)
 #define  CPU_EN				BIT(7)
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index b5705cba8318..d6619edd53e5 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -15,6 +15,7 @@
 #define MTK_HDR_XMIT_TAGGED_TPID_8100	1
 #define MTK_HDR_RECV_SOURCE_PORT_MASK	GENMASK(2, 0)
 #define MTK_HDR_XMIT_DP_BIT_MASK	GENMASK(5, 0)
+#define MTK_HDR_XMIT_SA_DIS		BIT(6)
 
 static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
@@ -22,6 +23,9 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	u8 *mtk_tag;
 	bool is_vlan_skb = true;
+	unsigned char *dest = eth_hdr(skb)->h_dest;
+	bool is_multicast_skb = is_multicast_ether_addr(dest) &&
+				!is_broadcast_ether_addr(dest);
 
 	/* Build the special tag after the MAC Source Address. If VLAN header
 	 * is present, it's required that VLAN header and special tag is
@@ -47,6 +51,10 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 		     MTK_HDR_XMIT_UNTAGGED;
 	mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
 
+	/* Disable SA learning for multicast frames */
+	if (unlikely(is_multicast_skb))
+		mtk_tag[1] |= MTK_HDR_XMIT_SA_DIS;
+
 	/* Tag control information is kept for 802.1Q */
 	if (!is_vlan_skb) {
 		mtk_tag[2] = 0;
@@ -61,6 +69,9 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 {
 	int port;
 	__be16 *phdr, hdr;
+	unsigned char *dest = eth_hdr(skb)->h_dest;
+	bool is_multicast_skb = is_multicast_ether_addr(dest) &&
+				!is_broadcast_ether_addr(dest);
 
 	if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
 		return NULL;
@@ -86,6 +97,10 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb->dev)
 		return NULL;
 
+	/* Only unicast or broadcast frames are offloaded */
+	if (likely(!is_multicast_skb))
+		skb->offload_fwd_mark = 1;
+
 	return skb;
 }
 
-- 
2.26.1

[RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[DENG Qingfang]

When a client moves from a DSA user port to a software port in a bridge,
it cannot reach any other clients that connected to the DSA user ports.
That is because SA learning on the CPU port is disabled, so the switch
ignores the client's frames from the CPU port and still thinks it is at
the user port.

Fix it by enabling SA learning on the CPU port.

To prevent the switch from learning from flooding frames from the CPU
port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
and let the switch flood them instead of trapping to the CPU port.
Multicast frames still need to be trapped to the CPU port for snooping,
so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
to disable SA learning.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by: DENG Qingfang <dqfext@gmail.com>
---
 drivers/net/dsa/mt7530.c |  9 ++-------
 drivers/net/dsa/mt7530.h |  1 +
 net/dsa/tag_mtk.c        | 15 +++++++++++++++
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 5c444cd722bd..34e4aadfa705 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -628,11 +628,8 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
 	mt7530_write(priv, MT7530_PVC_P(port),
 		     PORT_SPEC_TAG);
 
-	/* Disable auto learning on the cpu port */
-	mt7530_set(priv, MT7530_PSC_P(port), SA_DIS);
-
-	/* Unknown unicast frame fordwarding to the cpu port */
-	mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(port)));
+	/* Unknown multicast frame forwarding to the cpu port */
+	mt7530_rmw(priv, MT7530_MFC, UNM_FFP_MASK, UNM_FFP(BIT(port)));
 
 	/* Set CPU port number */
 	if (priv->id == ID_MT7621)
@@ -1294,8 +1291,6 @@ mt7530_setup(struct dsa_switch *ds)
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);
 
-	mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
-
 	for (i = 0; i < MT7530_NUM_PORTS; i++) {
 		/* Disable forwarding by default on all ports */
 		mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 979bb6374678..82af4d2d406e 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -31,6 +31,7 @@ enum {
 #define MT7530_MFC			0x10
 #define  BC_FFP(x)			(((x) & 0xff) << 24)
 #define  UNM_FFP(x)			(((x) & 0xff) << 16)
+#define  UNM_FFP_MASK			UNM_FFP(~0)
 #define  UNU_FFP(x)			(((x) & 0xff) << 8)
 #define  UNU_FFP_MASK			UNU_FFP(~0)
 #define  CPU_EN				BIT(7)
diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
index b5705cba8318..d6619edd53e5 100644
--- a/net/dsa/tag_mtk.c
+++ b/net/dsa/tag_mtk.c
@@ -15,6 +15,7 @@
 #define MTK_HDR_XMIT_TAGGED_TPID_8100	1
 #define MTK_HDR_RECV_SOURCE_PORT_MASK	GENMASK(2, 0)
 #define MTK_HDR_XMIT_DP_BIT_MASK	GENMASK(5, 0)
+#define MTK_HDR_XMIT_SA_DIS		BIT(6)
 
 static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
@@ -22,6 +23,9 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	u8 *mtk_tag;
 	bool is_vlan_skb = true;
+	unsigned char *dest = eth_hdr(skb)->h_dest;
+	bool is_multicast_skb = is_multicast_ether_addr(dest) &&
+				!is_broadcast_ether_addr(dest);
 
 	/* Build the special tag after the MAC Source Address. If VLAN header
 	 * is present, it's required that VLAN header and special tag is
@@ -47,6 +51,10 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
 		     MTK_HDR_XMIT_UNTAGGED;
 	mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
 
+	/* Disable SA learning for multicast frames */
+	if (unlikely(is_multicast_skb))
+		mtk_tag[1] |= MTK_HDR_XMIT_SA_DIS;
+
 	/* Tag control information is kept for 802.1Q */
 	if (!is_vlan_skb) {
 		mtk_tag[2] = 0;
@@ -61,6 +69,9 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 {
 	int port;
 	__be16 *phdr, hdr;
+	unsigned char *dest = eth_hdr(skb)->h_dest;
+	bool is_multicast_skb = is_multicast_ether_addr(dest) &&
+				!is_broadcast_ether_addr(dest);
 
 	if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
 		return NULL;
@@ -86,6 +97,10 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb->dev)
 		return NULL;
 
+	/* Only unicast or broadcast frames are offloaded */
+	if (likely(!is_multicast_skb))
+		skb->offload_fwd_mark = 1;
+
 	return skb;
 }
 
-- 
2.26.1


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[Vladimir Oltean]

Hi Qingfang,

On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqfext@gmail.com> wrote:
>
> When a client moves from a DSA user port to a software port in a bridge,
> it cannot reach any other clients that connected to the DSA user ports.
> That is because SA learning on the CPU port is disabled, so the switch
> ignores the client's frames from the CPU port and still thinks it is at
> the user port.
>
> Fix it by enabling SA learning on the CPU port.
>
> To prevent the switch from learning from flooding frames from the CPU
> port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
> and let the switch flood them instead of trapping to the CPU port.
> Multicast frames still need to be trapped to the CPU port for snooping,
> so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
> to disable SA learning.
>
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---

I think enabling learning on the CPU port would fix the problem
sometimes, but not always. (actually nothing can solve it always, see
below)
The switch learns the new route only if it receives any packets from
the CPU port, with a SA equal to the station you're trying to reach.
But what if the station is not sending any traffic at the moment,
because it is simply waiting for connections to it first (just an
example)?
Unless there is any traffic already coming from the destination
station too, your patch won't work.
I am currently facing a similar situation with the ocelot/felix
switches, but in that case, enabling SA learning on the CPU port is
not possible.
The way I dealt with it is by forcing a flush of the FDB entries on
the port, in the following scenarios:
- link goes down
- port leaves its bridge
So traffic towards a destination that has migrated away will
temporarily be flooded again (towards the CPU port as well).
There is still one case which isn't treated using this approach: when
the station migrates away from a switch port that is not directly
connected to this one. So no "link down" events would get generated in
that case. We would still have to wait until the address expires in
that case. I don't think that particular situation can be solved.
My point is: if we agree that this is a larger problem, then DSA
should have a .port_fdb_flush method and schedule a workqueue whenever
necessary. Yes, it is a costly operation, but it will still probably
take a lot less than the 300 seconds that the bridge configures for
address ageing.

Thoughts?

>  drivers/net/dsa/mt7530.c |  9 ++-------
>  drivers/net/dsa/mt7530.h |  1 +
>  net/dsa/tag_mtk.c        | 15 +++++++++++++++
>  3 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 5c444cd722bd..34e4aadfa705 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -628,11 +628,8 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
>         mt7530_write(priv, MT7530_PVC_P(port),
>                      PORT_SPEC_TAG);
>
> -       /* Disable auto learning on the cpu port */
> -       mt7530_set(priv, MT7530_PSC_P(port), SA_DIS);
> -
> -       /* Unknown unicast frame fordwarding to the cpu port */
> -       mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(port)));
> +       /* Unknown multicast frame forwarding to the cpu port */
> +       mt7530_rmw(priv, MT7530_MFC, UNM_FFP_MASK, UNM_FFP(BIT(port)));
>
>         /* Set CPU port number */
>         if (priv->id == ID_MT7621)
> @@ -1294,8 +1291,6 @@ mt7530_setup(struct dsa_switch *ds)
>         /* Enable and reset MIB counters */
>         mt7530_mib_reset(ds);
>
> -       mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
> -
>         for (i = 0; i < MT7530_NUM_PORTS; i++) {
>                 /* Disable forwarding by default on all ports */
>                 mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 979bb6374678..82af4d2d406e 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -31,6 +31,7 @@ enum {
>  #define MT7530_MFC                     0x10
>  #define  BC_FFP(x)                     (((x) & 0xff) << 24)
>  #define  UNM_FFP(x)                    (((x) & 0xff) << 16)
> +#define  UNM_FFP_MASK                  UNM_FFP(~0)
>  #define  UNU_FFP(x)                    (((x) & 0xff) << 8)
>  #define  UNU_FFP_MASK                  UNU_FFP(~0)
>  #define  CPU_EN                                BIT(7)
> diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
> index b5705cba8318..d6619edd53e5 100644
> --- a/net/dsa/tag_mtk.c
> +++ b/net/dsa/tag_mtk.c
> @@ -15,6 +15,7 @@
>  #define MTK_HDR_XMIT_TAGGED_TPID_8100  1
>  #define MTK_HDR_RECV_SOURCE_PORT_MASK  GENMASK(2, 0)
>  #define MTK_HDR_XMIT_DP_BIT_MASK       GENMASK(5, 0)
> +#define MTK_HDR_XMIT_SA_DIS            BIT(6)
>
>  static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>                                     struct net_device *dev)
> @@ -22,6 +23,9 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>         struct dsa_port *dp = dsa_slave_to_port(dev);
>         u8 *mtk_tag;
>         bool is_vlan_skb = true;
> +       unsigned char *dest = eth_hdr(skb)->h_dest;
> +       bool is_multicast_skb = is_multicast_ether_addr(dest) &&
> +                               !is_broadcast_ether_addr(dest);
>
>         /* Build the special tag after the MAC Source Address. If VLAN header
>          * is present, it's required that VLAN header and special tag is
> @@ -47,6 +51,10 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>                      MTK_HDR_XMIT_UNTAGGED;
>         mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
>
> +       /* Disable SA learning for multicast frames */
> +       if (unlikely(is_multicast_skb))
> +               mtk_tag[1] |= MTK_HDR_XMIT_SA_DIS;
> +
>         /* Tag control information is kept for 802.1Q */
>         if (!is_vlan_skb) {
>                 mtk_tag[2] = 0;
> @@ -61,6 +69,9 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
>  {
>         int port;
>         __be16 *phdr, hdr;
> +       unsigned char *dest = eth_hdr(skb)->h_dest;
> +       bool is_multicast_skb = is_multicast_ether_addr(dest) &&
> +                               !is_broadcast_ether_addr(dest);
>
>         if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
>                 return NULL;
> @@ -86,6 +97,10 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
>         if (!skb->dev)
>                 return NULL;
>
> +       /* Only unicast or broadcast frames are offloaded */
> +       if (likely(!is_multicast_skb))
> +               skb->offload_fwd_mark = 1;
> +
>         return skb;
>  }
>
> --
> 2.26.1
>

Thanks,
-Vladimir

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[Vladimir Oltean]

Hi Qingfang,

On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqfext@gmail.com> wrote:
>
> When a client moves from a DSA user port to a software port in a bridge,
> it cannot reach any other clients that connected to the DSA user ports.
> That is because SA learning on the CPU port is disabled, so the switch
> ignores the client's frames from the CPU port and still thinks it is at
> the user port.
>
> Fix it by enabling SA learning on the CPU port.
>
> To prevent the switch from learning from flooding frames from the CPU
> port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
> and let the switch flood them instead of trapping to the CPU port.
> Multicast frames still need to be trapped to the CPU port for snooping,
> so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
> to disable SA learning.
>
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---

I think enabling learning on the CPU port would fix the problem
sometimes, but not always. (actually nothing can solve it always, see
below)
The switch learns the new route only if it receives any packets from
the CPU port, with a SA equal to the station you're trying to reach.
But what if the station is not sending any traffic at the moment,
because it is simply waiting for connections to it first (just an
example)?
Unless there is any traffic already coming from the destination
station too, your patch won't work.
I am currently facing a similar situation with the ocelot/felix
switches, but in that case, enabling SA learning on the CPU port is
not possible.
The way I dealt with it is by forcing a flush of the FDB entries on
the port, in the following scenarios:
- link goes down
- port leaves its bridge
So traffic towards a destination that has migrated away will
temporarily be flooded again (towards the CPU port as well).
There is still one case which isn't treated using this approach: when
the station migrates away from a switch port that is not directly
connected to this one. So no "link down" events would get generated in
that case. We would still have to wait until the address expires in
that case. I don't think that particular situation can be solved.
My point is: if we agree that this is a larger problem, then DSA
should have a .port_fdb_flush method and schedule a workqueue whenever
necessary. Yes, it is a costly operation, but it will still probably
take a lot less than the 300 seconds that the bridge configures for
address ageing.

Thoughts?

>  drivers/net/dsa/mt7530.c |  9 ++-------
>  drivers/net/dsa/mt7530.h |  1 +
>  net/dsa/tag_mtk.c        | 15 +++++++++++++++
>  3 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 5c444cd722bd..34e4aadfa705 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -628,11 +628,8 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
>         mt7530_write(priv, MT7530_PVC_P(port),
>                      PORT_SPEC_TAG);
>
> -       /* Disable auto learning on the cpu port */
> -       mt7530_set(priv, MT7530_PSC_P(port), SA_DIS);
> -
> -       /* Unknown unicast frame fordwarding to the cpu port */
> -       mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(port)));
> +       /* Unknown multicast frame forwarding to the cpu port */
> +       mt7530_rmw(priv, MT7530_MFC, UNM_FFP_MASK, UNM_FFP(BIT(port)));
>
>         /* Set CPU port number */
>         if (priv->id == ID_MT7621)
> @@ -1294,8 +1291,6 @@ mt7530_setup(struct dsa_switch *ds)
>         /* Enable and reset MIB counters */
>         mt7530_mib_reset(ds);
>
> -       mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
> -
>         for (i = 0; i < MT7530_NUM_PORTS; i++) {
>                 /* Disable forwarding by default on all ports */
>                 mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 979bb6374678..82af4d2d406e 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -31,6 +31,7 @@ enum {
>  #define MT7530_MFC                     0x10
>  #define  BC_FFP(x)                     (((x) & 0xff) << 24)
>  #define  UNM_FFP(x)                    (((x) & 0xff) << 16)
> +#define  UNM_FFP_MASK                  UNM_FFP(~0)
>  #define  UNU_FFP(x)                    (((x) & 0xff) << 8)
>  #define  UNU_FFP_MASK                  UNU_FFP(~0)
>  #define  CPU_EN                                BIT(7)
> diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
> index b5705cba8318..d6619edd53e5 100644
> --- a/net/dsa/tag_mtk.c
> +++ b/net/dsa/tag_mtk.c
> @@ -15,6 +15,7 @@
>  #define MTK_HDR_XMIT_TAGGED_TPID_8100  1
>  #define MTK_HDR_RECV_SOURCE_PORT_MASK  GENMASK(2, 0)
>  #define MTK_HDR_XMIT_DP_BIT_MASK       GENMASK(5, 0)
> +#define MTK_HDR_XMIT_SA_DIS            BIT(6)
>
>  static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>                                     struct net_device *dev)
> @@ -22,6 +23,9 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>         struct dsa_port *dp = dsa_slave_to_port(dev);
>         u8 *mtk_tag;
>         bool is_vlan_skb = true;
> +       unsigned char *dest = eth_hdr(skb)->h_dest;
> +       bool is_multicast_skb = is_multicast_ether_addr(dest) &&
> +                               !is_broadcast_ether_addr(dest);
>
>         /* Build the special tag after the MAC Source Address. If VLAN header
>          * is present, it's required that VLAN header and special tag is
> @@ -47,6 +51,10 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
>                      MTK_HDR_XMIT_UNTAGGED;
>         mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
>
> +       /* Disable SA learning for multicast frames */
> +       if (unlikely(is_multicast_skb))
> +               mtk_tag[1] |= MTK_HDR_XMIT_SA_DIS;
> +
>         /* Tag control information is kept for 802.1Q */
>         if (!is_vlan_skb) {
>                 mtk_tag[2] = 0;
> @@ -61,6 +69,9 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
>  {
>         int port;
>         __be16 *phdr, hdr;
> +       unsigned char *dest = eth_hdr(skb)->h_dest;
> +       bool is_multicast_skb = is_multicast_ether_addr(dest) &&
> +                               !is_broadcast_ether_addr(dest);
>
>         if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
>                 return NULL;
> @@ -86,6 +97,10 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
>         if (!skb->dev)
>                 return NULL;
>
> +       /* Only unicast or broadcast frames are offloaded */
> +       if (likely(!is_multicast_skb))
> +               skb->offload_fwd_mark = 1;
> +
>         return skb;
>  }
>
> --
> 2.26.1
>

Thanks,
-Vladimir

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[DENG Qingfang]

Hi Vladimir,

On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Qingfang,
>
> On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqfext@gmail.com> wrote:
> >
> > When a client moves from a DSA user port to a software port in a bridge,
> > it cannot reach any other clients that connected to the DSA user ports.
> > That is because SA learning on the CPU port is disabled, so the switch
> > ignores the client's frames from the CPU port and still thinks it is at
> > the user port.
> >
> > Fix it by enabling SA learning on the CPU port.
> >
> > To prevent the switch from learning from flooding frames from the CPU
> > port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
> > and let the switch flood them instead of trapping to the CPU port.
> > Multicast frames still need to be trapped to the CPU port for snooping,
> > so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
> > to disable SA learning.
> >
> > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > ---
>
> I think enabling learning on the CPU port would fix the problem
> sometimes, but not always. (actually nothing can solve it always, see
> below)
> The switch learns the new route only if it receives any packets from
> the CPU port, with a SA equal to the station you're trying to reach.
> But what if the station is not sending any traffic at the moment,
> because it is simply waiting for connections to it first (just an
> example)?
> Unless there is any traffic already coming from the destination
> station too, your patch won't work.
> I am currently facing a similar situation with the ocelot/felix
> switches, but in that case, enabling SA learning on the CPU port is
> not possible.

Why is it not possible?

Then try my previous RFC patch
"net: bridge: fix client roaming from DSA user port"
It tries removing entries from the switch when the client moves to another port.

> The way I dealt with it is by forcing a flush of the FDB entries on
> the port, in the following scenarios:
> - link goes down
> - port leaves its bridge
> So traffic towards a destination that has migrated away will
> temporarily be flooded again (towards the CPU port as well).
> There is still one case which isn't treated using this approach: when
> the station migrates away from a switch port that is not directly
> connected to this one. So no "link down" events would get generated in
> that case. We would still have to wait until the address expires in
> that case. I don't think that particular situation can be solved.

You're right. Every switch has this issue, even Linux bridge.

> My point is: if we agree that this is a larger problem, then DSA
> should have a .port_fdb_flush method and schedule a workqueue whenever
> necessary. Yes, it is a costly operation, but it will still probably
> take a lot less than the 300 seconds that the bridge configures for
> address ageing.
>
> Thoughts?
>
> >  drivers/net/dsa/mt7530.c |  9 ++-------
> >  drivers/net/dsa/mt7530.h |  1 +
> >  net/dsa/tag_mtk.c        | 15 +++++++++++++++
> >  3 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index 5c444cd722bd..34e4aadfa705 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -628,11 +628,8 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
> >         mt7530_write(priv, MT7530_PVC_P(port),
> >                      PORT_SPEC_TAG);
> >
> > -       /* Disable auto learning on the cpu port */
> > -       mt7530_set(priv, MT7530_PSC_P(port), SA_DIS);
> > -
> > -       /* Unknown unicast frame fordwarding to the cpu port */
> > -       mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(port)));
> > +       /* Unknown multicast frame forwarding to the cpu port */
> > +       mt7530_rmw(priv, MT7530_MFC, UNM_FFP_MASK, UNM_FFP(BIT(port)));
> >
> >         /* Set CPU port number */
> >         if (priv->id == ID_MT7621)
> > @@ -1294,8 +1291,6 @@ mt7530_setup(struct dsa_switch *ds)
> >         /* Enable and reset MIB counters */
> >         mt7530_mib_reset(ds);
> >
> > -       mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
> > -
> >         for (i = 0; i < MT7530_NUM_PORTS; i++) {
> >                 /* Disable forwarding by default on all ports */
> >                 mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
> > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> > index 979bb6374678..82af4d2d406e 100644
> > --- a/drivers/net/dsa/mt7530.h
> > +++ b/drivers/net/dsa/mt7530.h
> > @@ -31,6 +31,7 @@ enum {
> >  #define MT7530_MFC                     0x10
> >  #define  BC_FFP(x)                     (((x) & 0xff) << 24)
> >  #define  UNM_FFP(x)                    (((x) & 0xff) << 16)
> > +#define  UNM_FFP_MASK                  UNM_FFP(~0)
> >  #define  UNU_FFP(x)                    (((x) & 0xff) << 8)
> >  #define  UNU_FFP_MASK                  UNU_FFP(~0)
> >  #define  CPU_EN                                BIT(7)
> > diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
> > index b5705cba8318..d6619edd53e5 100644
> > --- a/net/dsa/tag_mtk.c
> > +++ b/net/dsa/tag_mtk.c
> > @@ -15,6 +15,7 @@
> >  #define MTK_HDR_XMIT_TAGGED_TPID_8100  1
> >  #define MTK_HDR_RECV_SOURCE_PORT_MASK  GENMASK(2, 0)
> >  #define MTK_HDR_XMIT_DP_BIT_MASK       GENMASK(5, 0)
> > +#define MTK_HDR_XMIT_SA_DIS            BIT(6)
> >
> >  static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
> >                                     struct net_device *dev)
> > @@ -22,6 +23,9 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
> >         struct dsa_port *dp = dsa_slave_to_port(dev);
> >         u8 *mtk_tag;
> >         bool is_vlan_skb = true;
> > +       unsigned char *dest = eth_hdr(skb)->h_dest;
> > +       bool is_multicast_skb = is_multicast_ether_addr(dest) &&
> > +                               !is_broadcast_ether_addr(dest);
> >
> >         /* Build the special tag after the MAC Source Address. If VLAN header
> >          * is present, it's required that VLAN header and special tag is
> > @@ -47,6 +51,10 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
> >                      MTK_HDR_XMIT_UNTAGGED;
> >         mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
> >
> > +       /* Disable SA learning for multicast frames */
> > +       if (unlikely(is_multicast_skb))
> > +               mtk_tag[1] |= MTK_HDR_XMIT_SA_DIS;
> > +
> >         /* Tag control information is kept for 802.1Q */
> >         if (!is_vlan_skb) {
> >                 mtk_tag[2] = 0;
> > @@ -61,6 +69,9 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
> >  {
> >         int port;
> >         __be16 *phdr, hdr;
> > +       unsigned char *dest = eth_hdr(skb)->h_dest;
> > +       bool is_multicast_skb = is_multicast_ether_addr(dest) &&
> > +                               !is_broadcast_ether_addr(dest);
> >
> >         if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
> >                 return NULL;
> > @@ -86,6 +97,10 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
> >         if (!skb->dev)
> >                 return NULL;
> >
> > +       /* Only unicast or broadcast frames are offloaded */
> > +       if (likely(!is_multicast_skb))
> > +               skb->offload_fwd_mark = 1;
> > +
> >         return skb;
> >  }
> >
> > --
> > 2.26.1
> >
>
> Thanks,
> -Vladimir

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[DENG Qingfang]

Hi Vladimir,

On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Qingfang,
>
> On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqfext@gmail.com> wrote:
> >
> > When a client moves from a DSA user port to a software port in a bridge,
> > it cannot reach any other clients that connected to the DSA user ports.
> > That is because SA learning on the CPU port is disabled, so the switch
> > ignores the client's frames from the CPU port and still thinks it is at
> > the user port.
> >
> > Fix it by enabling SA learning on the CPU port.
> >
> > To prevent the switch from learning from flooding frames from the CPU
> > port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
> > and let the switch flood them instead of trapping to the CPU port.
> > Multicast frames still need to be trapped to the CPU port for snooping,
> > so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
> > to disable SA learning.
> >
> > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > ---
>
> I think enabling learning on the CPU port would fix the problem
> sometimes, but not always. (actually nothing can solve it always, see
> below)
> The switch learns the new route only if it receives any packets from
> the CPU port, with a SA equal to the station you're trying to reach.
> But what if the station is not sending any traffic at the moment,
> because it is simply waiting for connections to it first (just an
> example)?
> Unless there is any traffic already coming from the destination
> station too, your patch won't work.
> I am currently facing a similar situation with the ocelot/felix
> switches, but in that case, enabling SA learning on the CPU port is
> not possible.

Why is it not possible?

Then try my previous RFC patch
"net: bridge: fix client roaming from DSA user port"
It tries removing entries from the switch when the client moves to another port.

> The way I dealt with it is by forcing a flush of the FDB entries on
> the port, in the following scenarios:
> - link goes down
> - port leaves its bridge
> So traffic towards a destination that has migrated away will
> temporarily be flooded again (towards the CPU port as well).
> There is still one case which isn't treated using this approach: when
> the station migrates away from a switch port that is not directly
> connected to this one. So no "link down" events would get generated in
> that case. We would still have to wait until the address expires in
> that case. I don't think that particular situation can be solved.

You're right. Every switch has this issue, even Linux bridge.

> My point is: if we agree that this is a larger problem, then DSA
> should have a .port_fdb_flush method and schedule a workqueue whenever
> necessary. Yes, it is a costly operation, but it will still probably
> take a lot less than the 300 seconds that the bridge configures for
> address ageing.
>
> Thoughts?
>
> >  drivers/net/dsa/mt7530.c |  9 ++-------
> >  drivers/net/dsa/mt7530.h |  1 +
> >  net/dsa/tag_mtk.c        | 15 +++++++++++++++
> >  3 files changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index 5c444cd722bd..34e4aadfa705 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -628,11 +628,8 @@ mt7530_cpu_port_enable(struct mt7530_priv *priv,
> >         mt7530_write(priv, MT7530_PVC_P(port),
> >                      PORT_SPEC_TAG);
> >
> > -       /* Disable auto learning on the cpu port */
> > -       mt7530_set(priv, MT7530_PSC_P(port), SA_DIS);
> > -
> > -       /* Unknown unicast frame fordwarding to the cpu port */
> > -       mt7530_set(priv, MT7530_MFC, UNU_FFP(BIT(port)));
> > +       /* Unknown multicast frame forwarding to the cpu port */
> > +       mt7530_rmw(priv, MT7530_MFC, UNM_FFP_MASK, UNM_FFP(BIT(port)));
> >
> >         /* Set CPU port number */
> >         if (priv->id == ID_MT7621)
> > @@ -1294,8 +1291,6 @@ mt7530_setup(struct dsa_switch *ds)
> >         /* Enable and reset MIB counters */
> >         mt7530_mib_reset(ds);
> >
> > -       mt7530_clear(priv, MT7530_MFC, UNU_FFP_MASK);
> > -
> >         for (i = 0; i < MT7530_NUM_PORTS; i++) {
> >                 /* Disable forwarding by default on all ports */
> >                 mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
> > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> > index 979bb6374678..82af4d2d406e 100644
> > --- a/drivers/net/dsa/mt7530.h
> > +++ b/drivers/net/dsa/mt7530.h
> > @@ -31,6 +31,7 @@ enum {
> >  #define MT7530_MFC                     0x10
> >  #define  BC_FFP(x)                     (((x) & 0xff) << 24)
> >  #define  UNM_FFP(x)                    (((x) & 0xff) << 16)
> > +#define  UNM_FFP_MASK                  UNM_FFP(~0)
> >  #define  UNU_FFP(x)                    (((x) & 0xff) << 8)
> >  #define  UNU_FFP_MASK                  UNU_FFP(~0)
> >  #define  CPU_EN                                BIT(7)
> > diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c
> > index b5705cba8318..d6619edd53e5 100644
> > --- a/net/dsa/tag_mtk.c
> > +++ b/net/dsa/tag_mtk.c
> > @@ -15,6 +15,7 @@
> >  #define MTK_HDR_XMIT_TAGGED_TPID_8100  1
> >  #define MTK_HDR_RECV_SOURCE_PORT_MASK  GENMASK(2, 0)
> >  #define MTK_HDR_XMIT_DP_BIT_MASK       GENMASK(5, 0)
> > +#define MTK_HDR_XMIT_SA_DIS            BIT(6)
> >
> >  static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
> >                                     struct net_device *dev)
> > @@ -22,6 +23,9 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
> >         struct dsa_port *dp = dsa_slave_to_port(dev);
> >         u8 *mtk_tag;
> >         bool is_vlan_skb = true;
> > +       unsigned char *dest = eth_hdr(skb)->h_dest;
> > +       bool is_multicast_skb = is_multicast_ether_addr(dest) &&
> > +                               !is_broadcast_ether_addr(dest);
> >
> >         /* Build the special tag after the MAC Source Address. If VLAN header
> >          * is present, it's required that VLAN header and special tag is
> > @@ -47,6 +51,10 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb,
> >                      MTK_HDR_XMIT_UNTAGGED;
> >         mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
> >
> > +       /* Disable SA learning for multicast frames */
> > +       if (unlikely(is_multicast_skb))
> > +               mtk_tag[1] |= MTK_HDR_XMIT_SA_DIS;
> > +
> >         /* Tag control information is kept for 802.1Q */
> >         if (!is_vlan_skb) {
> >                 mtk_tag[2] = 0;
> > @@ -61,6 +69,9 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
> >  {
> >         int port;
> >         __be16 *phdr, hdr;
> > +       unsigned char *dest = eth_hdr(skb)->h_dest;
> > +       bool is_multicast_skb = is_multicast_ether_addr(dest) &&
> > +                               !is_broadcast_ether_addr(dest);
> >
> >         if (unlikely(!pskb_may_pull(skb, MTK_HDR_LEN)))
> >                 return NULL;
> > @@ -86,6 +97,10 @@ static struct sk_buff *mtk_tag_rcv(struct sk_buff *skb, struct net_device *dev,
> >         if (!skb->dev)
> >                 return NULL;
> >
> > +       /* Only unicast or broadcast frames are offloaded */
> > +       if (likely(!is_multicast_skb))
> > +               skb->offload_fwd_mark = 1;
> > +
> >         return skb;
> >  }
> >
> > --
> > 2.26.1
> >
>
> Thanks,
> -Vladimir

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[Vladimir Oltean]

Hi Qingfang,

On Mon, 4 May 2020 at 15:47, DENG Qingfang <dqfext@gmail.com> wrote:
>
> Hi Vladimir,
>
> On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Hi Qingfang,
> >
> > On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqfext@gmail.com> wrote:
> > >
> > > When a client moves from a DSA user port to a software port in a bridge,
> > > it cannot reach any other clients that connected to the DSA user ports.
> > > That is because SA learning on the CPU port is disabled, so the switch
> > > ignores the client's frames from the CPU port and still thinks it is at
> > > the user port.
> > >
> > > Fix it by enabling SA learning on the CPU port.
> > >
> > > To prevent the switch from learning from flooding frames from the CPU
> > > port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
> > > and let the switch flood them instead of trapping to the CPU port.
> > > Multicast frames still need to be trapped to the CPU port for snooping,
> > > so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
> > > to disable SA learning.
> > >
> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > > ---
> >
> > I think enabling learning on the CPU port would fix the problem
> > sometimes, but not always. (actually nothing can solve it always, see
> > below)
> > The switch learns the new route only if it receives any packets from
> > the CPU port, with a SA equal to the station you're trying to reach.
> > But what if the station is not sending any traffic at the moment,
> > because it is simply waiting for connections to it first (just an
> > example)?
> > Unless there is any traffic already coming from the destination
> > station too, your patch won't work.
> > I am currently facing a similar situation with the ocelot/felix
> > switches, but in that case, enabling SA learning on the CPU port is
> > not possible.
>
> Why is it not possible?
>

Because learning on the CPU port is not supported on this hardware.

> Then try my previous RFC patch
> "net: bridge: fix client roaming from DSA user port"
> It tries removing entries from the switch when the client moves to another port.
>

Your patch only deletes FDB entries of packets received in the
fastpath by the software bridge, which as I said, won't work if the
software bridge doesn't receive packets in the first place due to a
stale FDB entry.

> > The way I dealt with it is by forcing a flush of the FDB entries on
> > the port, in the following scenarios:
> > - link goes down
> > - port leaves its bridge
> > So traffic towards a destination that has migrated away will
> > temporarily be flooded again (towards the CPU port as well).
> > There is still one case which isn't treated using this approach: when
> > the station migrates away from a switch port that is not directly
> > connected to this one. So no "link down" events would get generated in
> > that case. We would still have to wait until the address expires in
> > that case. I don't think that particular situation can be solved.
>
> You're right. Every switch has this issue, even Linux bridge.
>
> > My point is: if we agree that this is a larger problem, then DSA
> > should have a .port_fdb_flush method and schedule a workqueue whenever
> > necessary. Yes, it is a costly operation, but it will still probably
> > take a lot less than the 300 seconds that the bridge configures for
> > address ageing.
> >
> > Thoughts?
> >

> >
> > Thanks,
> > -Vladimir

Regards,
-Vladimir

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[Vladimir Oltean]

Hi Qingfang,

On Mon, 4 May 2020 at 15:47, DENG Qingfang <dqfext@gmail.com> wrote:
>
> Hi Vladimir,
>
> On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > Hi Qingfang,
> >
> > On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqfext@gmail.com> wrote:
> > >
> > > When a client moves from a DSA user port to a software port in a bridge,
> > > it cannot reach any other clients that connected to the DSA user ports.
> > > That is because SA learning on the CPU port is disabled, so the switch
> > > ignores the client's frames from the CPU port and still thinks it is at
> > > the user port.
> > >
> > > Fix it by enabling SA learning on the CPU port.
> > >
> > > To prevent the switch from learning from flooding frames from the CPU
> > > port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
> > > and let the switch flood them instead of trapping to the CPU port.
> > > Multicast frames still need to be trapped to the CPU port for snooping,
> > > so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
> > > to disable SA learning.
> > >
> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > > ---
> >
> > I think enabling learning on the CPU port would fix the problem
> > sometimes, but not always. (actually nothing can solve it always, see
> > below)
> > The switch learns the new route only if it receives any packets from
> > the CPU port, with a SA equal to the station you're trying to reach.
> > But what if the station is not sending any traffic at the moment,
> > because it is simply waiting for connections to it first (just an
> > example)?
> > Unless there is any traffic already coming from the destination
> > station too, your patch won't work.
> > I am currently facing a similar situation with the ocelot/felix
> > switches, but in that case, enabling SA learning on the CPU port is
> > not possible.
>
> Why is it not possible?
>

Because learning on the CPU port is not supported on this hardware.

> Then try my previous RFC patch
> "net: bridge: fix client roaming from DSA user port"
> It tries removing entries from the switch when the client moves to another port.
>

Your patch only deletes FDB entries of packets received in the
fastpath by the software bridge, which as I said, won't work if the
software bridge doesn't receive packets in the first place due to a
stale FDB entry.

> > The way I dealt with it is by forcing a flush of the FDB entries on
> > the port, in the following scenarios:
> > - link goes down
> > - port leaves its bridge
> > So traffic towards a destination that has migrated away will
> > temporarily be flooded again (towards the CPU port as well).
> > There is still one case which isn't treated using this approach: when
> > the station migrates away from a switch port that is not directly
> > connected to this one. So no "link down" events would get generated in
> > that case. We would still have to wait until the address expires in
> > that case. I don't think that particular situation can be solved.
>
> You're right. Every switch has this issue, even Linux bridge.
>
> > My point is: if we agree that this is a larger problem, then DSA
> > should have a .port_fdb_flush method and schedule a workqueue whenever
> > necessary. Yes, it is a costly operation, but it will still probably
> > take a lot less than the 300 seconds that the bridge configures for
> > address ageing.
> >
> > Thoughts?
> >

> >
> > Thanks,
> > -Vladimir

Regards,
-Vladimir

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[DENG Qingfang]

Hi Vladimir,

On Mon, May 4, 2020 at 9:15 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Qingfang,
>
> On Mon, 4 May 2020 at 15:47, DENG Qingfang <dqfext@gmail.com> wrote:
> >
> > Hi Vladimir,
> >
> > On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > Hi Qingfang,
> > >
> > > On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqfext@gmail.com> wrote:
> > > >
> > > > When a client moves from a DSA user port to a software port in a bridge,
> > > > it cannot reach any other clients that connected to the DSA user ports.
> > > > That is because SA learning on the CPU port is disabled, so the switch
> > > > ignores the client's frames from the CPU port and still thinks it is at
> > > > the user port.
> > > >
> > > > Fix it by enabling SA learning on the CPU port.
> > > >
> > > > To prevent the switch from learning from flooding frames from the CPU
> > > > port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
> > > > and let the switch flood them instead of trapping to the CPU port.
> > > > Multicast frames still need to be trapped to the CPU port for snooping,
> > > > so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
> > > > to disable SA learning.
> > > >
> > > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > > > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > > > ---
> > >
> > > I think enabling learning on the CPU port would fix the problem
> > > sometimes, but not always. (actually nothing can solve it always, see
> > > below)
> > > The switch learns the new route only if it receives any packets from
> > > the CPU port, with a SA equal to the station you're trying to reach.
> > > But what if the station is not sending any traffic at the moment,
> > > because it is simply waiting for connections to it first (just an
> > > example)?
> > > Unless there is any traffic already coming from the destination
> > > station too, your patch won't work.
> > > I am currently facing a similar situation with the ocelot/felix
> > > switches, but in that case, enabling SA learning on the CPU port is
> > > not possible.
> >
> > Why is it not possible?
> >
>
> Because learning on the CPU port is not supported on this hardware.
>
> > Then try my previous RFC patch
> > "net: bridge: fix client roaming from DSA user port"
> > It tries removing entries from the switch when the client moves to another port.
> >
>
> Your patch only deletes FDB entries of packets received in the
> fastpath by the software bridge, which as I said, won't work if the
> software bridge doesn't receive packets in the first place due to a
> stale FDB entry.

As I said before, ALL switches including software linux bridge have this issue.
In this case, you'd better ensure the client sends packets first after
migration, which
most clients already do in switches + wireless APs setup.

>
> > > The way I dealt with it is by forcing a flush of the FDB entries on
> > > the port, in the following scenarios:
> > > - link goes down
> > > - port leaves its bridge
> > > So traffic towards a destination that has migrated away will
> > > temporarily be flooded again (towards the CPU port as well).
> > > There is still one case which isn't treated using this approach: when
> > > the station migrates away from a switch port that is not directly
> > > connected to this one. So no "link down" events would get generated in
> > > that case. We would still have to wait until the address expires in
> > > that case. I don't think that particular situation can be solved.
> >
> > You're right. Every switch has this issue, even Linux bridge.
> >
> > > My point is: if we agree that this is a larger problem, then DSA
> > > should have a .port_fdb_flush method and schedule a workqueue whenever
> > > necessary. Yes, it is a costly operation, but it will still probably
> > > take a lot less than the 300 seconds that the bridge configures for
> > > address ageing.
> > >
> > > Thoughts?
> > >
>
> > >
> > > Thanks,
> > > -Vladimir
>
> Regards,
> -Vladimir

Regards,
Qingfang

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[DENG Qingfang]

Hi Vladimir,

On Mon, May 4, 2020 at 9:15 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> Hi Qingfang,
>
> On Mon, 4 May 2020 at 15:47, DENG Qingfang <dqfext@gmail.com> wrote:
> >
> > Hi Vladimir,
> >
> > On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > Hi Qingfang,
> > >
> > > On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqfext@gmail.com> wrote:
> > > >
> > > > When a client moves from a DSA user port to a software port in a bridge,
> > > > it cannot reach any other clients that connected to the DSA user ports.
> > > > That is because SA learning on the CPU port is disabled, so the switch
> > > > ignores the client's frames from the CPU port and still thinks it is at
> > > > the user port.
> > > >
> > > > Fix it by enabling SA learning on the CPU port.
> > > >
> > > > To prevent the switch from learning from flooding frames from the CPU
> > > > port, set skb->offload_fwd_mark to 1 for unicast and broadcast frames,
> > > > and let the switch flood them instead of trapping to the CPU port.
> > > > Multicast frames still need to be trapped to the CPU port for snooping,
> > > > so set the SA_DIS bit of the MTK tag to 1 when transmitting those frames
> > > > to disable SA learning.
> > > >
> > > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > > > Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> > > > ---
> > >
> > > I think enabling learning on the CPU port would fix the problem
> > > sometimes, but not always. (actually nothing can solve it always, see
> > > below)
> > > The switch learns the new route only if it receives any packets from
> > > the CPU port, with a SA equal to the station you're trying to reach.
> > > But what if the station is not sending any traffic at the moment,
> > > because it is simply waiting for connections to it first (just an
> > > example)?
> > > Unless there is any traffic already coming from the destination
> > > station too, your patch won't work.
> > > I am currently facing a similar situation with the ocelot/felix
> > > switches, but in that case, enabling SA learning on the CPU port is
> > > not possible.
> >
> > Why is it not possible?
> >
>
> Because learning on the CPU port is not supported on this hardware.
>
> > Then try my previous RFC patch
> > "net: bridge: fix client roaming from DSA user port"
> > It tries removing entries from the switch when the client moves to another port.
> >
>
> Your patch only deletes FDB entries of packets received in the
> fastpath by the software bridge, which as I said, won't work if the
> software bridge doesn't receive packets in the first place due to a
> stale FDB entry.

As I said before, ALL switches including software linux bridge have this issue.
In this case, you'd better ensure the client sends packets first after
migration, which
most clients already do in switches + wireless APs setup.

>
> > > The way I dealt with it is by forcing a flush of the FDB entries on
> > > the port, in the following scenarios:
> > > - link goes down
> > > - port leaves its bridge
> > > So traffic towards a destination that has migrated away will
> > > temporarily be flooded again (towards the CPU port as well).
> > > There is still one case which isn't treated using this approach: when
> > > the station migrates away from a switch port that is not directly
> > > connected to this one. So no "link down" events would get generated in
> > > that case. We would still have to wait until the address expires in
> > > that case. I don't think that particular situation can be solved.
> >
> > You're right. Every switch has this issue, even Linux bridge.
> >
> > > My point is: if we agree that this is a larger problem, then DSA
> > > should have a .port_fdb_flush method and schedule a workqueue whenever
> > > necessary. Yes, it is a costly operation, but it will still probably
> > > take a lot less than the 300 seconds that the bridge configures for
> > > address ageing.
> > >
> > > Thoughts?
> > >
>
> > >
> > > Thanks,
> > > -Vladimir
>
> Regards,
> -Vladimir

Regards,
Qingfang

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[Chuanhong Guo]

Hi!

On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> I think enabling learning on the CPU port would fix the problem
> sometimes, but not always. (actually nothing can solve it always, see
> below)
> The switch learns the new route only if it receives any packets from
> the CPU port, with a SA equal to the station you're trying to reach.
> But what if the station is not sending any traffic at the moment,
> because it is simply waiting for connections to it first (just an
> example)?
> Unless there is any traffic already coming from the destination
> station too, your patch won't work.

This is just the limitation of connecting two bridges together.

> I am currently facing a similar situation with the ocelot/felix
> switches, but in that case, enabling SA learning on the CPU port is
> not possible.
> The way I dealt with it is by forcing a flush of the FDB entries on
> the port, in the following scenarios:
> - link goes down
> - port leaves its bridge
> So traffic towards a destination that has migrated away will
> temporarily be flooded again (towards the CPU port as well).

In previous discussion in thread:
"net: bridge: fix client roaming from DSA user port"
It's currently established that linux treats a DSA switch with
forwarding offload capability as its own bridge.

If the switch can't learn from cpu port, you either need
to propose a change of this already established behaviour
so that software bridge can sync its fdb with hardware
(making sw bridge and hardware switch behave as
one bridge instead of two) or write extra code to help
managing hardware fdb. (so that the switch matches
current behaviour.)

> There is still one case which isn't treated using this approach: when
> the station migrates away from a switch port that is not directly
> connected to this one. So no "link down" events would get generated in
> that case. We would still have to wait until the address expires in
> that case. I don't think that particular situation can be solved.
> My point is: if we agree that this is a larger problem, then DSA
> should have a .port_fdb_flush method and schedule a workqueue whenever
> necessary. Yes, it is a costly operation, but it will still probably
> take a lot less than the 300 seconds that the bridge configures for
> address ageing.

I think flushing fdb on port topology changes doesn't solve the
problem targeted by this patch.
Anyway, this mt7530 patch is proposed because current
mt7530 driver failed to match the established behaviour for
DSA/switchdev. I think it's better to start a new thread if
you'd like to propose these fundamental behaviour changes,
because this patch is already a result of previously proposed
behaviour changes being rejected.

-- 
Regards,
Chuanhong Guo

Re: [RFC PATCH net-next] net: dsa: mt7530: fix roaming from DSA user ports

[Chuanhong Guo]

Hi!

On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> I think enabling learning on the CPU port would fix the problem
> sometimes, but not always. (actually nothing can solve it always, see
> below)
> The switch learns the new route only if it receives any packets from
> the CPU port, with a SA equal to the station you're trying to reach.
> But what if the station is not sending any traffic at the moment,
> because it is simply waiting for connections to it first (just an
> example)?
> Unless there is any traffic already coming from the destination
> station too, your patch won't work.

This is just the limitation of connecting two bridges together.

> I am currently facing a similar situation with the ocelot/felix
> switches, but in that case, enabling SA learning on the CPU port is
> not possible.
> The way I dealt with it is by forcing a flush of the FDB entries on
> the port, in the following scenarios:
> - link goes down
> - port leaves its bridge
> So traffic towards a destination that has migrated away will
> temporarily be flooded again (towards the CPU port as well).

In previous discussion in thread:
"net: bridge: fix client roaming from DSA user port"
It's currently established that linux treats a DSA switch with
forwarding offload capability as its own bridge.

If the switch can't learn from cpu port, you either need
to propose a change of this already established behaviour
so that software bridge can sync its fdb with hardware
(making sw bridge and hardware switch behave as
one bridge instead of two) or write extra code to help
managing hardware fdb. (so that the switch matches
current behaviour.)

> There is still one case which isn't treated using this approach: when
> the station migrates away from a switch port that is not directly
> connected to this one. So no "link down" events would get generated in
> that case. We would still have to wait until the address expires in
> that case. I don't think that particular situation can be solved.
> My point is: if we agree that this is a larger problem, then DSA
> should have a .port_fdb_flush method and schedule a workqueue whenever
> necessary. Yes, it is a costly operation, but it will still probably
> take a lot less than the 300 seconds that the bridge configures for
> address ageing.

I think flushing fdb on port topology changes doesn't solve the
problem targeted by this patch.
Anyway, this mt7530 patch is proposed because current
mt7530 driver failed to match the established behaviour for
DSA/switchdev. I think it's better to start a new thread if
you'd like to propose these fundamental behaviour changes,
because this patch is already a result of previously proposed
behaviour changes being rejected.

-- 
Regards,
Chuanhong Guo

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek