Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: DENG Qingfang <dqfext@gmail.com>
Cc: "Sean Wang" <sean.wang@mediatek.com>,
	"Landen Chao" <Landen.Chao@mediatek.com>,
	"Andrew Lunn" <andrew@lunn.ch>,
	"Vivien Didelot" <vivien.didelot@gmail.com>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
	"Eric Woudstra" <ericwouds@gmail.com>,
	"René van Dorst" <opensource@vdorst.com>,
	"Frank Wunderlich" <frank-w@public-files.de>,
	"Ilya Lipnitskiy" <ilya.lipnitskiy@gmail.com>
Subject: Re: [PATCH net-next v2 2/4] net: dsa: mt7530: use independent VLAN learning on VLAN-unaware bridges
Date: Tue, 3 Aug 2021 19:48:53 +0300	[thread overview]
Message-ID: <20210803164853.gxw4zfxmmgs2kgry@skbuf> (raw)
In-Reply-To: <20210803160405.3025624-3-dqfext@gmail.com>

On Wed, Aug 04, 2021 at 12:04:02AM +0800, DENG Qingfang wrote:
> Consider the following bridge configuration, where bond0 is not
> offloaded:
> 
>          +-- br0 --+
>         / /   |     \
>        / /    |      \
>       /  |    |     bond0
>      /   |    |     /   \
>    swp0 swp1 swp2 swp3 swp4
>      .        .       .
>      .        .       .
>      A        B       C
> 
> Ideally, when the switch receives a packet from swp3 or swp4, it should
> forward the packet to the CPU, according to the port matrix and unknown
> unicast flood settings.
> 
> But packet loss will happen if the destination address is at one of the
> offloaded ports (swp0~2). For example, when client C sends a packet to
> A, the FDB lookup will indicate that it should be forwarded to swp0, but
> the port matrix of swp3 and swp4 is configured to only allow the CPU to
> be its destination, so it is dropped.
> 
> However, this issue does not happen if the bridge is VLAN-aware. That is
> because VLAN-aware bridges use independent VLAN learning, i.e. use VID
> for FDB lookup, on offloaded ports. As swp3 and swp4 are not offloaded,
> shared VLAN learning with default filter ID of 0 is used instead. So the
> lookup for A with filter ID 0 never hits and the packet can be forwarded
> to the CPU.
> 
> In the current code, only two combinations were used to toggle user
> ports' VLAN awareness: one is PCR.PORT_VLAN set to port matrix mode with
> PVC.VLAN_ATTR set to transparent port, the other is PCR.PORT_VLAN set to
> security mode with PVC.VLAN_ATTR set to user port.
> 
> It turns out that only PVC.VLAN_ATTR contributes to VLAN awareness, and
> port matrix mode just skips the VLAN table lookup. The reference manual
> is somehow misleading when describing PORT_VLAN modes. It states that
> PORT_MEM (VLAN port member) is used for destination if the VLAN table
> lookup hits, but actually **PORT_MEM & PORT_MATRIX** (bitwise AND of
> VLAN port member and port matrix) is used instead, which means we can
> have two or more separate VLAN-aware bridges with the same PVID and
> traffic won't leak between them.
> 
> Therefore, to solve this, enable independent VLAN learning with PVID 0
> on VLAN-unaware bridges, by setting their PCR.PORT_VLAN to fallback
> mode, while leaving standalone ports in port matrix mode. The CPU port
> is always set to fallback mode to serve those bridges.
> 
> During testing, it is found that FDB lookup with filter ID of 0 will
> also hit entries with VID 0 even with independent VLAN learning. To
> avoid that, install all VLANs with filter ID of 1.
> 
> Signed-off-by: DENG Qingfang <dqfext@gmail.com>
> ---
> v1 -> v2: use FID enum instead of hardcoding.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

> @@ -1629,11 +1651,12 @@ mt7530_port_vlan_del(struct dsa_switch *ds, int port,
>  	/* PVID is being restored to the default whenever the PVID port
>  	 * is being removed from the VLAN.
>  	 */
> -	if (pvid == vlan->vid)
> -		pvid = G0_PORT_VID_DEF;
> +	if (priv->ports[port].pvid == vlan->vid) {
> +		priv->ports[port].pvid = G0_PORT_VID_DEF;
> +		mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
> +			   G0_PORT_VID_DEF);
> +	}

After this patch set gets merged, can you also please take a look at the
following:

Documentation/networking/switchdev.rst says:

When the bridge has VLAN filtering enabled and a PVID is not configured on the
ingress port, untagged and 802.1p tagged packets must be dropped. When the bridge
has VLAN filtering enabled and a PVID exists on the ingress port, untagged and
priority-tagged packets must be accepted and forwarded according to the
bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering
disabled, the presence/lack of a PVID should not influence the packet
forwarding decision.

I'm not sure if this happens or not with mt7530, since the driver
attempts to change the pvid back to 0. You are not changing this
behavior in this series, so no reason to deal with it as part of it.

>  
> -	mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK, pvid);
> -	priv->ports[port].pvid = pvid;
>  
>  	mutex_unlock(&priv->reg_mutex);
>  

  reply	other threads:[~2021-08-03 16:49 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-03 16:04 [PATCH net-next v2 0/4] mt7530 software fallback bridging fix DENG Qingfang
2021-08-03 16:04 ` [PATCH net-next v2 1/4] net: dsa: mt7530: enable assisted learning on CPU port DENG Qingfang
2021-08-03 16:04 ` [PATCH net-next v2 2/4] net: dsa: mt7530: use independent VLAN learning on VLAN-unaware bridges DENG Qingfang
2021-08-03 16:48   ` Vladimir Oltean [this message]
2021-08-03 17:13     ` DENG Qingfang
2021-08-03 16:04 ` [PATCH net-next v2 3/4] net: dsa: mt7530: set STP state on filter ID 1 DENG Qingfang
2021-08-03 16:50   ` Vladimir Oltean
2021-08-03 16:04 ` [PATCH net-next v2 4/4] net: dsa: mt7530: always install FDB entries with IVL and FID 1 DENG Qingfang
2021-08-03 16:51   ` Vladimir Oltean
2021-08-03 17:53     ` DENG Qingfang
2021-08-03 18:23       ` Vladimir Oltean
2021-08-04  9:40 ` [PATCH net-next v2 0/4] mt7530 software fallback bridging fix patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210803164853.gxw4zfxmmgs2kgry@skbuf \
    --to=olteanv@gmail.com \
    --cc=Landen.Chao@mediatek.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=dqfext@gmail.com \
    --cc=ericwouds@gmail.com \
    --cc=f.fainelli@gmail.com \
    --cc=frank-w@public-files.de \
    --cc=ilya.lipnitskiy@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=opensource@vdorst.com \
    --cc=sean.wang@mediatek.com \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [PATCH net-next v2 2/4] net: dsa: mt7530: use independent VLAN learning on VLAN-unaware bridges' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).