[U-Boot] [PATCH v4 1/2] dlmalloc: fix malloc range at end of ram

[Simon Glass]

Hi,

On Mon, 29 Apr 2019 at 07:19, Tom Rini <trini at konsulko.com> wrote:
>
> On Mon, Apr 29, 2019 at 03:06:39PM +0200, Heiko Schocher wrote:
> > Hello Simon,
> >
> > Am 25.04.2019 um 21:24 schrieb Simon Goldschmidt:
> > >Am 25.04.2019 um 12:50 schrieb Tom Rini:
> > >>On Thu, Apr 25, 2019 at 09:32:22AM +0200, Simon Goldschmidt wrote:
> > >>>On Thu, Apr 25, 2019 at 1:59 AM Simon Glass <sjg at chromium.org> wrote:
> > >>>>
> > >>>>Hi,
> > >>>>
> > >>>>On Wed, 24 Apr 2019 at 05:53, Tom Rini <trini at konsulko.com> wrote:
> > >>>>>
> > >>>>>On Wed, Apr 24, 2019 at 01:49:52PM +0200, Simon Goldschmidt wrote:
> > >>>>>>On Wed, Apr 24, 2019 at 1:27 PM Tom Rini <trini at konsulko.com> wrote:
> > >>>>>>>
> > >>>>>>>On Tue, Apr 23, 2019 at 09:54:10PM -0600, Simon Glass wrote:
> > >>>>>>>>On Mon, 1 Apr 2019 at 14:01, Simon Goldschmidt
> > >>>>>>>><simon.k.r.goldschmidt at gmail.com> wrote:
> > >>>>>>>>>
> > >>>>>>>>>If the malloc range passed to mem_malloc_init() is at the end of address
> > >>>>>>>>>range and 'start + size' overflows to 0, following allocations fail as
> > >>>>>>>>>mem_malloc_end is zero (which looks like uninitialized).
> > >>>>>>>>>
> > >>>>>>>>>Fix this by subtracting 1 of 'start + size' overflows to zero.
> > >>>>>>>>>
> > >>>>>>>>>Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>
> > >>>>>>>>>---
> > >>>>>>>>>
> > >>>>>>>>>Changes in v4: None
> > >>>>>>>>>Changes in v3: None
> > >>>>>>>>>
> > >>>>>>>>>  common/dlmalloc.c | 4 ++++
> > >>>>>>>>>  1 file changed, 4 insertions(+)
> > >>>>>>>>
> > >>>>>>>>Reviewed-by: Simon Glass <sjg at chromium.org>
> > >>>>>>>
> > >>>>>>>So, the problem with this patch is that it increases the generic malloc
> > >>>>>>>code size ever so slightly and blows up smartweb :(
> > >>>>>>
> > >>>>>>Ehrm, ok, so how do we proceed?
> > >>>>>
> > >>>>>A good question.  Take a look at spl/u-boot-spl.map on smartweb and see
> > >>>>>if, of the malloc functions it doesn't discard there's something that
> > >>>>>maybe could be optimized somewhere?
> > >>>>
> > >>>>I wonder if we should have a Kconfig option like SPL_CHECKS which
> > >>>>enables these sorts of minor checks, which may only fix one board at
> > >>>>the cost of code size?
> > >>>>
> > >>>>Then it could be enabled by default, but disabled on this board?
> > >>>
> > >>>For a bigger change, this might be an idea, but for a change that I can cut
> > >>>down to 16 or even 8 bytes code size increasement, I don't think having a
> > >>>new option would be good.
> > >>>
> > >>>Anyway, I just tried at work and I don't get the overflow. Tom, which gcc
> > >>>are you using to get the size error? It works for me on Debian 9 but doesn't
> > >>>work with Ubuntu (both times, default cross compiler toolchain installed).
> > >>
> > >>I'm using the gcc-7.3 from kernel.org that we use in travis/etc.
> > >
> > >Ok, so I have gcc-7.3 on my Ubuntu machine as well. I don't know why 6.3
> > >seems to produce smaller binaries (I thought they were getting smaller
> > >with new versions, not larger).
> > >
> > >However, I've stripped down that patch to +8 Bytes only and sent v5.
> >
> > Thanks!
> >
> > Sorry for digging so late in, but I was on vacation...
> >
> > Hmm.. the smartweb board has only 4k sram for SPL, and I have no chance
> > to convert it to DM to get rid of some compiler warnings ...
> >
> > I am unsure what to do now with this hardware ...
>
> Well, with regards to SPL + DM, this is one of the cases wherein we just
> have-to allow for the SPL driver code at least to be a one-off.  If the
> "whatever ROM loads of our code" stage can set things up enough such
> that we can hand off to a full U-Boot, that's great.  If not, this is
> then a case where TPL comes in to play, and that really is as one-off as
> needed, to load a more general SPL and so forth.

I think DM in SPL is a good thing, so long as there is space. If not,
then we should allow non-DM also. I wonder if we need our policy to be
written down somewhere?

>
> But, I'm fine with saying smartweb keeps and maintains whatever SPL code
> it needs to use.  It's just that in this case, it's not at all a DM
> thing, it's a change in malloc.

Regards,
Simon