2011-05-25 09:09:25

by y

[permalink] [raw]
Subject: [PATCH 1/4] include/linux/printk.h is not self-contained

From: Geert Uytterhoeven <[email protected]>

<linux/printk.h> needs to include
- <stdarg.h> for "va_list",
- <linux/linkage.h> for "asmlinkage",
- <linux/types.h> for "bool".

Signed-off-by: Geert Uytterhoeven <[email protected]>
---
include/linux/printk.h | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index ee048e7..6388bc8 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -1,6 +1,10 @@
#ifndef __KERNEL_PRINTK__
#define __KERNEL_PRINTK__

+#include <stdarg.h>
+#include <linux/linkage.h>
+#include <linux/types.h>
+
extern const char linux_banner[];
extern const char linux_proc_banner[];

--
1.7.0.4


2011-05-25 14:42:34

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 1/4] include/linux/printk.h is not self-contained

On Wed, 2011-05-25 at 11:09 +0200, [email protected] wrote:
> From: Geert Uytterhoeven <[email protected]>
> <linux/printk.h> needs to include
> - <stdarg.h> for "va_list",
> - <linux/linkage.h> for "asmlinkage",
> - <linux/types.h> for "bool".

I wonder if printk should be self-contained.

If so, this patch should also add:

#include <linux/dynamic_debug.h>
#include <linux/ratelimit.h>

If not, maybe add:

#ifndef _LINUX_KERNEL_H
#error don't include <linux/printk.h> directly, use <linux/kernel.h>
#endif

Also asmlinkage is an ancient artifact and should
be removed from the declarations of early_printk,
vprintk, and printk.

2011-05-25 15:20:12

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 1/4] include/linux/printk.h is not self-contained

On Wed, May 25, 2011 at 16:42, Joe Perches <[email protected]> wrote:
> On Wed, 2011-05-25 at 11:09 +0200, [email protected] wrote:

Sorry, that email address is a relic from typing one too many "y" responses
to git-send-email questions.

>> From: Geert Uytterhoeven <[email protected]>
>> <linux/printk.h> needs to include
>>   - <stdarg.h> for "va_list",
>>   - <linux/linkage.h> for "asmlinkage",
>>   - <linux/types.h> for "bool".
>
> I wonder if printk should be self-contained.
>
> If so, this patch should also add:
>
> #include <linux/dynamic_debug.h>
> #include <linux/ratelimit.h>

Hmm, it does compile without those, as long as you don't use
dynamic debug or ratelimit.

> If not, maybe add:
>
> #ifndef _LINUX_KERNEL_H
> #error don't include <linux/printk.h> directly, use <linux/kernel.h>
> #endif

That's another possibility. We already have several users, though.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

2011-05-25 15:52:11

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: [PATCH 1/4] include/linux/printk.h is not self-contained

On Wed, May 25, 2011 at 6:20 PM, Geert Uytterhoeven
<[email protected]> wrote:
>> If not, maybe add:
>>
>> #ifndef _LINUX_KERNEL_H
>> #error don't include <linux/printk.h> directly, use <linux/kernel.h>
>> #endif
>
> That's another possibility. We already have several users, though.

Eh, it is because of sheer amount of printk() users headers aren't switched,
from kernel.h to printk.h

2011-05-25 17:02:48

by Joe Perches

[permalink] [raw]
Subject: [PATCH 0/4] printk: cleanups of direct includes

printk.h is a subfile of kernel.h, do not allow it to
be included directly.

Joe Perches (4):
treewide: Remove direct includes of printk.h
staging: Remove direct includes of printk.h
printk: Don't allow direct inclusion
treewide: Remove asmlinkage from printk

arch/arm/kernel/early_printk.c | 2 +-
arch/ia64/kvm/vmm.c | 2 +-
arch/x86/kernel/early_printk.c | 2 +-
drivers/staging/brcm80211/brcmfmac/dhd_sdio.c | 1 -
drivers/staging/brcm80211/util/bcmutils.c | 1 -
drivers/staging/usbip/usbip_common.h | 2 +-
fs/ext4/inode.c | 1 -
include/linux/oprofile.h | 2 +-
include/linux/printk.h | 10 +++++++---
kernel/printk.c | 6 +++---
kernel/sysctl.c | 3 +--
11 files changed, 16 insertions(+), 16 deletions(-)

--
1.7.4.rc3

2011-05-25 17:02:50

by Joe Perches

[permalink] [raw]
Subject: [PATCH 1/4] treewide: Remove direct includes of printk.h

Make the uses kernel.h instead.

