[U-Boot] net: asix: Fix AX88772B when used with DriverModel

[Joe Hershberger]

On Sat, Sep 10, 2016 at 1:48 PM, Marek Vasut <marex at 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 at 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 at 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 at 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.  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