| Message ID | fb412d760810311623i2b0f93a3r5aa0eb297e154b4a@mail.gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Oct 31, 2008 at 06:23:28PM -0500, Hollis Blanchard wrote: >On Wed, Oct 22, 2008 at 9:28 AM, Christian Ehrhardt ><ehrhardt@linux.vnet.ibm.com> wrote: >> Hi Ilya, >> I just tried your patch on my 440 board because it would help us in our >> environment. >> Unfortunately I run into a bug on early boot (mark_bootmem). >> >> A log can be found in this mail, this is the bug when running with 64k page >> size. >> I tried this with and without your 2/2 265k patch and also with page size >> configured to 16k, the error is the same in all cases. >> >> I used an earlier version of your patch in the past and it worked fine. >> Applying this old patch causes the same problem. >> Therefore I expect that there was some other code changed that breaks with >> page size != 4k. > >This patch seems to solve the problem for me, but I have to run and >haven't yet worked out if it's the right fix. > >diff --git a/mm/bootmem.c b/mm/bootmem.c >--- a/mm/bootmem.c >+++ b/mm/bootmem.c >@@ -300,7 +300,7 @@ static int __init mark_bootmem(unsigned > unsigned long max; > > if (pos < bdata->node_min_pfn || >- pos >= bdata->node_low_pfn) { >+ pos > bdata->node_low_pfn) { > BUG_ON(pos != start); > continue; > } >@@ -399,7 +399,7 @@ int __init reserve_bootmem(unsigned long > unsigned long start, end; > > start = PFN_DOWN(addr); >- end = PFN_UP(addr + size); >+ end = PFN_DOWN(addr + size); > > return mark_bootmem(start, end, 1, flags); > } > >Looks like the breakage may have been accidentally introduced by >Johannes Weiner <hannes@saeurebad.de> on Jul 24 (post 2.6.26). > >FWIW, the boards Christian and I are hitting the problem on are >Sequoias with 256MB of RAM. cuImage is reporting only 0xffff000 bytes >of RAM though, which may be exacerbating the situation. That is on purpose. The chip has an errata that causes badness if you use the last XX bytes of DRAM. I forget exactly what XX is, but we just remove the last page. josh
On Sat, 2008-11-01 at 07:30 -0400, Josh Boyer wrote: > > That is on purpose. The chip has an errata that causes badness if > you use the last XX bytes of DRAM. I forget exactly what XX is, but > we just remove the last page. Doing that from the device-tree is very hairy tho... you end up with informations in there that aren't aligned etc... oh well. Ben.
On Sun, Nov 02, 2008 at 08:55:02AM +1100, Benjamin Herrenschmidt wrote: >On Sat, 2008-11-01 at 07:30 -0400, Josh Boyer wrote: >> >> That is on purpose. The chip has an errata that causes badness if >> you use the last XX bytes of DRAM. I forget exactly what XX is, but >> we just remove the last page. > >Doing that from the device-tree is very hairy tho... you end up with >informations in there that aren't aligned etc... oh well. What? -ENOTVERBOSEENOUGH. I don't see how this is really different from U-Boot just passing in a smaller memory size in the old arch/ppc world. (And I think U-Boot will actually fixup the device tree in a similar manner itself these days.) So if there are problems with this, please do tell. josh
On Sun, 2008-11-02 at 08:41 -0500, Josh Boyer wrote: > On Sun, Nov 02, 2008 at 08:55:02AM +1100, Benjamin Herrenschmidt wrote: > >On Sat, 2008-11-01 at 07:30 -0400, Josh Boyer wrote: > >> > >> That is on purpose. The chip has an errata that causes badness if > >> you use the last XX bytes of DRAM. I forget exactly what XX is, but > >> we just remove the last page. > > > >Doing that from the device-tree is very hairy tho... you end up with > >informations in there that aren't aligned etc... oh well. > > What? -ENOTVERBOSEENOUGH. > > I don't see how this is really different from U-Boot just passing in > a smaller memory size in the old arch/ppc world. (And I think U-Boot > will actually fixup the device tree in a similar manner itself these > days.) So if there are problems with this, please do tell. Is it cropping the memory nodes or using the reserve map ? Ben.
On Mon, 03 Nov 2008 08:33:16 +1100 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Sun, 2008-11-02 at 08:41 -0500, Josh Boyer wrote: > > On Sun, Nov 02, 2008 at 08:55:02AM +1100, Benjamin Herrenschmidt wrote: > > >On Sat, 2008-11-01 at 07:30 -0400, Josh Boyer wrote: > > >> > > >> That is on purpose. The chip has an errata that causes badness if > > >> you use the last XX bytes of DRAM. I forget exactly what XX is, but > > >> we just remove the last page. > > > > > >Doing that from the device-tree is very hairy tho... you end up with > > >informations in there that aren't aligned etc... oh well. > > > > What? -ENOTVERBOSEENOUGH. > > > > I don't see how this is really different from U-Boot just passing in > > a smaller memory size in the old arch/ppc world. (And I think U-Boot > > will actually fixup the device tree in a similar manner itself these > > days.) So if there are problems with this, please do tell. > > Is it cropping the memory nodes or using the reserve map ? Cropping the size of the memory node. That was simplest to do from the cuboot wrapper at the time. If marking it reserved via a reserve map is more elegant and correct, we could do that. But I will still like to know what about the other way is hairy please. josh
> Cropping the size of the memory node. That was simplest to do from the > cuboot wrapper at the time. If marking it reserved via a reserve map > is more elegant and correct, we could do that. > > But I will still like to know what about the other way is hairy please. I don't like it :-) Bad feeling ... don't like having a memory node entry that isn't aligned to some large power of two typically. Cheers, Ben.
On Mon, 03 Nov 2008 11:43:54 +1100 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > > > Cropping the size of the memory node. That was simplest to do from the > > cuboot wrapper at the time. If marking it reserved via a reserve map > > is more elegant and correct, we could do that. > > > > But I will still like to know what about the other way is hairy please. > > I don't like it :-) Bad feeling ... don't like having a memory > node entry that isn't aligned to some large power of two typically. Erm, ok. And does your heebie-geebies extend to people using the mem= parameter in a similar fashion? josh
On Mon, 2008-11-03 at 06:26 -0500, Josh Boyer wrote: > On Mon, 03 Nov 2008 11:43:54 +1100 > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > > > > > > Cropping the size of the memory node. That was simplest to do from the > > > cuboot wrapper at the time. If marking it reserved via a reserve map > > > is more elegant and correct, we could do that. > > > > > > But I will still like to know what about the other way is hairy please. > > > > I don't like it :-) Bad feeling ... don't like having a memory > > node entry that isn't aligned to some large power of two typically. > > Erm, ok. And does your heebie-geebies extend to people using the mem= > parameter in a similar fashion? Nah, not really. It's not that it won't work, I suppose it does, though I would have preferred a way to "reserve" that memory rather than take it off. In fact, that last page could be used for other things, for example it could be used as a dummy page to point stale DMA to or whatever else. Ben.
On Fri, Oct 31, 2008 at 06:23:28PM -0500, Hollis Blanchard wrote: >On Wed, Oct 22, 2008 at 9:28 AM, Christian Ehrhardt ><ehrhardt@linux.vnet.ibm.com> wrote: >> Hi Ilya, >> I just tried your patch on my 440 board because it would help us in our >> environment. >> Unfortunately I run into a bug on early boot (mark_bootmem). >> >> A log can be found in this mail, this is the bug when running with 64k page >> size. >> I tried this with and without your 2/2 265k patch and also with page size >> configured to 16k, the error is the same in all cases. >> >> I used an earlier version of your patch in the past and it worked fine. >> Applying this old patch causes the same problem. >> Therefore I expect that there was some other code changed that breaks with >> page size != 4k. > >This patch seems to solve the problem for me, but I have to run and >haven't yet worked out if it's the right fix. > >diff --git a/mm/bootmem.c b/mm/bootmem.c >--- a/mm/bootmem.c >+++ b/mm/bootmem.c >@@ -300,7 +300,7 @@ static int __init mark_bootmem(unsigned > unsigned long max; > > if (pos < bdata->node_min_pfn || >- pos >= bdata->node_low_pfn) { >+ pos > bdata->node_low_pfn) { > BUG_ON(pos != start); > continue; > } >@@ -399,7 +399,7 @@ int __init reserve_bootmem(unsigned long > unsigned long start, end; > > start = PFN_DOWN(addr); >- end = PFN_UP(addr + size); >+ end = PFN_DOWN(addr + size); > > return mark_bootmem(start, end, 1, flags); > } Hollis, if I'm understanding things correctly this patch is no longer needed if we do the memory reserve in the boot wrapper for the errata. Correct? josh
On Tue, 2008-11-11 at 08:19 -0500, Josh Boyer wrote: > On Fri, Oct 31, 2008 at 06:23:28PM -0500, Hollis Blanchard wrote: > >On Wed, Oct 22, 2008 at 9:28 AM, Christian Ehrhardt > ><ehrhardt@linux.vnet.ibm.com> wrote: > >> Hi Ilya, > >> I just tried your patch on my 440 board because it would help us in our > >> environment. > >> Unfortunately I run into a bug on early boot (mark_bootmem). > >> > >> A log can be found in this mail, this is the bug when running with 64k page > >> size. > >> I tried this with and without your 2/2 265k patch and also with page size > >> configured to 16k, the error is the same in all cases. > >> > >> I used an earlier version of your patch in the past and it worked fine. > >> Applying this old patch causes the same problem. > >> Therefore I expect that there was some other code changed that breaks with > >> page size != 4k. > > > >This patch seems to solve the problem for me, but I have to run and > >haven't yet worked out if it's the right fix. > > > >diff --git a/mm/bootmem.c b/mm/bootmem.c > >--- a/mm/bootmem.c > >+++ b/mm/bootmem.c > >@@ -300,7 +300,7 @@ static int __init mark_bootmem(unsigned > > unsigned long max; > > > > if (pos < bdata->node_min_pfn || > >- pos >= bdata->node_low_pfn) { > >+ pos > bdata->node_low_pfn) { > > BUG_ON(pos != start); > > continue; > > } > >@@ -399,7 +399,7 @@ int __init reserve_bootmem(unsigned long > > unsigned long start, end; > > > > start = PFN_DOWN(addr); > >- end = PFN_UP(addr + size); > >+ end = PFN_DOWN(addr + size); > > > > return mark_bootmem(start, end, 1, flags); > > } > > > Hollis, if I'm understanding things correctly this patch is no > longer needed if we do the memory reserve in the boot wrapper for > the errata. Correct? Correct.
diff --git a/mm/bootmem.c b/mm/bootmem.c --- a/mm/bootmem.c +++ b/mm/bootmem.c @@ -300,7 +300,7 @@ static int __init mark_bootmem(unsigned unsigned long max; if (pos < bdata->node_min_pfn || - pos >= bdata->node_low_pfn) { + pos > bdata->node_low_pfn) { BUG_ON(pos != start); continue; } @@ -399,7 +399,7 @@ int __init reserve_bootmem(unsigned long unsigned long start, end; start = PFN_DOWN(addr); - end = PFN_UP(addr + size); + end = PFN_DOWN(addr + size); return mark_bootmem(start, end, 1, flags); }