Hi Vladimir,
On Mon, May 4, 2020 at 6:23 PM Vladimir Oltean <olte...@gmail.com> wrote:
>
> Hi Qingfang,
>
> On Sat, 25 Apr 2020 at 15:03, DENG Qingfang <dqf...@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 <dqf...@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
