Skip to contentSkip to conversation

x86: silence warning in /dev/mem support

PATCH In review Pavel Machek 3 participants 7 messages Apr 16, 2015 – Apr 20, 2015

Subsystems (2)
x86 x86 MM
lore
Share & tools
lore

Pavel Machek

author
#0 lore raw

PATCH 1/1 x86: silence warning in /dev/mem support

The compiler is right, the code is tricky, but it is also correct AFAICT.

Signed-off-by Pavel Machek <[email protected]>
arch/x86/mm/ioremap.c +1 -1
index fdf617c..8a8dce8 100644
@@ -341,7 +341,7 @@ void *xlate_dev_mem_ptr(phys_addr_t phys)
 
 	addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
 	if (addr)
-		addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
+		addr = (void *)((unsigned long)addr | (unsigned long) (phys & ~PAGE_MASK));
 
 	return addr;
 }
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#1 lore raw
Quoted 10 lines · Pavel Machek

The compiler is right, the code is tricky, but it is also correct AFAICT.

Signed-off-by Pavel Machek <[email protected]>
arch/x86/mm/ioremap.c +0 -0
index fdf617c..8a8dce8 100644

arch/x86/mm/ioremap.c void *xlate_dev_mem_ptr(phys_addr_t phys) · line 344

Quoted 5 lines
arch/x86/mm/ioremap.c +1 -1
@@ -341,7 +341,7 @@ void *xlate_dev_mem_ptr(phys_addr_t phys)
 
 	addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
 	if (addr)
-		addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
+		addr = (void *)((unsigned long)addr | (unsigned long) (phys & ~PAGE_MASK));

What warning did it generate and which compiler version?

Thanks,

Ingo

author
#2 lore raw
Quoted 14 lines · Ingo Molnar
Quoted 11 lines · Pavel Machek

The compiler is right, the code is tricky, but it is also correct AFAICT.

Signed-off-by Pavel Machek <[email protected]>
arch/x86/mm/ioremap.c +0 -0
index fdf617c..8a8dce8 100644
@@ -341,7 +341,7 @@ void *xlate_dev_mem_ptr(phys_addr_t phys)
Quoted 6 lines

arch/x86/mm/ioremap.c void *xlate_dev_mem_ptr(phys_addr_t phys) · line 344

Quoted 4 lines
arch/x86/mm/ioremap.c +1 -1
 	addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
 	if (addr)
-		addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
+		addr = (void *)((unsigned long)addr | (unsigned long) (phys & ~PAGE_MASK));

What warning did it generate and which compiler version?

CC arch/x86/mm/ioremap.o
arch/x86/mm/ioremap.c: In function ‘xlate_dev_mem_ptr’:
arch/x86/mm/ioremap.c:363:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

     addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
               ^

LD arch/x86/mm/built-in.o

pavel@amd:/data/l/linux$ gcc --version
gcc (Debian 4.9.2-10) 4.9.2

