ARM:asm:io.h use static inline

PATCH In review Jeroen Hofstee 3 participants 7 messages Jun 22, 2014 – Aug 30, 2014 Arm (32-bit)

Share & tools

Jeroen Hofstee

author

2014-06-22 21:10 UTC #0 lore raw

PATCH 1/1 ARM:asm:io.h use static inline

When compiling u-boot with W=1 the extern inline void for read* is likely causing the most noise. gcc / clang will warn there is never a actual declaration for these functions. Instead of declaring these extern make them static inline so it is actually declared.

cc Albert ARIBAUD <[email protected]>

Signed-off-by Jeroen Hofstee <[email protected]>

---
 arch/arm/include/asm/io.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

arch/arm/include/asm/io.h +6 -6

index 9f35fd6..7b6189b 100644
@@ -75,7 +75,7 @@ static inline phys_addr_t virt_to_phys(void * vaddr)
 #define __arch_putw(v,a)		(*(volatile unsigned short *)(a) = (v))
 #define __arch_putl(v,a)		(*(volatile unsigned int *)(a) = (v))
 
-extern inline void __raw_writesb(unsigned long addr, const void *data,
+static inline void __raw_writesb(unsigned long addr, const void *data,
 				 int bytelen)
 {
 	uint8_t *buf = (uint8_t *)data;

@@ -83,7 +83,7 @@ extern inline void __raw_writesb(unsigned long addr, const void *data,
 		__arch_putb(*buf++, addr);
 }
 
-extern inline void __raw_writesw(unsigned long addr, const void *data,
+static inline void __raw_writesw(unsigned long addr, const void *data,
 				 int wordlen)
 {
 	uint16_t *buf = (uint16_t *)data;

@@ -91,7 +91,7 @@ extern inline void __raw_writesw(unsigned long addr, const void *data,
 		__arch_putw(*buf++, addr);
 }
 
-extern inline void __raw_writesl(unsigned long addr, const void *data,
+static inline void __raw_writesl(unsigned long addr, const void *data,
 				 int longlen)
 {
 	uint32_t *buf = (uint32_t *)data;

@@ -99,21 +99,21 @@ extern inline void __raw_writesl(unsigned long addr, const void *data,
 		__arch_putl(*buf++, addr);
 }
 
-extern inline void __raw_readsb(unsigned long addr, void *data, int bytelen)
+static inline void __raw_readsb(unsigned long addr, void *data, int bytelen)
 {
 	uint8_t *buf = (uint8_t *)data;
 	while(bytelen--)
 		*buf++ = __arch_getb(addr);
 }
 
-extern inline void __raw_readsw(unsigned long addr, void *data, int wordlen)
+static inline void __raw_readsw(unsigned long addr, void *data, int wordlen)
 {
 	uint16_t *buf = (uint16_t *)data;
 	while(wordlen--)
 		*buf++ = __arch_getw(addr);
 }
 
-extern inline void __raw_readsl(unsigned long addr, void *data, int longlen)
+static inline void __raw_readsl(unsigned long addr, void *data, int longlen)
 {
 	uint32_t *buf = (uint32_t *)data;
 	while(longlen--)

Signature · 1 line

1.8.3.2

Albert ARIBAUD

2014-07-05 09:13 UTC #1 lore raw

PATCH 1/1 ARM:asm:io.h use static inline

Hi Jeroen,

Quoted 9 lines · Jeroen Hofstee

When compiling u-boot with W=1 the extern inline void for read* is likely causing the most noise. gcc / clang will warn there is never a actual declaration for these functions. Instead of declaring these extern make them static inline so it is actually declared.
cc Albert ARIBAUD <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>
---

Ok, so the obvious question: what makes you decide to switch to 'static inline' rather than provide the extern versions that 'extern static' calls for?

(see e.g. http://elinux.org/Extern_Vs_Static_Inline for an example discussion on 'static inline' vs 'extern inline'. Note: this link is not intended as an expression of my stance on the issue, as I don't have any.)

Amicalement,

Signature · 1 line

Albert.

Jeroen Hofstee

author

2014-07-05 11:36 UTC #2 lore raw

PATCH 1/1 ARM:asm:io.h use static inline

Hello Albert,

Quoted 11 lines · Albert ARIBAUD

Hi Jeroen,
Quoted 6 lines · Jeroen Hofstee
When compiling u-boot with W=1 the extern inline void for read* is likely causing the most noise. gcc / clang will warn there is never a actual declaration for these functions. Instead of declaring these extern make them static inline so it is actually declared.

Quoted 7 lines

Quoted 3 lines
cc Albert ARIBAUD <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>
---
Ok, so the obvious question: what makes you decide to switch to 'static inline' rather than provide the extern versions that 'extern static' calls for?

Assuming your question is, why didn't you just add the prototypes instead?

Well if we wanted to be brave gnu99 citizens we should provide the prototypes, the extern inline version and a separated definition in case the compiler fails / is not in the mood to inline the function. This quite fragile / some housekeeping. Furthermore it is gnu specific and likely fails with gcc -std=c99 as well.

Making them static inline there is always a single definition and it is up to the compiler to either inline it or make it a static function by it self. Since we were already relying on the compiler to inline it (at least I am unaware that there are non inline version around), this boils down to the same thing, but without warnings.

And... I can likely drop this one as well[1], although I haven't checked yet. linux does the same btw for __LINUX_ARM_ARCH__ >= 6.

I can check binary size if that something you wonder about...

Regards,
Jeroen

[1] https://github.com/jhofstee/u-boot/commit/5cd261fecc5397bf5abef82f6a781d8b04992654

Albert ARIBAUD

2014-07-05 13:21 UTC #3 lore raw

PATCH 1/1 ARM:asm:io.h use static inline

Hi Jeroen,

(sorry for the near-duplicate, and see question at end)

Quoted 18 lines · Jeroen Hofstee

Hello Albert,
Quoted 15 lines · Albert ARIBAUD
Hi Jeroen,
Quoted 9 lines · Jeroen Hofstee
When compiling u-boot with W=1 the extern inline void for read* is likely causing the most noise. gcc / clang will warn there is never a actual declaration for these functions. Instead of declaring these extern make them static inline so it is actually declared.
cc Albert ARIBAUD <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>
---

Quoted 5 lines

Quoted 3 lines
Ok, so the obvious question: what makes you decide to switch to 'static inline' rather than provide the extern versions that 'extern static' calls for?
Assuming your question is, why didn't you just add the prototypes instead?

It was more along the lines of "were you aware that you had a choice there?"

Quoted 12 lines

Well if we wanted to be brave gnu99 citizens we should provide the prototypes, the extern inline version and a separated definition in case the compiler fails / is not in the mood to inline the function. This quite fragile / some housekeeping. Furthermore it is gnu specific and likely fails with gcc -std=c99 as well.
Making them static inline there is always a single definition and it is up to the compiler to either inline it or make it a static function by it self. Since we were already relying on the compiler to inline it (at least I am unaware that there are non inline version around), this boils down to the same thing, but without warnings.

Quoted 4 lines

And... I can likely drop this one as well[1], although I haven't checked yet. linux does the same btw for __LINUX_ARM_ARCH__ >= 6.
I can check binary size if that something you wonder about...

As I said, I have no stance on whether 'static inline' or 'extern inline' was better / more appropriate / other (please specify). I just wanted to make sure that you had considered both possibilities before choosing one of them. I am now assured that you have, so all is fine.

Quoted 4 lines

Regards,
Jeroen
[1] https://github.com/jhofstee/u-boot/commit/5cd261fecc5397bf5abef82f6a781d8b04992654

Which board do you get warnings for?

Amicalement,

Signature · 1 line

Albert.

Jeroen Hofstee

author

2014-07-05 13:30 UTC #4 lore raw

PATCH 1/1 ARM:asm:io.h use static inline

Hello Albert,

Quoted 24 lines · Albert ARIBAUD

Hi Jeroen,
(sorry for the near-duplicate, and see question at end)
Quoted 17 lines · Jeroen Hofstee
Hello Albert,
Quoted 14 lines · Albert ARIBAUD
Hi Jeroen,
Quoted 9 lines · Jeroen Hofstee
When compiling u-boot with W=1 the extern inline void for read* is likely causing the most noise. gcc / clang will warn there is never a actual declaration for these functions. Instead of declaring these extern make them static inline so it is actually declared.
cc Albert ARIBAUD <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>
---

Quoted 6 lines

Quoted 4 lines
Quoted 3 lines
Ok, so the obvious question: what makes you decide to switch to 'static inline' rather than provide the extern versions that 'extern static' calls for?
Assuming your question is, why didn't you just add the prototypes instead?
It was more along the lines of "were you aware that you had a choice there?"

yes I was aware of that.

Quoted 12 lines

Quoted 12 lines
Well if we wanted to be brave gnu99 citizens we should provide the prototypes, the extern inline version and a separated definition in case the compiler fails / is not in the mood to inline the function. This quite fragile / some housekeeping. Furthermore it is gnu specific and likely fails with gcc -std=c99 as well.
Making them static inline there is always a single definition and it is up to the compiler to either inline it or make it a static function by it self. Since we were already relying on the compiler to inline it (at least I am unaware that there are non inline version around), this boils down to the same thing, but without warnings.

Quoted 8 lines

Quoted 4 lines
And... I can likely drop this one as well[1], although I haven't checked yet. linux does the same btw for __LINUX_ARM_ARCH__ >= 6.
I can check binary size if that something you wonder about...
As I said, I have no stance on whether 'static inline' or 'extern inline' was better / more appropriate / other (please specify). I just wanted to make sure that you had considered both possibilities before choosing one of them. I am now assured that you have, so all is fine.

Quoted 5 lines

Quoted 4 lines
Regards,
Jeroen
[1] https://github.com/jhofstee/u-boot/commit/5cd261fecc5397bf5abef82f6a781d8b04992654
Which board do you get warnings for?

Basically any, but as the commit message says, it is just for removing warnings when building with make W=1 or clang (which actually errors, without the other patch). There is no hurry in applying it. But e.g. for the twister I am sure you will see them, with the additional warnings enabled.

Regards,
Jeroen

Albert ARIBAUD

2014-07-05 14:58 UTC #5 lore raw

PATCH 1/1 ARM:asm:io.h use static inline

Hi Jeroen,

Quoted 58 lines · Jeroen Hofstee

Hello Albert,
Quoted 24 lines · Albert ARIBAUD
Hi Jeroen,
(sorry for the near-duplicate, and see question at end)
Quoted 17 lines · Jeroen Hofstee
Hello Albert,
Quoted 14 lines · Albert ARIBAUD
Hi Jeroen,
Quoted 9 lines · Jeroen Hofstee
When compiling u-boot with W=1 the extern inline void for read* is likely causing the most noise. gcc / clang will warn there is never a actual declaration for these functions. Instead of declaring these extern make them static inline so it is actually declared.
cc Albert ARIBAUD <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>
---
Quoted 6 lines
Quoted 4 lines
Quoted 3 lines
Ok, so the obvious question: what makes you decide to switch to 'static inline' rather than provide the extern versions that 'extern static' calls for?
Assuming your question is, why didn't you just add the prototypes instead?
It was more along the lines of "were you aware that you had a choice there?"
yes I was aware of that.
Quoted 12 lines
Quoted 12 lines
Well if we wanted to be brave gnu99 citizens we should provide the prototypes, the extern inline version and a separated definition in case the compiler fails / is not in the mood to inline the function. This quite fragile / some housekeeping. Furthermore it is gnu specific and likely fails with gcc -std=c99 as well.
Making them static inline there is always a single definition and it is up to the compiler to either inline it or make it a static function by it self. Since we were already relying on the compiler to inline it (at least I am unaware that there are non inline version around), this boils down to the same thing, but without warnings.
Quoted 8 lines
Quoted 4 lines
And... I can likely drop this one as well[1], although I haven't checked yet. linux does the same btw for __LINUX_ARM_ARCH__ >= 6.
I can check binary size if that something you wonder about...
As I said, I have no stance on whether 'static inline' or 'extern inline' was better / more appropriate / other (please specify). I just wanted to make sure that you had considered both possibilities before choosing one of them. I am now assured that you have, so all is fine.
ok
Quoted 3 lines
Quoted 3 lines
Regards,
Jeroen

Quoted 7 lines

Quoted 2 lines
Quoted 1 line
[1] https://github.com/jhofstee/u-boot/commit/5cd261fecc5397bf5abef82f6a781d8b04992654
Which board do you get warnings for?
Basically any, but as the commit message says, it is just for removing warnings when building with make W=1 or clang (which actually errors, without the other patch). There is no hurry in applying it. But e.g. for the twister I am sure you will see them, with the additional warnings enabled.

Ok, seeing that it is not needed for customary builds, I'll defer this until after 2014.07.

Quoted 2 lines

Regards,
Jeroen

Amicalement,

Signature · 1 line

Albert.

Tom Rini

2014-08-30 15:13 UTC #6 lore raw

In reply to Jeroen Hofstee

Quoted 8 lines · Jeroen Hofstee

When compiling u-boot with W=1 the extern inline void for read* is likely causing the most noise. gcc / clang will warn there is never a actual declaration for these functions. Instead of declaring these extern make them static inline so it is actually declared.
cc Albert ARIBAUD <[email protected]>
Signed-off-by Jeroen Hofstee <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140830/80444ad4/attachment.pgp>