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

[Adam Ford]

On Thu, Feb 21, 2019 at 6:51 AM Marek Vasut <marex at denx.de> wrote:
>
> 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.

This happened to me for a different board where the SPI-NOR flash
driver was updated, but it required me to increase my malloc size.  I
don't think it would have been appropriate for me to ask the
maintainer to reject a patch when where was a simple solution, and the
author helped me identify how to make the updated infrastructure work.
   I think the benefits outweigh the cons if (and only if) the
solution is only to increase the malloc size.  Many of us have very
restricted SPL space.

> >>
> >> 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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot