[PATCH v4 net-next] net: ftmac100: support mtu > 1500

[PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Sergei Antonov]

The ftmac100 controller considers some packets FTL (frame
too long) and drops them. An example of a dropped packet:
6 bytes - dst MAC
6 bytes - src MAC
2 bytes - EtherType IPv4 (0800)
1504 bytes - IPv4 packet

Do the following to let the driver receive these packets.
Set FTMAC100_MACCR_RX_FTL when mtu>1500 in the MAC Control Register.
For received packets marked with FTMAC100_RXDES0_FTL check if packet
length (with FCS excluded) is within expected limits, that is not
greater than netdev->mtu + 14 (Ethernet headers). Otherwise trigger
an error.

Fixes: 8d77c036b57c ("net: add Faraday FTMAC100 10/100 Ethernet driver")
Signed-off-by: Sergei Antonov <saproj@gmail.com>
Suggested-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/ethernet/faraday/ftmac100.c | 29 ++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
index d95d78230828..f89b53845f21 100644
--- a/drivers/net/ethernet/faraday/ftmac100.c
+++ b/drivers/net/ethernet/faraday/ftmac100.c
@@ -159,6 +159,7 @@ static void ftmac100_set_mac(struct ftmac100 *priv, const unsigned char *mac)
 static int ftmac100_start_hw(struct ftmac100 *priv)
 {
 	struct net_device *netdev = priv->netdev;
+	unsigned int maccr;
 
 	if (ftmac100_reset(priv))
 		return -EIO;
@@ -175,7 +176,20 @@ static int ftmac100_start_hw(struct ftmac100 *priv)
 
 	ftmac100_set_mac(priv, netdev->dev_addr);
 
-	iowrite32(MACCR_ENABLE_ALL, priv->base + FTMAC100_OFFSET_MACCR);
+	maccr = MACCR_ENABLE_ALL;
+
+	/* We have to set FTMAC100_MACCR_RX_FTL in case MTU > 1500
+	 * and do extra length check in ftmac100_rx_packet_error().
+	 * Otherwise the controller silently drops these packets.
+	 *
+	 * When the MTU of the interface is standard 1500, rely on
+	 * the controller's functionality to drop too long packets
+	 * and save some CPU time.
+	 */
+	if (netdev->mtu > 1500)
+		maccr |= FTMAC100_MACCR_RX_FTL;
+
+	iowrite32(maccr, priv->base + FTMAC100_OFFSET_MACCR);
 	return 0;
 }
 
@@ -337,9 +351,18 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
 		error = true;
 	}
 
-	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) {
+	/* If the frame-too-long flag FTMAC100_RXDES0_FTL is set, check
+	 * if ftmac100_rxdes_frame_length(rxdes) exceeds the currently
+	 * set MTU plus ETH_HLEN.
+	 * The controller would set FTMAC100_RXDES0_FTL for all incoming
+	 * frames longer than 1518 (includeing FCS) in the presense of
+	 * FTMAC100_MACCR_RX_FTL in the MAC Control Register.
+	 */
+	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes) &&
+		     ftmac100_rxdes_frame_length(rxdes) > netdev->mtu + ETH_HLEN)) {
 		if (net_ratelimit())
-			netdev_info(netdev, "rx frame too long\n");
+			netdev_info(netdev, "rx frame too long (%u)\n",
+				    ftmac100_rxdes_frame_length(rxdes));
 
 		netdev->stats.rx_length_errors++;
 		error = true;
-- 
2.34.1

Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Sergei Antonov]

On Wed, 19 Oct 2022 at 19:21, Sergei Antonov <saproj@gmail.com> wrote:
>
> The ftmac100 controller considers some packets FTL (frame
> too long) and drops them. An example of a dropped packet:
> 6 bytes - dst MAC
> 6 bytes - src MAC
> 2 bytes - EtherType IPv4 (0800)
> 1504 bytes - IPv4 packet
>
> Do the following to let the driver receive these packets.
> Set FTMAC100_MACCR_RX_FTL when mtu>1500 in the MAC Control Register.
> For received packets marked with FTMAC100_RXDES0_FTL check if packet
> length (with FCS excluded) is within expected limits, that is not
> greater than netdev->mtu + 14 (Ethernet headers). Otherwise trigger
> an error.
>
> Fixes: 8d77c036b57c ("net: add Faraday FTMAC100 10/100 Ethernet driver")
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> ---

Sorry, forgot the changelog:
v4:
* Set FTMAC100_MACCR_RX_FTL depending on the "mtu > 1500" condition.
* DSA tagging seems unrelated to the issue - updated description and a
code comment accordingly.

