* [PATCH] Fix virt_to_phys() warnings
@ 2009-06-27 0:13 Kevin Cernekee
2009-07-02 5:46 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Cernekee @ 2009-06-27 0:13 UTC (permalink / raw)
To: linux-kernel
These warnings were observed on MIPS32 using 2.6.31-rc1 and gcc-4.2.0:
mm/page_alloc.c: In function 'alloc_pages_exact':
mm/page_alloc.c:1986: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
drivers/usb/mon/mon_bin.c: In function 'mon_alloc_buff':
drivers/usb/mon/mon_bin.c:1264: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index f8d9045..0f7a30b 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1261,7 +1261,7 @@ static int mon_alloc_buff(struct mon_pgmap *map, int npages)
return -ENOMEM;
}
map[n].ptr = (unsigned char *) vaddr;
- map[n].pg = virt_to_page(vaddr);
+ map[n].pg = virt_to_page((void *) vaddr);
}
return 0;
}
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5d714f8..f6180db 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1983,7 +1983,7 @@ void *alloc_pages_exact(size_t size, gfp_t gfp_mask)
unsigned long alloc_end = addr + (PAGE_SIZE << order);
unsigned long used = addr + PAGE_ALIGN(size);
- split_page(virt_to_page(addr), order);
+ split_page(virt_to_page((void *)addr), order);
while (used < alloc_end) {
free_page(used);
used += PAGE_SIZE;
--
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix virt_to_phys() warnings
2009-06-27 0:13 [PATCH] Fix virt_to_phys() warnings Kevin Cernekee
@ 2009-07-02 5:46 ` Andrew Morton
2009-07-02 7:07 ` Andi Kleen
2009-07-02 10:40 ` Ralf Baechle
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Morton @ 2009-07-02 5:46 UTC (permalink / raw)
To: Kevin Cernekee; +Cc: linux-kernel, Ralf Baechle
On Fri, 26 Jun 2009 17:13:20 -0700 Kevin Cernekee <cernekee@gmail.com> wrote:
> These warnings were observed on MIPS32 using 2.6.31-rc1 and gcc-4.2.0:
>
> mm/page_alloc.c: In function 'alloc_pages_exact':
> mm/page_alloc.c:1986: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
>
> drivers/usb/mon/mon_bin.c: In function 'mon_alloc_buff':
> drivers/usb/mon/mon_bin.c:1264: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
>
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index f8d9045..0f7a30b 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -1261,7 +1261,7 @@ static int mon_alloc_buff(struct mon_pgmap *map, int npages)
> return -ENOMEM;
> }
> map[n].ptr = (unsigned char *) vaddr;
> - map[n].pg = virt_to_page(vaddr);
> + map[n].pg = virt_to_page((void *) vaddr);
> }
> return 0;
> }
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5d714f8..f6180db 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1983,7 +1983,7 @@ void *alloc_pages_exact(size_t size, gfp_t gfp_mask)
> unsigned long alloc_end = addr + (PAGE_SIZE << order);
> unsigned long used = addr + PAGE_ALIGN(size);
>
> - split_page(virt_to_page(addr), order);
> + split_page(virt_to_page((void *)addr), order);
> while (used < alloc_end) {
> free_page(used);
> used += PAGE_SIZE;
The virt_to_foo() functions were written back in the Linux dark ages and
they're horrid. A byzantine mixture of macros and typecasts which make
it really hard to work out what type their argument actually should be.
virt_to_page() takes an argument of type `unsigned long'. (except for
the include/asm-generic/page.h version which takes any damn type at
all).
virt_to_phys() takes an argument of `const void *' (weird, huh?)
But arch/mips/include/asm/page.h does
#define virt_to_page(kaddr) pfn_to_page(PFN_DOWN(virt_to_phys(kaddr)))
thereby passing an `unsigned long' where a void* was expected.
So perhaps something along these lines:
--- a/arch/mips/include/asm/page.h~a
+++ a/arch/mips/include/asm/page.h
@@ -184,7 +184,11 @@ typedef struct { unsigned long pgprot; }
#endif
-#define virt_to_page(kaddr) pfn_to_page(PFN_DOWN(virt_to_phys(kaddr)))
+static inline struct page *virt_to_page(unsigned long kaddr)
+{
+ return pfn_to_page(PFN_DOWN(virt_to_phys((void *)kaddr)));
+}
+
#define virt_addr_valid(kaddr) pfn_valid(PFN_DOWN(virt_to_phys(kaddr)))
#define VM_DATA_DEFAULT_FLAGS (VM_READ | VM_WRITE | VM_EXEC | \
_
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix virt_to_phys() warnings
2009-07-02 5:46 ` Andrew Morton
@ 2009-07-02 7:07 ` Andi Kleen
2009-07-02 8:26 ` Ralf Baechle
2009-07-02 10:40 ` Ralf Baechle
1 sibling, 1 reply; 6+ messages in thread
From: Andi Kleen @ 2009-07-02 7:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: Kevin Cernekee, linux-kernel, Ralf Baechle
Andrew Morton <akpm@linux-foundation.org> writes:
>
> #define virt_to_page(kaddr) pfn_to_page(PFN_DOWN(virt_to_phys(kaddr)))
>
> thereby passing an `unsigned long' where a void* was expected.
>
>
> So perhaps something along these lines:
I'm not quite sure it's the case on MIPS, but I remember on x86 one of
the reasons this was done as a macro was to avoid a include dependency
loop with struct page.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix virt_to_phys() warnings
2009-07-02 7:07 ` Andi Kleen
@ 2009-07-02 8:26 ` Ralf Baechle
0 siblings, 0 replies; 6+ messages in thread
From: Ralf Baechle @ 2009-07-02 8:26 UTC (permalink / raw)
To: Andi Kleen; +Cc: Andrew Morton, Kevin Cernekee, linux-kernel
On Thu, Jul 02, 2009 at 09:07:46AM +0200, Andi Kleen wrote:
> From: Andi Kleen <andi@firstfloor.org>
> Date: Thu, 02 Jul 2009 09:07:46 +0200
> To: Andrew Morton <akpm@linux-foundation.org>
> Cc: Kevin Cernekee <cernekee@gmail.com>, linux-kernel@vger.kernel.org,
> Ralf Baechle <ralf@linux-mips.org>
> Subject: Re: [PATCH] Fix virt_to_phys() warnings
> Content-Type: text/plain; charset=us-ascii
>
> Andrew Morton <akpm@linux-foundation.org> writes:
> >
> > #define virt_to_page(kaddr) pfn_to_page(PFN_DOWN(virt_to_phys(kaddr)))
> >
> > thereby passing an `unsigned long' where a void* was expected.
> >
> >
> > So perhaps something along these lines:
>
> I'm not quite sure it's the case on MIPS, but I remember on x86 one of
> the reasons this was done as a macro was to avoid a include dependency
> loop with struct page.
The MIPS <asm/page.h> uses a forward declaration for the same reason since
a very long time. These days struct page is defined in <linux/mm_types.h>
but that file needs pgprot_t (which is defined in <asm/page.h> ...) to be
defined first so can't be included from the to of page.h.
Ralf
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix virt_to_phys() warnings
2009-07-02 5:46 ` Andrew Morton
2009-07-02 7:07 ` Andi Kleen
@ 2009-07-02 10:40 ` Ralf Baechle
2009-07-02 16:25 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Ralf Baechle @ 2009-07-02 10:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: Kevin Cernekee, linux-kernel
On Wed, Jul 01, 2009 at 10:46:27PM -0700, Andrew Morton wrote:
> > mm/page_alloc.c: In function 'alloc_pages_exact':
> > mm/page_alloc.c:1986: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
> >
> > drivers/usb/mon/mon_bin.c: In function 'mon_alloc_buff':
> > drivers/usb/mon/mon_bin.c:1264: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
> >
> > Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> > ---
> > diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> > index f8d9045..0f7a30b 100644
> > --- a/drivers/usb/mon/mon_bin.c
> > +++ b/drivers/usb/mon/mon_bin.c
> > @@ -1261,7 +1261,7 @@ static int mon_alloc_buff(struct mon_pgmap *map, int npages)
> > return -ENOMEM;
> > }
> > map[n].ptr = (unsigned char *) vaddr;
> > - map[n].pg = virt_to_page(vaddr);
> > + map[n].pg = virt_to_page((void *) vaddr);
> > }
> > return 0;
> > }
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 5d714f8..f6180db 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1983,7 +1983,7 @@ void *alloc_pages_exact(size_t size, gfp_t gfp_mask)
> > unsigned long alloc_end = addr + (PAGE_SIZE << order);
> > unsigned long used = addr + PAGE_ALIGN(size);
> >
> > - split_page(virt_to_page(addr), order);
> > + split_page(virt_to_page((void *)addr), order);
> > while (used < alloc_end) {
> > free_page(used);
> > used += PAGE_SIZE;
>
> The virt_to_foo() functions were written back in the Linux dark ages and
> they're horrid. A byzantine mixture of macros and typecasts which make
> it really hard to work out what type their argument actually should be.
No disagreement there.
> virt_to_page() takes an argument of type `unsigned long'. (except for
> the include/asm-generic/page.h version which takes any damn type at
> all).
You meant it takes a pointer argument, for example:
mm/page_alloc.c:
__free_pages(virt_to_page((void *)addr), order);
mm/slob.c:
static inline struct slob_page *slob_page(const void *addr)
{
return (struct slob_page *)virt_to_page(addr);
}
> virt_to_phys() takes an argument of `const void *' (weird, huh?)
volatile void * seems to be the most common type. MIPS is the only arch
that accepts volatile const void *; Alpha uses a plain void *.
I don't recall why MIPS uses volatile const - probably to silence warnings
about discarding qualifiers from pointer target type.
> But arch/mips/include/asm/page.h does
>
> #define virt_to_page(kaddr) pfn_to_page(PFN_DOWN(virt_to_phys(kaddr)))
>
> thereby passing an `unsigned long' where a void* was expected.
>
>
> So perhaps something along these lines:
>
>
> --- a/arch/mips/include/asm/page.h~a
> +++ a/arch/mips/include/asm/page.h
> @@ -184,7 +184,11 @@ typedef struct { unsigned long pgprot; }
>
> #endif
>
> -#define virt_to_page(kaddr) pfn_to_page(PFN_DOWN(virt_to_phys(kaddr)))
> +static inline struct page *virt_to_page(unsigned long kaddr)
> +{
> + return pfn_to_page(PFN_DOWN(virt_to_phys((void *)kaddr)));
> +}
> +
Doesn't compile because pfn_to_page is defined in <asm-generic/memory_model.h>
which will only be included further down and moving the order of the order
of things around to get this to build is getting ugly, so I gave up on it.
Anyway, since virt_to_page and virt_to_phys (imho :-) take the same
argument times I'm in favor of Kevin's patch.
Ralf
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix virt_to_phys() warnings
2009-07-02 10:40 ` Ralf Baechle
@ 2009-07-02 16:25 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2009-07-02 16:25 UTC (permalink / raw)
To: Ralf Baechle; +Cc: Kevin Cernekee, linux-kernel
On Thu, 2 Jul 2009 11:40:27 +0100 Ralf Baechle <ralf@linux-mips.org> wrote:
> On Wed, Jul 01, 2009 at 10:46:27PM -0700, Andrew Morton wrote:
>
> > > mm/page_alloc.c: In function 'alloc_pages_exact':
> > > mm/page_alloc.c:1986: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
> > >
> > > drivers/usb/mon/mon_bin.c: In function 'mon_alloc_buff':
> > > drivers/usb/mon/mon_bin.c:1264: warning: passing argument 1 of 'virt_to_phys' makes pointer from integer without a cast
> > >
> > > Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> > > ---
> > > diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> > > index f8d9045..0f7a30b 100644
> > > --- a/drivers/usb/mon/mon_bin.c
> > > +++ b/drivers/usb/mon/mon_bin.c
> > > @@ -1261,7 +1261,7 @@ static int mon_alloc_buff(struct mon_pgmap *map, int npages)
> > > return -ENOMEM;
> > > }
> > > map[n].ptr = (unsigned char *) vaddr;
> > > - map[n].pg = virt_to_page(vaddr);
> > > + map[n].pg = virt_to_page((void *) vaddr);
> > > }
> > > return 0;
> > > }
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 5d714f8..f6180db 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -1983,7 +1983,7 @@ void *alloc_pages_exact(size_t size, gfp_t gfp_mask)
> > > unsigned long alloc_end = addr + (PAGE_SIZE << order);
> > > unsigned long used = addr + PAGE_ALIGN(size);
> > >
> > > - split_page(virt_to_page(addr), order);
> > > + split_page(virt_to_page((void *)addr), order);
> > > while (used < alloc_end) {
> > > free_page(used);
> > > used += PAGE_SIZE;
> >
> > The virt_to_foo() functions were written back in the Linux dark ages and
> > they're horrid. A byzantine mixture of macros and typecasts which make
> > it really hard to work out what type their argument actually should be.
>
> No disagreement there.
>
> > virt_to_page() takes an argument of type `unsigned long'. (except for
> > the include/asm-generic/page.h version which takes any damn type at
> > all).
>
> You meant it takes a pointer argument, for example:
>
> mm/page_alloc.c:
> __free_pages(virt_to_page((void *)addr), order);
> mm/slob.c:
> static inline struct slob_page *slob_page(const void *addr)
> {
> return (struct slob_page *)virt_to_page(addr);
> }
>
lol. I think you're right. What a crock.
looky, they're popping up everywhere:
: static void perf_mmap_free_page(unsigned long addr)
: {
: struct page *page = virt_to_page(addr);
:
And mm/nommu.c:
: int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
: unsigned long start, int nr_pages, int flags,
: struct page **pages, struct vm_area_struct **vmas)
: {
: ...
: pages[i] = virt_to_page(start);
: static void free_page_series(unsigned long from, unsigned long to)
: {
: for (; from < to; from += PAGE_SIZE) {
: struct page *page = virt_to_page(from);
Heaven knows how many more there are. I guess we should drag x86's
virt_to_page()/virt_to_phys() into the 1980's by doing some
compile-time arg checking?
>
> ...
>
> Anyway, since virt_to_page and virt_to_phys (imho :-) take the same
> argument times I'm in favor of Kevin's patch.
yup, I'll queue it.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-07-02 16:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-06-27 0:13 [PATCH] Fix virt_to_phys() warnings Kevin Cernekee
2009-07-02 5:46 ` Andrew Morton
2009-07-02 7:07 ` Andi Kleen
2009-07-02 8:26 ` Ralf Baechle
2009-07-02 10:40 ` Ralf Baechle
2009-07-02 16:25 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox