[U-Boot] [U-Boot, v3, 1/2] fs: fat: dynamically allocate memory for temporary buffer

[Marek Vasut]

On 2/21/19 1:13 PM, Michal Simek wrote:
> On 21. 02. 19 10:04, Marek Vasut wrote:
>> On 2/21/19 9:55 AM, Alexander Graf wrote:
>>>
>>>
>>> On 21.02.19 09:49, Marek Vasut wrote:
>>>> On 2/21/19 9:44 AM, Alexander Graf wrote:
>>>>>
>>>>>
>>>>> On 21.02.19 09:41, Marek Vasut wrote:
>>>>>> On 2/21/19 9:40 AM, Chee, Tien Fong wrote:
>>>>>>> On Thu, 2019-02-21 at 09:29 +0100, Alexander Graf wrote:
>>>>>>>>
>>>>>>>> On 21.02.19 09:23, Chee, Tien Fong wrote:
>>>>>>>>>
>>>>>>>>> On Thu, 2019-02-21 at 08:45 +0100, Michal Simek wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Tom,
>>>>>>>>>>
>>>>>>>>>> On 20. 02. 19 2:58, Tom Rini wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Feb 11, 2019 at 02:56:19PM +0800, tien.fong.chee at intel.
>>>>>>>>>>> com
>>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Drop the statically allocated get_contents_vfatname_block and
>>>>>>>>>>>> dynamically allocate a buffer only if required. This saves
>>>>>>>>>>>> 64KiB of memory.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Stefan Agner <stefan.ag... at toradex.com>
>>>>>>>>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>>>>>>> Applied to u-boot/master, thanks!
>>>>>>>>>> please remove this patch (better both of them because they were
>>>>>>>>>> in
>>>>>>>>>> series)
>>>>>>>>> I think patch 2/2 should be safe, because no memory size is
>>>>>>>>> changed.
>>>>>>>>> Basically, it just to release the allocated memory immediately when
>>>>>>>>> it's not required, so other can re-use it.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> because they are breaking at least ZynqMP SPL. It is also too
>>>>>>>>>> late in cycle to create random fix.
>>>>>>>>>>
>>>>>>>>>> You can't simply move 64KB from code to malloc without reflecting
>>>>>>>>>> this
>>>>>>>>>> by changing MALLOC space size.
>>>>>>>>>>
>>>>>>>>>> Other boards with SPL fat could be also affected by this if they
>>>>>>>>>> don't
>>>>>>>>>> allocate big malloc space.
>>>>>>>>> So, any suggestion to get the patch 1/2 accepted? inform all board
>>>>>>>>> maintainers to test it out?
>>>>>>>> You already received feedback that it does break ZynqMP, so the
>>>>>>>> current
>>>>>>>> approach won't work.
>>>>>>>>
>>>>>>>> How about you create a new kconfig option that allows you to say
>>>>>>>> whether
>>>>>>>> you want to use malloc or .bss for temporary data in the FAT driver.
>>>>>>>> You
>>>>>>>> can then have an _SPL_ version of that kconfig and check for it with
>>>>>>>> IS_ENABLED() which should automatically tell you the right answer
>>>>>>>> depending on whether you're in an SPL build or not.
>>>>>>>>
>>>>>>>> Then you can set the SPL version to default malloc and the non-SPL
>>>>>>>> version to default .bss.
>>>>>>> Marek and Tom rini,
>>>>>>>
>>>>>>> Are you guys okay with Alex's suggestion?
>>>>>>
>>>>>> I'm not a big fan of adding more and more ifdeffery.
>>>>>> Is there some other option ?
>>>>>
>>>>> Is RAM up already at this point? Maybe we could improve the SPL malloc
>>>>> mechanism to move allocations into DRAM once it's available.
>>>>
>>>> Well, the FAT buffers waste some 64kiB of bss, so we can use that area
>>>> for malloc instead, no ?
>>>
>>> Yes, but that means you need to review every single board that uses FAT
>>> in SPL today and adjust its malloc region size.
>>
>> That's quite likely ... I still think this patch is beneficial, it's
>> much better to dynamically allocate the cluster size than have this
>> 64kiB chunk of BSS carved out.
>>
> 
> ok. I have played with it a little bit and the patch exposed different
> problem with one of my out of tree patch I am working on.
> 
> Anyway that's being said I still think that patches like this shouldn't
> come to the tree at this stage because it requires checking on other
> boards. IIRC similar patch was around in past and there was also any
> issue with it.
> 
> Tom: up2you if you want to keep it in the tree or not.

This shouldn't have come in after RC2, so revert and let's fix it for
next release.

-- 
Best regards,
Marek Vasut