Netdev Archive on
help / color / mirror / Atom feed
From: Vladimir Oltean <>
To: Kurt Kanzenbach <>
Cc: Andrew Lunn <>,
	Vivien Didelot <>,
	Florian Fainelli <>,
	"David S. Miller" <>,
	Jakub Kicinski <>,, Rob Herring <>,,
	Sebastian Andrzej Siewior <>,
	Richard Cochran <>,
	Kamil Alkhouri <>,
Subject: Re: [PATCH v5 2/7] net: dsa: Add DSA driver for Hirschmann Hellcreek switches
Date: Mon, 7 Sep 2020 16:21:09 +0300	[thread overview]
Message-ID: <20200907132109.234ha7xst37dtqcj@skbuf> (raw)
In-Reply-To: <87eendah1c.fsf@kurt>

On Mon, Sep 07, 2020 at 02:49:03PM +0200, Kurt Kanzenbach wrote:
> On Mon Sep 07 2020, Vladimir Oltean wrote:
> > On Mon, Sep 07, 2020 at 08:05:25AM +0200, Kurt Kanzenbach wrote:
> >> Well, that depends on whether hellcreek_vlan_add() is called for
> >> creating that vlan interfaces. In general: As soon as both ports are
> >> members of the same vlan that traffic is switched.
> >
> > That's indeed what I would expect.
> > Not only that, but with your pvid-based setup, you only ensure port
> > separation for untagged traffic anyway.
> Why? Tagged traffic is dropped unless the vlan is configured somehow. By
> default, I've configured vlan 2 and 3 to reflect the port separation for
> DSA. At reset the ports aren't members of any vlan.

Wait, so what is the out-of-reset state of "ptcfg & HR_PTCFG_INGRESSFLT"?
If it is filtering by default (and even if it isn't, but you can make
it), then I suppose you can keep it like that, and try to model your
ports something like this:

- force "ethtool -k swpN | grep rx-vlan-filter" to return "on (fixed)".
- enforce a check that in standalone mode, you can't have an 8021q upper
  interface with the same VLAN ID on more than 1 port at the same time.
  This will be the only way in which you can terminate VLAN traffic on
  standalone ports.

If you do this, I think you should be compliant with the stack.

> We could also skip the initial VLAN configuration completely. At the end
> of the day it's a TSN switch and the user will setup the vlan
> configuration anyway.

Hmm, a driver is supposed to be use case agnostic.
I know people who are using a 5-port TSN switch (not this one) as a
1-port SGMII-to-RMII electrical adapter, with some hardware-accelerated
I think it would be good if you could maintain a sane mode of operation
in standalone mode, it tends to come in as useful at times.

> > I don't think you even need to call hellcreek_vlan_add() for VID 100
> > to be switched between ports, because your .port_vlan_filtering
> > callback does not in fact disable VLAN awareness, it just configures
> > the ports to not drop unknown VLANs. So, arguably, VLAN classification
> > is still performed. An untagged packet is classified to the PVID, a
> > tagged packet is classified to the VID in the packet. So tagged
> > packets bypass the separation.
> >
> > So, I think that's not ok. I think the only proper way to solve this is
> > to inform the IP designers that VLANs are no substitute for a port
> > forwarding matrix (a lookup table that answers the question "can port i
> > forward to port j"). Switch ports that are individually addressable by
> > the network stack are a fundamental assumption of the switchdev
> > framework.
> As I said before, there is no port forwarding matrix. There are only
> vlans and the fdb. There's also a global flag for setting vlan unaware
> mode and a port option for vlan tag required. That's it. I guess, we
> have to deal with it somehow.

Yes, understood. But it's quite a strange omission.

> >
> >> > I remember asking in Message-ID: <20200716082935.snokd33kn52ixk5h@skbuf>
> >> > whether it would be possible for you to set
> >> > ds->configure_vlan_while_not_filtering = true during hellcreek_setup.
> >> > Did anything unexpected happen while trying that?
> >>
> >> No, that comment got lost.
> >>
> >> So looking at the flag: Does it mean the driver can receive vlan
> >> configurations when a bridge without vlan filtering is used? That might
> >> be problematic as this driver uses vlans for the port separation by
> >> default. This is undone when vlan filtering is set to 1 meaning vlan
> >> configurations can be received without any problems.
> >
> > Yes.
> > Generally speaking, the old DSA behavior is something that we're trying
> > to get rid of, once all drivers set the option to true. So a new driver
> > should not rely on it even if it needs something like that.
> OK. when a new driver should set the flag, then I'll set it. So, all
> vlan requests programming requests should be "buffered" and executed
> when vlan filtering is enabled? What is it good for?

It is good for correct functionality of the hardware, I don't get the
question? If your driver makes private use of VLAN tags beyond what the
upper layers ask for, then it should keep track of them. DSA has, in the
past, ignored VLAN switchdev operations from the bridge when not in
vlan_filtering mode, for unknown reasons. This is known to break some
command sequences (see below), so the consensus at the time was to stop
doing that, and introduce this temporary compatibility flag.

Some tests to make sure you're passing are:

1. Statically creating an 802.1Q bridge:

ip link add br0 type bridge vlan_filtering 1
ip link set swp1 master br0
ip link set swp2 master br0

2. Dynamically turning an 802.1D bridge into an 802.1Q bridge:

ip link add br0 type bridge
ip link set swp1 master br0
ip link set swp2 master br0
ip link set br0 type bridge vlan_filtering 1
# at this moment in time, if you don't have
# configure_vlan_while_not_filtering = true, then the VLAN tables of
# swp1 and swp2 will be missing the default_pvid (by default 1) of br0.


  reply	other threads:[~2020-09-07 17:25 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-04  6:27 [PATCH v5 0/7] Hirschmann Hellcreek DSA driver Kurt Kanzenbach
2020-09-04  6:27 ` [PATCH v5 1/7] net: dsa: Add tag handling for Hirschmann Hellcreek switches Kurt Kanzenbach
2020-09-04  6:27 ` [PATCH v5 2/7] net: dsa: Add DSA driver " Kurt Kanzenbach
2020-09-05 20:42   ` Vladimir Oltean
2020-09-07  6:05     ` Kurt Kanzenbach
2020-09-07 10:48       ` Vladimir Oltean
2020-09-07 12:49         ` Kurt Kanzenbach
2020-09-07 13:21           ` Vladimir Oltean [this message]
2020-09-07 14:01             ` Kurt Kanzenbach
2020-09-07 15:17         ` Richard Cochran
2020-09-04  6:27 ` [PATCH v5 3/7] net: dsa: hellcreek: Add PTP clock support Kurt Kanzenbach
2020-09-04  6:27 ` [PATCH v5 4/7] net: dsa: hellcreek: Add support for hardware timestamping Kurt Kanzenbach
2020-09-04  6:27 ` [PATCH v5 5/7] net: dsa: hellcreek: Add PTP status LEDs Kurt Kanzenbach
2020-09-04  6:27 ` [PATCH v5 6/7] dt-bindings: Add vendor prefix for Hirschmann Kurt Kanzenbach
2020-09-04  6:27 ` [PATCH v5 7/7] dt-bindings: net: dsa: Add documentation for Hellcreek switches Kurt Kanzenbach

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:

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

  git send-email \
    --in-reply-to=20200907132109.234ha7xst37dtqcj@skbuf \ \ \ \ \ \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH v5 2/7] net: dsa: Add DSA driver for Hirschmann Hellcreek switches' \

* 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).