[PATCH v3 net] net: ftmac100: support frames with DSA tag

[PATCH v3 net] net: ftmac100: support frames with DSA tag

[Sergei Antonov]

Fixes the problem when frames coming from DSA were discarded.
DSA tag might make frame size >1518. Such frames are discarded
by the controller when FTMAC100_MACCR_RX_FTL is not set in the
MAC Control Register, see datasheet [1].

Set FTMAC100_MACCR_RX_FTL 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. In the presence of DSA netdev->mtu is 1504 to accommodate
for VLAN tag.

[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>
Suggested-by: Andrew Lunn <andrew@lunn.ch>
---

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

v1 -> v2:
* Typos in description fixed.

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

diff --git a/drivers/net/ethernet/faraday/ftmac100.c b/drivers/net/ethernet/faraday/ftmac100.c
index d95d78230828..9187331e83dc 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)
@@ -337,9 +338,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 v3 net] net: ftmac100: support frames with DSA tag

[Vladimir Oltean]

On Thu, Oct 13, 2022 at 06:57:24PM +0300, Sergei Antonov wrote:
> Fixes the problem when frames coming from DSA were discarded.
> DSA tag might make frame size >1518. Such frames are discarded
> by the controller when FTMAC100_MACCR_RX_FTL is not set in the
> MAC Control Register, see datasheet [1].
> 
> Set FTMAC100_MACCR_RX_FTL 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. In the presence of DSA netdev->mtu is 1504 to accommodate
> for VLAN tag.

Which VLAN tag do you mean? Earlier you mentioned a tail tag as can be
seen and parsed by net/dsa/tag_trailer.c. Don't conflate the two, one
has nothing to do with the other.

I think this patch is at the limit of what can reasonably be considered a
bug fix, especially since inefficient use of resources does not constitute
a bug in itself. And the justification provided in the commit message
certainly does not help its cause.

DSA generally works on the assumption that netdev drivers need no change
to operate as DSA masters. However, in this particular case, it has
historically operated using the "wishful thinking" assumption that
dev_set_mtu(master, 1504) will always be enough to work, even if this
call fails (hence the reason for making its failure non-fatal).

More context (to supplement Andrew's message from Message-ID Y0gcblXFum4GsSve@lunn.ch:
https://patchwork.ozlabs.org/project/netdev/patch/20200421123110.13733-2-olteanv@gmail.com/

My humble opinion is that "reasonable and noninvasive changes"
for drivers to work as DSA masters is a more desirable goal, and hence,
not every master <-> switch pair that doesn't work out of the box should
be considered a bug.

Here's how I would approach your particular issue:

1. retarget from "net" to "net-next"

2. observe the ftmac100 code. The RX_BUF_SIZE macro is set to 2044, with a
   comment that it must be smaller than 0x7ff.

3. compare with the datasheet. There it can be seen that RXBUF_SIZE is
   an 11-bit field, confirming that packets larger than 2048 bytes must
   be processed as scatter/gather (multiple BDs).

4. back to the code, the driver does not support scatter gather processing,
   but it does not push the MTU limit as far as single-buffer RX can go, either.
   It puts it to a quite random 1518, inconsistent in itself to the FTL
   field which drops frames with MTU larger than 1500 (so dev->mtu values between
   1500 and 1518 are incorrectly handled).
   It could go as far as 2047 - VLAN_ETH_HLEN, and the Frame Too Long
   bit should be unset to allow this.

5. the code currently relies on the FTL field to not trigger the BUG_ON()
   right below, if scatter/gather frames are received. But the FTL bit
   needs to go. I would completely remove the ftmac100_rxdes_frame_too_long()
   check from the fast path, instead of your approach to just make it
   more complicated. If you simply call ftmac100_rx_drop_packet() instead
   of BUG_ON() when receiving a BD which is non-final, you get a simpler
   way of protecting against multi-buffer frames, while not having to
   rely on the comparison between frame length and netdev->mtu at all.
   (side note: it isn't a problem if you receive larger frames than the
   MTU, as long as you receive the frames <= than the MTU).

6. Actually implement the ndo_change_mtu() for this driver, and even though you
   don't make any hardware change based on the precise value, you could do one
   important thing. Depending on whether the dev->mtu is <= 1500 or not, you could
   set or unset the FTL bit, and this could allow for less CPU cycles spent
   dropping S/G frames when the MTU of the interface is standard 1500.

With these changes, you would leave the ftmac100 driver in a more civilized
state, you wouldn't need to implement S/G RX unless you wanted to, and
as a side effect, it would also operate well as a DSA master.

Re: [PATCH v3 net] net: ftmac100: support frames with DSA tag

[Sergei Antonov]

On Mon, 17 Oct 2022 at 18:55, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Thu, Oct 13, 2022 at 06:57:24PM +0300, Sergei Antonov wrote:
> > Fixes the problem when frames coming from DSA were discarded.
> > DSA tag might make frame size >1518. Such frames are discarded
> > by the controller when FTMAC100_MACCR_RX_FTL is not set in the
> > MAC Control Register, see datasheet [1].
> >
> > Set FTMAC100_MACCR_RX_FTL 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. In the presence of DSA netdev->mtu is 1504 to accommodate
> > for VLAN tag.
>
> Which VLAN tag do you mean? Earlier you mentioned a tail tag as can be
> seen and parsed by net/dsa/tag_trailer.c. Don't conflate the two, one
> has nothing to do with the other.

I used print_hex_dump_bytes() in the ftmac100 driver to see what is
inside the received packet. Here is how the 1518 byte long packet
looks:
6 bytes - dst MAC
6 bytes - src MAC
4 bytes - 08 00 45 10
2 bytes - 0x5dc - data length (1500)
1500 bytes - data

I am not sure what is the proper name for these 08 00 45 10 bytes.
Tell me the correct name to use in the next version of the patch :).
Thanks for your feedback. I will make an improved version of the patch
targeted for net-next.