Signed-off-by: Joe Perches <[email protected]>
---
fs/ext4/inode.c | 1 -
include/linux/oprofile.h | 2 +-
kernel/sysctl.c | 3 +--
3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2e95819..cffecac 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -39,7 +39,6 @@
#include <linux/bio.h>
#include <linux/workqueue.h>
#include <linux/kernel.h>
-#include <linux/printk.h>
#include <linux/slab.h>
#include <linux/ratelimit.h>

diff --git a/include/linux/oprofile.h b/include/linux/oprofile.h
index 7f5cfd3..ef7d5b1 100644
--- a/include/linux/oprofile.h
+++ b/include/linux/oprofile.h
@@ -14,10 +14,10 @@
#define OPROFILE_H

#include <linux/types.h>
+#include <linux/kernel.h>
#include <linux/spinlock.h>
#include <linux/init.h>
#include <linux/errno.h>
-#include <linux/printk.h>
#include <asm/atomic.h>

/* Each escaped entry is prefixed by ESCAPE_CODE
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4fc9244..814d227 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -24,14 +24,13 @@
#include <linux/slab.h>
#include <linux/sysctl.h>
#include <linux/signal.h>
-#include <linux/printk.h>
+#include <linux/kernel.h>
#include <linux/proc_fs.h>
#include <linux/security.h>
#include <linux/ctype.h>
#include <linux/kmemcheck.h>
#include <linux/fs.h>
#include <linux/init.h>
-#include <linux/kernel.h>
#include <linux/kobject.h>
#include <linux/net.h>
#include <linux/sysrq.h>
--
1.7.4.rc3

2011-05-25 17:05:03

by Joe Perches

[permalink] [raw]
Subject: [PATCH 2/4] staging: Remove direct includes of printk.h

Make the uses kernel.h instead.

Signed-off-by: Joe Perches <[email protected]>
---
drivers/staging/brcm80211/brcmfmac/dhd_sdio.c | 1 -
drivers/staging/brcm80211/util/bcmutils.c | 1 -
drivers/staging/usbip/usbip_common.h | 2 +-
3 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c b/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
index a71c6f8..b00029a 100644
--- a/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
+++ b/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
@@ -16,7 +16,6 @@

#include <linux/types.h>
#include <linux/kernel.h>
-#include <linux/printk.h>
#include <linux/pci_ids.h>
#include <linux/netdevice.h>
#include <bcmdefs.h>
diff --git a/drivers/staging/brcm80211/util/bcmutils.c b/drivers/staging/brcm80211/util/bcmutils.c
index 43e5bb3..1c8b6c6 100644
--- a/drivers/staging/brcm80211/util/bcmutils.c
+++ b/drivers/staging/brcm80211/util/bcmutils.c
@@ -21,7 +21,6 @@
#include <linux/pci.h>
#include <linux/netdevice.h>
#include <linux/sched.h>
-#include <linux/printk.h>
#include <bcmdefs.h>
#include <stdarg.h>
#include <bcmutils.h>
diff --git a/drivers/staging/usbip/usbip_common.h b/drivers/staging/usbip/usbip_common.h
index 4a641c5..0f9176d 100644
--- a/drivers/staging/usbip/usbip_common.h
+++ b/drivers/staging/usbip/usbip_common.h
@@ -24,7 +24,7 @@
#include <linux/device.h>
#include <linux/interrupt.h>
#include <linux/net.h>
-#include <linux/printk.h>
+#include <linux/kernel.h>
#include <linux/spinlock.h>
#include <linux/types.h>
#include <linux/usb.h>
--
1.7.4.rc3

2011-05-25 17:04:49

by Joe Perches

[permalink] [raw]
Subject: [PATCH 3/4] printk: Don't allow direct inclusion

printk.h is a subfile of kernel.h, do not allow
it to be included directly.

Signed-off-by: Joe Perches <[email protected]>
---
include/linux/printk.h | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index ee048e7..3736545 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -1,3 +1,7 @@
+#ifndef _LINUX_KERNEL_H
+#error "Do not include <linux/printk.h>, use <linux/kernel.h> instead"
+#endif
+
#ifndef __KERNEL_PRINTK__
#define __KERNEL_PRINTK__

--
1.7.4.rc3

2011-05-25 17:02:52

by Joe Perches

[permalink] [raw]
Subject: [PATCH 4/4] treewide: Remove asmlinkage from printk

Remove the now unnecessary asmlinkage attribute from the various
printk prototypes and uses.

Signed-off-by: Joe Perches <[email protected]>
---
arch/arm/kernel/early_printk.c | 2 +-
arch/ia64/kvm/vmm.c | 2 +-
arch/x86/kernel/early_printk.c | 2 +-
include/linux/printk.h | 6 +++---
kernel/printk.c | 6 +++---
5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/arm/kernel/early_printk.c b/arch/arm/kernel/early_printk.c
index 85aa2b2..46798d5 100644
--- a/arch/arm/kernel/early_printk.c
+++ b/arch/arm/kernel/early_printk.c
@@ -36,7 +36,7 @@ static struct console early_console = {
.index = -1,
};

-asmlinkage void early_printk(const char *fmt, ...)
+void early_printk(const char *fmt, ...)
{
char buf[512];
int n;
diff --git a/arch/ia64/kvm/vmm.c b/arch/ia64/kvm/vmm.c
index f0b9cac..40331ac 100644
--- a/arch/ia64/kvm/vmm.c
+++ b/arch/ia64/kvm/vmm.c
@@ -81,7 +81,7 @@ static void vcpu_debug_exit(struct kvm_vcpu *vcpu)
local_irq_restore(psr);
}

-asmlinkage int printk(const char *fmt, ...)
+int printk(const char *fmt, ...)
{
struct kvm_vcpu *vcpu = current_vcpu;
va_list args;
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index cd28a35..b9b323a 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -173,7 +173,7 @@ static struct console early_serial_console = {
static struct console *early_console = &early_vga_console;
static int __initdata early_console_initialized;

-asmlinkage void early_printk(const char *fmt, ...)
+void early_printk(const char *fmt, ...)
{
char buf[512];
int n;
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 3736545..e52bc4b 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -90,16 +90,16 @@ int no_printk(const char *fmt, ...)
return 0;
}

-extern asmlinkage __attribute__ ((format (printf, 1, 2)))
+extern __attribute__ ((format (printf, 1, 2)))
void early_printk(const char *fmt, ...);

extern int printk_needs_cpu(int cpu);
extern void printk_tick(void);

#ifdef CONFIG_PRINTK
-asmlinkage __attribute__ ((format (printf, 1, 0)))
+__attribute__ ((format (printf, 1, 0)))
int vprintk(const char *fmt, va_list args);
-asmlinkage __attribute__ ((format (printf, 1, 2))) __cold
+__attribute__ ((format (printf, 1, 2))) __cold
int printk(const char *fmt, ...);

/*
diff --git a/kernel/printk.c b/kernel/printk.c
index da8ca81..735056c 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -46,7 +46,7 @@
/*
* Architectures can override it:
*/
-void asmlinkage __attribute__((weak)) early_printk(const char *fmt, ...)
+void __attribute__((weak)) early_printk(const char *fmt, ...)
{
}

