Date: Tue, 25 Jul 2023 20:03:02 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Daniel Mack <daniel.mack@...oplot.com>
Cc: netdev@...r.kernel.org, andrew@...n.ch, f.fainelli@...il.com
Subject: Re: [PATCH] net: dsa: mv88e6xxx: use distinct FIDs for each bridge

Hi Daniel,

On Mon, Jul 24, 2023 at 12:10:59PM +0200, Daniel Mack wrote:
> Allow setups with overlapping VLAN IDs in different bridges by settting
> the FID of all bridge ports to the index of the bridge.
> 
> Read the FID back when detecting overlaps.
> 
> Signed-off-by: Daniel Mack <daniel.mack@...oplot.com>
> ---

You will have to give more details about what you're trying to do and
how, because my non-expert understanding of Marvell hardware is that
it's simply not possible to allow overlapping VLAN IDs in different
bridges without mapping them to the same FID, aka having them share the
same address database - which is not what we want.

In fact we should be going in the opposite direction, and eventually
convert mv88e6xxx to report ds->fdb_isolation = true - the fact that all
VLAN-unaware bridges share MV88E6XXX_VID_BRIDGED and MV88E6XXX_FID_BRIDGED
prevents us from doing that right now. Your patch is not exactly a correct
step in that direction.

My understanding is that there are some newer Marvell switches with an
8K entry VTU where VLANs might map to one FID or another depending on
port, but your patch does not handle that in the way that I would
expect.

Currently, if you want to use multiple bridges, you need to do one of
the following:
- create them with "vlan_default_pvid 0" and use them in "vlan_filtering 0"
  mode
- disable CONFIG_BRIDGE_VLAN_FILTERING (has the same effect as the above)
- if you want to use them in "vlan_filtering 1" mode, then each bridge
  needs to be created with a different vlan_default_pvid value (this value
  is irrelevant for the bridge operating in "vlan_filtering 0" mode).

>  drivers/net/dsa/mv88e6xxx/chip.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index c7d51a539451..dff271981c69 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -2028,6 +2028,7 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	struct mv88e6xxx_vtu_entry vlan;
>  	int err;
> +	u16 fid;
>  
>  	/* DSA and CPU ports have to be members of multiple vlans */
>  	if (dsa_port_is_dsa(dp) || dsa_port_is_cpu(dp))
> @@ -2037,11 +2038,16 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
>  	if (err)
>  		return err;
>  
> +	err = mv88e6xxx_port_get_fid(chip, port, &fid);
> +	if (err)
> +		return err;
> +
>  	if (!vlan.valid)
>  		return 0;
>  
>  	dsa_switch_for_each_user_port(other_dp, ds) {
>  		struct net_device *other_br;
> +		u16 other_fid;
>  
>  		if (vlan.member[other_dp->index] ==
>  		    MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_NON_MEMBER)
> @@ -2054,6 +2060,10 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
>  		if (!other_br)
>  			continue;
>  
> +		err = mv88e6xxx_port_get_fid(chip, other_dp->index, &other_fid);
> +		if (err == 0 && fid != other_fid)
> +			continue;
> +
>  		dev_err(ds->dev, "p%d: hw VLAN %d already used by port %d in %s\n",
>  			port, vlan.vid, other_dp->index, netdev_name(other_br));
>  		return -EOPNOTSUPP;
> @@ -2948,6 +2958,11 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port,
>  		*tx_fwd_offload = true;
>  	}
>  
> +	/* Set the port's FID to the bridge index so bridges can have
> +	 * overlapping VLANs.
> +	 */
> +	err = mv88e6xxx_port_set_fid(chip, port, bridge.num);

To be clear, mv88e6xxx_port_set_fid() changes the port-default FID of
the packets, which is used to classify frames when the port 8021Q mode
is disabled and the port-default FID is not in the VTU. Currently, I
believe that is never the case in this driver, so the port-default FID
is never used, and the other call to mv88e6xxx_port_set_fid() could just
as well be removed.

IIRC, in standalone mode we install MV88E6XXX_VID_STANDALONE in the VTU
and set the port 8021Q mode to disabled, which means that all packets
will get classified to MV88E6XXX_FID_STANDALONE through the VTU mapping
and not through the port-default FID whose value you're changing now.

For VLAN-unaware bridge ports it's a similar story as for standalone
(802.1Q mode disabled), except that the port-default VID, as set up by
mv88e6xxx_port_commit_pvid(), is MV88E6XXX_VID_BRIDGED. This is also
present in the VTU, and is mapped to MV88E6XXX_FID_BRIDGED.

Each bridge VLAN (used only in VLAN-aware mode) has its own unique FID
as allocated by mv88e6xxx_atu_new(), which is always greater than 2
because of the first 2 FIDs being reserved by the driver. The VTU is not
namespaced per bridge. If br0 creates an ATU for VID 1 and gets a FID
for it, br1 either has to use the same FID for VID 1, or not use VID 1
at all. The driver writers decided that the 2nd option would be better.

I think you are trying to bypass that restriction in a way that doesn't
make much sense. Correct me if I'm wrong, but if I follow the intention
of your patch, I think that what you're thinking is that the port's
Default FID does something else (somehow namespaces all VLANs)?

To see where your proposal falls short, try to have communication
between 2 stations in different bridges br0 and br1, towards MAC address
00:01:02:03:04:05. There is a single ATU (single FID), and if you have
learning enabled, the ATU entry will bounce back and forth between a
port in br0, and a port in br1. Forwarding is still restricted, so when
another station tries to ping the 00:01:02:03:04:05 address and that
happens to be learned on a br1 port, you'll just see packet loss because
the switch won't permit communication between br0 and br1.

> + unlock: mv88e6xxx_reg_unlock(chip);
>  
> -- 2.41.0
> 
> 

Sorry, but:

pw-bot: cr

Powered by blists - more mailing lists