[U-Boot] [PATCH] net: asix: Fix AX88772B when used with DriverModel
A previous patch (net: asix: fix operation without eeprom) added a two-byte shift to the packet buffer when receiving a packet on the AX88772B.
This shift was not included when the driver was updated to work with DriverModel. Testing on a Marvell DB-88F6820-ACM showed that the adapter was not functioning correctly (EHCI timeouts).
This patch brings the two-byte shift to the DriverModel implementation of ops->recv (asix_eth_recv).
Testing on the same board, we were able to TFTP a file over and confirm that the crc32 was correct.
Signed-off-by: Joshua Scott joshua.scott@alliedtelesis.co.nz --- drivers/usb/eth/asix.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/usb/eth/asix.c b/drivers/usb/eth/asix.c index ad083cf..a610ae4 100644 --- a/drivers/usb/eth/asix.c +++ b/drivers/usb/eth/asix.c @@ -819,6 +819,11 @@ int asix_eth_recv(struct udevice *dev, int flags, uchar **packetp) }
*packetp = ptr + sizeof(packet_len); + + if ((ueth->pusb_dev->descriptor.idVendor == ASIX_USB_VENDOR_ID) && + (ueth->pusb_dev->descriptor.idProduct == AX88772B_USB_PRODUCT_ID)) + *packetp += 2; + return packet_len;
err:
On Mon, Sep 5, 2016 at 11:03 PM, Joshua Scott joshua.scott@alliedtelesis.co.nz wrote:
A previous patch (net: asix: fix operation without eeprom) added a two-byte shift to the packet buffer when receiving a packet on the AX88772B.
This shift was not included when the driver was updated to work with DriverModel. Testing on a Marvell DB-88F6820-ACM showed that the adapter was not functioning correctly (EHCI timeouts).
This patch brings the two-byte shift to the DriverModel implementation of ops->recv (asix_eth_recv).
Testing on the same board, we were able to TFTP a file over and confirm that the crc32 was correct.
Signed-off-by: Joshua Scott joshua.scott@alliedtelesis.co.nz
Acked-by: Joe Hershberger joe.hershberger@ni.com
On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote:
Hi Joshua,
https://patchwork.ozlabs.org/patch/666191/ was applied to u-boot- net.git.
Thanks! -Joe
No, sorry, but this is really the wrong approach! As discussed before rather than Joshua's patch the one from Alban should long since have been applied:
https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.html
I will send a revert ASAP and hope Alban's patch will finally make its way into master to fix this once and for all!
On 09/09/2016 11:06 PM, Marcel Ziswiler wrote:
On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote:
Hi Joshua,
https://patchwork.ozlabs.org/patch/666191/ was applied to u-boot- net.git.
Thanks! -Joe
No, sorry, but this is really the wrong approach! As discussed before rather than Joshua's patch the one from Alban should long since have been applied:
https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.html
I will send a revert ASAP and hope Alban's patch will finally make its way into master to fix this once and for all!
Can you, instead of sending a revert, just send a subsequent patch to fix this once and for all ?
Thanks for taking care of this mess :)
On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote:
On 09/09/2016 11:06 PM, Marcel Ziswiler wrote:
On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote:
Hi Joshua,
https://patchwork.ozlabs.org/patch/666191/ was applied to u-boot- net.git.
Thanks! -Joe
No, sorry, but this is really the wrong approach! As discussed before rather than Joshua's patch the one from Alban should long since have been applied:
https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.html
I will send a revert ASAP and hope Alban's patch will finally make its way into master to fix this once and for all!
Can you, instead of sending a revert, just send a subsequent patch to fix this once and for all ?
Sure, I will just squash my revert and Alban's fix together and send that one along ASAP.
Thanks for taking care of this mess :)
You are very welcome.
On 09/10/2016 01:13 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote:
On 09/09/2016 11:06 PM, Marcel Ziswiler wrote:
On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote:
Hi Joshua,
https://patchwork.ozlabs.org/patch/666191/ was applied to u-boot- net.git.
Thanks! -Joe
No, sorry, but this is really the wrong approach! As discussed before rather than Joshua's patch the one from Alban should long since have been applied:
https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.html
I will send a revert ASAP and hope Alban's patch will finally make its way into master to fix this once and for all!
Can you, instead of sending a revert, just send a subsequent patch to fix this once and for all ?
Sure, I will just squash my revert and Alban's fix together and send that one along ASAP.
Thanks
Thanks for taking care of this mess :)
You are very welcome.
On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote:
On 09/10/2016 01:13 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote:
On 09/09/2016 11:06 PM, Marcel Ziswiler wrote:
On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote:
Hi Joshua,
https://patchwork.ozlabs.org/patch/666191/ was applied to u- boot- net.git.
Thanks! -Joe
No, sorry, but this is really the wrong approach! As discussed before rather than Joshua's patch the one from Alban should long since have been applied:
https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.htm l
I will send a revert ASAP and hope Alban's patch will finally make its way into master to fix this once and for all!
Can you, instead of sending a revert, just send a subsequent patch to fix this once and for all ?
Sure, I will just squash my revert and Alban's fix together and send that one along ASAP.
Thanks
Don't thank me too early yet. While it works on Colibri T20 it currently fails on Colibri T30. More network and/or USB brokenness... Currently bisecting...
Thanks for taking care of this mess :)
You are very welcome.
How I do love U-Boot.
On Sat, 2016-09-10 at 02:18 +0200, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote:
On 09/10/2016 01:13 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote:
On 09/09/2016 11:06 PM, Marcel Ziswiler wrote:
On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote:
Hi Joshua,
https://patchwork.ozlabs.org/patch/666191/ was applied to u- boot- net.git.
Thanks! -Joe
No, sorry, but this is really the wrong approach! As discussed before rather than Joshua's patch the one from Alban should long since have been applied:
https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.h tm l
I will send a revert ASAP and hope Alban's patch will finally make its way into master to fix this once and for all!
Can you, instead of sending a revert, just send a subsequent patch to fix this once and for all ?
Sure, I will just squash my revert and Alban's fix together and send that one along ASAP.
Thanks
Don't thank me too early yet. While it works on Colibri T20 it currently fails on Colibri T30. More network and/or USB brokenness... Currently bisecting...
Thanks for taking care of this mess :)
You are very welcome.
How I do love U-Boot.
And the winner is:
commit aa7a648747d8c704a9a81c9e493d386930724e9d Author: Joe Hershberger joe.hershberger@ni.com Date: Mon Aug 15 14:42:15 2016 -0500
net: Stop including NFS overhead in defragment max
On 09/10/2016 03:34 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 02:18 +0200, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote:
On 09/10/2016 01:13 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote:
On 09/09/2016 11:06 PM, Marcel Ziswiler wrote:
On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote: > > > > Hi Joshua, > > https://patchwork.ozlabs.org/patch/666191/ was applied to > u- > boot- > net.git. > > Thanks! > -Joe No, sorry, but this is really the wrong approach! As discussed before rather than Joshua's patch the one from Alban should long since have been applied:
https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.h tm l
I will send a revert ASAP and hope Alban's patch will finally make its way into master to fix this once and for all!
Can you, instead of sending a revert, just send a subsequent patch to fix this once and for all ?
Sure, I will just squash my revert and Alban's fix together and send that one along ASAP.
Thanks
Don't thank me too early yet. While it works on Colibri T20 it currently fails on Colibri T30. More network and/or USB brokenness... Currently bisecting...
Thanks for taking care of this mess :)
You are very welcome.
How I do love U-Boot.
And the winner is:
commit aa7a648747d8c704a9a81c9e493d386930724e9d Author: Joe Hershberger joe.hershberger@ni.com Date: Mon Aug 15 14:42:15 2016 -0500
net: Stop including NFS overhead in defragment max
Uh oh, why is this aforementioned patch even correct ? And why don't we just revert it ? And why didn't anyone notice any problems with it ?
Hi Marek,
On Sat, Sep 10, 2016 at 5:01 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 03:34 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 02:18 +0200, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote:
On 09/10/2016 01:13 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote:
On 09/09/2016 11:06 PM, Marcel Ziswiler wrote: > > > > On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote: >> >> >> >> Hi Joshua, >> >> https://patchwork.ozlabs.org/patch/666191/ was applied to >> u- >> boot- >> net.git. >> >> Thanks! >> -Joe > No, sorry, but this is really the wrong approach! As > discussed > before > rather than Joshua's patch the one from Alban should long > since > have > been applied: > > https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.h > tm > l > > I will send a revert ASAP and hope Alban's patch will finally > make > its > way into master to fix this once and for all! > Can you, instead of sending a revert, just send a subsequent patch to fix this once and for all ?
Sure, I will just squash my revert and Alban's fix together and send that one along ASAP.
Thanks
Don't thank me too early yet. While it works on Colibri T20 it currently fails on Colibri T30. More network and/or USB brokenness... Currently bisecting...
Thanks for taking care of this mess :)
You are very welcome.
How I do love U-Boot.
And the winner is:
commit aa7a648747d8c704a9a81c9e493d386930724e9d Author: Joe Hershberger joe.hershberger@ni.com Date: Mon Aug 15 14:42:15 2016 -0500
net: Stop including NFS overhead in defragment maxUh oh, why is this aforementioned patch even correct ? And why don't we just revert it ? And why didn't anyone notice any problems with it ?
Before that patch, on at least some platforms, lots of memory was being wasted just because of trying to single-source the size of NFS overhead. That patch removed that.
The comment from that patch: "If a board needs a specific different defragment size, that board can override this setting."
That is the case here.
Cheers, -Joe
On 09/10/2016 06:28 PM, Joe Hershberger wrote:
Hi Marek,
On Sat, Sep 10, 2016 at 5:01 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 03:34 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 02:18 +0200, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote:
On 09/10/2016 01:13 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote: > > > On 09/09/2016 11:06 PM, Marcel Ziswiler wrote: >> >> >> >> On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote: >>> >>> >>> >>> Hi Joshua, >>> >>> https://patchwork.ozlabs.org/patch/666191/ was applied to >>> u- >>> boot- >>> net.git. >>> >>> Thanks! >>> -Joe >> No, sorry, but this is really the wrong approach! As >> discussed >> before >> rather than Joshua's patch the one from Alban should long >> since >> have >> been applied: >> >> https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.h >> tm >> l >> >> I will send a revert ASAP and hope Alban's patch will finally >> make >> its >> way into master to fix this once and for all! >> > Can you, instead of sending a revert, just send a subsequent > patch to > fix this once and for all ? Sure, I will just squash my revert and Alban's fix together and send that one along ASAP.
Thanks
Don't thank me too early yet. While it works on Colibri T20 it currently fails on Colibri T30. More network and/or USB brokenness... Currently bisecting...
> > Thanks for taking care of this mess :) You are very welcome.
How I do love U-Boot.
And the winner is:
commit aa7a648747d8c704a9a81c9e493d386930724e9d Author: Joe Hershberger joe.hershberger@ni.com Date: Mon Aug 15 14:42:15 2016 -0500
net: Stop including NFS overhead in defragment maxUh oh, why is this aforementioned patch even correct ? And why don't we just revert it ? And why didn't anyone notice any problems with it ?
Before that patch, on at least some platforms, lots of memory was being wasted just because of trying to single-source the size of NFS overhead. That patch removed that.
The comment from that patch: "If a board needs a specific different defragment size, that board can override this setting."
That is the case here.
Can we be sure that this patch will not break other board(s) ?
Hi Marek,
On Sat, Sep 10, 2016 at 11:51 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 06:28 PM, Joe Hershberger wrote:
Hi Marek,
On Sat, Sep 10, 2016 at 5:01 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 03:34 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 02:18 +0200, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote:
On 09/10/2016 01:13 AM, Marcel Ziswiler wrote: > > > On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote: >> >> >> On 09/09/2016 11:06 PM, Marcel Ziswiler wrote: >>> >>> >>> >>> On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote: >>>> >>>> >>>> >>>> Hi Joshua, >>>> >>>> https://patchwork.ozlabs.org/patch/666191/ was applied to >>>> u- >>>> boot- >>>> net.git. >>>> >>>> Thanks! >>>> -Joe >>> No, sorry, but this is really the wrong approach! As >>> discussed >>> before >>> rather than Joshua's patch the one from Alban should long >>> since >>> have >>> been applied: >>> >>> https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.h >>> tm >>> l >>> >>> I will send a revert ASAP and hope Alban's patch will finally >>> make >>> its >>> way into master to fix this once and for all! >>> >> Can you, instead of sending a revert, just send a subsequent >> patch to >> fix this once and for all ? > Sure, I will just squash my revert and Alban's fix together and > send > that one along ASAP. Thanks
Don't thank me too early yet. While it works on Colibri T20 it currently fails on Colibri T30. More network and/or USB brokenness... Currently bisecting...
> >> >> Thanks for taking care of this mess :) > You are very welcome.
How I do love U-Boot.
And the winner is:
commit aa7a648747d8c704a9a81c9e493d386930724e9d Author: Joe Hershberger joe.hershberger@ni.com Date: Mon Aug 15 14:42:15 2016 -0500
net: Stop including NFS overhead in defragment maxUh oh, why is this aforementioned patch even correct ? And why don't we just revert it ? And why didn't anyone notice any problems with it ?
Before that patch, on at least some platforms, lots of memory was being wasted just because of trying to single-source the size of NFS overhead. That patch removed that.
The comment from that patch: "If a board needs a specific different defragment size, that board can override this setting."
That is the case here.
Can we be sure that this patch will not break other board(s) ?
It will likely affect 2 other boards in the same way...
include/configs/apalis_t30.h: 56 #define CONFIG_TFTP_BLOCKSIZE 16384 include/configs/colibri_imx7.h: 49 #define CONFIG_TFTP_BLOCKSIZE 16384 include/configs/colibri_t30.h: 52 #define CONFIG_TFTP_BLOCKSIZE 16384
Cheers, -Joe
On 09/10/2016 07:24 PM, Joe Hershberger wrote:
Hi Marek,
On Sat, Sep 10, 2016 at 11:51 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 06:28 PM, Joe Hershberger wrote:
Hi Marek,
On Sat, Sep 10, 2016 at 5:01 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 03:34 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 02:18 +0200, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote: > > On 09/10/2016 01:13 AM, Marcel Ziswiler wrote: >> >> >> On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote: >>> >>> >>> On 09/09/2016 11:06 PM, Marcel Ziswiler wrote: >>>> >>>> >>>> >>>> On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote: >>>>> >>>>> >>>>> >>>>> Hi Joshua, >>>>> >>>>> https://patchwork.ozlabs.org/patch/666191/ was applied to >>>>> u- >>>>> boot- >>>>> net.git. >>>>> >>>>> Thanks! >>>>> -Joe >>>> No, sorry, but this is really the wrong approach! As >>>> discussed >>>> before >>>> rather than Joshua's patch the one from Alban should long >>>> since >>>> have >>>> been applied: >>>> >>>> https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.h >>>> tm >>>> l >>>> >>>> I will send a revert ASAP and hope Alban's patch will finally >>>> make >>>> its >>>> way into master to fix this once and for all! >>>> >>> Can you, instead of sending a revert, just send a subsequent >>> patch to >>> fix this once and for all ? >> Sure, I will just squash my revert and Alban's fix together and >> send >> that one along ASAP. > Thanks Don't thank me too early yet. While it works on Colibri T20 it currently fails on Colibri T30. More network and/or USB brokenness... Currently bisecting...
> >> >>> >>> Thanks for taking care of this mess :) >> You are very welcome. How I do love U-Boot.
And the winner is:
commit aa7a648747d8c704a9a81c9e493d386930724e9d Author: Joe Hershberger joe.hershberger@ni.com Date: Mon Aug 15 14:42:15 2016 -0500
net: Stop including NFS overhead in defragment maxUh oh, why is this aforementioned patch even correct ? And why don't we just revert it ? And why didn't anyone notice any problems with it ?
Before that patch, on at least some platforms, lots of memory was being wasted just because of trying to single-source the size of NFS overhead. That patch removed that.
The comment from that patch: "If a board needs a specific different defragment size, that board can override this setting."
That is the case here.
Can we be sure that this patch will not break other board(s) ?
It will likely affect 2 other boards in the same way...
include/configs/apalis_t30.h: 56 #define CONFIG_TFTP_BLOCKSIZE 16384 include/configs/colibri_imx7.h: 49 #define CONFIG_TFTP_BLOCKSIZE 16384 include/configs/colibri_t30.h: 52 #define CONFIG_TFTP_BLOCKSIZE 16384
I _think_ you're mixing IP_PKTSIZE and CONFIG_TFTP_BLOCKSIZE (I might be wrong, I'm no network stack expert). My biggest concern about the aa7a648747d8c704a9a81c9e493d386930724e9d patch is that it might cause silent memory corruption on a lot of systems. Are you positive this is not the case, ever ?
On Sat, Sep 10, 2016 at 1:48 PM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 07:24 PM, Joe Hershberger wrote:
Hi Marek,
On Sat, Sep 10, 2016 at 11:51 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 06:28 PM, Joe Hershberger wrote:
Hi Marek,
On Sat, Sep 10, 2016 at 5:01 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 03:34 AM, Marcel Ziswiler wrote:
On Sat, 2016-09-10 at 02:18 +0200, Marcel Ziswiler wrote: > On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote: >> >> On 09/10/2016 01:13 AM, Marcel Ziswiler wrote: >>> >>> >>> On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote: >>>> >>>> >>>> On 09/09/2016 11:06 PM, Marcel Ziswiler wrote: >>>>> >>>>> >>>>> >>>>> On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi Joshua, >>>>>> >>>>>> https://patchwork.ozlabs.org/patch/666191/ was applied to >>>>>> u- >>>>>> boot- >>>>>> net.git. >>>>>> >>>>>> Thanks! >>>>>> -Joe >>>>> No, sorry, but this is really the wrong approach! As >>>>> discussed >>>>> before >>>>> rather than Joshua's patch the one from Alban should long >>>>> since >>>>> have >>>>> been applied: >>>>> >>>>> https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.h >>>>> tm >>>>> l >>>>> >>>>> I will send a revert ASAP and hope Alban's patch will finally >>>>> make >>>>> its >>>>> way into master to fix this once and for all! >>>>> >>>> Can you, instead of sending a revert, just send a subsequent >>>> patch to >>>> fix this once and for all ? >>> Sure, I will just squash my revert and Alban's fix together and >>> send >>> that one along ASAP. >> Thanks > Don't thank me too early yet. While it works on Colibri T20 it > currently fails on Colibri T30. More network and/or USB brokenness... > Currently bisecting... > >> >>> >>>> >>>> Thanks for taking care of this mess :) >>> You are very welcome. > How I do love U-Boot.
And the winner is:
commit aa7a648747d8c704a9a81c9e493d386930724e9d Author: Joe Hershberger joe.hershberger@ni.com Date: Mon Aug 15 14:42:15 2016 -0500
net: Stop including NFS overhead in defragment maxUh oh, why is this aforementioned patch even correct ? And why don't we just revert it ? And why didn't anyone notice any problems with it ?
Before that patch, on at least some platforms, lots of memory was being wasted just because of trying to single-source the size of NFS overhead. That patch removed that.
The comment from that patch: "If a board needs a specific different defragment size, that board can override this setting."
That is the case here.
Can we be sure that this patch will not break other board(s) ?
It will likely affect 2 other boards in the same way...
include/configs/apalis_t30.h: 56 #define CONFIG_TFTP_BLOCKSIZE 16384 include/configs/colibri_imx7.h: 49 #define CONFIG_TFTP_BLOCKSIZE 16384 include/configs/colibri_t30.h: 52 #define CONFIG_TFTP_BLOCKSIZE 16384
I _think_ you're mixing IP_PKTSIZE and CONFIG_TFTP_BLOCKSIZE (I might be wrong, I'm no network stack expert).
The difference between these two is 4 bytes.
It's the fact that the eth + ip + udp + tftp headers + block size (data payload) for the TFTP packets is bigger than the max defrag (i.e. the buffer size for reconstructing the packet) as a result of this patch. There's nothing magic about this relationship, it's just implied and not explicit since it's possible to be some other network transfer besides TFTP and NFS, but those are the two I'm aware of that care about defrag.
My biggest concern about the aa7a648747d8c704a9a81c9e493d386930724e9d patch is that it might cause silent memory corruption on a lot of systems. Are you positive this is not the case, ever ?
Not on its own. The change is a reduction in default memory used for defrag. If a board was expecting it to be larger then it will have an issue (like we see on the t30). For TFTP, grep can tell us the 3 boards that are expecting more than they will get by default.
The only board that overrides the default NFS read size is:
include/configs/tqma6.h: 107 #define CONFIG_NFS_READ_SIZE 4096
That board is expecting only 4k, where as the default for IP_DEFRAG is 16k. That board will be fine.
There are only a handful that even specify IP_DEFRAG:
include/configs/apalis_t30.h: 54 #define CONFIG_IP_DEFRAG include/configs/bfin_adi_common.h: 241 # define CONFIG_IP_DEFRAG include/configs/colibri_imx7.h: 48 #define CONFIG_IP_DEFRAG include/configs/colibri_t20.h: 44 #define CONFIG_IP_DEFRAG include/configs/colibri_t30.h: 50 #define CONFIG_IP_DEFRAG include/configs/ma5d4evk.h: 71 #define CONFIG_IP_DEFRAG include/configs/sandbox.h: 128 #define CONFIG_IP_DEFRAG include/configs/tqma6.h: 105 #define CONFIG_IP_DEFRAG
I think if we also adjust the apalis_t30 and the colibri_imx7 then we are good.
Cheers, -Joe
On 09/10/2016 09:29 PM, Joe Hershberger wrote:
On Sat, Sep 10, 2016 at 1:48 PM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 07:24 PM, Joe Hershberger wrote:
Hi Marek,
On Sat, Sep 10, 2016 at 11:51 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 06:28 PM, Joe Hershberger wrote:
Hi Marek,
On Sat, Sep 10, 2016 at 5:01 AM, Marek Vasut marex@denx.de wrote:
On 09/10/2016 03:34 AM, Marcel Ziswiler wrote: > On Sat, 2016-09-10 at 02:18 +0200, Marcel Ziswiler wrote: >> On Sat, 2016-09-10 at 01:23 +0200, Marek Vasut wrote: >>> >>> On 09/10/2016 01:13 AM, Marcel Ziswiler wrote: >>>> >>>> >>>> On Sat, 2016-09-10 at 01:04 +0200, Marek Vasut wrote: >>>>> >>>>> >>>>> On 09/09/2016 11:06 PM, Marcel Ziswiler wrote: >>>>>> >>>>>> >>>>>> >>>>>> On Fri, 2016-09-09 at 13:57 -0500, Joe Hershberger wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi Joshua, >>>>>>> >>>>>>> https://patchwork.ozlabs.org/patch/666191/ was applied to >>>>>>> u- >>>>>>> boot- >>>>>>> net.git. >>>>>>> >>>>>>> Thanks! >>>>>>> -Joe >>>>>> No, sorry, but this is really the wrong approach! As >>>>>> discussed >>>>>> before >>>>>> rather than Joshua's patch the one from Alban should long >>>>>> since >>>>>> have >>>>>> been applied: >>>>>> >>>>>> https://www.mail-archive.com/u-boot@lists.denx.de/msg221455.h >>>>>> tm >>>>>> l >>>>>> >>>>>> I will send a revert ASAP and hope Alban's patch will finally >>>>>> make >>>>>> its >>>>>> way into master to fix this once and for all! >>>>>> >>>>> Can you, instead of sending a revert, just send a subsequent >>>>> patch to >>>>> fix this once and for all ? >>>> Sure, I will just squash my revert and Alban's fix together and >>>> send >>>> that one along ASAP. >>> Thanks >> Don't thank me too early yet. While it works on Colibri T20 it >> currently fails on Colibri T30. More network and/or USB brokenness... >> Currently bisecting... >> >>> >>>> >>>>> >>>>> Thanks for taking care of this mess :) >>>> You are very welcome. >> How I do love U-Boot. > > And the winner is: > > commit aa7a648747d8c704a9a81c9e493d386930724e9d > Author: Joe Hershberger joe.hershberger@ni.com > Date: Mon Aug 15 14:42:15 2016 -0500 > > net: Stop including NFS overhead in defragment max >
Uh oh, why is this aforementioned patch even correct ? And why don't we just revert it ? And why didn't anyone notice any problems with it ?
Before that patch, on at least some platforms, lots of memory was being wasted just because of trying to single-source the size of NFS overhead. That patch removed that.
The comment from that patch: "If a board needs a specific different defragment size, that board can override this setting."
That is the case here.
Can we be sure that this patch will not break other board(s) ?
It will likely affect 2 other boards in the same way...
include/configs/apalis_t30.h: 56 #define CONFIG_TFTP_BLOCKSIZE 16384 include/configs/colibri_imx7.h: 49 #define CONFIG_TFTP_BLOCKSIZE 16384 include/configs/colibri_t30.h: 52 #define CONFIG_TFTP_BLOCKSIZE 16384
I _think_ you're mixing IP_PKTSIZE and CONFIG_TFTP_BLOCKSIZE (I might be wrong, I'm no network stack expert).
The difference between these two is 4 bytes.
It's the fact that the eth + ip + udp + tftp headers + block size (data payload) for the TFTP packets is bigger than the max defrag (i.e. the buffer size for reconstructing the packet) as a result of this patch.
And that is OK ?
There's nothing magic about this relationship, it's just implied and not explicit since it's possible to be some other network transfer besides TFTP and NFS, but those are the two I'm aware of that care about defrag.
I don't understand this part, sorry.
My biggest concern about the aa7a648747d8c704a9a81c9e493d386930724e9d patch is that it might cause silent memory corruption on a lot of systems. Are you positive this is not the case, ever ?
Not on its own. The change is a reduction in default memory used for defrag. If a board was expecting it to be larger then it will have an issue (like we see on the t30). For TFTP, grep can tell us the 3 boards that are expecting more than they will get by default.
I see. Shouldn't we check for such condition and error out then ?
The only board that overrides the default NFS read size is:
include/configs/tqma6.h: 107 #define CONFIG_NFS_READ_SIZE 4096
That board is expecting only 4k, where as the default for IP_DEFRAG is 16k. That board will be fine.
There are only a handful that even specify IP_DEFRAG:
include/configs/apalis_t30.h: 54 #define CONFIG_IP_DEFRAG include/configs/bfin_adi_common.h: 241 # define CONFIG_IP_DEFRAG include/configs/colibri_imx7.h: 48 #define CONFIG_IP_DEFRAG include/configs/colibri_t20.h: 44 #define CONFIG_IP_DEFRAG include/configs/colibri_t30.h: 50 #define CONFIG_IP_DEFRAG include/configs/ma5d4evk.h: 71 #define CONFIG_IP_DEFRAG include/configs/sandbox.h: 128 #define CONFIG_IP_DEFRAG include/configs/tqma6.h: 105 #define CONFIG_IP_DEFRAG
I think if we also adjust the apalis_t30 and the colibri_imx7 then we are good.
Cheers, -Joe
participants (5)
-
Joe Hershberger -
Joe Hershberger -
Joshua Scott -
Marcel Ziswiler -
Marek Vasut