@@ -704,7 +704,7 @@ static int have_callable_console(void)
* See the vsnprintf() documentation for format string extensions over C99.
*/

-asmlinkage int printk(const char *fmt, ...)
+int printk(const char *fmt, ...)
{
va_list args;
int r;
@@ -794,7 +794,7 @@ static inline void printk_delay(void)
}
}

-asmlinkage int vprintk(const char *fmt, va_list args)
+int vprintk(const char *fmt, va_list args)
{
int printed_len = 0;
int current_log_level = default_message_loglevel;
--
1.7.4.rc3

2011-05-25 17:09:58

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH 1/4] include/linux/printk.h is not self-contained

On 05/25/11 02:09, [email protected] wrote:
> From: Geert Uytterhoeven <[email protected]>
>
> <linux/printk.h> needs to include
> - <stdarg.h> for "va_list",
> - <linux/linkage.h> for "asmlinkage",
> - <linux/types.h> for "bool".
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> include/linux/printk.h | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index ee048e7..6388bc8 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -1,6 +1,10 @@
> #ifndef __KERNEL_PRINTK__
> #define __KERNEL_PRINTK__
>
> +#include <stdarg.h>
> +#include <linux/linkage.h>
> +#include <linux/types.h>
> +
> extern const char linux_banner[];
> extern const char linux_proc_banner[];
>

I agree with this patch along with Joe's 2 header additions...
but I get build errors with it (as reported yesterday):


