Re: [PATCH] add a printk_init variant storing format strings in __initdata

Re: [PATCH] add a printk_init variant storing format strings in __initdata

[MinChan Kim]

It is important about embedded system, too.
So I add CC, linux-embedded.

On Wed, Jun 4, 2008 at 5:16 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 3 Jun 2008 10:27:32 +0100 Andy Whitcroft <apw@shadowen.org> wrote:
>
>>
>> [As gcc seems unable to help us out selecting the appropriate data segment
>> for the code, how about we did something like this?]
>>
>> When using printk from __init functions it would be desirable to place
>> the printk format strings in __initdata.  Add a printk_init() variant
>> which does this.
>>
>> This printk_init() is necessarily a #define so that we can declare the
>> format string in static scope and mark it __initdata.  We then call a
>> newly introduced __printk_init() variant which is identicle to printk() but
>> marked __init itself.  By ensuring that an __init variant of printk is used
>> we get proper section violation warnings when this is used incorrectly:
>>
>>     WARNING: vmlinux.o(.text+0x3): Section mismatch in reference from the
>>       function something() to the variable .init.data:__printk_init_fmt.31426
>>     The function something() references
>>     the variable __initdata __printk_init_fmt.31426.
>>     This is often because something lacks a __initdata
>>     annotation or the annotation of __printk_init_fmt.31426 is wrong.
>>
>> Note I have followed printk's pattern for __cold annotations.
>>
>
> Ho hum.  This give everyone another way in which to bury everyone else
> with patches.
>
> Wouldn't it be great if checkpatch were to detect
> fail-to-use-printk_init() in an __init function?
>
> oh, speaking of checkpatch: please use it :)
>
>> ---
>>  include/linux/kernel.h |   10 ++++++++++
>>  kernel/printk.c        |   12 ++++++++++++
>>  2 files changed, 22 insertions(+), 0 deletions(-)
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 792bf0a..7754196 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -180,6 +180,13 @@ struct pid;
>>  extern struct pid *session_of_pgrp(struct pid *pgrp);
>>
>>  #ifdef CONFIG_PRINTK
>> +#define printk_init(fmt, args...) \
>> +do { \
>> +        static char __printk_init_fmt[] __initdata = fmt; \
>> +        __printk_init(__printk_init_fmt, ##args); \
>> +} while (0)
>> +asmlinkage int __printk_init(const char * fmt, ...)
>> +     __attribute__ ((format (printf, 1, 2))) __cold;
>>  asmlinkage int vprintk(const char *fmt, va_list args)
>>       __attribute__ ((format (printf, 1, 0)));
>>  asmlinkage int printk(const char * fmt, ...)
>> @@ -196,6 +203,9 @@ extern int __printk_ratelimit(int ratelimit_jiffies, int ratelimit_burst);
>>  extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
>>                                  unsigned int interval_msec);
>>  #else
>> +asmlinkage int printk_init(const char * fmt, ...)
>> +     __attribute__ ((format (printf, 1, 2))) __cold;
>> +static inline int __cold printk_init(const char *s, ...) { return 0; }
>>  static inline int vprintk(const char *s, va_list args)
>>       __attribute__ ((format (printf, 1, 0)));
>>  static inline int vprintk(const char *s, va_list args) { return 0; }
>> diff --git a/kernel/printk.c b/kernel/printk.c
>> index 8fb01c3..992a5c0 100644
>> --- a/kernel/printk.c
>> +++ b/kernel/printk.c
>> @@ -616,6 +616,18 @@ asmlinkage int printk(const char *fmt, ...)
>>       return r;
>>  }
>>
>> +asmlinkage __init int __printk_init(const char *fmt, ...)
>> +{
>> +     va_list args;
>> +     int r;
>> +
>> +     va_start(args, fmt);
>> +     r = vprintk(fmt, args);
>> +     va_end(args);
>> +
>> +     return r;
>> +}
>
> We're going to want to be able to call printk_init() from modules.
> Please fix and test that, if we decide to proceed.
>
> Oh, and we're going to need printk_meminit() and printk_cpuinit() and
> whatever.
>
> Which probably means that __printk_init() can't be __init, unless all
> the CONFIG_ settings which control __cpuinit, __meminit etc are blowing
> in the right direction.
>
> It would be good if we could get some idea of the savings here, because
> boy this is going to be a pain.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Kinds regards,
MinChan Kim