Alexey, On 04/09/19 6:53 PM, Alexey Brodkin wrote: > Hi Faiz, > >> -----Original Message----- >> From: Faiz Abbas <faiz_ab...@ti.com> >> Sent: Wednesday, September 4, 2019 4:09 PM >> To: Alexey Brodkin <abrod...@synopsys.com> >> Cc: paule...@forallsecure.com; tr...@konsulko.com; u-boot@lists.denx.de >> Subject: Re: [PATCH] Revert "part: Allocate only one legacy_mbr buffer" >> >> Hi Alexey, >> >> On 04/09/19 6:27 PM, Alexey Brodkin wrote: >>> Hi Faiz, >>> >>> [snip] >>> >>>>>>> I guess what you really want to do is to allocate buffer for "mbr" >>>>>>> dynamically of size which is max(sizeof(legacy_mbr), dev_desc->blksz). >>>>>>> >>>>>> >>>>>> With the assumption that blksz is always greater than >>>>>> sizeof(legacy_mbr), this should work: >>>>>> >>>>>> ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, DIV_ROUND_UP(dev_desc->blksz, >>>>>> sizeof(legacy_mbr))); >>>>> >>>>> No this won't work. See ALLOC_CACHE_ALIGN_BUFFER does static allocation >>>>> in compile-time while blksz is set in runtime. >>>>> >>>>> That's why I mentioned switch to runtime allocation. >>>>> >>>> >>>> Hmm. So the problem isn't as much about allocating too much in the >>>> buffer but about using dynamically determined size for static array >>>> declaration. >>> >>> In fact it was a problem of huge static allocation I discovered just by >>> chance building U-Boot for a very memory-limited device, see [1]. >>> >>>> This is being used all over this disk/part_dos.c file. >>>> Shouldn't we fix all of that? Not sure how it has been working all along. >>> >>> That part I don't quite understand. What's being used, where and how? >>> And what's the problem with dynamic allocation of the buffer for MBR? >>> >> >> Isn't the following line (being used in different functions in >> disk/part_dos.c) also problematic because we don't know the value of >> blksz at compile time? >> >> ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz); > > Ok I see now. Looks like I missed that Variable-Length Array (VLA) feature > added to C99 so up-to-date GCC apparently supports it. > > But I would strongly recommend to get rid of VLA usage by all means, > see [1] & [2]. > > [1] https://lwn.net/Articles/749064/ > [2] https://lwn.net/Articles/749089/ >
Interesting. This is news to me. Thanks, Faiz _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot