| Message ID | 20110126165340.14306.98359.stgit@localhost6.localdomain6 |
|---|---|
| State | RFC |
| Headers | show |
On Wed, Jan 26, 2011 at 9:57 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > LMB doesn't currently handle allocating regions based at physical > address 0. This patch reworks the lmb_alloc functions to return > all ones when allocation fails instead of zero so that callers can > differentiate between a region allocated at zero and a failed > allocation > > Signed-off-by: Grant Likely <grant.likely@secretlab.ca> Oops, sent this one out too early. It will not apply against current mainline since it it based on an older tree. Still, please take a look and let me know it this looks appropriate. I'll get this rebased and retested against top of tree. g. > --- > common/image.c | 8 ++++---- > include/lmb.h | 1 + > lib/lmb.c | 21 ++++++++++++++++----- > 3 files changed, 21 insertions(+), 9 deletions(-) > > diff --git a/common/image.c b/common/image.c > index 11504de..c1757bd 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -1053,7 +1053,7 @@ int boot_ramdisk_high (struct lmb *lmb, ulong rd_data, ulong rd_len, > else > *initrd_start = (ulong)lmb_alloc (lmb, rd_len, 0x1000); > > - if (*initrd_start == 0) { > + if (*initrd_start == LMB_ALLOC_ERROR) { > puts ("ramdisk - allocation error\n"); > goto error; > } > @@ -1228,7 +1228,7 @@ int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base, > of_start = (unsigned long)lmb_alloc_base(lmb, of_len, 0x1000, > (CONFIG_SYS_BOOTMAPSZ + bootmap_base)); > > - if (of_start == 0) { > + if (of_start == (unsigned long)LMB_ALLOC_ERROR) { > puts("device tree - allocation error\n"); > goto error; > } > @@ -1614,7 +1614,7 @@ int boot_get_cmdline (struct lmb *lmb, ulong *cmd_start, ulong *cmd_end, > cmdline = (char *)(ulong)lmb_alloc_base(lmb, CONFIG_SYS_BARGSIZE, 0xf, > CONFIG_SYS_BOOTMAPSZ + bootmap_base); > > - if (cmdline == NULL) > + if (cmdline == (char*)(ulong)LMB_ALLOC_ERROR) > return -1; > > if ((s = getenv("bootargs")) == NULL) > @@ -1651,7 +1651,7 @@ int boot_get_kbd (struct lmb *lmb, bd_t **kbd, ulong bootmap_base) > { > *kbd = (bd_t *)(ulong)lmb_alloc_base(lmb, sizeof(bd_t), 0xf, > CONFIG_SYS_BOOTMAPSZ + bootmap_base); > - if (*kbd == NULL) > + if (*kbd == (bd_t *)(ulong)LMB_ALLOC_ERROR) > return -1; > > **kbd = *(gd->bd); > diff --git a/include/lmb.h b/include/lmb.h > index 43082a3..f927b86 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -15,6 +15,7 @@ > */ > > #define MAX_LMB_REGIONS 8 > +#define LMB_ALLOC_ERROR ((phys_addr_t) ~0) > > struct lmb_property { > phys_addr_t base; > diff --git a/lib/lmb.c b/lib/lmb.c > index c5e75fb..de3c325 100644 > --- a/lib/lmb.c > +++ b/lib/lmb.c > @@ -114,6 +114,9 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t > unsigned long coalesced = 0; > long adjacent, i; > > + debug("lmb_add_region(lmb=%x, base=%.8lx, size=%.8lx)\n", > + (ulong)rgn, (ulong)base, (ulong)size); > + > if ((rgn->cnt == 1) && (rgn->region[0].size == 0)) { > rgn->region[0].base = base; > rgn->region[0].size = size; > @@ -266,9 +269,11 @@ phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phys_ > > alloc = __lmb_alloc_base(lmb, size, align, max_addr); > > - if (alloc == 0) > - printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", > + if (alloc == LMB_ALLOC_ERROR) { > + debug("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", > (ulong)size, (ulong)max_addr); > + lmb_dump_all(lmb); > + } > > return alloc; > } > @@ -289,10 +294,15 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy > phys_addr_t base = 0; > phys_addr_t res_base; > > + debug("lmb_alloc_base(lmb=%lx, size=%lx, align=%li, max_addr=%lx)\n", > + (ulong)lmb, (ulong)size, (ulong)align, (ulong)max_addr); > + > for (i = lmb->memory.cnt-1; i >= 0; i--) { > phys_addr_t lmbbase = lmb->memory.region[i].base; > phys_size_t lmbsize = lmb->memory.region[i].size; > > + debug("--loop i=%i, lmbbase=%lx, lmbsize=%lx\n", > + i, (ulong)lmbbase, (ulong)lmbsize); > if (lmbsize < size) > continue; > if (max_addr == LMB_ALLOC_ANYWHERE) > @@ -303,14 +313,15 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy > } else > continue; > > - while (base && lmbbase <= base) { > + while (lmbbase <= base) { > j = lmb_overlaps_region(&lmb->reserved, base, size); > + debug("----loop base=%lx, j=%i\n", (ulong)base, j); > if (j < 0) { > /* This area isn't reserved, take it */ > if (lmb_add_region(&lmb->reserved, base, > lmb_align_up(size, > align)) < 0) > - return 0; > + return LMB_ALLOC_ERROR; > return base; > } > res_base = lmb->reserved.region[j].base; > @@ -319,7 +330,7 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy > base = lmb_align_down(res_base - size, align); > } > } > - return 0; > + return LMB_ALLOC_ERROR; > } > > int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr) > >
Dear Grant Likely, In message <20110126165340.14306.98359.stgit@localhost6.localdomain6> you wrote: > LMB doesn't currently handle allocating regions based at physical > address 0. This patch reworks the lmb_alloc functions to return > all ones when allocation fails instead of zero so that callers can > differentiate between a region allocated at zero and a failed > allocation > > Signed-off-by: Grant Likely <grant.likely@secretlab.ca> ... > --- a/lib/lmb.c > +++ b/lib/lmb.c > @@ -114,6 +114,9 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t ... > - if (alloc == 0) > - printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", > + if (alloc == LMB_ALLOC_ERROR) { > + debug("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", > (ulong)size, (ulong)max_addr); > + lmb_dump_all(lmb); > + } Do you intentionally change the printf() into a debug() here? If yes, what is your rationale for not printing a proper error message in production code? Best regards, Wolfgang Denk
On Wed, Jan 26, 2011 at 11:30 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Grant Likely, > > In message <20110126165340.14306.98359.stgit@localhost6.localdomain6> you wrote: >> LMB doesn't currently handle allocating regions based at physical >> address 0. This patch reworks the lmb_alloc functions to return >> all ones when allocation fails instead of zero so that callers can >> differentiate between a region allocated at zero and a failed >> allocation >> >> Signed-off-by: Grant Likely <grant.likely@secretlab.ca> > ... >> --- a/lib/lmb.c >> +++ b/lib/lmb.c >> @@ -114,6 +114,9 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t > ... >> - if (alloc == 0) >> - printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", >> + if (alloc == LMB_ALLOC_ERROR) { >> + debug("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", >> (ulong)size, (ulong)max_addr); >> + lmb_dump_all(lmb); >> + } > > Do you intentionally change the printf() into a debug() here? If yes, > what is your rationale for not printing a proper error message in > production code? No, that was a mistake. I'll correct it. g. > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > "I knew then (in 1970) that a 4-kbyte minicomputer would cost as much > as a house. So I reasoned that after college, I'd have to live > cheaply in an apartment and put all my money into owning a computer." > - Apple co-founder Steve Wozniak, EE Times, June 6, 1988, pg 45 >
differentiate between a region allocated at zero and a failed allocation Signed-off-by: Grant Likely <grant.likely@secretlab.ca> --- common/image.c | 8 ++++---- include/lmb.h | 1 + lib/lmb.c | 21 ++++++++++++++++----- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/common/image.c b/common/image.c index 11504de..c1757bd 100644 --- a/common/image.c +++ b/common/image.c @@ -1053,7 +1053,7 @@ int boot_ramdisk_high (struct lmb *lmb, ulong rd_data, ulong rd_len, else *initrd_start = (ulong)lmb_alloc (lmb, rd_len, 0x1000); - if (*initrd_start == 0) { + if (*initrd_start == LMB_ALLOC_ERROR) { puts ("ramdisk - allocation error\n"); goto error; } @@ -1228,7 +1228,7 @@ int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base, of_start = (unsigned long)lmb_alloc_base(lmb, of_len, 0x1000, (CONFIG_SYS_BOOTMAPSZ + bootmap_base)); - if (of_start == 0) { + if (of_start == (unsigned long)LMB_ALLOC_ERROR) { puts("device tree - allocation error\n"); goto error; } @@ -1614,7 +1614,7 @@ int boot_get_cmdline (struct lmb *lmb, ulong *cmd_start, ulong *cmd_end, cmdline = (char *)(ulong)lmb_alloc_base(lmb, CONFIG_SYS_BARGSIZE, 0xf, CONFIG_SYS_BOOTMAPSZ + bootmap_base); - if (cmdline == NULL) + if (cmdline == (char*)(ulong)LMB_ALLOC_ERROR) return -1; if ((s = getenv("bootargs")) == NULL) @@ -1651,7 +1651,7 @@ int boot_get_kbd (struct lmb *lmb, bd_t **kbd, ulong bootmap_base) { *kbd = (bd_t *)(ulong)lmb_alloc_base(lmb, sizeof(bd_t), 0xf, CONFIG_SYS_BOOTMAPSZ + bootmap_base); - if (*kbd == NULL) + if (*kbd == (bd_t *)(ulong)LMB_ALLOC_ERROR) return -1; **kbd = *(gd->bd); diff --git a/include/lmb.h b/include/lmb.h index 43082a3..f927b86 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -15,6 +15,7 @@ */ #define MAX_LMB_REGIONS 8 +#define LMB_ALLOC_ERROR ((phys_addr_t) ~0) struct lmb_property { phys_addr_t base; diff --git a/lib/lmb.c b/lib/lmb.c index c5e75fb..de3c325 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -114,6 +114,9 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t unsigned long coalesced = 0; long adjacent, i; + debug("lmb_add_region(lmb=%x, base=%.8lx, size=%.8lx)\n", + (ulong)rgn, (ulong)base, (ulong)size); + if ((rgn->cnt == 1) && (rgn->region[0].size == 0)) { rgn->region[0].base = base; rgn->region[0].size = size; @@ -266,9 +269,11 @@ phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phys_ alloc = __lmb_alloc_base(lmb, size, align, max_addr); - if (alloc == 0) - printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", + if (alloc == LMB_ALLOC_ERROR) { + debug("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n", (ulong)size, (ulong)max_addr); + lmb_dump_all(lmb); + } return alloc; } @@ -289,10 +294,15 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy phys_addr_t base = 0; phys_addr_t res_base; + debug("lmb_alloc_base(lmb=%lx, size=%lx, align=%li, max_addr=%lx)\n", + (ulong)lmb, (ulong)size, (ulong)align, (ulong)max_addr); + for (i = lmb->memory.cnt-1; i >= 0; i--) { phys_addr_t lmbbase = lmb->memory.region[i].base; phys_size_t lmbsize = lmb->memory.region[i].size; + debug("--loop i=%i, lmbbase=%lx, lmbsize=%lx\n", + i, (ulong)lmbbase, (ulong)lmbsize); if (lmbsize < size) continue; if (max_addr == LMB_ALLOC_ANYWHERE) @@ -303,14 +313,15 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy } else continue; - while (base && lmbbase <= base) { + while (lmbbase <= base) { j = lmb_overlaps_region(&lmb->reserved, base, size); + debug("----loop base=%lx, j=%i\n", (ulong)base, j); if (j < 0) { /* This area isn't reserved, take it */ if (lmb_add_region(&lmb->reserved, base, lmb_align_up(size, align)) < 0) - return 0; + return LMB_ALLOC_ERROR; return base; } res_base = lmb->reserved.region[j].base; @@ -319,7 +330,7 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy base = lmb_align_down(res_base - size, align); } } - return 0; + return LMB_ALLOC_ERROR; } int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)