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

[Michal Simek]

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.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs