Re: net: dsa: lantiq_gswip: getting the first selftests to pass

[Vladimir Oltean <olteanv@gmail.com>]

On Sun, Jul 31, 2022 at 10:49:35PM +0200, Martin Blumenstingl wrote:
> While working on my patches a more practical question came up while I
> was breaking the driver and then trying to make local_termination.sh
> pass again.
> At the start of run_tests for the standalone port scenario I am
> getting the following values:
>   rcv_dmac = 00:01:02:03:04:02
>   MACVLAN_ADDR = 00:00:de:ad:be:ef
> My expectation is that port_fdb_add() is called with these MAC
> addresses. I verified that dsa_switch_supports_uc_filtering() returns
> true, but still I

Unfinished phrase, I suppose you wanted to say that you don't get a call
to port_fdb_add() with $MACVLAN_ADDR. I don't see why that would be the
case. Try to put some prints in dsa_slave_sync_uc(), since that's the
path through which $MACVLAN_ADDR comes in. On the other hand, $rcv_dmac
comes from dsa_slave_open().

> > I'll do so if you have a specific question about something apparently
> > not mapping to the expectations.
> I still have an issue which I believe is related to the FDB handling.
> 
> I *think* that I have implemented FDB isolation correctly in my
> work-in-progress branch [0].
> 
> The GSWIP140 datasheet (page 82) has a "MAC Learning disable and MAC
> Learning Limitation Description" (table 26).
> In the xRX200 vendor kernel I cannot find the LNDIS bit in
> PCE_PCTRL_3, so I suspect it has only been added in newer GSWIP
> revisions (xRX200 is at least one major IP revision behind GSW140).
> Maybe Hauke knows?
> So what I'm doing to disable learning is setting the "learning limit"
> (which limits the number of entries that can be learned) for that port
> to zero.
> 
>  My problem is that whenever I disable learning a lot of tests from
> local_termination.sh are failing, including:
> - TEST: lan2: Unicast IPv4 to primary MAC address
> - TEST: lan2: Unicast IPv4 to macvlan MAC address
> 
> Setting the PLIMMOD bit to 1 means that GSWIP won't drop the packet if
> the learning limit is exceeded (the default value seems to be 0).

Yes, I'm not sure why you'd want to drop packets that aren't learned, so
I would expect PLIMMOD to be 1 if you're disabling learning via LRNLIM=0.

> This at least works around the first failing test (Unicast IPv4 to
> primary MAC address).

Not sure why "works around" is the choice of words here.

> Based on your understanding of my issue: I am going in the right
> direction when I'm saying that this is an FDB issue?

I don't know why the tests fail. I downloaded your code and I see that
you touch PLIMMOD from ds->ops->port_set_host_flood(). Why not just
leave it at 1? It's a global bit anyway, it affects what happens with
all ports that have source address learning disabled, it seems a very
odd choice to do what you're doing.

When you have learning disabled on standalone ports (by design), and
local_termination.sh receives packets on $swp1, a standalone port
(by design), don't you agree that the MAC SA of these packets won't be
learned, and you're telling the switch "yeah, drop them"?

> [0] https://github.com/xdarklight/linux/commits/lantiq-gswip-integration-20220730

There are many things to say about the code, however it's a bit
difficult to review it just by looking at the Github commits.
For example I'm looking at gswip_port_fdb(), I don't really understand
why this:

	for (i = max_ports; i < ARRAY_SIZE(priv->vlans); i++) {
		if (priv->vlans[i].bridge == bridge) {
			fid = priv->vlans[i].fid;
			break;
		}
	}

is not this:

	for (i = max_ports; i < ARRAY_SIZE(priv->vlans); i++) {
		if (priv->vlans[i].bridge == bridge &&
		    priv->vlans[i].vid == vid) {
			fid = priv->vlans[i].fid;
			break;
		}
	}