Date:   Tue, 16 Nov 2021 01:45:46 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Woojung Huh <woojung.huh@...rochip.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>,
        kernel@...gutronix.de, Jakub Kicinski <kuba@...nel.org>,
        UNGLinuxDriver@...rochip.com,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [RFC PATCH net-next] net: dsa: microchip: implement multi-bridge
 support

On Fri, Nov 12, 2021 at 08:58:23AM +0100, Oleksij Rempel wrote:
> On Wed, Nov 10, 2021 at 02:36:40PM +0200, Vladimir Oltean wrote:
> > On Mon, Nov 08, 2021 at 12:10:34PM +0100, Oleksij Rempel wrote:
> > > Current driver version is able to handle only one bridge at time.
> > > Configuring two bridges on two different ports would end up shorting this
> > > bridges by HW. To reproduce it:
> > > 
> > > 	ip l a name br0 type bridge
> > > 	ip l a name br1 type bridge
> > > 	ip l s dev br0 up
> > > 	ip l s dev br1 up
> > > 	ip l s lan1 master br0
> > > 	ip l s dev lan1 up
> > > 	ip l s lan2 master br1
> > > 	ip l s dev lan2 up
> > > 
> > > 	Ping on lan1 and get response on lan2, which should not happen.
> > > 
> > > This happened, because current driver version is storing one global "Port VLAN
> > > Membership" and applying it to all ports which are members of any
> > > bridge.
> > > To solve this issue, we need to handle each port separately.
> > > 
> > > This patch is dropping the global port member storage and calculating
> > > membership dynamically depending on STP state and bridge participation.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> > > ---
> > 
> > Because there wasn't any restriction in the driver against multiple
> > bridges, I would be tempted to send to the "net" tree and provide a
> > Fixes: tag.
> 
> This patch looks too intrusive to me. It will be hard to backport it to
> older versions. How about have two patches: add limit to one bridge for
> net and add support for multiple bridges for net-next?

That would work.

> > > +	dp = dsa_to_port(ds, port);
> > > +
> > > +	for (i = 0; i < ds->num_ports; i++) {
> > > +		const struct dsa_port *dpi = dsa_to_port(ds, i);
> > 
> > Other drivers name this "other_dp", I don't think that name is too bad.
> > Also, you can use "dsa_switch_for_each_user_port", which is also more
> > efficient, although you can't if you target 'stable' with this change,
> > since it has been introduced rather recently.
> 
> ok
> 
> > > +		struct ksz_port *pi = &dev->ports[i];
> > 
> > and this could be "other_p" rather than "pi".
> 
> ok
> 
> > > +		u8 val = 0;
> > > +
> > > +		if (!dsa_is_user_port(ds, i))
> > >  			continue;
> > > -		p = &dev->ports[i];
> > > -		if (!(dev->member & (1 << i)))
> > > +		if (port == i)
> > >  			continue;
> > > +		if (!dp->bridge_dev || dp->bridge_dev != dpi->bridge_dev)
> > > +			continue;
> > > +
> > > +		pi = &dev->ports[i];
> > > +		if (pi->stp_state != BR_STATE_DISABLED)
> > > +			val |= BIT(dsa_upstream_port(ds, i));
> > >  
> > 
> > This is saying:
> > For each {user port, other port} pair, if the other port isn't DISABLED,
> > then allow the user port to forward towards the CPU port of the other port.
> > What sense does that make? You don't support multiple CPU ports, so this
> > port's CPU port is that port's CPU port, and you have one more (broken)
> > forwarding rule towards the CPU port below.
> 
> Ok, understand.
> 
> > > -		/* Port is a member of the bridge and is forwarding. */
> > > -		if (p->stp_state == BR_STATE_FORWARDING &&
> > > -		    p->member != dev->member)
> > > -			dev->dev_ops->cfg_port_member(dev, i, dev->member);
> > > +		if (pi->stp_state == BR_STATE_FORWARDING &&
> > > +		    p->stp_state == BR_STATE_FORWARDING) {
> > > +			val |= BIT(port);
> > > +			port_member |= BIT(i);
> > > +		}
> > > +
> > > +		dev->dev_ops->cfg_port_member(dev, i, val);
> > >  	}
> > > +
> > > +	if (p->stp_state != BR_STATE_DISABLED)
> > > +		port_member |= BIT(dsa_upstream_port(ds, port));
> > 
> > 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? You need to have a port state in which data
plane packets are blocked, but BPDUs can pass. 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), 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.

BTW, see the "bridge link set" section in "man bridge" for a list of
what you should do in each STP state.

Powered by blists - more mailing lists