Re: [PATCH net-next v4 11/11] net: dsa: realtek: rtl8365mb: multiple cpu ports, non cpu extint

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, Jan 21, 2022 at 12:13:58AM -0300, Luiz Angelo Daros de Luca wrote:
> > :) device tree properties are not the fix for everything!
> 
> I'm still getting used to it ;-)
> 
> In this thread, Alvin suggested adding a new property to define which
> port will be used as trap_port instead of using the last CPU port.
> Should I try something different?
> 
>         switch1 {
>                compatible = "realtek,rtl8367s";
>                reg = <29>;
> 
>                realtek,trap-port = <&port7>;
> 
>                ports {
>                         ....
>                         port7: port@7 {
>                             ...
>                        };
>         };
> 
> Should I do something differently?

To clarify, I don't know what a trap_port is. I just saw this
description in rtl8365mb.c:

 * @trap_port: forward trapped frames to this port

but I still don't know to which packets does this configuration apply
(where are the packet traps installed, and for what kind of packets).

Speculating here, but it appears quite arbitrary, and I'd guess also
broken, to make the trap_port the last CPU port. Is this also part of
the things which you didn't really test? See commit 8d5f7954b7c8 ("net:
dsa: felix: break at first CPU port during init and teardown") for a
similar issue with this. When there are multiple 'ethernet = <&phandle>'
properties in the device tree, DSA makes the owners of all those
phandles a DSA master, and all those switch ports as CPU ports. But out
of all those CPU ports, only the first one is an active CPU port. The
others have no dp->cpu_dp pointing to them.
See dsa_tree_setup_default_cpu() -> dsa_tree_find_first_cpu().
Even when DSA gets full-blown support for multiple CPU ports, I think
it's safe to say that this default will remain the way it is: a single
CPU port will be active to begin with: the first one. Given that fact
(and depending on what you need to do with the trap_port info exactly),
it might be broken to set as the trap port a CPU port that isn't used.
Stuff like dsa_port_host_fdb_add()/dsa_port_host_fdb_del() will be
broken, because they rely on the dp->cpu_dp association, and
dp->cpu_dp->index will be != trap_port.

> > I think I know what the problem is. But I'd need to know what the driver
> > for the DSA master is, to confirm. To be precise, what I'd like to check
> > is the value of master->vlan_features.
> 
> Here it is 0x1099513266227 (I hope).

That's quite an extraordinary set of vlan_features. In that number, I
notice BIT(2) is set, which corresponds to __UNUSED_NETIF_F_1. So it
probably isn't correctly printed.

This is what I would have liked to see:

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 22241afcac81..b41f1b414c69 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1909,6 +1909,7 @@ void dsa_slave_setup_tagger(struct net_device *slave)
 	p->xmit = cpu_dp->tag_ops->xmit;
 
 	slave->features = master->vlan_features | NETIF_F_HW_TC;
+	netdev_err(slave, "master %s vlan_features 0x%llx\n", master->name, master->vlan_features);
 	slave->hw_features |= NETIF_F_HW_TC;
 	slave->features |= NETIF_F_LLTX;
 	if (slave->needed_tailroom)

And I don't think you fully answered Florian's questions either, really.
Can we see the a link to the code of the Ethernet controller whose role
is to be a host port (DSA master) for the rtl8365mb switch? If that DSA
master is a DSA switch itself, could you please unroll the chain all the
way with more links to drivers? No matter whether upstream or downstream,
just what you use.

I hate to guess, but since both you and Arınç have mentioned the
mt7620a/mt7621 SoCs, I'd guess that the top-most DSA driver in both
cases is "mediatek,eth-mac" (drivers/net/ethernet/mediatek/mtk_eth_soc.c).
If so, this would confirm my suspicions, since it sets its vlan_features
to include NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM. Please confirm that
master->vlan_features contains these 2 bits.

> Oh, this DSA driver still does not implement vlan nor bridge offload.
> Maybe it would matter.

It doesn't matter. The vlan_features is a confusing name for what it
really does here. I'll explain in a bit once you clarify the other
things I asked for.