On 09/10/2016 09:29 PM, Joe Hershberger wrote: > On Sat, Sep 10, 2016 at 1:48 PM, Marek Vasut <ma...@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 <ma...@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 <ma...@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.hershber...@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 > -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
