[PATCH] net: dsa: ksz: Increase the tag alignment

[PATCH] net: dsa: ksz: Increase the tag alignment

[Marek Vasut]

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 */
-- 
2.18.0

RE: [PATCH] net: dsa: ksz: Increase the tag alignment

[Tristram.Ha]

> -	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?

Re: [PATCH] net: dsa: ksz: Increase the tag alignment

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: dsa: ksz: Increase the tag alignment

[Florian Fainelli]

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
-- 
Florian

Re: [PATCH] net: dsa: ksz: Increase the tag alignment

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

RE: [PATCH] net: dsa: ksz: Increase the tag alignment

[Tristram.Ha]

> >>>> -	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.

Re: [PATCH] net: dsa: ksz: Increase the tag alignment

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

RE: [PATCH] net: dsa: ksz: Increase the tag alignment

[Tristram.Ha]

> 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.

Re: [PATCH] net: dsa: ksz: Increase the tag alignment

[Marek Vasut]

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.
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH] net: dsa: ksz: Increase the tag alignment

[Florian Fainelli]

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....
-- 
Florian

Re: [PATCH] net: dsa: ksz: Increase the tag alignment

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut