[PATCH] clocksource: Fix build in non-OF case

[PATCH] clocksource: Fix build in non-OF case

[Mark Brown]

Commit 4d10f054 (clocksource: make CLOCKSOURCE_OF_DECLARE type safe)
made CLOCKSOURCE_OF_DECLARE reference the function pointer in both the
OF and non-OF cases. In the non-OF case this is likely to introduce
build failures as users may reasonably conditionally compile the OF
specific code, either the macro ought to be used within CONFIG_OF and
not have a !CONFIG_OF case or the macro ought not to reference its
arguments in that case.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---

The commit above is in -next.

 include/linux/clocksource.h |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index ac33184..b84a2f3 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -344,11 +344,7 @@ extern void clocksource_of_init(void);
 		     .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
 #else
 static inline void clocksource_of_init(void) {}
-#define CLOCKSOURCE_OF_DECLARE(name, compat, fn)			\
-	static const struct of_device_id __clksrc_of_table_##name	\
-		__unused __section(__clksrc_of_table)			\
-		 = { .compatible = compat,				\
-		     .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
+#define CLOCKSOURCE_OF_DECLARE(name, compat, fn)
 #endif
 
 #endif /* _LINUX_CLOCKSOURCE_H */
-- 
1.7.10.4

[PATCH] clocksource: Fix build in non-OF case

[Mark Brown]

Commit 4d10f054 (clocksource: make CLOCKSOURCE_OF_DECLARE type safe)
made CLOCKSOURCE_OF_DECLARE reference the function pointer in both the
OF and non-OF cases. In the non-OF case this is likely to introduce
build failures as users may reasonably conditionally compile the OF
specific code, either the macro ought to be used within CONFIG_OF and
not have a !CONFIG_OF case or the macro ought not to reference its
arguments in that case.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---

The commit above is in -next.

 include/linux/clocksource.h |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index ac33184..b84a2f3 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -344,11 +344,7 @@ extern void clocksource_of_init(void);
 		     .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
 #else
 static inline void clocksource_of_init(void) {}
-#define CLOCKSOURCE_OF_DECLARE(name, compat, fn)			\
-	static const struct of_device_id __clksrc_of_table_##name	\
-		__unused __section(__clksrc_of_table)			\
-		 = { .compatible = compat,				\
-		     .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
+#define CLOCKSOURCE_OF_DECLARE(name, compat, fn)
 #endif
 
 #endif /* _LINUX_CLOCKSOURCE_H */
-- 
1.7.10.4

Re: [PATCH] clocksource: Fix build in non-OF case

[Arnd Bergmann]

On Thursday 28 March 2013, Mark Brown wrote:
> Commit 4d10f054 (clocksource: make CLOCKSOURCE_OF_DECLARE type safe)
> made CLOCKSOURCE_OF_DECLARE reference the function pointer in both the
> OF and non-OF cases. In the non-OF case this is likely to introduce
> build failures as users may reasonably conditionally compile the OF
> specific code, either the macro ought to be used within CONFIG_OF and
> not have a !CONFIG_OF case or the macro ought not to reference its
> arguments in that case.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Hi Mark,

Axel Lin reported the same problem and I fixed the below code earlier
today by using the correct __attribute__((unused)) and dropping the
section magic for the non-OF case. My patch now looks contains the
change below. I also proposed a fix for the clocksource driver
at http://lkml.org/lkml/2013/3/26/103.

	Arnd

----
@@ -332,16 +332,23 @@ extern int clocksource_mmio_init(void __iomem *, const char *,
 
 extern int clocksource_i8253_init(void);
 
+struct device_node;
+typedef void(*clocksource_of_init_fn)(struct device_node *);
 #ifdef CONFIG_CLKSRC_OF
 extern void clocksource_of_init(void);
 
 #define CLOCKSOURCE_OF_DECLARE(name, compat, fn)                       \
        static const struct of_device_id __clksrc_of_table_##name       \
                __used __section(__clksrc_of_table)                     \
-                = { .compatible = compat, .data = fn };
+                = { .compatible = compat,                              \
+                    .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
 #else
 static inline void clocksource_of_init(void) {}
-#define CLOCKSOURCE_OF_DECLARE(name, compat, fn)
+#define CLOCKSOURCE_OF_DECLARE(name, compat, fn)                       \
+       static const struct of_device_id __clksrc_of_table_##name       \
+               __attribute__((unused))                                 \
+                = { .compatible = compat,                              \
+                    .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
 #endif
 
 #endif /* _LINUX_CLOCKSOURCE_H */

[PATCH] clocksource: Fix build in non-OF case

[Arnd Bergmann]

On Thursday 28 March 2013, Mark Brown wrote:
> Commit 4d10f054 (clocksource: make CLOCKSOURCE_OF_DECLARE type safe)
> made CLOCKSOURCE_OF_DECLARE reference the function pointer in both the
> OF and non-OF cases. In the non-OF case this is likely to introduce
> build failures as users may reasonably conditionally compile the OF
> specific code, either the macro ought to be used within CONFIG_OF and
> not have a !CONFIG_OF case or the macro ought not to reference its
> arguments in that case.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Hi Mark,

Axel Lin reported the same problem and I fixed the below code earlier
today by using the correct __attribute__((unused)) and dropping the
section magic for the non-OF case. My patch now looks contains the
change below. I also proposed a fix for the clocksource driver
at http://lkml.org/lkml/2013/3/26/103.

	Arnd

----
@@ -332,16 +332,23 @@ extern int clocksource_mmio_init(void __iomem *, const char *,
 
 extern int clocksource_i8253_init(void);
 
+struct device_node;
+typedef void(*clocksource_of_init_fn)(struct device_node *);
 #ifdef CONFIG_CLKSRC_OF
 extern void clocksource_of_init(void);
 
 #define CLOCKSOURCE_OF_DECLARE(name, compat, fn)                       \
        static const struct of_device_id __clksrc_of_table_##name       \
                __used __section(__clksrc_of_table)                     \
-                = { .compatible = compat, .data = fn };
+                = { .compatible = compat,                              \
+                    .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
 #else
 static inline void clocksource_of_init(void) {}
-#define CLOCKSOURCE_OF_DECLARE(name, compat, fn)
+#define CLOCKSOURCE_OF_DECLARE(name, compat, fn)                       \
+       static const struct of_device_id __clksrc_of_table_##name       \
+               __attribute__((unused))                                 \
+                = { .compatible = compat,                              \
+                    .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
 #endif
 
 #endif /* _LINUX_CLOCKSOURCE_H */

Re: [PATCH] clocksource: Fix build in non-OF case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Thu, Mar 28, 2013 at 12:39:46PM +0000, Arnd Bergmann wrote:

> Axel Lin reported the same problem and I fixed the below code earlier
> today by using the correct __attribute__((unused)) and dropping the
> section magic for the non-OF case. My patch now looks contains the

That still looks like it'll reference the function?

> change below. I also proposed a fix for the clocksource driver
> at http://lkml.org/lkml/2013/3/26/103.

This is a different driver that I'm trying to look at here, the s3c24xx
one which is still not merged.  

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] clocksource: Fix build in non-OF case

[Mark Brown]

On Thu, Mar 28, 2013 at 12:39:46PM +0000, Arnd Bergmann wrote:

> Axel Lin reported the same problem and I fixed the below code earlier
> today by using the correct __attribute__((unused)) and dropping the
> section magic for the non-OF case. My patch now looks contains the

That still looks like it'll reference the function?

> change below. I also proposed a fix for the clocksource driver
> at http://lkml.org/lkml/2013/3/26/103.

This is a different driver that I'm trying to look at here, the s3c24xx
one which is still not merged.  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130328/c4ff6eb6/attachment.sig>

Re: [PATCH] clocksource: Fix build in non-OF case

[Arnd Bergmann]

On Thursday 28 March 2013, Mark Brown wrote:
> On Thu, Mar 28, 2013 at 12:39:46PM +0000, Arnd Bergmann wrote:
> 
> > Axel Lin reported the same problem and I fixed the below code earlier
> > today by using the correct __attribute__((unused)) and dropping the
> > section magic for the non-OF case. My patch now looks contains the
> 
> That still looks like it'll reference the function?

Yes, that is intentional. The idea is to create a reference to the
function so gcc doesn't complain about unused symbols if the function
gets marked static, but at the same time mark the data structure we
define as unused so gcc can drop the structure as well as the function
if they are not referenced from anywhere else.  This should let us
get away with fewer #ifdef hacks in the code, better build-time coverage
but without producing larger object code.

> > change below. I also proposed a fix for the clocksource driver
> > at http://lkml.org/lkml/2013/3/26/103.
> 
> This is a different driver that I'm trying to look at here, the s3c24xx
> one which is still not merged.  

Ah, sorry about that. It seems to have the same bug.

	Arnd

[PATCH] clocksource: Fix build in non-OF case

[Arnd Bergmann]

On Thursday 28 March 2013, Mark Brown wrote:
> On Thu, Mar 28, 2013 at 12:39:46PM +0000, Arnd Bergmann wrote:
> 
> > Axel Lin reported the same problem and I fixed the below code earlier
> > today by using the correct __attribute__((unused)) and dropping the
> > section magic for the non-OF case. My patch now looks contains the
> 
> That still looks like it'll reference the function?

Yes, that is intentional. The idea is to create a reference to the
function so gcc doesn't complain about unused symbols if the function
gets marked static, but at the same time mark the data structure we
define as unused so gcc can drop the structure as well as the function
if they are not referenced from anywhere else.  This should let us
get away with fewer #ifdef hacks in the code, better build-time coverage
but without producing larger object code.

> > change below. I also proposed a fix for the clocksource driver
> > at http://lkml.org/lkml/2013/3/26/103.
> 
> This is a different driver that I'm trying to look at here, the s3c24xx
> one which is still not merged.  

Ah, sorry about that. It seems to have the same bug.

	Arnd

Re: [PATCH] clocksource: Fix build in non-OF case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

On Thu, Mar 28, 2013 at 01:08:22PM +0000, Arnd Bergmann wrote:
> On Thursday 28 March 2013, Mark Brown wrote:

> > That still looks like it'll reference the function?

> Yes, that is intentional. The idea is to create a reference to the
> function so gcc doesn't complain about unused symbols if the function
> gets marked static, but at the same time mark the data structure we
> define as unused so gcc can drop the structure as well as the function
> if they are not referenced from anywhere else.  This should let us
> get away with fewer #ifdef hacks in the code, better build-time coverage
> but without producing larger object code.

So GCC is supposed to be smart enough to figure this out and users need
to not do the ifdefs?  I have to say this does seem a bit surprising
from a user point of view but it does make sense from a general niceness
point of view.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] clocksource: Fix build in non-OF case

[Mark Brown]

On Thu, Mar 28, 2013 at 01:08:22PM +0000, Arnd Bergmann wrote:
> On Thursday 28 March 2013, Mark Brown wrote:

> > That still looks like it'll reference the function?

> Yes, that is intentional. The idea is to create a reference to the
> function so gcc doesn't complain about unused symbols if the function
> gets marked static, but at the same time mark the data structure we
> define as unused so gcc can drop the structure as well as the function
> if they are not referenced from anywhere else.  This should let us
> get away with fewer #ifdef hacks in the code, better build-time coverage
> but without producing larger object code.

So GCC is supposed to be smart enough to figure this out and users need
to not do the ifdefs?  I have to say this does seem a bit surprising
from a user point of view but it does make sense from a general niceness
point of view.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130328/b570317d/attachment.sig>

Re: [PATCH] clocksource: Fix build in non-OF case

[Arnd Bergmann]

On Thursday 28 March 2013, Mark Brown wrote:
> On Thu, Mar 28, 2013 at 01:08:22PM +0000, Arnd Bergmann wrote:
> > On Thursday 28 March 2013, Mark Brown wrote:
> 
> > > That still looks like it'll reference the function?
> 
> > Yes, that is intentional. The idea is to create a reference to the
> > function so gcc doesn't complain about unused symbols if the function
> > gets marked static, but at the same time mark the data structure we
> > define as unused so gcc can drop the structure as well as the function
> > if they are not referenced from anywhere else.  This should let us
> > get away with fewer #ifdef hacks in the code, better build-time coverage
> > but without producing larger object code.
> 
> So GCC is supposed to be smart enough to figure this out and users need
> to not do the ifdefs?  I have to say this does seem a bit surprising
> from a user point of view but it does make sense from a general niceness
> point of view.

Yes, I'm pretty sure that all gcc-4.x versions can do this right at -Os
and -O2 levels. The new gcc-4.8 -Og level may get it wrong but is also
broken for many other things we do in the kernel, just like building with
gcc -O0.

Since we recently introduced the IS_ENABLED() macro to test for preprocessor
symbols, I think there is a general trend away from any #ifdefs in driver
code.

	Arnd

[PATCH] clocksource: Fix build in non-OF case

[Arnd Bergmann]

On Thursday 28 March 2013, Mark Brown wrote:
> On Thu, Mar 28, 2013 at 01:08:22PM +0000, Arnd Bergmann wrote:
> > On Thursday 28 March 2013, Mark Brown wrote:
> 
> > > That still looks like it'll reference the function?
> 
> > Yes, that is intentional. The idea is to create a reference to the
> > function so gcc doesn't complain about unused symbols if the function
> > gets marked static, but at the same time mark the data structure we
> > define as unused so gcc can drop the structure as well as the function
> > if they are not referenced from anywhere else.  This should let us
> > get away with fewer #ifdef hacks in the code, better build-time coverage
> > but without producing larger object code.
> 
> So GCC is supposed to be smart enough to figure this out and users need
> to not do the ifdefs?  I have to say this does seem a bit surprising
> from a user point of view but it does make sense from a general niceness
> point of view.

Yes, I'm pretty sure that all gcc-4.x versions can do this right at -Os
and -O2 levels. The new gcc-4.8 -Og level may get it wrong but is also
broken for many other things we do in the kernel, just like building with
gcc -O0.

Since we recently introduced the IS_ENABLED() macro to test for preprocessor
symbols, I think there is a general trend away from any #ifdefs in driver
code.

	Arnd

Re: [PATCH] clocksource: Fix build in non-OF case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 584 bytes --]

On Thu, Mar 28, 2013 at 02:47:51PM +0000, Arnd Bergmann wrote:

> Since we recently introduced the IS_ENABLED() macro to test for preprocessor
> symbols, I think there is a general trend away from any #ifdefs in driver
> code.

I have to say I'm not seeing any evidence of this in the patches I'm
seeing, and I'm not sure what the connection with IS_ENABLED() is?  It's
a reasonable idea, just likely to surprise people given the amount of
boilerplate we've got for things like PM - I guess we'd need to make
sure those macros do references as well, they're the main users of
ifdefs.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] clocksource: Fix build in non-OF case

[Mark Brown]

On Thu, Mar 28, 2013 at 02:47:51PM +0000, Arnd Bergmann wrote:

> Since we recently introduced the IS_ENABLED() macro to test for preprocessor
> symbols, I think there is a general trend away from any #ifdefs in driver
> code.

I have to say I'm not seeing any evidence of this in the patches I'm
seeing, and I'm not sure what the connection with IS_ENABLED() is?  It's
a reasonable idea, just likely to surprise people given the amount of
boilerplate we've got for things like PM - I guess we'd need to make
sure those macros do references as well, they're the main users of
ifdefs.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130329/fa5af154/attachment.sig>