netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: Tobias Waldekranz <tobias@waldekranz.com>
Cc: DENG Qingfang <dqfext@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Roopa Prabhu <roopa@nvidia.com>,
	Nikolay Aleksandrov <nikolay@nvidia.com>,
	netdev@vger.kernel.org
Subject: Re: [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port
Date: Wed, 3 Feb 2021 12:14:36 +0200	[thread overview]
Message-ID: <20210203101436.xpukhaiseak6wvbe@skbuf> (raw)
In-Reply-To: <8735yd5wnt.fsf@waldekranz.com>

On Wed, Feb 03, 2021 at 10:27:02AM +0100, Tobias Waldekranz wrote:
> On Mon, Feb 01, 2021 at 14:24, DENG Qingfang <dqfext@gmail.com> wrote:
> > I've tested your patch series on kernel 5.4 and found that it only works
> > when VLAN filtering is enabled.
> > After some debugging, I noticed DSA will add static entries to ATU 0 if
> > VLAN filtering is disabled, regardless of default_pvid of the bridge,
> > which is also the ATU# used by the bridge.
> By default, a bridge will use a default PVID of 1, even when VLAN
> filtering is disabled (nbp_vlan_init). Yet it will assign all packets to
> VLAN 0 on ingress (br_handle_frame_finish->br_allowed_ingress).
>
> The switch OTOH, will use the PVID of the port for all packets when
> 802.1Q is disabled, thus assigning all packets to VLAN 1 when VLAN
> filtering is disabled.
>
> Andrew, Vladimir: Should mv88e6xxx always set the PVID to 0 when VLAN
> filtering is disabled?

For Ocelot/Felix, after trying to fight with some other fallout caused
by a mismatch between hardware pvid and bridge pvid when vlan_filtering=0:
https://patchwork.ozlabs.org/project/netdev/patch/20201015173355.564934-1-vladimir.oltean@nxp.com/
I did this and lived happily ever after:

  commit 75e5a554c87fd48f67d1674cfd34e47e3b454fb3
  Author: Vladimir Oltean <vladimir.oltean@nxp.com>
  Date:   Sat Oct 31 12:29:10 2020 +0200

      net: mscc: ocelot: use the pvid of zero when bridged with vlan_filtering=0

      Currently, mscc_ocelot ports configure pvid=0 in standalone mode, and
      inherit the pvid from the bridge when one is present.

      When the bridge has vlan_filtering=0, the software semantics are that
      packets should be received regardless of whether there's a pvid
      configured on the ingress port or not. However, ocelot does not observe
      those semantics today.

      Moreover, changing the PVID is also a problem with vlan_filtering=0.
      We are privately remapping the VID of FDB, MDB entries to the port's
      PVID when those are VLAN-unaware (i.e. when the VID of these entries
      comes to us as 0). But we have no logic of adjusting that remapping when
      the user changes the pvid and vlan_filtering is 0. So stale entries
      would be left behind, and untagged traffic will stop matching on them.

      And even if we were to solve that, there's an even bigger problem. If
      swp0 has pvid 1, and swp1 has pvid 2, and both are under a vlan_filtering=0
      bridge, they should be able to forward traffic between one another.
      However, with ocelot they wouldn't do that.

      The simplest way of fixing this is to never configure the pvid based on
      what the bridge is asking for, when vlan_filtering is 0. Only if there
      was a VLAN that the bridge couldn't mangle, that we could use as pvid....
      So, turns out, there's 0 just for that. And for a reason: IEEE
      802.1Q-2018, page 247, Table 9-2-Reserved VID values says:

              The null VID. Indicates that the tag header contains only
              priority information; no VID is present in the frame.
              This VID value shall not be configured as a PVID or a member
              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              of a VID Set, or configured in any FDB entry, or used in any
              Management operation.

      So, aren't we doing exactly what 802.1Q says not to? Well, in a way, but
      what we're doing here is just driver-level bookkeeping, all for the
      better. The fact that we're using a pvid of 0 is not observable behavior
      from the outside world: the network stack does not see the classified
      VLAN that the switch uses, in vlan_filtering=0 mode. And we're also more
      consistent with the standalone mode now.

      And now that we use the pvid of 0 in this mode, there's another advantage:
      we don't need to perform any VID remapping for FDB and MDB entries either,
      we can just use the VID of 0 that the bridge is passing to us.

      The only gotcha is that every time we change the vlan_filtering setting,
      we need to reapply the pvid (either to 0, or to the value from the bridge).
      A small side-effect visible in the patch is that ocelot_port_set_pvid
      needs to be moved above ocelot_port_vlan_filtering, so that it can be
      called from there without forward-declarations.

      Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
      Signed-off-by: Jakub Kicinski <kuba@kernel.org>

  reply	other threads:[~2021-02-03 10:15 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-16  1:25 [RFC net-next 0/7] net: dsa: Sync local bridge FDB addresses to hardware Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 1/7] net: bridge: switchdev: Refactor br_switchdev_fdb_notify Tobias Waldekranz
2021-01-17 17:24   ` Vladimir Oltean
2021-01-16  1:25 ` [RFC net-next 2/7] net: bridge: switchdev: Include local flag in FDB notifications Tobias Waldekranz
2021-01-17 19:30   ` Vladimir Oltean
2021-01-18 18:58     ` Tobias Waldekranz
2021-01-18 19:27       ` Vladimir Oltean
2021-01-18 20:19         ` Tobias Waldekranz
2021-01-18 21:03           ` Vladimir Oltean
2021-01-18 21:17           ` Nikolay Aleksandrov
2021-01-18 21:22             ` Nikolay Aleksandrov
2021-01-18 21:39               ` Nikolay Aleksandrov
2021-01-18 21:50                 ` Vladimir Oltean
2021-01-18 21:53                   ` Nikolay Aleksandrov
2021-01-18 22:06                     ` Vladimir Oltean
2021-01-18 22:09                       ` Vladimir Oltean
2021-01-18 22:42                       ` Nikolay Aleksandrov
2021-01-19  0:42                         ` Vladimir Oltean
2021-01-19 10:14                           ` Nikolay Aleksandrov
2021-01-18 19:28     ` Ido Schimmel
2021-01-16  1:25 ` [RFC net-next 3/7] net: bridge: switchdev: Send FDB notifications for host addresses Tobias Waldekranz
2021-01-18 11:28   ` Vladimir Oltean
2021-01-16  1:25 ` [RFC net-next 4/7] net: dsa: Include local addresses in assisted CPU port learning Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 5/7] net: dsa: Include bridge " Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 6/7] net: dsa: Sync static FDB entries on foreign interfaces to hardware Tobias Waldekranz
2021-01-16  1:25 ` [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port Tobias Waldekranz
2021-02-01  6:24 ` DENG Qingfang
2021-02-03  9:27   ` Tobias Waldekranz
2021-02-03 10:14     ` Vladimir Oltean [this message]
2021-02-03 10:42       ` Tobias Waldekranz

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=20210203101436.xpukhaiseak6wvbe@skbuf \
    --to=olteanv@gmail.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=dqfext@gmail.com \
    --cc=f.fainelli@gmail.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=nikolay@nvidia.com \
    --cc=roopa@nvidia.com \
    --cc=tobias@waldekranz.com \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [RFC net-next 7/7] net: dsa: mv88e6xxx: Request assisted learning on CPU port' \
    /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).