Re: [PATCH v3 net] net: ftmac100: support frames with DSA tag

[Vladimir Oltean]

On Wed, Oct 19, 2022 at 04:57:05PM +0300, Sergei Antonov wrote:
> > Which VLAN tag do you mean? Earlier you mentioned a tail tag as can be
> > seen and parsed by net/dsa/tag_trailer.c. Don't conflate the two, one
> > has nothing to do with the other.
> 
> I used print_hex_dump_bytes() in the ftmac100 driver to see what is
> inside the received packet. Here is how the 1518 byte long packet
> looks:
> 6 bytes - dst MAC
> 6 bytes - src MAC
> 4 bytes - 08 00 45 10
> 2 bytes - 0x5dc - data length (1500)
> 1500 bytes - data
> 
> I am not sure what is the proper name for these 08 00 45 10 bytes.
> Tell me the correct name to use in the next version of the patch :).

Humm, how about IPv4 EtherType (08 00) plus the first 2 bytes from the
IPv4 header? 802.1Q EtherType is 0x8100.

Re: [PATCH v3 net] net: ftmac100: support frames with DSA tag

[Sergei Antonov]

On Mon, 17 Oct 2022 at 18:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> 6. Actually implement the ndo_change_mtu() for this driver, and even though you
>    don't make any hardware change based on the precise value, you could do one
>    important thing. Depending on whether the dev->mtu is <= 1500 or not, you could
>    set or unset the FTL bit, and this could allow for less CPU cycles spent
>    dropping S/G frames when the MTU of the interface is standard 1500.

Can I do the mtu>1500 check and set FTL in ndo_open() (which is called
after ndo_change_mtu())? I am submitting a v4 of the patch, and your
feedback to it would be helpful. I got rid of all mentions of DSA
tagging there, so patch description and a code comment are new. Thanks
for clearing things up regarding EtherType 0800.

> With these changes, you would leave the ftmac100 driver in a more civilized
> state, you wouldn't need to implement S/G RX unless you wanted to, and
> as a side effect, it would also operate well as a DSA master.

What does S/G stand for?

Re: [PATCH v3 net] net: ftmac100: support frames with DSA tag

[Vladimir Oltean]

On Wed, Oct 19, 2022 at 07:19:11PM +0300, Sergei Antonov wrote:
> On Mon, 17 Oct 2022 at 18:55, Vladimir Oltean <olteanv@gmail.com> wrote:
> > 6. Actually implement the ndo_change_mtu() for this driver, and even though you
> >    don't make any hardware change based on the precise value, you could do one
> >    important thing. Depending on whether the dev->mtu is <= 1500 or not, you could
> >    set or unset the FTL bit, and this could allow for less CPU cycles spent
> >    dropping S/G frames when the MTU of the interface is standard 1500.
> 
> Can I do the mtu>1500 check and set FTL in ndo_open() (which is called
> after ndo_change_mtu())?

Through which code path is ndo_open called after ndo_change_mtu()?
To my knowledge they are independent. The MTU can be changed even on an
interface which is up, and the driver needs to handle this.

> I am submitting a v4 of the patch, and your feedback to it would be
> helpful.

Well, I see you posted v4 within 2 minutes of requesting further
feedback....

> I got rid of all mentions of DSA tagging there, so patch description
> and a code comment are new. Thanks for clearing things up regarding
> EtherType 0800.

I didn't ask to get rid of all mentions of DSA tagging. On the contrary,
it is relevant to mention that DSA is the reason you are making these
changes. The changes would have looked quite different if you needed to
increase the MTU for other reasons (also see below).

> > With these changes, you would leave the ftmac100 driver in a more civilized
> > state, you wouldn't need to implement S/G RX unless you wanted to, and
> > as a side effect, it would also operate well as a DSA master.
> 
> What does S/G stand for?

S/G means scatter/gather, or in other words, the ability to create an
skb from multiple RX buffer descriptors. Since each BD is limited to 2K,
this limits the size of the skb also to 2K, whereas a scatter/gather skb
is essentially unlimited in size regardless of the length of individual
buffers.

What I was saying is that for DSA on mv88e6060 (which doesn't support
jumbo frames AFAIK) you're essentially still fine within the confines of
single buffer skb, you just need to allow MTU values just a little bit
higher than 1500, but not as high as, say, 9k, which would require more
effort/rework.