* [PATCH] riscv: fixup max_low_pfn with PFN_DOWN. @ 2019-01-12 8:16 guoren 2019-01-15 15:36 ` Christoph Hellwig 0 siblings, 1 reply; 7+ messages in thread From: guoren @ 2019-01-12 8:16 UTC (permalink / raw) To: palmer, aou; +Cc: linux-arch, linux-riscv, linux-kernel, Guo Ren, Mao Han From: Guo Ren <ren_guo@c-sky.com> max_low_pfn should be pfn_size not byte_size. Signed-off-by: Guo Ren <ren_guo@c-sky.com> Signed-off-by: Mao Han <mao_han@c-sky.com> Cc: Palmer Dabbelt <palmer@sifive.com> Cc: Albert Ou <aou@eecs.berkeley.edu> --- arch/riscv/kernel/setup.c | 2 +- arch/riscv/mm/init.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index fc8006a..5463e67 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -174,7 +174,7 @@ static void __init setup_bootmem(void) BUG_ON(mem_size == 0); set_max_mapnr(PFN_DOWN(mem_size)); - max_low_pfn = memblock_end_of_DRAM(); + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); #ifdef CONFIG_BLK_DEV_INITRD setup_initrd(); diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c index 1d9bfaf..658ebf6 100644 --- a/arch/riscv/mm/init.c +++ b/arch/riscv/mm/init.c @@ -28,7 +28,8 @@ static void __init zone_sizes_init(void) unsigned long max_zone_pfns[MAX_NR_ZONES] = { 0, }; #ifdef CONFIG_ZONE_DMA32 - max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, max_low_pfn)); + max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, + (unsigned long) PFN_PHYS(max_low_pfn))); #endif max_zone_pfns[ZONE_NORMAL] = max_low_pfn; -- 2.7.4 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv: fixup max_low_pfn with PFN_DOWN. 2019-01-12 8:16 [PATCH] riscv: fixup max_low_pfn with PFN_DOWN guoren @ 2019-01-15 15:36 ` Christoph Hellwig 2019-01-15 16:10 ` Guo Ren 0 siblings, 1 reply; 7+ messages in thread From: Christoph Hellwig @ 2019-01-15 15:36 UTC (permalink / raw) To: guoren Cc: linux-arch, aou, palmer, linux-kernel, Mao Han, Guo Ren, linux-riscv On Sat, Jan 12, 2019 at 04:16:27PM +0800, guoren@kernel.org wrote: > From: Guo Ren <ren_guo@c-sky.com> > > max_low_pfn should be pfn_size not byte_size. > > Signed-off-by: Guo Ren <ren_guo@c-sky.com> > Signed-off-by: Mao Han <mao_han@c-sky.com> > Cc: Palmer Dabbelt <palmer@sifive.com> > Cc: Albert Ou <aou@eecs.berkeley.edu> > --- > arch/riscv/kernel/setup.c | 2 +- > arch/riscv/mm/init.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index fc8006a..5463e67 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -174,7 +174,7 @@ static void __init setup_bootmem(void) > BUG_ON(mem_size == 0); > > set_max_mapnr(PFN_DOWN(mem_size)); > - max_low_pfn = memblock_end_of_DRAM(); > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); I know it is used just above, but can we please just switch this code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly obsfucates what is going on? > > #ifdef CONFIG_BLK_DEV_INITRD > setup_initrd(); > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > index 1d9bfaf..658ebf6 100644 > --- a/arch/riscv/mm/init.c > +++ b/arch/riscv/mm/init.c > @@ -28,7 +28,8 @@ static void __init zone_sizes_init(void) > unsigned long max_zone_pfns[MAX_NR_ZONES] = { 0, }; > > #ifdef CONFIG_ZONE_DMA32 > - max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, max_low_pfn)); > + max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, > + (unsigned long) PFN_PHYS(max_low_pfn))); > #endif Same comment as above here, plus I think we should just use memblock_end_of_DRAM directly, e.g. something like: static const phys_addr_t max_dma32_addr = 4UL * SZ_1G; max_zone_pfns[ZONE_DMA32] = min(memblock_end_of_DRAM(), max_dma32_addr) >> PAGE_SHIFT; _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv: fixup max_low_pfn with PFN_DOWN. 2019-01-15 15:36 ` Christoph Hellwig @ 2019-01-15 16:10 ` Guo Ren 2019-01-15 16:12 ` Christoph Hellwig 0 siblings, 1 reply; 7+ messages in thread From: Guo Ren @ 2019-01-15 16:10 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-arch, aou, palmer, linux-kernel, Mao Han, Guo Ren, linux-riscv On Tue, Jan 15, 2019 at 07:36:13AM -0800, Christoph Hellwig wrote: > On Sat, Jan 12, 2019 at 04:16:27PM +0800, guoren@kernel.org wrote: > > From: Guo Ren <ren_guo@c-sky.com> > > > > max_low_pfn should be pfn_size not byte_size. > > > > Signed-off-by: Guo Ren <ren_guo@c-sky.com> > > Signed-off-by: Mao Han <mao_han@c-sky.com> > > Cc: Palmer Dabbelt <palmer@sifive.com> > > Cc: Albert Ou <aou@eecs.berkeley.edu> > > --- > > arch/riscv/kernel/setup.c | 2 +- > > arch/riscv/mm/init.c | 3 ++- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index fc8006a..5463e67 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -174,7 +174,7 @@ static void __init setup_bootmem(void) > > BUG_ON(mem_size == 0); > > > > set_max_mapnr(PFN_DOWN(mem_size)); > > - max_low_pfn = memblock_end_of_DRAM(); > > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); > > I know it is used just above, but can we please just switch this > code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly > obsfucates what is going on? ??? #define PFN_DOWN(x) ((x) >> PAGE_SHIFT) phys_addr_t __init_memblock memblock_end_of_DRAM(void) { int idx = memblock.memory.cnt - 1; return (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size); } What's the problem? PFN_DOWN() couldn't be used with function call? > > > > > #ifdef CONFIG_BLK_DEV_INITRD > > setup_initrd(); > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > > index 1d9bfaf..658ebf6 100644 > > --- a/arch/riscv/mm/init.c > > +++ b/arch/riscv/mm/init.c > > @@ -28,7 +28,8 @@ static void __init zone_sizes_init(void) > > unsigned long max_zone_pfns[MAX_NR_ZONES] = { 0, }; > > > > #ifdef CONFIG_ZONE_DMA32 > > - max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, max_low_pfn)); > > + max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, > > + (unsigned long) PFN_PHYS(max_low_pfn))); > > #endif > > Same comment as above here, plus I think we should just use > memblock_end_of_DRAM directly, e.g. something like: > > static const phys_addr_t max_dma32_addr = 4UL * SZ_1G; > > max_zone_pfns[ZONE_DMA32] = > min(memblock_end_of_DRAM(), max_dma32_addr) >> PAGE_SHIFT; Em... The meaning of PFN_PHYS(max_low_pfn) != memblock_end_of_DRAM() in 32-bit highmem system. Of cause, riscv doesn't support highmem, so I think memblock_end_of_DRAM() is also OK. But... static void __init zone_sizes_init(void) { unsigned long max_zone_pfns[MAX_NR_ZONES] = { 0, }; #ifdef CONFIG_ZONE_DMA32 max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, max_low_pfn)); #endif max_zone_pfns[ZONE_NORMAL] = max_low_pfn; free_area_init_nodes(max_zone_pfns); } The max_low_pfn also used by ZONE_NORMAL, So shall we need change that? max_zone_pfns[ZONE_NORMAL] = PFN_DOWN(memblock_end_of_DRAM()); ^^^^^^^^ also must >> PAGE_SHIFT? My patch just want to point out that max_low_pfn is PFN not size. In fact there is no error for running without my patch :P Best Regards Guo Ren _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv: fixup max_low_pfn with PFN_DOWN. 2019-01-15 16:10 ` Guo Ren @ 2019-01-15 16:12 ` Christoph Hellwig 2019-01-16 1:07 ` Guo Ren 0 siblings, 1 reply; 7+ messages in thread From: Christoph Hellwig @ 2019-01-15 16:12 UTC (permalink / raw) To: Guo Ren Cc: linux-arch, aou, palmer, linux-kernel, Mao Han, Christoph Hellwig, Guo Ren, linux-riscv On Wed, Jan 16, 2019 at 12:10:00AM +0800, Guo Ren wrote: > > > set_max_mapnr(PFN_DOWN(mem_size)); > > > - max_low_pfn = memblock_end_of_DRAM(); > > > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); > > > > I know it is used just above, but can we please just switch this > > code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly > > obsfucates what is going on? > ??? > #define PFN_DOWN(x) ((x) >> PAGE_SHIFT) > > phys_addr_t __init_memblock memblock_end_of_DRAM(void) > { > int idx = memblock.memory.cnt - 1; > > return (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size); > } > > What's the problem? PFN_DOWN() couldn't be used with function call? PFN_DOWN gives you the correct result. But I think it actually drastically reduces readability over just opencoding it. > My patch just want to point out that max_low_pfn is PFN not size. In fact > there is no error for running without my patch :P No, I think your patch is correct. I just wonder if we could make the code easier to read. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv: fixup max_low_pfn with PFN_DOWN. 2019-01-15 16:12 ` Christoph Hellwig @ 2019-01-16 1:07 ` Guo Ren 2019-01-24 2:00 ` Palmer Dabbelt 0 siblings, 1 reply; 7+ messages in thread From: Guo Ren @ 2019-01-16 1:07 UTC (permalink / raw) To: Christoph Hellwig Cc: linux-arch, aou, palmer, linux-kernel, Mao Han, Guo Ren, linux-riscv Hi Christoph, I use PFN_DOWN() every where as possible and seems it's a habit problem. So let risc-v maintainer to choose "PFN_DOW()" or ">> PAGE_SHIFT". Also the same with "end_of_DRAM & max_low_pfn". Best Regards Guo Ren On Tue, Jan 15, 2019 at 08:12:54AM -0800, Christoph Hellwig wrote: > On Wed, Jan 16, 2019 at 12:10:00AM +0800, Guo Ren wrote: > > > > set_max_mapnr(PFN_DOWN(mem_size)); > > > > - max_low_pfn = memblock_end_of_DRAM(); > > > > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); > > > > > > I know it is used just above, but can we please just switch this > > > code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly > > > obsfucates what is going on? > > ??? > > #define PFN_DOWN(x) ((x) >> PAGE_SHIFT) > > > > phys_addr_t __init_memblock memblock_end_of_DRAM(void) > > { > > int idx = memblock.memory.cnt - 1; > > > > return (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size); > > } > > > > What's the problem? PFN_DOWN() couldn't be used with function call? > > PFN_DOWN gives you the correct result. But I think it actually > drastically reduces readability over just opencoding it. > > > My patch just want to point out that max_low_pfn is PFN not size. In fact > > there is no error for running without my patch :P > > No, I think your patch is correct. I just wonder if we could make > the code easier to read. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv: fixup max_low_pfn with PFN_DOWN. 2019-01-16 1:07 ` Guo Ren @ 2019-01-24 2:00 ` Palmer Dabbelt 2019-01-24 2:31 ` Guo Ren 0 siblings, 1 reply; 7+ messages in thread From: Palmer Dabbelt @ 2019-01-24 2:00 UTC (permalink / raw) To: guoren Cc: linux-arch, aou, linux-kernel, mao_han, Christoph Hellwig, ren_guo, linux-riscv On Tue, 15 Jan 2019 17:07:38 PST (-0800), guoren@kernel.org wrote: > Hi Christoph, > > I use PFN_DOWN() every where as possible and seems it's a habit > problem. So let risc-v maintainer to choose "PFN_DOW()" or > ">> PAGE_SHIFT". > > Also the same with "end_of_DRAM & max_low_pfn". PFN_DOWN makes sense to me, as that's what we're trying to do here (round a physical address down to page frame number). Am a I misunderstanding something? > > Best Regards > Guo Ren > > On Tue, Jan 15, 2019 at 08:12:54AM -0800, Christoph Hellwig wrote: >> On Wed, Jan 16, 2019 at 12:10:00AM +0800, Guo Ren wrote: >> > > > set_max_mapnr(PFN_DOWN(mem_size)); >> > > > - max_low_pfn = memblock_end_of_DRAM(); >> > > > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); >> > > >> > > I know it is used just above, but can we please just switch this >> > > code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly >> > > obsfucates what is going on? >> > ??? >> > #define PFN_DOWN(x) ((x) >> PAGE_SHIFT) >> > >> > phys_addr_t __init_memblock memblock_end_of_DRAM(void) >> > { >> > int idx = memblock.memory.cnt - 1; >> > >> > return (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size); >> > } >> > >> > What's the problem? PFN_DOWN() couldn't be used with function call? >> >> PFN_DOWN gives you the correct result. But I think it actually >> drastically reduces readability over just opencoding it. >> >> > My patch just want to point out that max_low_pfn is PFN not size. In fact >> > there is no error for running without my patch :P >> >> No, I think your patch is correct. I just wonder if we could make >> the code easier to read. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] riscv: fixup max_low_pfn with PFN_DOWN. 2019-01-24 2:00 ` Palmer Dabbelt @ 2019-01-24 2:31 ` Guo Ren 0 siblings, 0 replies; 7+ messages in thread From: Guo Ren @ 2019-01-24 2:31 UTC (permalink / raw) To: Palmer Dabbelt Cc: linux-arch, aou, linux-kernel, mao_han, Christoph Hellwig, ren_guo, linux-riscv On Wed, Jan 23, 2019 at 06:00:35PM -0800, Palmer Dabbelt wrote: > On Tue, 15 Jan 2019 17:07:38 PST (-0800), guoren@kernel.org wrote: > >Hi Christoph, > > > >I use PFN_DOWN() every where as possible and seems it's a habit > >problem. So let risc-v maintainer to choose "PFN_DOW()" or > >">> PAGE_SHIFT". > > > >Also the same with "end_of_DRAM & max_low_pfn". > > PFN_DOWN makes sense to me, as that's what we're trying to do here (round a > physical address down to page frame number). Am a I misunderstanding > something? > No, you got it :) Best Regards Guo Ren _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-24 2:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-12 8:16 [PATCH] riscv: fixup max_low_pfn with PFN_DOWN guoren 2019-01-15 15:36 ` Christoph Hellwig 2019-01-15 16:10 ` Guo Ren 2019-01-15 16:12 ` Christoph Hellwig 2019-01-16 1:07 ` Guo Ren 2019-01-24 2:00 ` Palmer Dabbelt 2019-01-24 2:31 ` Guo Ren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).