[PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[Sergei Antonov]

Despite the datasheet [1] saying the controller should allow incoming
packets of length >=1518, it only allows packets of length <=1514.

Since 1518 is a standard Ethernet maximum frame size, and it can
easily be encountered (in SSH for example), fix this behavior:

* Set FTMAC100_MACCR_RX_FTL in the MAC Control Register.
* Check for packet size > 1518 in ftmac100_rx_packet_error().

[1]
https://bitbucket.org/Kasreyn/mkrom-uc7112lx/src/master/documents/FIC8120_DS_v1.2.pdf

Fixes: 8d77c036b57c ("net: add Faraday FTMAC100 10/100 Ethernet driver")
Signed-off-by: Sergei Antonov <saproj@gmail.com>
---

v1 -> v2:
* Typos in description fixed.

 drivers/net/ethernet/faraday/ftmac100.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
index d95d78230828..34d0284079ff 100644
--- a/drivers/net/ethernet/faraday/ftmac100.c
+++ b/drivers/net/ethernet/faraday/ftmac100.c
@@ -154,6 +154,7 @@ static void ftmac100_set_mac(struct ftmac100 *priv, const unsigned char *mac)
 				 FTMAC100_MACCR_CRC_APD	| \
 				 FTMAC100_MACCR_FULLDUP	| \
 				 FTMAC100_MACCR_RX_RUNT	| \
+				 FTMAC100_MACCR_RX_FTL	| \
 				 FTMAC100_MACCR_RX_BROADPKT)
 
 static int ftmac100_start_hw(struct ftmac100 *priv)
@@ -320,6 +321,7 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
 {
 	struct net_device *netdev = priv->netdev;
 	bool error = false;
+	const unsigned int length = ftmac100_rxdes_frame_length(rxdes);
 
 	if (unlikely(ftmac100_rxdes_rx_error(rxdes))) {
 		if (net_ratelimit())
@@ -337,9 +339,16 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
 		error = true;
 	}
 
-	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) {
+	/* The frame-too-long flag 'FTMAC100_RXDES0_FTL' is described in the
+	 * datasheet as: "When set, it indicates that the received packet
+	 * length exceeds 1518 bytes." But testing shows that it is also set
+	 * when packet length is equal to 1518.
+	 * Since 1518 is a standard Ethernet maximum frame size, let it pass
+	 * and only trigger an error when packet length really exceeds it.
+	 */
+	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes) && length > 1518)) {
 		if (net_ratelimit())
-			netdev_info(netdev, "rx frame too long\n");
+			netdev_info(netdev, "rx frame too long (%u)\n", length);
 
 		netdev->stats.rx_length_errors++;
 		error = true;
-- 
2.34.1

RE: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[David Laight]

From: Sergei Antonov
> Sent: 12 October 2022 16:38
> 
> Despite the datasheet [1] saying the controller should allow incoming
> packets of length >=1518, it only allows packets of length <=1514.

Shouldn't that be <=1518 and <1518 ??

Although traditionally it was 1514+crc.
An extra 4 byte header is now allowed.
There is also the usefulness of supporting full length frames
with a PPPoE header.

Whether it actually makes sense to round up the receive buffer
size and associated max frame length to 1536 (cache line aligned)
is another matter (probably 1534 for 4n+2 alignment).

> Since 1518 is a standard Ethernet maximum frame size, and it can
> easily be encountered (in SSH for example), fix this behavior:
> 
> * Set FTMAC100_MACCR_RX_FTL in the MAC Control Register.

What does that do?
Looks like it might cause 'Frame Too Long' packets be returned.
In which case should the code just have ignored it since
longer frames would be discarded completely??

> * Check for packet size > 1518 in ftmac100_rx_packet_error().
> 
> [1]
> https://bitbucket.org/Kasreyn/mkrom-uc7112lx/src/master/documents/FIC8120_DS_v1.2.pdf
> 
> Fixes: 8d77c036b57c ("net: add Faraday FTMAC100 10/100 Ethernet driver")
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> ---
> 
> v1 -> v2:
> * Typos in description fixed.
> 
>  drivers/net/ethernet/faraday/ftmac100.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
> index d95d78230828..34d0284079ff 100644
> --- a/drivers/net/ethernet/faraday/ftmac100.c
> +++ b/drivers/net/ethernet/faraday/ftmac100.c
> @@ -154,6 +154,7 @@ static void ftmac100_set_mac(struct ftmac100 *priv, const unsigned char *mac)
>  				 FTMAC100_MACCR_CRC_APD	| \
>  				 FTMAC100_MACCR_FULLDUP	| \
>  				 FTMAC100_MACCR_RX_RUNT	| \
> +				 FTMAC100_MACCR_RX_FTL	| \
>  				 FTMAC100_MACCR_RX_BROADPKT)
> 
>  static int ftmac100_start_hw(struct ftmac100 *priv)
> @@ -320,6 +321,7 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
>  {
>  	struct net_device *netdev = priv->netdev;
>  	bool error = false;
> +	const unsigned int length = ftmac100_rxdes_frame_length(rxdes);

Do you need to read this value this early in the function?
Looks like it is only used when overlong packets are reported.

	David

> 
>  	if (unlikely(ftmac100_rxdes_rx_error(rxdes))) {
>  		if (net_ratelimit())
> @@ -337,9 +339,16 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
>  		error = true;
>  	}
> 
> -	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) {
> +	/* The frame-too-long flag 'FTMAC100_RXDES0_FTL' is described in the
> +	 * datasheet as: "When set, it indicates that the received packet
> +	 * length exceeds 1518 bytes." But testing shows that it is also set
> +	 * when packet length is equal to 1518.
> +	 * Since 1518 is a standard Ethernet maximum frame size, let it pass
> +	 * and only trigger an error when packet length really exceeds it.
> +	 */
> +	if (unlikely(ftmac100_rxdes_frame_too_long(rxdes) && length > 1518)) {
>  		if (net_ratelimit())
> -			netdev_info(netdev, "rx frame too long\n");
> +			netdev_info(netdev, "rx frame too long (%u)\n", length);
> 
>  		netdev->stats.rx_length_errors++;
>  		error = true;
> --
> 2.34.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[Sergei Antonov]

On Wed, 12 Oct 2022 at 19:13, David Laight <David.Laight@aculab.com> wrote:
>
> From: Sergei Antonov
> > Sent: 12 October 2022 16:38
> >
> > Despite the datasheet [1] saying the controller should allow incoming
> > packets of length >=1518, it only allows packets of length <=1514.
>
> Shouldn't that be <=1518 and <1518 ??

Oh, thanks for noticing. But still it should be slightly different:
<= 1518 and <=1514
Here is my test results of different packet sizes:
packets of 1518 / 1517 / 1516 / 1515 bytes did not come to the driver
(before my patch)
packets of 1514 and less bytes did come

> Although traditionally it was 1514+crc.
> An extra 4 byte header is now allowed.
> There is also the usefulness of supporting full length frames
> with a PPPoE header.
>
> Whether it actually makes sense to round up the receive buffer
> size and associated max frame length to 1536 (cache line aligned)
> is another matter (probably 1534 for 4n+2 alignment).
>
> > Since 1518 is a standard Ethernet maximum frame size, and it can
> > easily be encountered (in SSH for example), fix this behavior:
> >
> > * Set FTMAC100_MACCR_RX_FTL in the MAC Control Register.
>
> What does that do?

If FTMAC100_MACCR_RX_FTL is not set:
  the driver does not receive the "long" packet at all. Looks like the
controller discards the packet without bothering the driver.
If FTMAC100_MACCR_RX_FTL is set:
  the driver receives the "long" packet marked by the
FTMAC100_RXDES0_FTL flag. And these packets were discarded by the
driver (before my patch).

> Looks like it might cause 'Frame Too Long' packets be returned.
> In which case should the code just have ignored it since
> longer frames would be discarded completely??

Is there such a thing as a response packet which is sent in return to
FTL packet? Did not know that. My testcases were SSH and SCP programs
on Ubuntu 22 and they simply hang trying to connect to the ftmac100
device - no retransmissions or retries with smaller frames happened.

> > * Check for packet size > 1518 in ftmac100_rx_packet_error().
> >
> > [1]
> > https://bitbucket.org/Kasreyn/mkrom-uc7112lx/src/master/documents/FIC8120_DS_v1.2.pdf
> >
> > Fixes: 8d77c036b57c ("net: add Faraday FTMAC100 10/100 Ethernet driver")
> > Signed-off-by: Sergei Antonov <saproj@gmail.com>
> > ---
> >
> > v1 -> v2:
> > * Typos in description fixed.
> >
> >  drivers/net/ethernet/faraday/ftmac100.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
> > index d95d78230828..34d0284079ff 100644
> > --- a/drivers/net/ethernet/faraday/ftmac100.c
> > +++ b/drivers/net/ethernet/faraday/ftmac100.c
> > @@ -154,6 +154,7 @@ static void ftmac100_set_mac(struct ftmac100 *priv, const unsigned char *mac)
> >                                FTMAC100_MACCR_CRC_APD | \
> >                                FTMAC100_MACCR_FULLDUP | \
> >                                FTMAC100_MACCR_RX_RUNT | \
> > +                              FTMAC100_MACCR_RX_FTL  | \
> >                                FTMAC100_MACCR_RX_BROADPKT)
> >
> >  static int ftmac100_start_hw(struct ftmac100 *priv)
> > @@ -320,6 +321,7 @@ static bool ftmac100_rx_packet_error(struct ftmac100 *priv,
> >  {
> >       struct net_device *netdev = priv->netdev;
> >       bool error = false;
> > +     const unsigned int length = ftmac100_rxdes_frame_length(rxdes);
>
> Do you need to read this value this early in the function?
> Looks like it is only used when overlong packets are reported.

I decided to make a variable in order to use it twice:
in the condition: "length > 1518"
in logging: "netdev_info(netdev, "rx frame too long (%u)\n", length);"
You are right saying it is not needed in most cases. Can we hope for
the optimizer to postpone the initialization of 'length' till it is
accessed?

Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[Andrew Lunn]

> > > +     const unsigned int length = ftmac100_rxdes_frame_length(rxdes);
> >
> > Do you need to read this value this early in the function?
> > Looks like it is only used when overlong packets are reported.
> 
> I decided to make a variable in order to use it twice:
> in the condition: "length > 1518"
> in logging: "netdev_info(netdev, "rx frame too long (%u)\n", length);"
> You are right saying it is not needed in most cases. Can we hope for
> the optimizer to postpone the initialization of 'length' till it is
> accessed?

Unlikely, since it is accessing a descriptor, and probably using
memory barriers. It is hard for the compiler to differ that until
needed.

But you could look at the .lst file, and it should be pretty obvious
if it has deferred it.

make driver/net/foo/bar.lst

should i think generate what you need.

   Andrew

RE: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[David Laight]

From: Sergei Antonov
> Sent: 12 October 2022 17:43
> 
> On Wed, 12 Oct 2022 at 19:13, David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Sergei Antonov
> > > Sent: 12 October 2022 16:38
> > >
> > > Despite the datasheet [1] saying the controller should allow incoming
> > > packets of length >=1518, it only allows packets of length <=1514.
> >
> > Shouldn't that be <=1518 and <1518 ??
> 
> Oh, thanks for noticing. But still it should be slightly different:
> <= 1518 and <=1514
> Here is my test results of different packet sizes:
> packets of 1518 / 1517 / 1516 / 1515 bytes did not come to the driver
> (before my patch)
> packets of 1514 and less bytes did come

I had to double check the frames sizes, not written an ethernet driver
for nearly 30 years! There is a nice description that is 90% accurate
at https://en.wikipedia.org/wiki/Ethernet_frame

Without an 802.1Q tag (probably a VLAN tag?) the max frame has
1514 data bytes (inc mac addresses, but excl crc).
Unless you are using VLANs that should be the frame limit.
The IP+TCP is limited to the 1500 byte payload.

So if the sender is generating longer packets it is buggy!

...
> > > Since 1518 is a standard Ethernet maximum frame size, and it can
> > > easily be encountered (in SSH for example), fix this behavior:
> > >
> > > * Set FTMAC100_MACCR_RX_FTL in the MAC Control Register.
> >
> > What does that do?
> 
> If FTMAC100_MACCR_RX_FTL is not set:
>   the driver does not receive the "long" packet at all. Looks like the
> controller discards the packet without bothering the driver.

Right so the existing check for the flag being set could never happen.

> If FTMAC100_MACCR_RX_FTL is set:
>   the driver receives the "long" packet marked by the
> FTMAC100_RXDES0_FTL flag. And these packets were discarded by the
> driver (before my patch).
> 
> > Looks like it might cause 'Frame Too Long' packets be returned.
> > In which case should the code just have ignored it since
> > longer frames would be discarded completely??
> 
> Is there such a thing as a response packet which is sent in return to
> FTL packet? Did not know that. My testcases were SSH and SCP programs
> on Ubuntu 22 and they simply hang trying to connect to the ftmac100
> device - no retransmissions or retries with smaller frames happened.

Overlong frames should be discarded.
The sender might choose to do PMTU (path MTU) detection,
but probably doesn't unless a router is involved.

...
> > Do you need to read this value this early in the function?
> > Looks like it is only used when overlong packets are reported.
> 
> I decided to make a variable in order to use it twice:
> in the condition: "length > 1518"
> in logging: "netdev_info(netdev, "rx frame too long (%u)\n", length);"
> You are right saying it is not needed in most cases. Can we hope for
> the optimizer to postpone the initialization of 'length' till it is
> accessed?

Unlikely unless there are no function calls and no volatile
memory accesses.
IMHO just because you can assign a value on the declaration
(of a local) doesn't mean it is a good idea.
Better to move it nearer the use (unless it is used throughout
the function).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[David Laight]

From: Sergei Antonov
> Sent: 12 October 2022 17:43
> 
> On Wed, 12 Oct 2022 at 19:13, David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Sergei Antonov
> > > Sent: 12 October 2022 16:38
> > >
> > > Despite the datasheet [1] saying the controller should allow incoming
> > > packets of length >=1518, it only allows packets of length <=1514.

Actually I bet the datasheet is correct.
The length check is probably done before the CRC is discarded.

...
> > Although traditionally it was 1514+crc.
> > An extra 4 byte header is now allowed.
> > There is also the usefulness of supporting full length frames
> > with a PPPoE header.
> >
> > Whether it actually makes sense to round up the receive buffer
> > size and associated max frame length to 1536 (cache line aligned)
> > is another matter (probably 1534 for 4n+2 alignment).
> >
> > > Since 1518 is a standard Ethernet maximum frame size, and it can
> > > easily be encountered (in SSH for example), fix this behavior:
> > >
> > > * Set FTMAC100_MACCR_RX_FTL in the MAC Control Register.
> >
> > What does that do?
> 
> If FTMAC100_MACCR_RX_FTL is not set:
>   the driver does not receive the "long" packet at all. Looks like the
> controller discards the packet without bothering the driver.
> If FTMAC100_MACCR_RX_FTL is set:
>   the driver receives the "long" packet marked by the
> FTMAC100_RXDES0_FTL flag. And these packets were discarded by the
> driver (before my patch).

There are other problems here.
Where does the extra data actually get written to?
What happens to very long packets?

I'm guessing the hardware has a reasonable interface where
there is a 'ring' of receive buffer descriptors.
If a frame is longer than a buffer the hardware will write
the frame to multiple buffers.

However if each buffer is long enough for a normal frame
and the hardware discards/truncates overlong frames then
the driver can assume there are no continuations.
(I used to use an array of 128 buffers of 512 bytes and
always copy the receive data - a single word aligned copy
unless a long frame crossed the ring end.)

It looks like the FTL bit actually controls whether
overlong frames are discarded or truncated.
(There may be another option to either set the frame
length or disable the feature completely.)

Now you might get away with packets that are only 4 bytes
overlong (one VLAN header) because the hardware always
writes the full received CRC into the buffer.
So when it discards a 1515-1518 frame the extra bytes
are from the frame.
If that is true it deserves a comment.

OTOH if it carries on writing into the rx ring buffer
then it might also 'overflow' into the next ring entry.
Indeed a long enough frame will fill the entire ring
(you'll need a buggy ethernet hub/switch of a 10/100M
HDX network).

You really do need to check whether it detects CRC errors
on overlong frames. If it (mostly) stops processing at
1518 bytes (inc crc) then it may not.
Also if the frame length field is (say) 16 bits then
a 64k+ frame will wrap the counter.
Which means that the frame length may be unreliable
for overlong frames.
(The count might saturate.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[Sergei Antonov]

On Thu, 13 Oct 2022 at 00:41, David Laight <David.Laight@aculab.com> wrote:
>
> From: Sergei Antonov
> > Sent: 12 October 2022 17:43
> >
> > On Wed, 12 Oct 2022 at 19:13, David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Sergei Antonov
> > > > Sent: 12 October 2022 16:38
> > > >
> > > > Despite the datasheet [1] saying the controller should allow incoming
> > > > packets of length >=1518, it only allows packets of length <=1514.
> > >
> > > Shouldn't that be <=1518 and <1518 ??
> >
> > Oh, thanks for noticing. But still it should be slightly different:
> > <= 1518 and <=1514
> > Here is my test results of different packet sizes:
> > packets of 1518 / 1517 / 1516 / 1515 bytes did not come to the driver
> > (before my patch)
> > packets of 1514 and less bytes did come
>
> I had to double check the frames sizes, not written an ethernet driver
> for nearly 30 years! There is a nice description that is 90% accurate
> at https://en.wikipedia.org/wiki/Ethernet_frame
>
> Without an 802.1Q tag (probably a VLAN tag?) the max frame has
> 1514 data bytes (inc mac addresses, but excl crc).
> Unless you are using VLANs that should be the frame limit.
> The IP+TCP is limited to the 1500 byte payload.

Exactly! Incoming packets first go through a switch chip (Marvell
88E6060), so packets should get tagged.

> So if the sender is generating longer packets it is buggy!

Looking into it.

On the sender computer:
sudo ifconfig eno1 mtu 1500 up
ssh receiver_computer

On the receiver computer:
in ftmac100_rx_packet_error() I call
ftmac100_rxdes_frame_length(rxdes) and it returns 1518. I suppose, it
is 1500 + 18(ethernet overhead) + 4(switch tag) - 4(crc).

Would you like me to dump the entire packet and verify?

> > If FTMAC100_MACCR_RX_FTL is set:
> >   the driver receives the "long" packet marked by the
> > FTMAC100_RXDES0_FTL flag. And these packets were discarded by the
> > driver (before my patch).
> >
> > > Looks like it might cause 'Frame Too Long' packets be returned.
> > > In which case should the code just have ignored it since
> > > longer frames would be discarded completely??
> >
> > Is there such a thing as a response packet which is sent in return to
> > FTL packet? Did not know that. My testcases were SSH and SCP programs
> > on Ubuntu 22 and they simply hang trying to connect to the ftmac100
> > device - no retransmissions or retries with smaller frames happened.
>
> Overlong frames should be discarded.
> The sender might choose to do PMTU (path MTU) detection,
> but probably doesn't unless a router is involved.

I am afraid the developers of ftmac100 controller did not take into
account the possibility of VAN tagging. So my patch is an attempt to
solve the issue. However, I am now doubtful about it. After my patch
the driver will not be correct for the case without a switch. Should I
instead of simple checking for "length > 1518" check a packet for VLAN
tag presence and then, depending on the result, for "length > 1514" or
"length > 1518"?

> ...
> > > Do you need to read this value this early in the function?
> > > Looks like it is only used when overlong packets are reported.
> >
> > I decided to make a variable in order to use it twice:
> > in the condition: "length > 1518"
> > in logging: "netdev_info(netdev, "rx frame too long (%u)\n", length);"
> > You are right saying it is not needed in most cases. Can we hope for
> > the optimizer to postpone the initialization of 'length' till it is
> > accessed?
>
> Unlikely unless there are no function calls and no volatile
> memory accesses.
> IMHO just because you can assign a value on the declaration
> (of a local) doesn't mean it is a good idea.
> Better to move it nearer the use (unless it is used throughout
> the function).

OK. I will rewrite this part in future versions of the patch.

Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[Andrew Lunn]

> > Without an 802.1Q tag (probably a VLAN tag?) the max frame has
> > 1514 data bytes (inc mac addresses, but excl crc).
> > Unless you are using VLANs that should be the frame limit.
> > The IP+TCP is limited to the 1500 byte payload.
> 
> Exactly! Incoming packets first go through a switch chip (Marvell
> 88E6060), so packets should get tagged.
> 
> > So if the sender is generating longer packets it is buggy!
> 
> Looking into it.
> 
> On the sender computer:
> sudo ifconfig eno1 mtu 1500 up
> ssh receiver_computer
> 
> On the receiver computer:
> in ftmac100_rx_packet_error() I call
> ftmac100_rxdes_frame_length(rxdes) and it returns 1518. I suppose, it
> is 1500 + 18(ethernet overhead) + 4(switch tag) - 4(crc).
> 
> Would you like me to dump the entire packet and verify?

You did not mention DSA before. That is an important fact.

What should happen is that the DSA framework should take the DSA frame
header into account. It should be calling into the MAC driver and
asking it to change its MTU to allow for the additional 4 bytes of the
switch tag.

But there is some history here. For a long time, it was just assumed
the MAC driver would accept any length of packet, i.e. the MRU,
Maximum Receive Unit, was big enough for DSA to just work. A Marvell
switch is normally combined with a Marvell MAC, and this was
true. This worked for a long time, until it did not work for some
combination of switch and MAC. It then became necessary to change the
MTU on the master interface, so it would actually receive the bigger
frames. But we had the backwards compatibility problem, that some MAC
drivers which work with DSA because there MRU is sufficient, don't
support changing the MTU. So we could not make it fatal if the change
of the MTU failed.

Does this driver support the MTU change op? If not, you should try
implementing it. If the hardware cannot actually receive longer
packets, you need to take the opposite approach. You need to make the
MTU on the slave interfaces smaller, so that packets from the switch
to the master interface are small enough to be correctly received when
including the switch header.

	  Andrew

Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[Vladimir Oltean]

On Thu, Oct 13, 2022 at 01:29:00PM +0300, Sergei Antonov wrote:
> On Thu, 13 Oct 2022 at 00:41, David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Sergei Antonov
> > > Sent: 12 October 2022 17:43
> > >
> > > On Wed, 12 Oct 2022 at 19:13, David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Sergei Antonov
> > > > > Sent: 12 October 2022 16:38
> > > > >
> > > > > Despite the datasheet [1] saying the controller should allow incoming
> > > > > packets of length >=1518, it only allows packets of length <=1514.
> > > >
> > > > Shouldn't that be <=1518 and <1518 ??
> > >
> > > Oh, thanks for noticing. But still it should be slightly different:
> > > <= 1518 and <=1514
> > > Here is my test results of different packet sizes:
> > > packets of 1518 / 1517 / 1516 / 1515 bytes did not come to the driver
> > > (before my patch)
> > > packets of 1514 and less bytes did come
> >
> > I had to double check the frames sizes, not written an ethernet driver
> > for nearly 30 years! There is a nice description that is 90% accurate
> > at https://en.wikipedia.org/wiki/Ethernet_frame
> >
> > Without an 802.1Q tag (probably a VLAN tag?) the max frame has
> > 1514 data bytes (inc mac addresses, but excl crc).
> > Unless you are using VLANs that should be the frame limit.
> > The IP+TCP is limited to the 1500 byte payload.
> 
> Exactly! Incoming packets first go through a switch chip (Marvell
> 88E6060), so packets should get tagged.

Well, this is the first time you mention of any switch DSA tag.

To my knowledge, what Linux understands by MTU is the maximum size of
the payload (SDU) accepted by the 802.3 MAC or 802.1Q layer. So a MTU of
1500 should allow a frame size, with Ethernet header and FCS, of 1518
octets, or optionally 1522 octets in case it is also VLAN-tagged.

A DSA header, or trailer, is part of the L2 payload, so DSA automatically
tries to increase the MTU on the DSA master to make sure that MTU-sized
frames going through the switch interface don't exceed the MTU of the
master when an extra tag gets inserted.

The only bug in the ftmac100 driver is that it reports netdev->max_mtu
of MAX_PKT_SIZE (1518) instead of 1500. It does *not* support this MTU,
it only supports 1500.

Had it properly reported 1500 as max MTU, you'd have seen that DSA tries
to call dev_set_mtu() on the ftmac100, to account for its tag length.
The driver does not implement ndo_change_mtu, only blindly says that
everything up to 1518 L2 payload length is accepted, and DSA thinks
everything is ok (1500 + 4 = 1504, still less than 1518).

Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[Sergei Antonov]

On Thu, 13 Oct 2022 at 07:24, David Laight <David.Laight@aculab.com> wrote:
>
> From: Sergei Antonov
> > Sent: 12 October 2022 17:43
> >
> > On Wed, 12 Oct 2022 at 19:13, David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Sergei Antonov
> > > > Sent: 12 October 2022 16:38
> > > >
> > > > Despite the datasheet [1] saying the controller should allow incoming
> > > > packets of length >=1518, it only allows packets of length <=1514.
>
> Actually I bet the datasheet is correct.
> The length check is probably done before the CRC is discarded.

Thanks for clarification.
I will make a v3 version of the patch without datasheet blaming and
using the approach with MTU suggested in this thread.

Re: [PATCH v2 net] net: ftmac100: do not reject packets bigger than 1514

[Sergei Antonov]

On Thu, 13 Oct 2022 at 17:10, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > Without an 802.1Q tag (probably a VLAN tag?) the max frame has
> > > 1514 data bytes (inc mac addresses, but excl crc).
> > > Unless you are using VLANs that should be the frame limit.
> > > The IP+TCP is limited to the 1500 byte payload.
> >
> > Exactly! Incoming packets first go through a switch chip (Marvell
> > 88E6060), so packets should get tagged.
> >
> > > So if the sender is generating longer packets it is buggy!
> >
> > Looking into it.
> >
> > On the sender computer:
> > sudo ifconfig eno1 mtu 1500 up
> > ssh receiver_computer
> >
> > On the receiver computer:
> > in ftmac100_rx_packet_error() I call
> > ftmac100_rxdes_frame_length(rxdes) and it returns 1518. I suppose, it
> > is 1500 + 18(ethernet overhead) + 4(switch tag) - 4(crc).
> >
> > Would you like me to dump the entire packet and verify?
>
> You did not mention DSA before. That is an important fact.
>
> What should happen is that the DSA framework should take the DSA frame
> header into account. It should be calling into the MAC driver and
> asking it to change its MTU to allow for the additional 4 bytes of the
> switch tag.
>
> But there is some history here. For a long time, it was just assumed
> the MAC driver would accept any length of packet, i.e. the MRU,
> Maximum Receive Unit, was big enough for DSA to just work. A Marvell
> switch is normally combined with a Marvell MAC, and this was
> true. This worked for a long time, until it did not work for some
> combination of switch and MAC. It then became necessary to change the
> MTU on the master interface, so it would actually receive the bigger
> frames. But we had the backwards compatibility problem, that some MAC
> drivers which work with DSA because there MRU is sufficient, don't
> support changing the MTU. So we could not make it fatal if the change
> of the MTU failed.
>
> Does this driver support the MTU change op? If not, you should try
> implementing it.

Well, the ftmac100 driver does not implement _change_mtu() function,
but netdev->mtu is correctly set to 1504 because of DSA. So I am
submitting a v3 version of the patch using netdev->mtuft. Please, have
a look at it.