v3:
* Corrected the explanation of the problem: datasheet is correct.
* Rewrote the code to use the currently set mtu to handle DSA frames.

v2:
* Typos in description fixed.

Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Vladimir Oltean]

On Wed, Oct 19, 2022 at 07:20:58PM +0300, Sergei Antonov wrote:
> The ftmac100 controller considers some packets FTL (frame
> too long) and drops them. An example of a dropped packet:
> 6 bytes - dst MAC
> 6 bytes - src MAC
> 2 bytes - EtherType IPv4 (0800)
> 1504 bytes - IPv4 packet

Why do you insist writing these confusing messages?

It's pretty straightforward. If the FTMAC100 is used as a DSA master,
then it is expected that frames which are MTU sized on the wire facing
the external switch port (1500 octets in L2 payload, plus L2 header)
also get a DSA tag when seen by the host port.

This extra tag increases the length of the packet as the host port sees
it, and the FTMAC100 is not prepared to handle frames whose length
exceeds 1518 octets (including FCS) at all.

Only a minimal rework is needed to support this configuration. Since
MTU-sized DSA-tagged frames still fit within a single buffer (RX_BUF_SIZE),
we just need to optimize the resource management rather than implement
multi buffer RX.

In ndo_mtu_change, we toggle the FTMAC100_MACCR_RX_FTL bit to tell the
hardware to drop (or not) frames with an L2 payload length larger than
1500. And since setting this bit, and accepting frames with the FTL bit
in the BD status, exposes us to the danger of receiving multi-buffer
frames (which we still do not support), we need to replace the BUG_ON()
with a proper call to ftmac100_rx_drop_packet(). We need to replicate
the MACCR configuration in ftmac100_start_hw() as well, since there is a
hardware reset there which clears previous settings.

The advantage of dynamically changing FTMAC100_MACCR_RX_FTL is that when
dev->mtu is at the default value of 1500, large frames are automatically
dropped in hardware and we do not spend CPU cycles dropping them.

> Do the following to let the driver receive these packets.
> Set FTMAC100_MACCR_RX_FTL when mtu>1500 in the MAC Control Register.
> For received packets marked with FTMAC100_RXDES0_FTL check if packet
> length (with FCS excluded) is within expected limits, that is not
> greater than netdev->mtu + 14 (Ethernet headers). Otherwise trigger
> an error.
> 
> Fixes: 8d77c036b57c ("net: add Faraday FTMAC100 10/100 Ethernet driver")

Please drop the Fixes: tag, I thought we agreed this patch wouldn't get
backported, since it does not fix any bug in this driver.

> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> Suggested-by: Vladimir Oltean <olteanv@gmail.com>

You essentially did nothing from what I suggested. Please remove this
tag, it is misleading.

> ---
>  drivers/net/ethernet/faraday/ftmac100.c | 29 ++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
> index d95d78230828..f89b53845f21 100644
> --- a/drivers/net/ethernet/faraday/ftmac100.c
> +++ b/drivers/net/ethernet/faraday/ftmac100.c
> @@ -159,6 +159,7 @@ static void ftmac100_set_mac(struct ftmac100 *priv, const unsigned char *mac)
>  static int ftmac100_start_hw(struct ftmac100 *priv)
>  {
>  	struct net_device *netdev = priv->netdev;
> +	unsigned int maccr;
>  
>  	if (ftmac100_reset(priv))
>  		return -EIO;
> @@ -175,7 +176,20 @@ static int ftmac100_start_hw(struct ftmac100 *priv)
>  
>  	ftmac100_set_mac(priv, netdev->dev_addr);
>  
> -	iowrite32(MACCR_ENABLE_ALL, priv->base + FTMAC100_OFFSET_MACCR);
> +	maccr = MACCR_ENABLE_ALL;
> +
> +	/* We have to set FTMAC100_MACCR_RX_FTL in case MTU > 1500
> +	 * and do extra length check in ftmac100_rx_packet_error().
> +	 * Otherwise the controller silently drops these packets.
> +	 *
> +	 * When the MTU of the interface is standard 1500, rely on
> +	 * the controller's functionality to drop too long packets
> +	 * and save some CPU time.
> +	 */
> +	if (netdev->mtu > 1500)
> +		maccr |= FTMAC100_MACCR_RX_FTL;

It is expected that ndo_change_mtu() handles this as well, so that the
MTU change takes effect right away even if the device is open.

> +
> +	iowrite32(maccr, priv->base + FTMAC100_OFFSET_MACCR);
>  	return 0;
>  }
>  
> @@ -337,9 +351,18 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
>  		error = true;
>  	}
>  
> -	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) {
> +	/* If the frame-too-long flag FTMAC100_RXDES0_FTL is set, check
> +	 * if ftmac100_rxdes_frame_length(rxdes) exceeds the currently
> +	 * set MTU plus ETH_HLEN.
> +	 * The controller would set FTMAC100_RXDES0_FTL for all incoming
> +	 * frames longer than 1518 (includeing FCS) in the presense of
> +	 * FTMAC100_MACCR_RX_FTL in the MAC Control Register.
> +	 */
> +	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes) &&
> +		     ftmac100_rxdes_frame_length(rxdes) > netdev->mtu + ETH_HLEN)) {

You didn't explain why you can't drop this altogether?

>  		if (net_ratelimit())
> -			netdev_info(netdev, "rx frame too long\n");
> +			netdev_info(netdev, "rx frame too long (%u)\n",
> +				    ftmac100_rxdes_frame_length(rxdes));
>  
>  		netdev->stats.rx_length_errors++;
>  		error = true;
> -- 
> 2.34.1
> 

Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Sergei Antonov]

