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

On Thu, Aug 11, 2022 at 11:23:02AM +0300, Sergei Antonov wrote:
> > > 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.

Having multiple DSA interfaces share the same MAC address is also not
entirely wrong, no, and is quite common. But here you'll have to do some
more reading.

The thinking goes, since each DSA netdev constitutes its own L3
termination interface, then they are either connected to different L2
networks, or they are connected together (back to back, in an external
cable loopback). The first case doesn't violate the rule that an L2
network should have a single MAC address per VLAN, and the back-to-back
case should work too. While the other would imply sending packets with a
MAC SA equal to the MAC DA. That's not illegal AFAIK either - as long as
no switch sees the packets*, at least.

There's also the possibility for ports to be bridged together, and in
that case, the L3 termination interface stops being the DSA netdev and
starts being the bridge itself, so the ports' MAC addresses stop
mattering too.

There are limitations though, for example this won't work:

                br0
               /   \
              /     \
 lan0 ---- lan1    lan2 ----- lan3

When all of lan0 - lan3 share the same MAC address, and lan0 wants to
ping lan3, br0 will say "oh, this packet is for me!" because the bridge
marks the MAC addresses of all its bridge ports in the FDB as BR_FDB_LOCAL
(local termination, no forwarding).

The last topology I presented is quite common in the kselftest framework,
and the reason why tools/testing/selftests/drivers/net/dsa/forwarding.config
sets STABLE_MAC_ADDRS=yes; so that each port is given a unique address.

*if you're thinking "well, DSA is a switch, too", then not quite. The
ports, when operating as standalone like mv88e6060 does, should have all
bridge layer functions disabled, like for example address learning.

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

After you said that the driver is moxart, I took a quick look at its few
lines of code and I think I know what the problem is.

Basically this driver has a "slow race" between:

static void moxart_mac_set_rx_mode(struct net_device *ndev)
{
	struct moxart_mac_priv_t *priv = netdev_priv(ndev);

	spin_lock_irq(&priv->txlock);

	(ndev->flags & IFF_PROMISC) ? (priv->reg_maccr |= RCV_ALL) : <- this
				      (priv->reg_maccr &= ~RCV_ALL);

	(ndev->flags & IFF_ALLMULTI) ? (priv->reg_maccr |= RX_MULTIPKT) :
				       (priv->reg_maccr &= ~RX_MULTIPKT);

	if ((ndev->flags & IFF_MULTICAST) && netdev_mc_count(ndev)) {
		priv->reg_maccr |= HT_MULTI_EN;
		moxart_mac_setmulticast(ndev);
	} else {
		priv->reg_maccr &= ~HT_MULTI_EN;
	}

	writel(priv->reg_maccr, priv->base + REG_MAC_CTRL);

	spin_unlock_irq(&priv->txlock);
}

and

static void moxart_mac_reset(struct net_device *ndev)
{
	struct moxart_mac_priv_t *priv = netdev_priv(ndev);

	writel(SW_RST, priv->base + REG_MAC_CTRL);
	while (readl(priv->base + REG_MAC_CTRL) & SW_RST)
		mdelay(10);

	writel(0, priv->base + REG_INTERRUPT_MASK);

	priv->reg_maccr = RX_BROADPKT | FULLDUP | CRC_APD | RX_FTL; <- this
}

(called from moxart_mac_open)

Basically moxart_mac_reset() overwrites the RCV_ALL bit from priv->reg_maccr,
not taking into consideration that an interface may be put in
promiscuous mode even while it's not down. See commit d2615bf45069
("net: core: Always propagate flag changes to interfaces") from November
2013 as proof that it can (and btw, DSA does exactly this). The moxart
driver dates from August 2013, so probably this wasn't an issue for a
few months, and then started being one.

Powered by blists - more mailing lists