| Message ID | 20181207174340.10839-1-marex@denx.de |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: dsa: ksz: Increase the tag alignment | expand |
> - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; > + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb- > >len; The requirement is the tail tag should be at the end of frame before FCS. When the length is less than 60 the MAC controller will pad the frame to legal size. That is why this function makes sure the padding is done manually. Increasing the size just increases the length of the frame and the chance to allocate new socket buffer. Example of using ping size of 18 will have the sizes of request and response differ by 4 bytes. Not that it matters much. Are you concerned the MAC controller will remove the VLAN tag and so the frame will not be sent? Or the switch removes the VLAN tag and is not able to send?
On 12/08/2018 01:52 AM, Tristram.Ha@microchip.com wrote: >> - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; >> + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb- >>> len; > > The requirement is the tail tag should be at the end of frame before FCS. > When the length is less than 60 the MAC controller will pad the frame to > legal size. That is why this function makes sure the padding is done > manually. Increasing the size just increases the length of the frame and the > chance to allocate new socket buffer. > > Example of using ping size of 18 will have the sizes of request and response > differ by 4 bytes. Not that it matters much. > > Are you concerned the MAC controller will remove the VLAN tag and so the frame > will not be sent? Or the switch removes the VLAN tag and is not able to send? With TI CPSW in dual-ethernet configuration, which adds internal VLAN tag at the end of the frame, the KSZ switch fails. The CPU will send out packets and the switch will reject them as corrupted. It needs this extra VLAN tag padding.
Le 12/7/18 à 8:25 PM, Marek Vasut a écrit : > On 12/08/2018 01:52 AM, Tristram.Ha@microchip.com wrote: >>> - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; >>> + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb- >>>> len; >> >> The requirement is the tail tag should be at the end of frame before FCS. >> When the length is less than 60 the MAC controller will pad the frame to >> legal size. That is why this function makes sure the padding is done >> manually. Increasing the size just increases the length of the frame and the >> chance to allocate new socket buffer. >> >> Example of using ping size of 18 will have the sizes of request and response >> differ by 4 bytes. Not that it matters much. >> >> Are you concerned the MAC controller will remove the VLAN tag and so the frame >> will not be sent? Or the switch removes the VLAN tag and is not able to send? > > With TI CPSW in dual-ethernet configuration, which adds internal VLAN > tag at the end of the frame, the KSZ switch fails. The CPU will send out > packets and the switch will reject them as corrupted. It needs this > extra VLAN tag padding. Oh so they add the internal VLAN at the end of the frame, not the beginning? That is quite surprising but that would not be the one single oddity found with CPSW I am afraid.. The way I would approach this is with layering where the padding needs to occur: - within the tag driver you need to make sure there is enough room to insert the KSZ tag - within the ethernet MAC driver (which comes last in the transmit path), you need to make sure there is enough room to insert that trailer VLAN tag to permit internal transmission
On 12/08/2018 06:35 PM, Florian Fainelli wrote: > Le 12/7/18 à 8:25 PM, Marek Vasut a écrit : >> On 12/08/2018 01:52 AM, Tristram.Ha@microchip.com wrote: >>>> - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; >>>> + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb- >>>>> len; >>> >>> The requirement is the tail tag should be at the end of frame before FCS. >>> When the length is less than 60 the MAC controller will pad the frame to >>> legal size. That is why this function makes sure the padding is done >>> manually. Increasing the size just increases the length of the frame and the >>> chance to allocate new socket buffer. >>> >>> Example of using ping size of 18 will have the sizes of request and response >>> differ by 4 bytes. Not that it matters much. >>> >>> Are you concerned the MAC controller will remove the VLAN tag and so the frame >>> will not be sent? Or the switch removes the VLAN tag and is not able to send? >> >> With TI CPSW in dual-ethernet configuration, which adds internal VLAN >> tag at the end of the frame, the KSZ switch fails. The CPU will send out >> packets and the switch will reject them as corrupted. It needs this >> extra VLAN tag padding. > > Oh so they add the internal VLAN at the end of the frame, not the > beginning? That is quite surprising but that would not be the one single > oddity found with CPSW I am afraid.. The way I would approach this is > with layering where the padding needs to occur: > > - within the tag driver you need to make sure there is enough room to > insert the KSZ tag > > - within the ethernet MAC driver (which comes last in the transmit > path), you need to make sure there is enough room to insert that trailer > VLAN tag to permit internal transmission So you think this is a bug in the CPSW instead ?
> >>>> - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; > >>>> + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb- > >>>>> len; > > Oh so they add the internal VLAN at the end of the frame, not the > > beginning? That is quite surprising but that would not be the one single > > oddity found with CPSW I am afraid.. The way I would approach this is > > with layering where the padding needs to occur: > > > > - within the tag driver you need to make sure there is enough room to > > insert the KSZ tag > > > > - within the ethernet MAC driver (which comes last in the transmit > > path), you need to make sure there is enough room to insert that trailer > > VLAN tag to permit internal transmission > > So you think this is a bug in the CPSW instead ? > I think what causes this problem. In the MAC controller driver cpsw.c the buffer is always padded to CPSW_MIN_PACKET_SIZE. Normally that size is 60 bytes, but after Linux 4.14 kernel it was changed to VLAN_ETH_ZLEN. The original size should work, but I do not know why it was changed. It seems there is a new function using the CPSW_RX_VLAN_ENCAP bit. It is similar to what I experienced with the Atmel MAC driver. The newer kernels added some changes that introduced a bug and broke the tail tagging code. I had to submit a fix to the MAC driver to correct that. I do not think this patch should apply generally, but I do not know how to fix the MAC driver to work in all cases. You can try temporarily to change CPSW_MIN_PACKET_SIZE back to 60. It is only used to assign to min_packet_size. It may be possible to use a different size like 60 instead of 64 in the skb_padto function.
On 12/10/2018 09:04 PM, Tristram.Ha@microchip.com wrote: >>>>>> - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; >>>>>> + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb- >>>>>>> len; >>> Oh so they add the internal VLAN at the end of the frame, not the >>> beginning? That is quite surprising but that would not be the one single >>> oddity found with CPSW I am afraid.. The way I would approach this is >>> with layering where the padding needs to occur: >>> >>> - within the tag driver you need to make sure there is enough room to >>> insert the KSZ tag >>> >>> - within the ethernet MAC driver (which comes last in the transmit >>> path), you need to make sure there is enough room to insert that trailer >>> VLAN tag to permit internal transmission >> >> So you think this is a bug in the CPSW instead ? >> > > I think what causes this problem. In the MAC controller driver cpsw.c > the buffer is always padded to CPSW_MIN_PACKET_SIZE. Normally that > size is 60 bytes, but after Linux 4.14 kernel it was changed to VLAN_ETH_ZLEN. > The original size should work, but I do not know why it was changed. It seems > there is a new function using the CPSW_RX_VLAN_ENCAP bit. > > It is similar to what I experienced with the Atmel MAC driver. The newer kernels > added some changes that introduced a bug and broke the tail tagging code. I had > to submit a fix to the MAC driver to correct that. > > I do not think this patch should apply generally, but I do not know how to fix the > MAC driver to work in all cases. > > You can try temporarily to change CPSW_MIN_PACKET_SIZE back to 60. > > It is only used to assign to min_packet_size. It may be possible to use a different > size like 60 instead of 64 in the skb_padto function. I am not looking for a hack-around, I am looking for a proper solution.
On 12/10/18 12:04 PM, Tristram.Ha@microchip.com wrote: >>>>>> - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; >>>>>> + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb- >>>>>>> len; >>> Oh so they add the internal VLAN at the end of the frame, not the >>> beginning? That is quite surprising but that would not be the one single >>> oddity found with CPSW I am afraid.. The way I would approach this is >>> with layering where the padding needs to occur: >>> >>> - within the tag driver you need to make sure there is enough room to >>> insert the KSZ tag >>> >>> - within the ethernet MAC driver (which comes last in the transmit >>> path), you need to make sure there is enough room to insert that trailer >>> VLAN tag to permit internal transmission >> >> So you think this is a bug in the CPSW instead ? >> > > I think what causes this problem. In the MAC controller driver cpsw.c > the buffer is always padded to CPSW_MIN_PACKET_SIZE. Normally that > size is 60 bytes, but after Linux 4.14 kernel it was changed to VLAN_ETH_ZLEN. > The original size should work, but I do not know why it was changed. It seems > there is a new function using the CPSW_RX_VLAN_ENCAP bit. > > It is similar to what I experienced with the Atmel MAC driver. The newer kernels > added some changes that introduced a bug and broke the tail tagging code. I had > to submit a fix to the MAC driver to correct that. That is presumably this commit that you are referring to: a52ad514fdf3b8a57ca4322c92d2d8d5c6182485 ("net: deprecate eth_change_mtu, remove usage")? > > I do not think this patch should apply generally, but I do not know how to fix the > MAC driver to work in all cases. If you find yourself fixing all possible MAC controllers to work with the trailer code then this does not scale, and clearly we must find a better way to advertise what minimum frame size is required, as well as possible carry some information about how big that trailer tag must be. Andrew recently did that by having the DSA code call back into the Ethernet MAC driver to re-configure the MTU to account for how many bytes the tagging code is going to need. Keep in mind that one of the design paradigm of DSA is that because you are typically dealing with external/discrete chips *ANY* Ethernet MAC controller must be working with the trailer/header/tagging code. MAC controllers typically require bytes somewhere at the front of the packet (usually) to insert controller specific metadata, so eventually the MAC driver is the best place to make sure the packet can successfully be transmitted. The main issue I see with trailer tags here is that if you have multiple of them, despite knowing in which order they are going to be appended to the frame, there is not enough metadata with the SKB to know where the next level should be inserting the trailer, in that case, CPSW does not know where to insert it because the KSZ trailer is already there and skb->len has already reflected that increase in lengths. Maybe you need to make use of skb->cb[] in that case to communicate where the trailer was inserted? > > You can try temporarily to change CPSW_MIN_PACKET_SIZE back to 60. > > It is only used to assign to min_packet_size. It may be possible to use a different > size like 60 instead of 64 in the skb_padto function. > An argument could be made that the stack should be passing up 60 bytes (sans FCS) frames to the Ethernet driver already, instead of having each Ethernet MAC controller have to decide how to manage RUNT packets, but there are many code paths within the stack (including things like raw sockets) that would be hard to fix. Also, each Ethernet MAC driver being different, some are able to do HW padding of packets with 0, while some require software to do it. Anyway....
> I am not looking for a hack-around, I am looking for a proper solution. > As you have the hardware you can try something to correct the problem. The change was introduced in commit 9421c9015047 on Nov 15. In the net-next tree it is about -42 commits. git log -42 cpsw.c git diff f5b589488ea5 cpsw.c shows the changes. As explained in the commit log this corrects the problem of sending a tagged frame and the VLAN tag is removed by the hardware. I do not think the implementation is 100% correct but maybe it is too much to check for VLAN tag every time a frame is sent. The change assumes a VLAN tag is always there and it may be dropped. The MAC controller uses VLAN for its dual MAC function, but it is added automatically and not done at system level. If the VLAN tag is removed all the time it is pointless to put it in the first place by software. The connected switch also has some functions to manipulate the VLAN tag. Are you actively using the second Ethernet device which is not connected to the switch and is using VLAN? There should be a way to use device tree to specify which function to enable.
On 12/10/2018 10:49 PM, Tristram.Ha@microchip.com wrote: >> I am not looking for a hack-around, I am looking for a proper solution. >> > > As you have the hardware you can try something to correct the problem. > > The change was introduced in commit 9421c9015047 on Nov 15. In the > net-next tree it is about -42 commits. > > git log -42 cpsw.c > > git diff f5b589488ea5 cpsw.c shows the changes. > > As explained in the commit log this corrects the problem of sending a > tagged frame and the VLAN tag is removed by the hardware. > > I do not think the implementation is 100% correct but maybe it is too > much to check for VLAN tag every time a frame is sent. The change assumes > a VLAN tag is always there and it may be dropped. > > The MAC controller uses VLAN for its dual MAC function, but it is added > automatically and not done at system level. If the VLAN tag is removed all > the time it is pointless to put it in the first place by software. The connected > switch also has some functions to manipulate the VLAN tag. > > Are you actively using the second Ethernet device which is not connected to > the switch and is using VLAN? Yes, I'm using it in dual-ethernet configuration, so I am using both ethernets. > There should be a way to use device tree to specify which function to enable. >
On 12/07/2018 06:43 PM, Marek Vasut wrote: > Make sure to cater even for network packets with VLAN tags in them, > increase the minimal packets size to account for those. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com> > Cc: Woojung Huh <woojung.huh@microchip.com> > Cc: David S. Miller <davem@davemloft.net> > Cc: Tristram Ha <Tristram.Ha@microchip.com> > --- > net/dsa/tag_ksz.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c > index 96411f70ab9f4..cad4406d9d4c2 100644 > --- a/net/dsa/tag_ksz.c > +++ b/net/dsa/tag_ksz.c > @@ -39,7 +39,7 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev) > int padlen; > u8 *tag; > > - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; > + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb->len; > > if (skb_tailroom(skb) >= padlen + KSZ_INGRESS_TAG_LEN) { > /* Let dsa_slave_xmit() free skb */ > I was looking at this function again after some suggestions by Florian on IRC to use MIN_MTU+VLAN_HLEN , and I wonder , cannot this whole ksz_xmit() function be replaced by skb_padto(skb, skb->len + length of the tag) instead of all this complex code ?
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c index 96411f70ab9f4..cad4406d9d4c2 100644 --- a/net/dsa/tag_ksz.c +++ b/net/dsa/tag_ksz.c @@ -39,7 +39,7 @@ static struct sk_buff *ksz_xmit(struct sk_buff *skb, struct net_device *dev) int padlen; u8 *tag; - padlen = (skb->len >= ETH_ZLEN) ? 0 : ETH_ZLEN - skb->len; + padlen = (skb->len >= VLAN_ETH_ZLEN) ? 0 : VLAN_ETH_ZLEN - skb->len; if (skb_tailroom(skb) >= padlen + KSZ_INGRESS_TAG_LEN) { /* Let dsa_slave_xmit() free skb */
Make sure to cater even for network packets with VLAN tags in them, increase the minimal packets size to account for those. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com> Cc: Woojung Huh <woojung.huh@microchip.com> Cc: David S. Miller <davem@davemloft.net> Cc: Tristram Ha <Tristram.Ha@microchip.com> --- net/dsa/tag_ksz.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)