CC arch/x86/kernel/asm-offsets.s
In file included from linux-next-20110524/include/linux/thread_info.h:53,
from linux-next-20110524/include/linux/preempt.h:9,
from linux-next-20110524/include/linux/spinlock.h:50,
from linux-next-20110524/include/linux/ratelimit.h:5,
from linux-next-20110524/include/linux/printk.h:8,
from linux-next-20110524/include/linux/kernel.h:20,
from linux-next-20110524/arch/x86/include/asm/percpu.h:44,
from linux-next-20110524/arch/x86/include/asm/current.h:5,
from linux-next-20110524/arch/x86/include/asm/processor.h:15,
from linux-next-20110524/arch/x86/include/asm/atomic.h:6,
from linux-next-20110524/include/linux/crypto.h:20,
from linux-next-20110524/arch/x86/kernel/asm-offsets.c:8:
linux-next-20110524/arch/x86/include/asm/thread_info.h:34: error: expected specifier-qualifier-list before 'mm_segment_t'
In file included from linux-next-20110524/include/linux/ratelimit.h:5,
from linux-next-20110524/include/linux/printk.h:8,
from linux-next-20110524/include/linux/kernel.h:20,
from linux-next-20110524/arch/x86/include/asm/percpu.h:44,
from linux-next-20110524/arch/x86/include/asm/current.h:5,
from linux-next-20110524/arch/x86/include/asm/processor.h:15,
from linux-next-20110524/arch/x86/include/asm/atomic.h:6,
from linux-next-20110524/include/linux/crypto.h:20,
from linux-next-20110524/arch/x86/kernel/asm-offsets.c:8:
linux-next-20110524/include/linux/spinlock.h: In function 'spin_unlock_wait':
linux-next-20110524/include/linux/spinlock.h:360: error: implicit declaration of function 'cpu_relax'
In file included from linux-next-20110524/arch/x86/include/asm/atomic.h:6,
from linux-next-20110524/include/linux/crypto.h:20,
from linux-next-20110524/arch/x86/kernel/asm-offsets.c:8:
linux-next-20110524/arch/x86/include/asm/processor.h: At top level:
linux-next-20110524/arch/x86/include/asm/processor.h:707: warning: conflicting types for 'cpu_relax'
linux-next-20110524/arch/x86/include/asm/processor.h:707: error: static declaration of 'cpu_relax' follows non-static declaration
linux-next-20110524/include/linux/spinlock.h:360: note: previous implicit declaration of 'cpu_relax' was here
In file included from linux-next-20110524/arch/x86/include/asm/i387.h:17,
from linux-next-20110524/arch/x86/include/asm/suspend_32.h:10,
from linux-next-20110524/arch/x86/include/asm/suspend.h:2,
from linux-next-20110524/arch/x86/kernel/asm-offsets.c:18:
linux-next-20110524/include/linux/regset.h: In function 'copy_regset_to_user':
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:338: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h: In function 'copy_regset_from_user':
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
linux-next-20110524/include/linux/regset.h:361: error: 'struct thread_info' has no member named 'addr_limit'
In file included from linux-next-20110524/arch/x86/kernel/asm-offsets.c:25:
linux-next-20110524/arch/x86/kernel/asm-offsets_32.c: In function 'foo':
linux-next-20110524/arch/x86/kernel/asm-offsets_32.c:32: error: 'struct thread_info' has no member named 'sysenter_return'
linux-next-20110524/arch/x86/kernel/asm-offsets.c: In function 'common':
linux-next-20110524/arch/x86/kernel/asm-offsets.c:34: error: 'struct thread_info' has no member named 'addr_limit'

--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

2011-05-26 07:38:04

by Roland Vossen

[permalink] [raw]
Subject: Re: [PATCH 2/4] staging: Remove direct includes of printk.h

On 05/25/2011 07:02 PM, Joe Perches wrote:
> Make the uses kernel.h instead.
>
> Signed-off-by: Joe Perches<[email protected]>

Acked-by: Roland Vossen <[email protected]>

2011-05-29 17:00:36

by julie Sullivan

[permalink] [raw]
Subject: Re: [PATCH 2/4] staging: Remove direct includes of printk.h

> --- a/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
> +++ b/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
> @@ -16,7 +16,6 @@
>
> ?#include <linux/types.h>
> ?#include <linux/kernel.h>
> -#include <linux/printk.h>


> --- a/drivers/staging/usbip/usbip_common.h
> +++ b/drivers/staging/usbip/usbip_common.h
> @@ -24,7 +24,7 @@
> ?#include <linux/device.h>
> ?#include <linux/interrupt.h>
> ?#include <linux/net.h>
> -#include <linux/printk.h>
> +#include <linux/kernel.h>
> ?#include <linux/spinlock.h>
> ?#include <linux/types.h>

types.h is also a subfile of kernel.h, right?
Or maybe you patched this one already - if so please excuse the noise :-)

Cheers
Julie

2011-05-29 20:40:48

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 2/4] staging: Remove direct includes of printk.h

On Sun, 2011-05-29 at 18:00 +0100, julie Sullivan wrote:
> > --- a/drivers/staging/brcm80211/brcmfmac/dhd_sdio.c
[]
> types.h is also a subfile of kernel.h, right?

Yes it is.

> Or maybe you patched this one already - if so please excuse the noise :-)

No I didn't patch that one.

I think it wouldn't be good to do that.
printk.h may be a special case because it was
moved out of kernel.h just to make it a bit neater.

You are welcome to if you choose to.

There are like 1500 or so files that have both kernel.h and types.h

$ grep --include=*.[ch] -rP -l "^\s*\#\s*include\s+\<linux/types\.h>" * | \
xargs grep -Pl "^\s*\#\s*include\s+\<linux/kernel\.h\>" | wc -l
1572