Re: [PATCH 1/7] dsa: marvell: Provide per device information about max frame size

[Russell King (Oracle)]

On Fri, Mar 10, 2023 at 02:02:35PM +0200, Vladimir Oltean wrote:
> It would be good if the commit message contained the procedure based on
> which you had made these changes - and preferably they were mechanical.
> Having a small C program written would be absolutely ideal.
> This is so that reviewers wouldn't have to do it in parallel...
> 
> My analysis has determined the following 3 categories:
> 
> static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
> {
> 	struct mv88e6xxx_chip *chip = ds->priv;
> 
> 	if (chip->info->ops->port_set_jumbo_size)
> 		return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; // 10210
> 	else if (chip->info->ops->set_max_frame_size)
> 		return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; // 1602
> 	return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; // 1492

The question concerning the 1492 MTU (and the others above) is
something that does need to be addressed, but I don't believe it
should be part of this patch series.

In order to properly address this, we need to do a bit of research.
Originally, the driver calculated the MTU by taking the frame size
(1522, 1632 or 10240) and subtracting VLAN_ETH_HLEN and ETH_FCS_LEN.

This would mean the frame sizes were 1500, 1610 and 10218. However,
as a result of:

commit b9c587fed61cf88bd45822c3159644445f6d5aa6
Author: Andrew Lunn <andrew@xxxxxxx>
Date:   Sun Sep 26 19:41:26 2021 +0200

    dsa: mv88e6xxx: Include tagger overhead when setting MTU for DSA and CPU ports

This was changed to include the EDSA_HLEN of 8 bytes. The question
is why - and that's a question for Andrew.

The frame size check is not well described looking at the 6176
functional specification. It takes about using an adjusted frame
size in the paragraph that talks about ingress headers, but then
it only takes about adjusting by two bytes which are sent before
the DA, only if MV88E6XXX_PORT_CTL0_HEADER is set (which we don't
touch).

Against the bits that control the maximum frame size, it does state
that "the definition of frame size is counting the frame bytes from
MAC-DA through Layer2 CRC of the frame".

No mention is made whether the EDSA header is included or not, the
assumption was that it wasn't prior to the commit above, but it
would appear that caused a problem, so the EDSA header was added.

Now, obviously, on external ports (those which don't use the EDSA
header) the EDSA header doesn't restrict the size of packets sent
or received on that port. However, the header does exist on the
CPU port - and the obvious question is, does the max frame size
apply, and if so does it apply with the EDSA header included or
excluded. We don't know from the documentation.

DSA ports (those between switches) don't use the EDSA header, but
instead use the DSA header which is four bytes long. Again, whether
that is included in the maximum frame size is unspecified.

Maybe Andrew has some input here as he made the above commit and
can remember why it was necessary.

However, to me, it seems to be rather absurd as it would mean that
on a device that only supports 1522 maximum packet size, the CPU
port using an EDSA header would be incapable of sending or
receiving a packet containing 1500 bytes of payload, VLAN header
and ethernet header, because as soon as the EDSA header is added
we're over the 1522 limit - and that would basically mean the
switch can't be used in a normal ethernet network to switch
such packets.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!