On Wed, 19 Oct 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Wed, Oct 19, 2022 at 07:20:58PM +0300, Sergei Antonov wrote:
> > The ftmac100 controller considers some packets FTL (frame
> > too long) and drops them. An example of a dropped packet:
> > 6 bytes - dst MAC
> > 6 bytes - src MAC
> > 2 bytes - EtherType IPv4 (0800)
> > 1504 bytes - IPv4 packet
>
> Why do you insist writing these confusing messages?

Yes, I see now it is not good. And I have a question.
By comparing the packet sent from the sending computer and the packet
received by ftmac100 I found that 4 bytes get appended to the packet:
80 02 00 00
I guess it is what 88E6060 switch adds. But it is not 0x8100. Then
what it is? I looked into include/linux/if_vlan.h but still don't get
it.

Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Vladimir Oltean]

On Wed, Oct 19, 2022 at 09:36:21PM +0300, Sergei Antonov wrote:
> On Wed, 19 Oct 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Wed, Oct 19, 2022 at 07:20:58PM +0300, Sergei Antonov wrote:
> > > The ftmac100 controller considers some packets FTL (frame
> > > too long) and drops them. An example of a dropped packet:
> > > 6 bytes - dst MAC
> > > 6 bytes - src MAC
> > > 2 bytes - EtherType IPv4 (0800)
> > > 1504 bytes - IPv4 packet
> >
> > Why do you insist writing these confusing messages?
> 
> Yes, I see now it is not good. And I have a question.
> By comparing the packet sent from the sending computer and the packet
> received by ftmac100 I found that 4 bytes get appended to the packet:
> 80 02 00 00
> I guess it is what 88E6060 switch adds. But it is not 0x8100. Then
> what it is? I looked into include/linux/if_vlan.h but still don't get
> it.

See trailer_rcv() in net/dsa/tag_trailer.c. The switch is telling you
that this is a packet received from source port 2 via a tail tag
(trailer, i.e. added to the end of the packet, meaning right before the FCS).
I know that mv88e6060 uses this tagging protocol because this is what
mv88e6060_get_tag_protocol() returns. Still nothing to do with if_vlan.h.

Also see if Documentation/networking/dsa/dsa.rst answers some of the
other questions.

Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Sergei Antonov]

On Wed, 19 Oct 2022 at 21:42, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Wed, Oct 19, 2022 at 09:36:21PM +0300, Sergei Antonov wrote:
> > On Wed, 19 Oct 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > On Wed, Oct 19, 2022 at 07:20:58PM +0300, Sergei Antonov wrote:
> > > > The ftmac100 controller considers some packets FTL (frame
> > > > too long) and drops them. An example of a dropped packet:
> > > > 6 bytes - dst MAC
> > > > 6 bytes - src MAC
> > > > 2 bytes - EtherType IPv4 (0800)
> > > > 1504 bytes - IPv4 packet
> > >
> > > Why do you insist writing these confusing messages?

Working on a better version of the patch. And here is another question.
Unless the flag is set, the controller drops packets bigger than 1518
(that is 1500 payload + 14 Ethernet header + 4 FCS). So if mtu is 1500
the driver can enable the controller's functionality (clear the
FTMAC100_MACCR_RX_FTL flag) and save CPU time. When mtu is less or
greater than 1500, should the driver do the following:
if (ftmac100_rxdes_frame_length(rxdes) > netdev->mtu + ETH_HLEN) {
    drop the packet
}
I mean, is it driver's duty to drop oversized packets?

In the current version of the driver, if, for example, mtu is 1400 and
the incoming packet is 1450 bytes it is not dropped. Or is mtu < 1500
incorrect for Ethernet cards so there is no need to write code
handling it?

Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Andrew Lunn]

