Remove warning in compilation of ioremap

PATCH Claudio Scordino 3 participants 7 messages Sep 9, 2008

Subsystems (2)

Memory management Arm (32-bit)

lore

Share & tools

lore

Claudio Scordino

author

2008-09-09 09:13 UTC #0 lore raw

Hi all.

[We already discussed this issue in linux-mm ML, but people suggested to post to linux-arm-kernel...]

When compiling Linux (latest kernel from Linus' git) on ARM, I noticed the following warning:

CC arch/arm/mm/ioremap.o
arch/arm/mm/ioremap.c: In function '__arm_ioremap_pfn':
arch/arm/mm/ioremap.c:83: warning: control may reach end of non-void function 'remap_area_pte' being inlined

If you look at the code, the problem is in a path including a BUG().

AFAIK, on ARM the code following BUG() is never executed: it's a NULL pointer dereference, so the handler of pagefault eventually calls do_exit(). Therefore, we may want to remove the goto as shown in the patch in attachment.

It's obviously a minor issue. But I don't like having meaningless
warnings during compilation: they just confuse output, and developers may miss some important warning message...

The need for the goto exists only if BUG() can return. If it doesn't, we can safely remove it as shown in the patch.

Is this possible ? Should we update this piece of code ? Who's in charge of maintaining it ?

Many thanks,

Claudio

Quoted 1 line

From 08d2e6f14230bf2252c54f5421d92def5e70f6dc Mon Sep 17 00:00:00 2001

From: Claudio Scordino <[email protected]>
Date: Mon, 8 Sep 2008 16:03:38 +0200
Subject: [PATCH 1/1] Fix compilation warning in remap_area_pte

Signed-off-by Claudio Scordino <[email protected]>

---
 arch/arm/mm/ioremap.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

arch/arm/mm/ioremap.c +4 -7

index b81dbf9..bc6eca0 100644
@@ -52,18 +52,15 @@ static int remap_area_pte(pmd_t *pmd, unsigned long addr, unsigned long end,
 		return -ENOMEM;
 
 	do {
-		if (!pte_none(*pte))
-			goto bad;
-
+		if (unlikely(!pte_none(*pte))){
+			printk(KERN_CRIT "%s: page already exists\n", __FUNCTION__);
+			BUG();
+		}
 		set_pte_ext(pte, pfn_pte(phys_addr >> PAGE_SHIFT, prot),
 			    type->prot_pte_ext);
 		phys_addr += PAGE_SIZE;
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 	return 0;
-
- bad:
-	printk(KERN_CRIT "remap_area_pte: page already exists\n");
-	BUG();
 }
 
 static inline int remap_area_pmd(pgd_t *pgd, unsigned long addr,

Signature · 1 line

1.5.4.3

Russell King - ARM Linux

2008-09-09 09:58 UTC #1 lore raw

Quoted 15 lines · Claudio Scordino

When compiling Linux (latest kernel from Linus' git) on ARM, I noticed the following warning:
CC arch/arm/mm/ioremap.o
arch/arm/mm/ioremap.c: In function '__arm_ioremap_pfn':
arch/arm/mm/ioremap.c:83: warning: control may reach end of non-void function 'remap_area_pte' being inlined
If you look at the code, the problem is in a path including a BUG().
AFAIK, on ARM the code following BUG() is never executed: it's a NULL pointer dereference, so the handler of pagefault eventually calls do_exit(). Therefore, we may want to remove the goto as shown in the patch in attachment.

Quoted 6 lines

It's obviously a minor issue. But I don't like having meaningless warnings during compilation: they just confuse output, and developers may miss some important warning message...
The need for the goto exists only if BUG() can return. If it doesn't, we can safely remove it as shown in the patch.

NAK. See patch 5211/2 in the patch system.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Claudio Scordino

author

2008-09-09 13:49 UTC #2 lore raw

Russell King - ARM Linux ha scritto:

Quoted 16 lines · Claudio Scordino

Quoted 15 lines
When compiling Linux (latest kernel from Linus' git) on ARM, I noticed the following warning:
CC arch/arm/mm/ioremap.o
arch/arm/mm/ioremap.c: In function '__arm_ioremap_pfn':
arch/arm/mm/ioremap.c:83: warning: control may reach end of non-void function 'remap_area_pte' being inlined
If you look at the code, the problem is in a path including a BUG().
AFAIK, on ARM the code following BUG() is never executed: it's a NULL pointer dereference, so the handler of pagefault eventually calls do_exit(). Therefore, we may want to remove the goto as shown in the patch in attachment.

Quoted 7 lines

Quoted 5 lines
It's obviously a minor issue. But I don't like having meaningless
warnings during compilation: they just confuse output, and developers may miss some important warning message...
The need for the goto exists only if BUG() can return. If it doesn't, we can safely remove it as shown in the patch.
NAK. See patch 5211/2 in the patch system.

Seen.

Thanks,

Claudio

David Anders

2008-09-09 13:08 UTC #3 lore raw

In reply to Claudio Scordino

Claudio,

i hope you have a better time getting this fixed than i have, i've been submitting patches as far back as 2.6.16:

http://lists.arm.linux.org.uk/lurker/message/20070906.135142.6c5e4d6f.en.html
http://lists.arm.linux.org.uk/lurker/message/20070906.140649.79f143a0.en.html

2.6.23 was when i gave up.

Dave Anders

Quoted 100 lines · Claudio Scordino

From: Claudio Scordino <[email protected]>
Subject: Remove warning in compilation of ioremap
To: [email protected]
Cc: [email protected], "Phil Blundell" <[email protected]>, "Luiz Fernando N. Capitulino" <[email protected]>
Date: Tuesday, September 9, 2008, 4:13 AM
Hi all.
[We already discussed this issue in linux-mm ML, but people suggested
to post to linux-arm-kernel...]
When compiling Linux (latest kernel from Linus' git) on ARM, I noticed
the following warning:
CC arch/arm/mm/ioremap.o
arch/arm/mm/ioremap.c: In function '__arm_ioremap_pfn':
arch/arm/mm/ioremap.c:83: warning: control may reach end of non-void
function 'remap_area_pte' being inlined
If you look at the code, the problem is in a path including a BUG().
AFAIK, on ARM the code following BUG() is never executed:
it's a NULL
pointer dereference, so the handler of pagefault eventually calls
do_exit(). Therefore, we may want to remove the goto as shown in the
patch in attachment.
It's obviously a minor issue. But I don't like having meaningless
warnings during compilation: they just confuse output, and developers
may miss some important warning message...
The need for the goto exists only if BUG() can return. If it doesn't,
we can safely remove it as shown in the patch.
Is this possible ? Should we update this piece of code ? Who's in
charge of maintaining it ?
Many thanks,
Claudio
Quoted 1 line
From 08d2e6f14230bf2252c54f5421d92def5e70f6dc Mon Sep
17 00:00:00 2001
From: Claudio Scordino <[email protected]>
Date: Mon, 8 Sep 2008 16:03:38 +0200
Subject: [PATCH 1/1] Fix compilation warning in
remap_area_pte
Signed-off-by: Claudio Scordino
<[email protected]>
---
 arch/arm/mm/ioremap.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)
arch/arm/mm/ioremap.c +4 -6
index b81dbf9..bc6eca0 100644
@@ -52,18 +52,15 @@ static int remap_area_pte(pmd_t *pmd,
 unsigned long addr, unsigned long end,
 		return -ENOMEM;
 
 	do {
-		if (!pte_none(*pte))
-			goto bad;
-
+		if (unlikely(!pte_none(*pte))){
+			printk(KERN_CRIT "%s: page already
 exists\n", __FUNCTION__);
+			BUG();
+		}
 		set_pte_ext(pte, pfn_pte(phys_addr >> PAGE_SHIFT,
 prot),
 			    type->prot_pte_ext);
 		phys_addr += PAGE_SIZE;
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 	return 0;
-
- bad:
-	printk(KERN_CRIT "remap_area_pte: page already
exists\n");
arch/arm/mm/ioremap.c +0 -1
-	BUG();
 }
 
 static inline int remap_area_pmd(pgd_t *pgd, unsigned long

Quoted 13 lines

addr,

Signature · 11 lines

1.5.4.3



-------------------------------------------------------------------
List admin:
http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:       
http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette: 
http://www.arm.linux.org.uk/mailinglists/etiquette.php

Russell King - ARM Linux

2008-09-09 13:55 UTC #4 lore raw

Quoted 9 lines · David Anders

Claudio,
i hope you have a better time getting this fixed than i have, i've been submitting patches as far back as 2.6.16:
http://lists.arm.linux.org.uk/lurker/message/20070906.135142.6c5e4d6f.en.html
http://lists.arm.linux.org.uk/lurker/message/20070906.140649.79f143a0.en.html
2.6.23 was when i gave up.

It's not like you were ignored, both of those messages contain replies from Erik Mouw, both of which were positive.

Unfortunately, I didn't see it as a high priority so it got left a little too long and I never got around to commenting about it (I thought it was a complex way of fixing what was a trivial problem.)

Take a look at the difference between yours:

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4563/1

Comapred with the one which has been merged:

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5211/2

Sorry.

David Anders

2008-09-09 14:59 UTC #5 lore raw

Russell,

Quoted 16 lines

Quoted 1 line
i hope you have a better time getting this fixed than
i have,
Quoted 1 line
i've been submitting patches as far back as
2.6.16:
Quoted 2 lines
http://lists.arm.linux.org.uk/lurker/message/20070906.135142.6c5e4d6f.en.html
Quoted 1 line
http://lists.arm.linux.org.uk/lurker/message/20070906.140649.79f143a0.en.html
Quoted 2 lines
2.6.23 was when i gave up.
It's not like you were ignored, both of those messages contain replies
from Erik Mouw, both of which were positive.

Quoted 6 lines

Unfortunately, I didn't see it as a high priority so it got left a little
too long and I never got around to commenting about it (I thought it was
a complex way of fixing what was a trivial problem.)

water under the bridge, just happy it got fixed.

Quoted 11 lines

Take a look at the difference between yours:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4563/1
Comapred with the one which has been merged:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5211/2
Sorry.

agreed there are differences between what got accepted and what i submited. however, i did not randomly choose the code used in that define. i went back to the mailing lists for x86,powerpc, parisc, mips and sh. the code format of BUG() had been discussed on each of these lists, with x86, powerpc, and parisc using the exact same code format as i submitted, with mips and sh using one almost identical.

from include/asm-x86/bug.h

#define BUG()				\
	do {				\
		asm volatile("ud2");	\
		for(;;) ;		\
	} while(0)

what i submited:

+#define BUG() \
+ do { \
+ (*(int *)0 = 0); \
+ for(;;) ; \
+ } while(0)

i apologize if this is a rant, no disrespect for your work or the pressure you are under to maintain LAK, but shall we always wait until there is a 100% elegant solution to a known issue before fixing it?

Dave Anders

Russell King - ARM Linux

2008-09-09 15:21 UTC #6 lore raw

Quoted 6 lines · David Anders

Russell,
...
i apologize if this is a rant, no disrespect for your work or the pressure you are under to maintain LAK, but shall we always wait until there is a 100% elegant solution to a known issue before fixing it?

How about realising that I'm not perfect, and sometimes need reminding (*after* a reasonable period of about a week or so)?

Rather than just saying "I submitted this, got ignored and gave up" ?

It's a bit like putting a slip of paper in a secretaries tray from your doctor requesting another appointment in 4 weeks time. You wait and wait for notification, which never comes.

What do you do? Do you give up? Or do you wait a reasonable time before telephoning the secretary to find out what's happening?

Guess what I did this morning? I telephoned the secretary to find out what was happening. Result is I now have an appointment. I didn't give up.

This is no different. People forget things. Things get mislayed. People have to be constantly hastled to do anything. That's life.