Skip to contentSkip to conversation

2.6.7-bk: asm/setup.h and linux/init.h

PATCH Russell King 4 participants 7 messages Jun 26, 2004

Subsystems (5)
MIPS SuperH SPARC + UltraSPARC (sparc/sparc64) PA-RISC architecture +1 more subsystems
lore
Share & tools
lore

Russell King

author
#0 lore raw

Hi,

I notice that recent changes resulted in asm/setup.h being included by linux/init.h, for the saved_command_line change:

@@ -66,6 +67,9 @@

extern initcall_t __con_initcall_start, __con_initcall_end; extern initcall_t __security_initcall_start, __security_initcall_end;
+
+/* Defined in init/main.c */
+extern char saved_command_line[COMMAND_LINE_SIZE];
#endif

#ifndef MODULE

Unfortunately, this causes problems on ARM, because asm/setup.h on ARM needs things like linux/types.h, which I'd rather not include here since:

1. it means that we'll be at odds with what other architectures include (and therefore will hide a missing linux/types.h include when developing on ARM.)
2. linux/init.h is included by the majority of the kernel, and I'd rather not have asm/setup.h added to the dependency of every single kernel source file.

Is there a reason why we can't delete asm/setup.h from linux/init.h and change that declaration to:

+extern char saved_command_line[];

?

IOW, like this (which works fine on ARM):

===== include/linux/init.h 1.32 vs edited =====

include/linux/init.h +1 -2
@@ -3,7 +3,6 @@
 
 #include <linux/config.h>
 #include <linux/compiler.h>
-#include <asm/setup.h>
 
 /* These macros are used to mark some functions or 
  * initialized data (doesn't apply to uninitialized data)

@@ -69,7 +68,7 @@
 extern initcall_t __security_initcall_start, __security_initcall_end;
 
 /* Defined in init/main.c */
-extern char saved_command_line[COMMAND_LINE_SIZE];
+extern char saved_command_line[];
 #endif
   
 #ifndef MODULE

===== init/main.c 1.148 vs edited =====

init/main.c +1 -0
@@ -47,6 +47,7 @@
 
 #include <asm/io.h>
 #include <asm/bugs.h>
+#include <asm/setup.h>
 
 /*
  * This is one of the first .c files built. Error out early
Signature · 4 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core
#1 lore raw
Quoted 5 lines · Russell King

Is there a reason why we can't delete asm/setup.h from linux/init.h and change that declaration to:

+extern char saved_command_line[];

Yes. A number of achitectures use something like

	strlcpy(saved_command_line, cmd_line, sizeof(saved_command_line));

and that "sizeof()" would require the full declaration.

However, I don't see any reason why we couldn't make that sizeof be COMMAND_LINE_SIZE instead, if somebody is willing to grep and do the conversion and make sure everybody who now uses it has <asm/setup.h> included.

Hmm?

Linus

author
#2 lore raw
Quoted 8 lines · Linus Torvalds
Quoted 4 lines · Russell King

Is there a reason why we can't delete asm/setup.h from linux/init.h and change that declaration to:

+extern char saved_command_line[];

Yes. A number of achitectures use something like

Quoted 8 lines
	strlcpy(saved_command_line, cmd_line, sizeof(saved_command_line));

and that "sizeof()" would require the full declaration.

However, I don't see any reason why we couldn't make that sizeof be COMMAND_LINE_SIZE instead, if somebody is willing to grep and do the conversion and make sure everybody who now uses it has <asm/setup.h> included.

Ok, grepping around for sizeof.*saved_command_line and COMMAND_LINE_SIZE resulted in this patch:

===== arch/alpha/kernel/setup.c 1.43 vs edited =====

arch/alpha/kernel/setup.c +1 -0
@@ -39,6 +39,7 @@
 #include <linux/reboot.h>
 #endif
 #include <linux/notifier.h>
+#include <asm/setup.h>
 #include <asm/io.h>
 
 extern struct notifier_block *panic_notifier_list;

===== arch/cris/kernel/setup.c 1.15 vs edited =====

arch/cris/kernel/setup.c +2 -0
@@ -18,6 +18,8 @@
 #include <linux/seq_file.h>
 #include <linux/tty.h>
 
+#include <asm/setup.h>
+
 /*
  * Setup options
  */

===== arch/h8300/kernel/setup.c 1.8 vs edited =====

arch/h8300/kernel/setup.c +2 -2
@@ -155,8 +155,8 @@
 #endif
 	/* Keep a copy of command line */
 	*cmdline_p = &command_line[0];
-	memcpy(saved_command_line, command_line, sizeof(saved_command_line));
-	saved_command_line[sizeof(saved_command_line)-1] = 0;
+	memcpy(saved_command_line, command_line, COMMAND_LINE_SIZE);
+	saved_command_line[COMMAND_LINE_SIZE-1] = 0;
 
 #ifdef DEBUG
 	if (strlen(*cmdline_p))

===== arch/ia64/kernel/setup.c 1.75 vs edited =====

arch/ia64/kernel/setup.c +2 -1
@@ -47,6 +47,7 @@
 #include <asm/sal.h>
 #include <asm/sections.h>
 #include <asm/serial.h>
+#include <asm/setup.h>
 #include <asm/smp.h>
 #include <asm/system.h>
 #include <asm/unistd.h>

@@ -284,7 +285,7 @@
 	ia64_patch_vtop((u64) __start___vtop_patchlist, (u64) __end___vtop_patchlist);
 
 	*cmdline_p = __va(ia64_boot_param->command_line);
-	strlcpy(saved_command_line, *cmdline_p, sizeof(saved_command_line));
+	strlcpy(saved_command_line, *cmdline_p, COMMAND_LINE_SIZE);
 
 	efi_init();
 	io_port_init();

===== arch/m68knommu/kernel/setup.c 1.8 vs edited =====

arch/m68knommu/kernel/setup.c +2 -2
@@ -216,8 +216,8 @@
 
 	/* Keep a copy of command line */
 	*cmdline_p = &command_line[0];
-	memcpy(saved_command_line, command_line, sizeof(saved_command_line));
-	saved_command_line[sizeof(saved_command_line)-1] = 0;
+	memcpy(saved_command_line, command_line, COMMAND_LINE_SIZE);
+	saved_command_line[COMMAND_LINE_SIZE-1] = 0;
 
 #ifdef DEBUG
 	if (strlen(*cmdline_p))

===== arch/mips/kernel/setup.c 1.16 vs edited =====

arch/mips/kernel/setup.c +2 -1
@@ -38,6 +38,7 @@
 #include <asm/bootinfo.h>
 #include <asm/cpu.h>
 #include <asm/sections.h>
+#include <asm/setup.h>
 #include <asm/system.h>
 
 struct cpuinfo_mips cpu_data[NR_CPUS];

@@ -489,7 +490,7 @@
 	do_earlyinitcalls();
 
 	strlcpy(command_line, arcs_cmdline, sizeof(command_line));
-	strlcpy(saved_command_line, command_line, sizeof(saved_command_line));
+	strlcpy(saved_command_line, command_line, COMMAND_LINE_SIZE);
 
 	*cmdline_p = command_line;

===== arch/parisc/kernel/setup.c 1.11 vs edited =====

arch/parisc/kernel/setup.c +1 -0
@@ -44,6 +44,7 @@
 #include <asm/machdep.h>	/* for pa7300lc_init() proto */
 #include <asm/pdc_chassis.h>
 #include <asm/io.h>
+#include <asm/setup.h>
 
 char	command_line[COMMAND_LINE_SIZE];

===== arch/ppc/kernel/setup.c 1.55 vs edited =====

arch/ppc/kernel/setup.c +1 -1
@@ -676,7 +676,7 @@
 	init_mm.brk = (unsigned long) klimit;
 
 	/* Save unparsed command line copy for /proc/cmdline */
-	strlcpy(saved_command_line, cmd_line, sizeof(saved_command_line));
+	strlcpy(saved_command_line, cmd_line, COMMAND_LINE_SIZE);
 	*cmdline_p = cmd_line;
 
 	/* set up the bootmem stuff with available memory */

===== arch/ppc64/kernel/setup.c 1.59 vs edited =====

arch/ppc64/kernel/setup.c +2 -1
@@ -44,6 +44,7 @@
 #include <asm/sections.h>
 #include <asm/btext.h>
 #include <asm/nvram.h>
+#include <asm/setup.h>
 #include <asm/system.h>
 
 extern unsigned long klimit;

@@ -629,7 +630,7 @@
 	init_mm.brk = klimit;
 	
 	/* Save unparsed command line copy for /proc/cmdline */
-	strlcpy(saved_command_line, cmd_line, sizeof(saved_command_line));
+	strlcpy(saved_command_line, cmd_line, COMMAND_LINE_SIZE);
 	*cmdline_p = cmd_line;
 
 	irqstack_early_init();

===== arch/sh/kernel/setup.c 1.19 vs edited =====

arch/sh/kernel/setup.c +1 -0
@@ -25,6 +25,7 @@
 #include <asm/io_generic.h>
 #include <asm/sections.h>
 #include <asm/irq.h>
+#include <asm/setup.h>
 
 #ifdef CONFIG_SH_KGDB
 #include <asm/kgdb.h>

