Date:   Tue, 16 Nov 2021 14:47:23 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     g@pengutronix.de, Woojung Huh <woojung.huh@microchip.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, UNGLinuxDriver@microchip.com,
        kernel@pengutronix.de, Jakub Kicinski <kuba@kernel.org>,
        Vivien Didelot <vivien.didelot@gmail.com>
Subject: Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge
 support
Message-ID: <20211116124723.kivonrdbgqdxlryd@skbuf>
References: <20211108111034.2735339-1-o.rempel@pengutronix.de>
 <20211110123640.z5hub3nv37dypa6m@skbuf>
 <20211112075823.GJ12195@pengutronix.de>
 <20211115234546.spi7hz2fsxddn4dz@skbuf>
 <20211116083903.GA16121@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211116083903.GA16121@pengutronix.de>
Precedence: bulk

On Tue, Nov 16, 2021 at 09:39:03AM +0100, Oleksij Rempel wrote:
> On Tue, Nov 16, 2021 at 01:45:46AM +0200, Vladimir Oltean wrote:
> 
> .....
> 
> > > > Why != DISABLED? I expect that dev_ops->cfg_port_member() affects only
> > > > data packet forwarding, not control packet forwarding, right?
> > > 
> > > No. According to the KSZ9477S datasheet:
> > > "The processor should program the “Static MAC Table” with the entries that it
> > > needs to receive (for example, BPDU packets). The “overriding” bit should be set
> > > so that the switch will forward those specific packets to the processor. The
> > > processor may send packets to the port(s) in this state. Address learning is
> > > disabled on the port in this state."
> > > 
> > > This part is not implemented.
> > > 
> > > In current driver implementation (before or after this patch), all
> > > packets are forwarded. It looks like, current STP implementation in this driver
> > > is not complete. If I create a loop, the bridge will permanently toggle one of
> > > ports between blocking and listening. 
> > > 
> > > Currently I do not know how to proceed with it. Remove stp callback and
> > > make proper, straightforward bride_join/leave? Implement common soft STP
> > > for all switches without HW STP support?
> > 
> > What does "soft STP" mean?
> 
> Some HW seems to provide configuration bits for ports STP states. For
> example by enabling it, I can just set listening state and it will only pass
> BPDU packets without need to program static MAC table. (At least, this
> would be my expectation)
> 
> For example like this:
> https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/mt7530.c#L1121
> 
> If this HW really exist and works as expected, how should I name it?

You are simply talking about the kind of registers that a switch exposes.
That doesn't really matter, the end result should be the same.
For example, sja1105 doesn't have named STP states, but it allows the
driver to specify whether data plane packets can be received and sent on
a given port.
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/sja1105/sja1105_main.c#L2030
On other switches it's even more interesting, you need to calibrate STP
states from the port forwarding mask.
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/ethernet/mscc/ocelot.c#L1476
In any case, the switch should be able (somehow) to distinguish
link-local multicast packets and send them to the CPU port regardless of
the source port's STP state, and also be able to inject a link-local
multicast packet into a port regardless of its STP state.
For Micrel/Microchip switches, at least BPDU injection should be
supported, I believe that's what this piece of code does:
https://elixir.bootlin.com/linux/v5.16-rc1/source/net/dsa/tag_ksz.c#L127

> > You need to have a port state in which data  plane packets are blocked,
> > but BPDUs can pass.
> 
> ack.
> 
> > Unless you trap all packets to the CPU and make the selection in software
> > (therefore, including the forwarding, I don't know if that is so desirable),
> 
> Yes, this is my point, on plain linux bridge with two simple USB ethernet
> adapters, I'm able to use STP without any HW offloading.
> 
> If my HW do not provide straightforward way to trap BPDU packets to CPU,
> i should be able to reuse functionality already provided by the linux
> bridge.

At least on net-next you can return -EOPNOTSUPP in ->port_bridge_join
and DSA will leave your port to operate in standalone mode and what will
happen is exactly what you describe. But if lack of STP support is what
you're trying to fix, there might be better ways of fixing it than doing
software bridging.

> Probably I need to signal it some how from dsa driver, to let linux
> bridge make proper decision and reduce logging noise.

What logging noise?

> For example:
> - Have flag like: ds->sta_without_bpdu_trap = true;

:-/ what would this flag do?

> - If no .port_mdb_add/.port_fdb_add callbacks are implemented, handle
>   all incoming packet by the linux bridge without making lots of noise,
>   and use .port_bridge_join/.port_bridge_leave to separate ports.

You can already let forwarding be partially done in software. For
example qca8k sets up the CPU port as the only destination for multicast
and for flooding:
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/qca8k.c#L1157
Notice how its tagging protocol driver does not call
dsa_default_offload_fwd_mark(), in order to let the bridge forward the
packets in software:
https://elixir.bootlin.com/linux/v5.16-rc1/source/net/dsa/tag_qca.c#L51

> - If .port_mdb_add/.port_fdb_add are implemented, program the static MAC table.

You want DSA to program the static MAC table for link-local traffic?
Why? DSA doesn't care how you trap your link-local traffic to the CPU,
it might vary wildly between one switch and another. Also,
->port_mdb_add() is for data plane multicast packets (aka not BPDUs),
and ->port_fdb_add() is for unicast data plane packets (again not
BPDUs). Your driver wouldn't even be the first one that traps link-local
traffic privately.
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/hirschmann/hellcreek.c#L1050
https://elixir.bootlin.com/linux/v5.16-rc1/source/drivers/net/dsa/sja1105/sja1105_main.c#L868
There isn't any good way for user space to have visibility into which
packets a switch will trap. There is devlink-trap which AFAIK allows you
to see but not modify the traps that are built-in to the hardware/driver:
https://www.kernel.org/doc/html/latest/networking/devlink/devlink-trap.html
There are also traps which you can add using the tc-trap action
(amazingly I could not find documentation for this). But I don't think
it would surprise anyone if you would trap BPDUs by default in the
driver - as mentioned, some switches already do this with no way to disable it.

> > you don't have much of a choice except to do what you've said above, program
> > the static MAC table with entries for 01-80-c2-00-00-0x which trap those
> > link-local multicast addresses to the CPU and set the STP state override
> > bit for them and for them only.
> 
> Hm... Microchip documentation do not describes it as STP state override. Only
> as "port state override".

Potato, patato, it should be the same thing.

> And since STP state is not directly configurable on this switch, it
> probably means receive/transmit enable state of the port.  So, packets
> with matching MAC should be forwarded even if port is in the receive
> disabled state. Correct?

In the context we've been discussing so far, "forwarding" has a pretty
specific meaning, which is autonomously redirecting from one front port
to another. For link-local packets, what you want is "trapping", i.e.
send to the CPU and to the CPU only.

> 
> > BTW, see the "bridge link set" section in "man bridge" for a list of
> > what you should do in each STP state.
> 
> ack. Thank you.
> 
> Regards,
> Oleksij
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |