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

[Vladimir Oltean <olteanv@gmail.com>]

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