[U-Boot] [PATCH 1/2] net: designware: fix tx packet length

[U-Boot] [PATCH 1/2] net: designware: fix tx packet length

[Simon Goldschmidt]

The designware driver has a bug in setting the tx length into the dma
descriptor: it always or's the length into the descriptor without
zeroing out the length mask before.

This results in occasional packets being transmitted with a length
greater than they should be (trailer). Due to the nature of Ethernet
allowing such a trailer, most packets seem to be parsed fine by remote
hosts, which is probably why this hasn't been noticed.

Fix this by correctly clearing the size mask before setting the new
length.

Tested on socfpga gen5.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 drivers/net/designware.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 19db0a8114..688cf9fef2 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -389,15 +389,17 @@ static int _dw_eth_send(struct dw_eth_dev *priv, void *packet, int length)
 
 #if defined(CONFIG_DW_ALTDESCRIPTOR)
 	desc_p->txrx_status |= DESC_TXSTS_TXFIRST | DESC_TXSTS_TXLAST;
-	desc_p->dmamac_cntl |= (length << DESC_TXCTRL_SIZE1SHFT) &
-			       DESC_TXCTRL_SIZE1MASK;
+	desc_p->dmamac_cntl = (desc_p->dmamac_cntl & ~DESC_TXCTRL_SIZE1MASK) |
+			      ((length << DESC_TXCTRL_SIZE1SHFT) &
+			      DESC_TXCTRL_SIZE1MASK);
 
 	desc_p->txrx_status &= ~(DESC_TXSTS_MSK);
 	desc_p->txrx_status |= DESC_TXSTS_OWNBYDMA;
 #else
-	desc_p->dmamac_cntl |= ((length << DESC_TXCTRL_SIZE1SHFT) &
-			       DESC_TXCTRL_SIZE1MASK) | DESC_TXCTRL_TXLAST |
-			       DESC_TXCTRL_TXFIRST;
+	desc_p->dmamac_cntl = (desc_p->dmamac_cntl & ~DESC_TXCTRL_SIZE1MASK) |
+			      ((length << DESC_TXCTRL_SIZE1SHFT) &
+			      DESC_TXCTRL_SIZE1MASK) | DESC_TXCTRL_TXLAST |
+			      DESC_TXCTRL_TXFIRST;
 
 	desc_p->txrx_status = DESC_TXSTS_OWNBYDMA;
 #endif
-- 
2.17.1

[U-Boot] [PATCH 2/2] net: designware: clear padding bytes

[Simon Goldschmidt]

Short frames are padded to the minimum allowed size of 60 bytes.
However, the designware driver sends old data in these padding bytes.
It is common practice to zero out these padding bytes ro prevent
leaking memory contents to other hosts.

Fix the padding code to zero out the padded bytes at the end.

Tested on socfpga gen5.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 drivers/net/designware.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 688cf9fef2..33463de0f8 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -380,9 +380,11 @@ static int _dw_eth_send(struct dw_eth_dev *priv, void *packet, int length)
 		return -EPERM;
 	}
 
-	length = max(length, ETH_ZLEN);
-
 	memcpy((void *)data_start, packet, length);
+	if (length < ETH_ZLEN) {
+		memset(&((char *)data_start)[length], 0, ETH_ZLEN - length);
+		length = ETH_ZLEN;
+	}
 
 	/* Flush data to be sent */
 	flush_dcache_range(data_start, data_end);
-- 
2.17.1

[U-Boot] [PATCH 1/2] net: designware: fix tx packet length

[Joe Hershberger]

On Sat, Nov 17, 2018 at 3:25 AM Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> The designware driver has a bug in setting the tx length into the dma
> descriptor: it always or's the length into the descriptor without
> zeroing out the length mask before.
>
> This results in occasional packets being transmitted with a length
> greater than they should be (trailer). Due to the nature of Ethernet
> allowing such a trailer, most packets seem to be parsed fine by remote
> hosts, which is probably why this hasn't been noticed.
>
> Fix this by correctly clearing the size mask before setting the new
> length.
>
> Tested on socfpga gen5.
>
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 2/2] net: designware: clear padding bytes

[Joe Hershberger]

On Sat, Nov 17, 2018 at 3:26 AM Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> Short frames are padded to the minimum allowed size of 60 bytes.
> However, the designware driver sends old data in these padding bytes.
> It is common practice to zero out these padding bytes ro prevent
> leaking memory contents to other hosts.
>
> Fix the padding code to zero out the padded bytes at the end.
>
> Tested on socfpga gen5.
>
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 2/2] net: designware: clear padding bytes

[Simon Goldschmidt]

On 17.11.2018 17:03, Joe Hershberger wrote:
> On Sat, Nov 17, 2018 at 3:26 AM Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
>> Short frames are padded to the minimum allowed size of 60 bytes.
>> However, the designware driver sends old data in these padding bytes.
>> It is common practice to zero out these padding bytes ro prevent
>> leaking memory contents to other hosts.
>>
>> Fix the padding code to zero out the padded bytes at the end.
>>
>> Tested on socfpga gen5.
>>
>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Having searched through the code, there are other drivers that increase 
the length to 60 bytes but don't zero out the padding.

