Re: [PATCH net-next 1/5] net: dsa: mv88e6xxx: Improve isolation of standalone ports

[Vladimir Oltean]

On Tue, Feb 01, 2022 at 08:56:32PM +0100, Tobias Waldekranz wrote:
> >> - sw0p1 and sw1p1 are bridged
> >
> > Do sw0p1 and sw1p1 even matter?
> 
> Strictly speaking, no - it was just to illustrate...
> 
> >> - sw0p2 and sw1p2 are in standalone mode
> >> - Learning must be enabled on sw0p3 in order for hardware forwarding
> >>   to work properly between bridged ports
> 
> ... this point, i.e. a clear example of why learning can't be disabled
> on DSA ports.

Ok, I understand now. It wasn't too clear.

> >> 1. A packet with SA :aa comes in on sw1p2
> >>    1a. Egresses sw1p0
> >>    1b. Ingresses sw0p3, ATU adds an entry for :aa towards port 3
> >>    1c. Egresses sw0p0
> >> 
> >> 2. A packet with DA :aa comes in on sw0p2
> >>    2a. If an ATU lookup is done at this point, the packet will be
> >>        incorrectly forwarded towards sw0p3. With this change in place,
> >>        the ATU is pypassed and the packet is forwarded in accordance
> >
> > s/pypassed/bypassed/
> >
> >>        whith the PVT, which only contains the CPU port.
> >
> > s/whith/with/
> >
> > What you describe is a bit convoluted, so let me try to rephrase it.
> > The mv88e6xxx driver configures all standalone ports to use the same
> > DefaultVID(0)/FID(0), and configures standalone user ports with no
> > learning via the Port Association Vector. Shared (cascade + CPU) ports
> > have learning enabled so that cross-chip bridging works without floods.
> > But since learning is per port and not per FID, it means that we enable
> > learning in FID 0, the one where the ATU was supposed to be always empty.
> > So we may end up taking wrong forwarding decisions for standalone ports,
> > notably when we should do software forwarding between ports of different
> > switches. By clearing MapDA, we force standalone ports to bypass any ATU
> > entries that might exist.
> 
> Are you saying you want me to replace the initial paragraph with your
> version, or are you saying the the example is convoluted and should be
> replaced by this text? Or is it only for the benefit of other readers?

Just for the sake of discussion, I wanted to make sure I understand what
you describe.

> > Question: can we disable learning per FID? I searched for this in the
> > limited documentation that I have, but I didn't see such option.
> > Doing this would be advantageous because we'd end up with a bit more
> > space in the ATU. With your solution we're just doing damage control.
> 
> As you discovered, and as I tried to lay out in the cover, this is only
> one part of the whole solution.

I'm not copied to the cover letter :) and I have some issues with my
email client / vger, where emails that I receive through the mailing list
sometimes take days to reach my inbox, whereas emails sent directly to
me reach my inbox instantaneously. So don't assume I read email that
wasn't targeted directly to me, sorry.

> >> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
> >> index 03382b66f800..5c347cc58baf 100644
> >> --- a/drivers/net/dsa/mv88e6xxx/port.h
> >> +++ b/drivers/net/dsa/mv88e6xxx/port.h
> >> @@ -425,7 +425,7 @@ int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
> >>  int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode);
> >>  int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port,
> >>  				 bool drop_untagged);
> >> -int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port);
> >> +int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port, bool map);
> >>  int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
> >>  				     int upstream_port);
> >>  int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
> >> diff --git a/include/net/dsa.h b/include/net/dsa.h
> >> index 57b3e4e7413b..30f3192616e5 100644
> >> --- a/include/net/dsa.h
> >> +++ b/include/net/dsa.h
> >> @@ -581,6 +581,18 @@ static inline bool dsa_is_upstream_port(struct dsa_switch *ds, int port)
> >>  	return port == dsa_upstream_port(ds, port);
> >>  }
> >>  
> >> +/* Return the local port used to reach the CPU port */
> >> +static inline unsigned int dsa_switch_upstream_port(struct dsa_switch *ds)
> >> +{
> >> +	int p;
> >> +
> >> +	for (p = 0; p < ds->num_ports; p++)
> >> +		if (!dsa_is_unused_port(ds, p))
> >> +			return dsa_upstream_port(ds, p);
> >
> > dsa_switch_for_each_available_port
> >
> > Although to be honest, the caller already has a dp, I wonder why you
> > need to complicate things and don't just call dsa_upstream_port(ds,
> > dp->index) directly.
> 
> Because dp refers to the port we are determining the permissions _for_,
> and ds refers to the chip we are configuring the PVT _on_.
> 
> I think other_dp and dp should swap names with each other. Because it is
> very easy to get confused. Or maybe s/dp/remote_dp/ and s/other_dp/dp/?

Sorry, my mistake, I was looking at the patch in the email client and
didn't recognize from the context that this is mv88e6xxx_port_vlan(),
and that the port is remote. So I retract the part about calling
dsa_upstream_port() directly, but please still consider using a more
appropriate port iterator for the implementation of dsa_switch_upstream_port().