diff mbox

[2/3] m68k/page_no.h: force __va argument to be unsigned long

Message ID 1530613795-6956-3-git-send-email-rppt@linux.vnet.ibm.com
State New, archived
Headers show

Commit Message

Mike Rapoport July 3, 2018, 10:29 a.m. UTC
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(-)

Comments

Michal Hocko July 3, 2018, 2:20 p.m. UTC | #1
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
Mike Rapoport July 3, 2018, 3:03 p.m. UTC | #2
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
>
Matthew Wilcox July 3, 2018, 3:05 p.m. UTC | #3
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?
Michal Hocko July 3, 2018, 3:14 p.m. UTC | #4
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.
Mike Rapoport July 3, 2018, 3:30 p.m. UTC | #5
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.
Mike Rapoport July 3, 2018, 3:39 p.m. UTC | #6
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
>
Mike Rapoport July 3, 2018, 3:47 p.m. UTC | #7
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
>
Michal Hocko July 3, 2018, 3:55 p.m. UTC | #8
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 mbox

Patch

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)