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

[Tobias Waldekranz]

On Tue, Feb 01, 2022 at 22:11, Vladimir Oltean <olteanv@xxxxxxxxx> wrote:
> 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.

No worries. I have recently started using get_maintainers.pl to auto
generate the recipient list, with the result that the cover is only sent
to the list. Ideally I would like send-email to use the union of all
recipients for the cover letter, but I haven't figured that one out yet.

I actually gave up on getting my mailinglists from my email provider,
now I just download it directly from lore. I hacked together a script
that will scrape a public-inbox repo and convert it to a Maildir:

https://github.com/wkz/notmuch-lore

As you can tell from the name, it is tailored for plugging into notmuch,
but the guts are pretty generic.

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

Will do!