Would it be better to do this in eth_send()? That would ensure every 
driver does it. I don't know the U-Boot net stack too well, but maybe we 
could even do the minimum length check in eth_send()?

Regards,
Simon

[U-Boot] [PATCH 1/2] net: designware: fix tx packet length

[Simon Goldschmidt]

Am 17.11.2018 um 17:02 schrieb Joe Hershberger:
> On Sat, Nov 17, 2018 at 3:25 AM Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
>>
>> The designware driver has a bug in setting the tx length into the dma
>> descriptor: it always or's the length into the descriptor without
>> zeroing out the length mask before.
>>
>> This results in occasional packets being transmitted with a length
>> greater than they should be (trailer). Due to the nature of Ethernet
>> allowing such a trailer, most packets seem to be parsed fine by remote
>> hosts, which is probably why this hasn't been noticed.
>>
>> Fix this by correctly clearing the size mask before setting the new
>> length.
>>
>> Tested on socfpga gen5.
>>
>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> 
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> 

Marek, this and 2/2 is a assigned to you in patchwork, will you push 
them or is it Joe's code as u-boot-net maintainer?

Thanks,
Simon

[U-Boot] [PATCH 1/2] net: designware: fix tx packet length

[Philipp Tomsich]

> On 17.11.2018, at 10:24, Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> wrote:
> 
> The designware driver has a bug in setting the tx length into the dma
> descriptor: it always or's the length into the descriptor without
> zeroing out the length mask before.
> 
> This results in occasional packets being transmitted with a length
> greater than they should be (trailer). Due to the nature of Ethernet
> allowing such a trailer, most packets seem to be parsed fine by remote
> hosts, which is probably why this hasn't been noticed.
> 
> Fix this by correctly clearing the size mask before setting the new
> length.
> 
> Tested on socfpga gen5.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

[U-Boot] [PATCH 1/2] net: designware: fix tx packet length

[Marek Vasut]

On 12/09/2018 09:50 PM, Simon Goldschmidt wrote:
> Am 17.11.2018 um 17:02 schrieb Joe Hershberger:
>> On Sat, Nov 17, 2018 at 3:25 AM Simon Goldschmidt
>> <simon.k.r.goldschmidt@gmail.com> wrote:
>>>
>>> The designware driver has a bug in setting the tx length into the dma
>>> descriptor: it always or's the length into the descriptor without
>>> zeroing out the length mask before.
>>>
>>> This results in occasional packets being transmitted with a length
>>> greater than they should be (trailer). Due to the nature of Ethernet
>>> allowing such a trailer, most packets seem to be parsed fine by remote
>>> hosts, which is probably why this hasn't been noticed.
>>>
>>> Fix this by correctly clearing the size mask before setting the new
>>> length.
>>>
>>> Tested on socfpga gen5.
>>>
>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>
>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>>
> 
> Marek, this and 2/2 is a assigned to you in patchwork, will you push
> them or is it Joe's code as u-boot-net maintainer?

This is net , so Joe should pick it .

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] net: designware: fix tx packet length

[Simon Goldschmidt]

Joe,

Am 10.12.2018 um 05:08 schrieb Marek Vasut:
> On 12/09/2018 09:50 PM, Simon Goldschmidt wrote:
>> Am 17.11.2018 um 17:02 schrieb Joe Hershberger:
>>> On Sat, Nov 17, 2018 at 3:25 AM Simon Goldschmidt
>>> <simon.k.r.goldschmidt@gmail.com> wrote:
>>>>
>>>> The designware driver has a bug in setting the tx length into the dma
>>>> descriptor: it always or's the length into the descriptor without
>>>> zeroing out the length mask before.
>>>>
>>>> This results in occasional packets being transmitted with a length
>>>> greater than they should be (trailer). Due to the nature of Ethernet
>>>> allowing such a trailer, most packets seem to be parsed fine by remote
>>>> hosts, which is probably why this hasn't been noticed.
>>>>
>>>> Fix this by correctly clearing the size mask before setting the new
>>>> length.
>>>>
>>>> Tested on socfpga gen5.
>>>>
>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
>>>
>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>>>
>>
>> Marek, this and 2/2 is a assigned to you in patchwork, will you push
>> them or is it Joe's code as u-boot-net maintainer?
> 
> This is net , so Joe should pick it .

I've assigned this one and 2/2 to you in patchwork, is that OK? Could 
you pick these after the release?

Thanks,
Simon

[U-Boot] net: designware: fix tx packet length

[Joe Hershberger]

Hi Simon,

https://patchwork.ozlabs.org/patch/999267/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] net: designware: clear padding bytes

[Joe Hershberger]

Hi Simon,

https://patchwork.ozlabs.org/patch/999268/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe