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

[Marek Vasut]

On 09/10/2016 09:29 PM, Joe Hershberger wrote:
> 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.

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