(Patch should apply to today's 4.1-rc0, too).

Best regards,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#3 lore raw
Quoted 26 lines · Pavel Machek
Quoted 14 lines · Ingo Molnar
Quoted 11 lines · Pavel Machek

The compiler is right, the code is tricky, but it is also correct AFAICT.

Signed-off-by Pavel Machek <[email protected]>
arch/x86/mm/ioremap.c +0 -0
index fdf617c..8a8dce8 100644
@@ -341,7 +341,7 @@ void *xlate_dev_mem_ptr(phys_addr_t phys)
Quoted 6 lines

arch/x86/mm/ioremap.c void *xlate_dev_mem_ptr(phys_addr_t phys) · line 344

Quoted 4 lines
arch/x86/mm/ioremap.c +1 -1
 	addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
 	if (addr)
-		addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
+		addr = (void *)((unsigned long)addr | (unsigned long) (phys & ~PAGE_MASK));

What warning did it generate and which compiler version?

CC arch/x86/mm/ioremap.o
arch/x86/mm/ioremap.c: In function ‘xlate_dev_mem_ptr’:
arch/x86/mm/ioremap.c:363:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

Quoted 6 lines
     addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
               ^

LD arch/x86/mm/built-in.o

pavel@amd:/data/l/linux$ gcc --version
gcc (Debian 4.9.2-10) 4.9.2

So this code has not been changed for a year or so in theory, and I've never seen this warning with GCC 4.9.1. I'm wondering how it triggered. Could you send me your .config that triggers this?

Must have been introduced in:

4707a341b4af /dev/mem: Use more consistent data types

Thanks,

Ingo

author
#4 lore raw

Hi!

Quoted 7 lines
Quoted 7 lines

CC arch/x86/mm/ioremap.o
arch/x86/mm/ioremap.c: In function ‘xlate_dev_mem_ptr’:
arch/x86/mm/ioremap.c:363:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

     addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
               ^

LD arch/x86/mm/built-in.o

Quoted 6 lines
Quoted 2 lines
pavel@amd:/data/l/linux$ gcc --version
gcc (Debian 4.9.2-10) 4.9.2

So this code has not been changed for a year or so in theory, and I've never seen this warning with GCC 4.9.1. I'm wondering how it triggered. Could you send me your .config that triggers this?

Config sent in private email. 32-bit kernel.

Best regards,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#5 lore raw
Quoted 9 lines · Pavel Machek

Hi!

Quoted 7 lines
Quoted 7 lines

CC arch/x86/mm/ioremap.o
arch/x86/mm/ioremap.c: In function ‘xlate_dev_mem_ptr’:
arch/x86/mm/ioremap.c:363:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

     addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
               ^

LD arch/x86/mm/built-in.o

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
pavel@amd:/data/l/linux$ gcc --version
gcc (Debian 4.9.2-10) 4.9.2

So this code has not been changed for a year or so in theory, and I've never seen this warning with GCC 4.9.1. I'm wondering how it triggered. Could you send me your .config that triggers this?

Config sent in private email. 32-bit kernel.

Ah, I see, the key is:

CONFIG_HIGHMEM64G=y

and neither x86 defconfig nor allyesconfig/allmodconfig selects HIGHMEM64G apparently, so this was rarely triggered.

Thanks,

Ingo

merge bot
#6 lore raw

[tip:x86/urgent] x86/mm: Clean up types in xlate_dev_mem_ptr()

In reply to Pavel Machek
x86/mm: Clean up types in xlate_dev_mem_ptr()
Commit-ID:  94d4b4765b7ddb8478b0d57663cf7a08e2263bbf
Gitweb:     http://git.kernel.org/tip/94d4b4765b7ddb8478b0d57663cf7a08e2263bbf
Author:     Ingo Molnar <[email protected]>
AuthorDate: Fri, 23 Nov 2012 19:19:07 +0100
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 20 Apr 2015 08:41:37 +0200

x86/mm: Clean up types in xlate_dev_mem_ptr()

Pavel Machek reported the following compiler warning on x86/32 CONFIG_HIGHMEM64G=y builds:

arch/x86/mm/ioremap.c:344:10: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

Clean up the types in this function by using a single natural type for internal calculations (unsigned long), to make it more apparent what's happening, and also to remove fragile casts.

Reported-by Pavel Machek <[email protected]>
Cc [email protected]
Cc [email protected]
Link http://lkml.kernel.org/r/20150416080440.GA507@amd open here
Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86/mm/ioremap.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
arch/x86/mm/ioremap.c +8 -6
index fdf617c..4bf037b 100644
@@ -332,18 +332,20 @@ EXPORT_SYMBOL(iounmap);
  */
 void *xlate_dev_mem_ptr(phys_addr_t phys)
 {
-	void *addr;
-	unsigned long start = phys & PAGE_MASK;
+	unsigned long start  = phys &  PAGE_MASK;
+	unsigned long offset = phys & ~PAGE_MASK;
+	unsigned long vaddr;
 
 	/* If page is RAM, we can use __va. Otherwise ioremap and unmap. */
 	if (page_is_ram(start >> PAGE_SHIFT))
 		return __va(phys);
 
-	addr = (void __force *)ioremap_cache(start, PAGE_SIZE);
-	if (addr)
-		addr = (void *)((unsigned long)addr | (phys & ~PAGE_MASK));
+	vaddr = (unsigned long)ioremap_cache(start, PAGE_SIZE);
+	/* Only add the offset on success and return NULL if the ioremap() failed: */
+	if (vaddr)
+		vaddr += offset;
 
-	return addr;
+	return (void *)vaddr;
 }
 
 void unxlate_dev_mem_ptr(phys_addr_t phys, void *addr)