| Message ID | 20221012153737.128424-1-saproj@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: ftmac100: do not reject packets bigger than 1514 | expand |
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)
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?
> > > + 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
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)
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)
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.
> > 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
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).
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.
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.
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;
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(-)