| Message ID | 1364473805-773-1-git-send-email-broonie@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */
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.
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
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.
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
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.
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 */
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(-)