On Mon, Oct 24, 2022 at 06:50:31PM +0300, Sergei Antonov wrote:
> On Wed, 19 Oct 2022 at 21:42, Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Wed, Oct 19, 2022 at 09:36:21PM +0300, Sergei Antonov wrote:
> > > On Wed, 19 Oct 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> > > >
> > > > On Wed, Oct 19, 2022 at 07:20:58PM +0300, Sergei Antonov wrote:
> > > > > The ftmac100 controller considers some packets FTL (frame
> > > > > too long) and drops them. An example of a dropped packet:
> > > > > 6 bytes - dst MAC
> > > > > 6 bytes - src MAC
> > > > > 2 bytes - EtherType IPv4 (0800)
> > > > > 1504 bytes - IPv4 packet
> > > >
> > > > Why do you insist writing these confusing messages?
> 
> Working on a better version of the patch. And here is another question.
> Unless the flag is set, the controller drops packets bigger than 1518
> (that is 1500 payload + 14 Ethernet header + 4 FCS). So if mtu is 1500
> the driver can enable the controller's functionality (clear the
> FTMAC100_MACCR_RX_FTL flag) and save CPU time. When mtu is less or
> greater than 1500, should the driver do the following:
> if (ftmac100_rxdes_frame_length(rxdes) > netdev->mtu + ETH_HLEN) {
>     drop the packet
> }
> I mean, is it driver's duty to drop oversized packets?

It is not well defined what should happen here. Some drivers will
deliver oversized packets to the stack, some will not.

The main purpose of the MTU is fragmentation on transmission. How do
you break a UDP or TCP PDU up into L2 frames. MTU is not really used
on reception. If the L2 frame is otherwise valid, i doubt the stack
will drop it if it is longer than expected.

     Andrew

Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Vladimir Oltean]

On Mon, Oct 24, 2022 at 06:09:02PM +0200, Andrew Lunn wrote:
> On Mon, Oct 24, 2022 at 06:50:31PM +0300, Sergei Antonov wrote:
> > On Wed, 19 Oct 2022 at 21:42, Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > On Wed, Oct 19, 2022 at 09:36:21PM +0300, Sergei Antonov wrote:
> > > > On Wed, 19 Oct 2022 at 19:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > >
> > > > > On Wed, Oct 19, 2022 at 07:20:58PM +0300, Sergei Antonov wrote:
> > > > > > The ftmac100 controller considers some packets FTL (frame
> > > > > > too long) and drops them. An example of a dropped packet:
> > > > > > 6 bytes - dst MAC
> > > > > > 6 bytes - src MAC
> > > > > > 2 bytes - EtherType IPv4 (0800)
> > > > > > 1504 bytes - IPv4 packet
> > > > >
> > > > > Why do you insist writing these confusing messages?
> > 
> > Working on a better version of the patch. And here is another question.
> > Unless the flag is set, the controller drops packets bigger than 1518
> > (that is 1500 payload + 14 Ethernet header + 4 FCS). So if mtu is 1500
> > the driver can enable the controller's functionality (clear the
> > FTMAC100_MACCR_RX_FTL flag) and save CPU time. When mtu is less or
> > greater than 1500, should the driver do the following:
> > if (ftmac100_rxdes_frame_length(rxdes) > netdev->mtu + ETH_HLEN) {
> >     drop the packet
> > }
> > I mean, is it driver's duty to drop oversized packets?
> 
> It is not well defined what should happen here. Some drivers will
> deliver oversized packets to the stack, some will not.
> 
> The main purpose of the MTU is fragmentation on transmission. How do
> you break a UDP or TCP PDU up into L2 frames. MTU is not really used
> on reception. If the L2 frame is otherwise valid, i doubt the stack
> will drop it if it is longer than expected.
> 
>      Andrew

I think the general wisdom is for a driver to not go out of its way to
accept packets larger than the MTU, and not go out of its way to drop
them either. The only given guarantee is that packets with an L2 length
<= dev->mtu are accepted.

The sad reality is that some layers like DSA rely on this marginal
behavior in some cases (controllers accept packets slightly larger than
dev->mtu) and this is why we took the decisions that we took (discussion
which I linked to in the v2 of this patch:
https://patchwork.ozlabs.org/project/netdev/patch/20200421123110.13733-2-olteanv@gmail.com/)

Re: [PATCH v4 net-next] net: ftmac100: support mtu > 1500

[Vladimir Oltean]

On Mon, Oct 24, 2022 at 07:21:45PM +0300, Vladimir Oltean wrote:
> The only given guarantee is that packets with an L2 length <= dev->mtu
> are accepted.

L2 payload* length, i.e. the bytes between the Ethernet/VLAN header and
the FCS.