Date:   Thu, 11 Aug 2022 11:23:02 +0300
From:   Sergei Antonov <saproj@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     netdev@...r.kernel.org, Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH] net: dsa: mv88e6060: report max mtu 1536

On Wed, 10 Aug 2022 at 22:38, Vladimir Oltean <olteanv@...il.com> wrote:
> > > The bug seems to have been introduced by commit 0abfd494deef ("net: dsa:
> > > use dedicated CPU port"), because, although before we'd be uselessly
> > > programming the port VLAN for a disabled port, now in doing so, we
> > > dereference a NULL pointer.
> >
> > The suggested fix with dsa_is_unused_port() works. I tested it on the
> > 'netdev/net.git' repo, see below. Should I submit it as a patch
> > (Fixes: 0abfd494deef)?
>
> Yes. See the section that talks about "git log -1 --pretty=fixes" in
> process/submitting-patches.rst for how the Fixes tag should actually
> look like.
>
> I thought about whether dsa_is_unused_port() is sufficient, since
> theoretically dsa_is_dsa_port() is also a possibility which isn't
> covered by the check. But I rechecked and it appears that the Marvell
> 6060 doesn't support cascade ports, so we should be fine with just that.

Great. I submitted a patch.

> > So I tested "dsa_is_unused_port()" and "switch@10" fixes with
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
> > What I did after system boot-up:
> >
> > ~ # dmesg | grep mv88
> > [    7.187296] mv88e6060 92000090.mdio--1-mii:10: switch Marvell 88E6060 (B0) detected
> > [    8.325712] mv88e6060 92000090.mdio--1-mii:10: switch Marvell 88E6060 (B0) detected
> > [    9.190299] mv88e6060 92000090.mdio--1-mii:10 lan2 (uninitialized): PHY [dsa-0.0:02] driver [Generic PHY] (irq=POLL)
> >
> > ~ # ip a
> > 1: lo: <LOOPBACK> mtu 65536 qdisc noop qlen 1000
> >     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> > 2: eth0: <BROADCAST,MULTICAST> mtu 1504 qdisc noop qlen 1000
> >     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
>
> The DSA master is super odd for starting with an all-zero MAC address.
> What driver handles this part? Normally, drivers are expected to work
> with a MAC address provided by the firmware (of_get_mac_address or
> other, perhaps proprietary, means) and fall back to eth_random_addr()
> if that is missing.

eth0 is handled by the CONFIG_ARM_MOXART_ETHER driver. By the way, I
had to change some code in it to make it work, and I am going to
submit a patch or two later.
The driver does not know its MAC address initially. On my hardware it
is stored in a flash memory chip, so I assign it using "ip link set
..." either manually or from an /etc/init.d script. A solution with
early MAC assignment in the moxart_mac_probe() function is doable. Do
you think I should implement it?

> > 3: lan2@...0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop qlen 1000
> >     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
>
> Here DSA inherits the MAC address of the master. It does this by default
> in dsa_slave_create() -> eth_hw_addr_inherit(). If the OF node for the
> DSA port has its own MAC address, that will have priority over the MAC
> address of the master.

I quickly tried setting a MAC address in the moxart_mac_probe()
function and DSA did inherit it. Looks like the way to go.

> > ~ # ip link set dev eth0 address 00:90:e8:00:10:03 up
>
> This shouldn't be necessary, neither assigning a MAC address nor putting
> the master up, see Documentation/networking/dsa/configuration.rst, the
> master comes up automatically.

Yes, it indeed does. Thanks.

> > ~ # ip a add 192.168.127.254/24 dev lan2
> >
> > ~ # ip link set dev lan2 address 00:90:e8:00:10:03 up
> > [   56.383801] DSA: failed to set STP state 3 (-95)
>
> errno 95 is EOPNOTSUPP, we shouldn't warn here, I'll submit a patch for
> that.

Great, I'll review it.

> > Is it correct for eth0 and lan2@...0 to have the same MAC?
>
> It is not wrong, it's a configuration that many deployed DSA systems use.

Is it also not wrong with several lanN@...0 interfaces? I'm asking it
because I will probably need to support hardware with more than one
port on the 6060.

> > I could not make it work with different MACs.
>
> That is a problem, and I believe it's a problem with the DSA master driver.
> See, the reason it should work is this. Switch ports don't really have a
> MAC address, since they forward everything and not really terminate anything.
> The MAC address of a switch port is a software construct which means
> that software L3 termination interfaces (of which we have one per port)
> should accept packets with some known MAC DA, and drop the rest, and
> everything should be fine.
>
> There are multiple kinds of DSA tags, but 6060 uses a trailer, and this
> will not shift the 'real' MAC DA of packets compared to where the DSA
> master expects to see them. So if the MAC address of the DSA master is A,
> the MAC address of lan2 is B, and you ping lan2 from the outside world,
> the DSA master will see packets with a MAC DA of B.
>
> So the DSA master sees packets with a MAC DA different from its own
> dev->dev_addr (A) and thinks it's within its right to drop them. Except
> that it isn't, because we do the following to prevent it:
>
> (1) in case the DSA master supports IFF_UNICAST_FLT, we call dev_uc_add()
>     from dsa_slave_set_mac_address() and from dsa_slave_open(), and this
>     informs it of our address B.
> (2) in case it doesn't support IFF_UNICAST_FLT, we just call
>     dsa_master_set_promiscuity() and that should keep it promiscuous and
>     it should accept packets regardless of MAC DA (that's the definition).
>
> So you should run some tcpdump and ethtool -S on the DSA master and see
> whether it receives any packets or it drops them. It's possible that
> tcpdump makes packets be accepted, since it puts the interface in
> promiscuous mode.

When I tried to make it work with different MAC addresses, I used
Wireshark and saw that ARP packets did not reach the interface unless
they were broadcast. I might have been a configuration issue rather
than driver issue. Thanks for the explanation! But I will happily
stick to the common MAC address solution if it is not wrong.

Powered by blists - more mailing lists