From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Brodkin Date: Wed, 4 Sep 2019 13:23:02 +0000 Subject: [U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer" In-Reply-To: <51d70f01-0fb8-ef8a-3ca8-3621fcceb23e@ti.com> References: <20190904092219.6045-1-faiz_abbas@ti.com> <554e1bc3-f6a1-a0cc-c724-16d00b249b70@ti.com> <51d70f01-0fb8-ef8a-3ca8-3621fcceb23e@ti.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Faiz, > -----Original Message----- > From: Faiz Abbas > Sent: Wednesday, September 4, 2019 4:09 PM > To: Alexey Brodkin > Cc: paulemge at forallsecure.com; trini at konsulko.com; u-boot at 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/ -Alexey