Messages in this thread | | | Date | Fri, 10 Mar 2023 14:04:39 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH 1/7] dsa: marvell: Provide per device information about max frame size |
| |
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@lunn.ch> 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!
| |