| Message ID | 1530613795-6956-3-git-send-email-rppt@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue 03-07-18 13:29:54, Mike Rapoport wrote: > Add explicit casting to unsigned long to the __va() parameter Why is this needed? > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> > --- > arch/m68k/include/asm/page_no.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/m68k/include/asm/page_no.h b/arch/m68k/include/asm/page_no.h > index e644c4d..6bbe520 100644 > --- a/arch/m68k/include/asm/page_no.h > +++ b/arch/m68k/include/asm/page_no.h > @@ -18,7 +18,7 @@ extern unsigned long memory_end; > #define __HAVE_ARCH_ALLOC_ZEROED_USER_HIGHPAGE > > #define __pa(vaddr) ((unsigned long)(vaddr)) > -#define __va(paddr) ((void *)(paddr)) > +#define __va(paddr) ((void *)((unsigned long)(paddr))) > > #define virt_to_pfn(kaddr) (__pa(kaddr) >> PAGE_SHIFT) > #define pfn_to_virt(pfn) __va((pfn) << PAGE_SHIFT) > -- > 2.7.4
On Tue, Jul 03, 2018 at 04:20:54PM +0200, Michal Hocko wrote: > On Tue 03-07-18 13:29:54, Mike Rapoport wrote: > > Add explicit casting to unsigned long to the __va() parameter > > Why is this needed? To make it consitent with other architecures and asm-generic :) But more importantly, __memblock_free_late() passes u64 to page_to_pfn(). On m68k-nommu this results in: CC mm/nobootmem.o In file included from arch/m68k/include/asm/page.h:49, from arch/m68k/include/asm/thread_info.h:6, from include/linux/thread_info.h:38, from include/asm-generic/preempt.h:5, from ./arch/m68k/include/generated/asm/preempt.h:1, from include/linux/preempt.h:81, from include/linux/spinlock.h:51, from include/linux/mmzone.h:8, from include/linux/gfp.h:6, from include/linux/slab.h:15, from mm/memblock.c:14: mm/memblock.c: In function '__memblock_free_late': arch/m68k/include/asm/page_no.h:21:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] #define __va(paddr) ((void *)(paddr)) ^ arch/m68k/include/asm/page_no.h:26:57: note: in definition of macro 'virt_to_page' #define virt_to_page(addr) (mem_map + (((unsigned long)(addr)-PAGE_OFFSET) >> PAGE_SHIFT)) ^~~~ arch/m68k/include/asm/page_no.h:24:26: note: in expansion of macro '__va' #define pfn_to_virt(pfn) __va((pfn) << PAGE_SHIFT) ^~~~ arch/m68k/include/asm/page_no.h:29:39: note: in expansion of macro 'pfn_to_virt' #define pfn_to_page(pfn) virt_to_page(pfn_to_virt(pfn)) ^~~~~~~~~~~ mm/memblock.c:1473:24: note: in expansion of macro 'pfn_to_page' __free_pages_bootmem(pfn_to_page(cursor), cursor, 0); ^~~~~~~~~~~ > > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> > > --- > > arch/m68k/include/asm/page_no.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/m68k/include/asm/page_no.h b/arch/m68k/include/asm/page_no.h > > index e644c4d..6bbe520 100644 > > --- a/arch/m68k/include/asm/page_no.h > > +++ b/arch/m68k/include/asm/page_no.h > > @@ -18,7 +18,7 @@ extern unsigned long memory_end; > > #define __HAVE_ARCH_ALLOC_ZEROED_USER_HIGHPAGE > > > > #define __pa(vaddr) ((unsigned long)(vaddr)) > > -#define __va(paddr) ((void *)(paddr)) > > +#define __va(paddr) ((void *)((unsigned long)(paddr))) > > > > #define virt_to_pfn(kaddr) (__pa(kaddr) >> PAGE_SHIFT) > > #define pfn_to_virt(pfn) __va((pfn) << PAGE_SHIFT) > > -- > > 2.7.4 > > -- > Michal Hocko > SUSE Labs >
On Tue, Jul 03, 2018 at 06:03:16PM +0300, Mike Rapoport wrote: > On Tue, Jul 03, 2018 at 04:20:54PM +0200, Michal Hocko wrote: > > On Tue 03-07-18 13:29:54, Mike Rapoport wrote: > > > Add explicit casting to unsigned long to the __va() parameter > > > > Why is this needed? > > To make it consitent with other architecures and asm-generic :) > > But more importantly, __memblock_free_late() passes u64 to page_to_pfn(). Why does memblock work in terms of u64 instead of phys_addr_t?
On Tue 03-07-18 08:05:35, Matthew Wilcox wrote: > On Tue, Jul 03, 2018 at 06:03:16PM +0300, Mike Rapoport wrote: > > On Tue, Jul 03, 2018 at 04:20:54PM +0200, Michal Hocko wrote: > > > On Tue 03-07-18 13:29:54, Mike Rapoport wrote: > > > > Add explicit casting to unsigned long to the __va() parameter > > > > > > Why is this needed? > > > > To make it consitent with other architecures and asm-generic :) > > > > But more importantly, __memblock_free_late() passes u64 to page_to_pfn(). > > Why does memblock work in terms of u64 instead of phys_addr_t? Yes, phys_addr_t was exactly that came to my mind as well. Casting physical address to unsigned long just screams for potential problems.
On Tue, Jul 03, 2018 at 08:05:35AM -0700, Matthew Wilcox wrote: > On Tue, Jul 03, 2018 at 06:03:16PM +0300, Mike Rapoport wrote: > > On Tue, Jul 03, 2018 at 04:20:54PM +0200, Michal Hocko wrote: > > > On Tue 03-07-18 13:29:54, Mike Rapoport wrote: > > > > Add explicit casting to unsigned long to the __va() parameter > > > > > > Why is this needed? > > > > To make it consitent with other architecures and asm-generic :) > > > > But more importantly, __memblock_free_late() passes u64 to page_to_pfn(). > > Why does memblock work in terms of u64 instead of phys_addr_t? Historically? It started off with unsigned long, then commit e5f270954364 ("[LMB]: Make lmb support large physical addressing") converted it to u64 for 32-bit systems sake. And the definition of ARCH_PHYS_ADDR_T_64BIT in commit 600715dcdf56 ("generic: add phys_addr_t for holding physical addresses")) came in later.
On Tue, Jul 03, 2018 at 05:14:01PM +0200, Michal Hocko wrote: > On Tue 03-07-18 08:05:35, Matthew Wilcox wrote: > > On Tue, Jul 03, 2018 at 06:03:16PM +0300, Mike Rapoport wrote: > > > On Tue, Jul 03, 2018 at 04:20:54PM +0200, Michal Hocko wrote: > > > > On Tue 03-07-18 13:29:54, Mike Rapoport wrote: > > > > > Add explicit casting to unsigned long to the __va() parameter > > > > > > > > Why is this needed? > > > > > > To make it consitent with other architecures and asm-generic :) > > > > > > But more importantly, __memblock_free_late() passes u64 to page_to_pfn(). > > > > Why does memblock work in terms of u64 instead of phys_addr_t? > > Yes, phys_addr_t was exactly that came to my mind as well. Casting > physical address to unsigned long just screams for potential problems. Heh, that's what we have: ~/git/linux $ git grep 'define __va.*\(unsigned long\)' | wc -l 22 > -- > Michal Hocko > SUSE Labs >
On Tue, Jul 03, 2018 at 05:14:01PM +0200, Michal Hocko wrote: > On Tue 03-07-18 08:05:35, Matthew Wilcox wrote: > > On Tue, Jul 03, 2018 at 06:03:16PM +0300, Mike Rapoport wrote: > > > On Tue, Jul 03, 2018 at 04:20:54PM +0200, Michal Hocko wrote: > > > > On Tue 03-07-18 13:29:54, Mike Rapoport wrote: > > > > > Add explicit casting to unsigned long to the __va() parameter > > > > > > > > Why is this needed? > > > > > > To make it consitent with other architecures and asm-generic :) > > > > > > But more importantly, __memblock_free_late() passes u64 to page_to_pfn(). > > > > Why does memblock work in terms of u64 instead of phys_addr_t? > > Yes, phys_addr_t was exactly that came to my mind as well. Casting > physical address to unsigned long just screams for potential problems. Not sure if for m68k-nommu the physical address can really go beyond 32 bits, but in general this is something that should be taken care of. I think adding the cast in m68k-nommu case is a viable band aid to allow sorting out the bootmem vs nobootmem. In any case care should be taken of all those #define __va(x) ((void *)((unsigned long)(x))) all around. Regardless, I can s/u64/phys_addr_t/ in memblock.c. > -- > Michal Hocko > SUSE Labs >
On Tue 03-07-18 18:47:51, Mike Rapoport wrote: > On Tue, Jul 03, 2018 at 05:14:01PM +0200, Michal Hocko wrote: > > On Tue 03-07-18 08:05:35, Matthew Wilcox wrote: > > > On Tue, Jul 03, 2018 at 06:03:16PM +0300, Mike Rapoport wrote: > > > > On Tue, Jul 03, 2018 at 04:20:54PM +0200, Michal Hocko wrote: > > > > > On Tue 03-07-18 13:29:54, Mike Rapoport wrote: > > > > > > Add explicit casting to unsigned long to the __va() parameter > > > > > > > > > > Why is this needed? > > > > > > > > To make it consitent with other architecures and asm-generic :) > > > > > > > > But more importantly, __memblock_free_late() passes u64 to page_to_pfn(). > > > > > > Why does memblock work in terms of u64 instead of phys_addr_t? > > > > Yes, phys_addr_t was exactly that came to my mind as well. Casting > > physical address to unsigned long just screams for potential problems. > > Not sure if for m68k-nommu the physical address can really go beyond 32 > bits, but in general this is something that should be taken care of. > > I think adding the cast in m68k-nommu case is a viable band aid to allow > sorting out the bootmem vs nobootmem. > > In any case care should be taken of all those > > #define __va(x) ((void *)((unsigned long)(x))) Yeah, sounds like a good idea to me. > all around. > > Regardless, I can s/u64/phys_addr_t/ in memblock.c. Yeah, sounds like a good thing to me.
diff --git a/arch/m68k/include/asm/page_no.h b/arch/m68k/include/asm/page_no.h index e644c4d..6bbe520 100644 --- a/arch/m68k/include/asm/page_no.h +++ b/arch/m68k/include/asm/page_no.h @@ -18,7 +18,7 @@ extern unsigned long memory_end; #define __HAVE_ARCH_ALLOC_ZEROED_USER_HIGHPAGE #define __pa(vaddr) ((unsigned long)(vaddr)) -#define __va(paddr) ((void *)(paddr)) +#define __va(paddr) ((void *)((unsigned long)(paddr))) #define virt_to_pfn(kaddr) (__pa(kaddr) >> PAGE_SHIFT) #define pfn_to_virt(pfn) __va((pfn) << PAGE_SHIFT)
Add explicit casting to unsigned long to the __va() parameter Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> --- arch/m68k/include/asm/page_no.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)