===== arch/sparc/kernel/setup.c 1.29 vs edited =====

arch/sparc/kernel/setup.c +1 -0
@@ -47,6 +47,7 @@
 #include <asm/hardirq.h>
 #include <asm/machines.h>
 #include <asm/cpudata.h>
+#include <asm/setup.h>
 
 struct screen_info screen_info = {
 	0, 0,			/* orig-x, orig-y */

===== arch/sparc64/kernel/setup.c 1.53 vs edited =====

arch/sparc64/kernel/setup.c +1 -0
@@ -47,6 +47,7 @@
 #include <asm/mmu_context.h>
 #include <asm/timer.h>
 #include <asm/sections.h>
+#include <asm/setup.h>
 
 #ifdef CONFIG_IP_PNP
 #include <net/ipconfig.h>

===== arch/v850/kernel/setup.c 1.6 vs edited =====

arch/v850/kernel/setup.c +3 -2
@@ -23,6 +23,7 @@
 #include <linux/init.h>
 
 #include <asm/irq.h>
+#include <asm/setup.h>
 
 #include "mach.h"
 

@@ -63,8 +64,8 @@
 {
 	/* Keep a copy of command line */
 	*cmdline = command_line;
-	memcpy (saved_command_line, command_line, sizeof saved_command_line);
-	saved_command_line[sizeof saved_command_line - 1] = '\0';
+	memcpy (saved_command_line, command_line, COMMAND_LINE_SIZE);
+	saved_command_line[COMMAND_LINE_SIZE - 1] = '\0';
 
 	console_verbose ();

===== arch/x86_64/kernel/head64.c 1.9 vs edited =====

arch/x86_64/kernel/head64.c +1 -0
@@ -16,6 +16,7 @@
 #include <asm/proto.h>
 #include <asm/smp.h>
 #include <asm/bootsetup.h>
+#include <asm/setup.h>
 
 /* Don't add a printk in there. printk relies on the PDA which is not initialized 
    yet. */

===== arch/x86_64/kernel/setup.c 1.43 vs edited =====

arch/x86_64/kernel/setup.c +1 -0
@@ -55,6 +55,7 @@
 #include <asm/bootsetup.h>
 #include <asm/smp.h>
 #include <asm/proto.h>
+#include <asm/setup.h>
 
 /*
  * Machine setup..

===== include/linux/init.h 1.32 vs edited =====

include/linux/init.h +1 -2
@@ -3,7 +3,6 @@
 
 #include <linux/config.h>
 #include <linux/compiler.h>
-#include <asm/setup.h>
 
 /* These macros are used to mark some functions or 
  * initialized data (doesn't apply to uninitialized data)

@@ -69,7 +68,7 @@
 extern initcall_t __security_initcall_start, __security_initcall_end;
 
 /* Defined in init/main.c */
-extern char saved_command_line[COMMAND_LINE_SIZE];
+extern char saved_command_line[];
 #endif
   
 #ifndef MODULE

===== init/main.c 1.148 vs edited =====

init/main.c +1 -0
@@ -47,6 +47,7 @@
 
 #include <asm/io.h>
 #include <asm/bugs.h>
+#include <asm/setup.h>
 
 /*
  * This is one of the first .c files built. Error out early
Signature · 4 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core
#3 lore raw

Could someone speak a few words on why a patch such as this is desirable?

Apparently, there is more value to avoiding non-essential include's than I realize. I'd have preferred doing a memcpy of "sizeof(some_struct)" over doing it for SOME_STRUCT_SIZE constant, and I'd have thought it appropriate to include whatever header files were needed to do that.

The simple "wc -l" size of the kernel source goes up a couple of lines, the code complexity is unchanged, and the generated code is unchanged. So obviously none of those factors motivate this change.

Signature · 3 lines
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373
#4 lore raw
Quoted 2 lines · Paul Jackson

Could someone speak a few words on why a patch such as this is desirable?

Read Russell's original post; including asm-arm/setup.h from linux/init.h enters #include hell. It's purely a practical matter, and while I agree with you in general on sizeof(), we need the constant somewhere anyway to declare the array.

Hope that clarifies,
Rusty.

Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell
#5 lore raw
Quoted 1 line

Russell's original post ... enters #include hell ...

Ah - ok. Thanks. Make sense.

Signature · 3 lines
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373
#6 lore raw
In reply to Russell King
Quoted 2 lines · Russell King

Ok, grepping around for sizeof.*saved_command_line and COMMAND_LINE_SIZE resulted in this patch:

Looks fine to me. Sorry for the breakage.

Rusty.

Signature · 1 line
Anyone who quotes me in their signature is an idiot -- Rusty Russell