| Message ID | 20221013155724.2911050-1-saproj@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,net] net: ftmac100: support frames with DSA tag | expand |
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.
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.
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.
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?
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.
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;
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(-)