[PATCH] x86: fix unconditional arch/x86/kernel/pcspeaker.c compiling
Applies to 2.6.24-rc8-git1
This patch results from Linux Tiny's efforts to hunt for unnecessary

code added unconditionally with "obj-y +="
This patch make the compilation of arch/x86/kernel/pcspeaker.c

depend on PC speaker support. Before that, this file was always

compiled even if pc speaker support wasn't enabled.
I'm checking that CONFIG_INPUT_PCSPKR is not set to "m",

as arch/x86/kernel/pcspeaker.c shouldn't be compiled as a module:

- It defines no init and exit functions

- It defines an initcall which only makes sense at

  kernel boot time.
Results: if PC speaker support is disabled

- It reduces the uncompressed kernel size by 218 bytes!!! ;)

- It must also bring a tiny reduction in boot time

  but suppressing a useless initcall.
Your comments?
---

Signed-off-by: Michael Opdenacker <michael@free-electrons.com>
diff -Naur linux-2.6.24-rc8-git1/arch/x86/kernel/Makefile_32 linux-2.6.24-rc8-git1-nopcspeaker/arch/x86/kernel/Makefile_32

--- linux-2.6.24-rc8-git1/arch/x86/kernel/Makefile_32	2008-01-17 09:48:58.000000000 +0100

+++ linux-2.6.24-rc8-git1-nopcspeaker/arch/x86/kernel/Makefile_32	2008-01-17 10:06:56.000000000 +0100

@@ -45,10 +45,13 @@
 obj-$(CONFIG_VMI)		+= vmi_32.o vmiclock_32.o

 obj-$(CONFIG_PARAVIRT)		+= paravirt_32.o

-obj-y				+= pcspeaker.o

-

 obj-$(CONFIG_SCx200)		+= scx200_32.o
+ifeq ($(CONFIG_INPUT_PCSPKR),y)

+	obj-y			+= pcspeaker.o

+endif

+

+

 # vsyscall_32.o contains the vsyscall DSO images as __initdata.

 # We must build both images before we can assemble it.

 # Note: kbuild does not track this dependency due to usage of .incbin

diff -Naur linux-2.6.24-rc8-git1/arch/x86/kernel/Makefile_64 linux-2.6.24-rc8-git1-nopcspeaker/arch/x86/kernel/Makefile_64

--- linux-2.6.24-rc8-git1/arch/x86/kernel/Makefile_64	2008-01-17 09:48:58.000000000 +0100

+++ linux-2.6.24-rc8-git1-nopcspeaker/arch/x86/kernel/Makefile_64	2008-01-17 10:19:08.000000000 +0100

@@ -40,6 +40,9 @@

 o...
[ message continues ]

I'm not sure this does what you want. What if CONFIG_INPUT_PCSPKR = m?
You ought to cc: this to Ingo (added). I'm pretty sure these Makefiles

have been unified in x86.git already.
--

Mathematics is the supreme nostalgia of our time.
--

Hi Matt,
Thanks for your feedback!
Does it make sense to compile arch/x86/kernel/pcspeaker.c as a module?

It defines no init and exit functions, and it defines an initcall, which

only makes sense at boot time.
That's why I didn't use

obj-$(CONFIG_INPUT_PCSPKR)+=pcspeaker.o
Michael.
--

Michael Opdenacker, Free Electrons

Free Embedded Linux Training Materials

on http://free-electrons.com/training

(More than 1500 pages!)
--

Hardly. No. Absolutely not. pcspeaker.c is a mere 20 lines - when

compiled (on x86_64) a mere 1824 bytes (and most of that is probably

ELF overhead). If you build that as a module, be sure to add an extra

6 to 7 kilobyte as module overhead. No, this is not really worth it.

--

Probably not. However, the above code won't build pcspeaker.o if

CONFIG_INPUT_PCSPKR = m. In other words, it'll break.
--

Mathematics is the supreme nostalgia of our time.
--

Here's a corrected version.
Even if this patch doesn't seem to cause any bug

(tested on a QEMU based PC), I'm wondering if this is "legal" (according

to Linux standards) not to declare a device we don't care about,

but that probably always exists in any PC-compatible machine?

Doesn't this hurt the consistency of the device model?
Another issue would be that we would no longer be able

to load the speaker driver module from a kernel which

wasn't originally compiled with support for this module.
Perhaps we should only use this trick when CONFIG_EMBEDDED is set.

In this case, we know we're using a limited, non-standard kernel,

and having a partial device tree could be acceptable.
What do you think?
Thanks,
Michael.
Signed-off-by: Michael Opdenacker <michael@free-electrons.com>
diff -Naur linux-2.6.24-rc8-git1/arch/x86/kernel/Makefile_32 linux-2.6.24-rc8-git1-nopcspeaker/arch/x86/kernel/Makefile_32

--- linux-2.6.24-rc8-git1/arch/x86/kernel/Makefile_32	2008-01-17 09:48:58.000000000 +0100

+++ linux-2.6.24-rc8-git1-nopcspeaker/arch/x86/kernel/Makefile_32	2008-01-17 22:43:33.000000000 +0100

@@ -45,10 +45,13 @@
 obj-$(CONFIG_VMI)		+= vmi_32.o vmiclock_32.o

 obj-$(CONFIG_PARAVIRT)		+= paravirt_32.o

-obj-y				+= pcspeaker.o

-

 obj-$(CONFIG_SCx200)		+= scx200_32.o
+ifdef CONFIG_INPUT_PCSPKR

+	obj-y			+= pcspeaker.o

+endif

+

+

 # vsyscall_32.o contains the vsyscall DSO images as __initdata.

 # We must build both images before we can assemble it.

 # Note: kbuild does not track this dependency due to usage of .incbin

diff -Naur linux-2.6.24-rc8-git1/arch/x86/kernel/Makefile_64 linux-2.6.24-rc8-git1-nopcspeaker/arch/x86/kernel/Makefile_64

--- linux-2.6.24-rc8-git1/arch/x86/kernel/Makefile_64	2008-01-17 09:48:58.000000000 +0100

+++ linux-2.6.24-rc8-git1-nopcspeaker/arch/x86/kernel/Makefile_64	2008-01-17 22:46:07.000000000 +0100

@@ -40,6 +40,9 @@

 obj-$(CONFIG_PCI)		+= early-quirks.o
 obj-y				+= topology.o

-obj-y				+= pcspeaker.o

+

+ifdef CONFIG_INPUT_PCSP...
[ message continues ]

why didnt you make this:
  obj-$(CONFIG_INPUT_PCSPKR)		+= pcspeaker.o
?
Your patch looks fine to me otherwise, obviously if someone disables

PCSPKR intentionally in the .config, the kernel should just do that.

Could you resend it with the above thing fixed, and against x86.git#mm?

The x86.git coordinates are at:
 http://redhat.com/~mingo/x86.git/README
	Ingo

--

Many thanks for your feedback.
That's what I did first, but if  CONFIG_INPUT_PCSPKR=m,

arch/x86/kernel/pcspeaker.c gets compiled as a module. While compiling

doesn't fail, is this still a valid module? It defines no init and exit

functions, and it defines an initcall, which only makes sense at boot time.
We could make pcspeaker.c depend on another switch, like

CONFIG_PCSPEAKER on mips. We could offer the possibility to disable it

when CONFIG_EMBEDDED is set.
What do you think?
Michael.
--

Michael Opdenacker, Free Electrons

Free Embedded Linux Training Materials

on http://free-electrons.com/training

(More than 1500 pages!)
--

i'm confused, the .ko definitely exists:
 europe:~> uname -r

 2.6.24-0.123.rc6.fc9
 europe:~> lsmod | grep pcspkr

 pcspkr                  6400  0
ah, this is a different .ko.
perhaps the right solution would be to only build it in if

CONFIG_PCSPEAKER is "y" or "m". I.e. your original patch?
	Ingo

--

i've added your patch to x86.git - see below.
	Ingo
----------->

Subject: x86: fix unconditional arch/x86/kernel/pcspeaker.o compiling

From: Michael Opdenacker <michael-lists@free-electrons.com>
do not add the pcspkr platform device if pcspkr support is disabled.
Signed-off-by: Michael Opdenacker <michael@free-electrons.com>

Signed-off-by: Ingo Molnar <mingo@elte.hu>

---

 arch/x86/kernel/Makefile |    3 +++

 1 file changed, 3 insertions(+)
Index: linux-x86.q/arch/x86/kernel/Makefile

===================================================================

--- linux-x86.q.orig/arch/x86/kernel/Makefile

+++ linux-x86.q/arch/x86/kernel/Makefile

@@ -68,7 +68,10 @@ obj-$(CONFIG_MGEODE_LX)		+= geode_32.o m
 obj-$(CONFIG_VMI)		+= vmi_32.o vmiclock_32.o

 obj-$(CONFIG_PARAVIRT)		+= paravirt.o paravirt_patch_$(BITS).o

+

+ifdef CONFIG_INPUT_PCSPKR

 obj-y				+= pcspeaker.o

+endif
 obj-$(CONFIG_SCx200)		+= scx200_32.o
--

However, wouldn't the Makefile look nicer if we introduced a

CONFIG_PCSPEAKER setting as in the mips platform? We would just have:
obj-$(CONFIG_PCSPEAKER)		+= pcspeaker.o
I can propose a corresponding patch, and I'd suggest to make

CONFIG_PCSPEAKER depend on CONFIG_EMBEDDED.
Michael.
--

Michael Opdenacker, Free Electrons

Free Embedded Linux Training Materials

on http://free-electrons.com/training

(More than 1500 pages!)
--

Yes, that would be cleaner. Not worth it for just x86, but there are

about 6 other arches that use CONFIG_INPUT_PCSPKR. Do that in one patch

on top of this one and send it to Andrew (who will pull the above into
No, don't. It's fine the way it is. If INPUT_PCSPKR isn't set, we don't

support the speaker, end of story.
Also, bear in mind that there is a fair amount of lower-hanging fruit

than this, so bouncing a bunch of patches back and forth to make this

perfect is not the best use of people's time.
--

Mathematics is the supreme nostalgia of our time.
--

Well, sorry for consuming your time with this very small change, but I'm

not sure I understand what you're suggesting... Here's what I'd do:
    * I'd define  CONFIG_PCSPEAKER for x86 (typically in

      arch/x86/Kconfig), to be set to "y"  when CONFIG_INPUT_SPKR is

      set. This is what minimizes the changes to make.

    * This way, we can just have:

      obj-$(CONFIG_PCSPEAKER) += pcspeaker.o
Did I understand things right? Anyway, the current situation is not so

Sounds fine! Don't hesitate to let us know about the lower-hanging fruit

you're thinking about. Here are the main things I have so far:
    * Ideas in the existing Linux-Tiny patchset.

    * Disable support for non-Intel processors in x86 (cyrix.c,

      centaur.c, transmeta.c, nexgen.c, umc.c in arch/x86/kernel/cpu).

      As far as I remember, I saved 15 KB when I first experimented with

      this).

    * Disable support for readahead, page writeback, pdflush and swap

      when we have no storage at all (typically booting from an

      initramfs). This corresponds to 69 KB of source code!
So, more ideas are welcome!
Thanks for your time,
Michael.
--

Michael Opdenacker, Free Electrons

Free Embedded Linux Training Materials

on http://free-electrons.com/training

(More than 1500 pages!)
--

Personally, I'm trying to figure out how to build a "hello world" kernel.
I.E. the minimal set of files I need to compile to get a kernel I can boot

under qemu that does something visible.  (Printing "hello world" to the

serial port via the early_serial_write(), perhaps?)
No VFS, no process management, preferably no memory management...  Just get to

a stub start_kernel() that calls early_serial_write() and then does a HLT.  I

note that start_kernel() calls 11 things before the first printk(), and that

without the early_printk stuff that's just going into a buffer.
Preferably, I even want to take this out of the kernel's build process and

build it from the command line or a small shell script.  (zImage packaging

and all.)  I.E. some kind of:
  gcc blah blah blah -o vmlinux

  wrap.sh vmlinux zImage
The reason for this is, of course, that I'm trying to build the kernel with my

tinycc fork, and the whole kernel build is kind of intimidating and

complicated.  (And I currently need to pregenerate asm-offsets.h until I

teach tinycc to produce assembly output or come up with an alternative.)
Currently, a "make allnoconfig" build runs for 479 lines, and builds radix

tree code and /dev/random and sysfs and all sorts of stuff.

Counter-intuitively, allnoconfig _isn't_ the smallest kernel you can build

(by a long shot), to do that you have to switch on the embedded menu and then

switch off everything in it (and all the stuff that shows up right _after_

it, which smells like a bug to me), and switch to SLOB and switch off the

block layer at the top level.  But doing that only gets the build down to 449

lines, and the resulting x86-64 kernel is over 400k.  That is _not_ a "hello

world" kernel.  Not even close. :)
Alas, working from the other end and building your way _up_ isn't trivial

either.  (But has the advantage that I'm starting much closer to my goal than 
Rob

--

"One of my most productive days was throwing away 1000 lines of code."

  - Ken Thompson.

...
[ message continues ]

Probably makes sense to define it right next to INPUT_PCSPKR in

drivers/input/Kconfig.
Then do the appropriate fix for all arches mentioned in INPUT_PCSPKR.
For extra points, you can move the duplicate pcspeaker.c code out of all

those arches and stash it somewhere in drivers/input. Presumably it's
That'd be nice, yes. It would probably make sense to be able to disable

just readahead support when we're working with only solid-state devices.
--

Mathematics is the supreme nostalgia of our time.
--

Here's the patch, after spending some time to get familiar with git.
The patch is against git x86/mm, and it seems to work fine on x86. However,

on x86, you no longer have /sys/devices/platform/pcspkr after

running "make allnoconfig". To get it, you have to explicitely add support

to CONFIG_INPUT_PCSPKR (m or y). I hope this is fine.
I'm copying the MIPS maintainer as this patch touches his tree too.
On MIPS, there should be no impact though, as CONFIG_PCSPEAKER is set in

defconfig files.
In other architectures where CONFIG_INPUT_PCSPKR can exist,

there is a change: when CONFIG_INPUT_PCSPKR is set, the platform device

will be added, while it didn't exist before. I hope this is fine.
In a nutshell, this patch looks good because it removes a small amount

of duplication between mips and x86. On the other hand, it introduces

changes that may not be wanted. Is this worth it?
Michael.
--

Signed-off-by: Michael Opdenacker <michael@free-electrons.com>
---

 arch/mips/kernel/Makefile      |    1 -

 arch/mips/kernel/pcspeaker.c   |   28 ----------------------------

 arch/x86/kernel/Makefile       |    4 ----

 arch/x86/kernel/pcspeaker.c    |   20 --------------------

 drivers/input/misc/Kconfig     |    4 ++++

 drivers/input/misc/Makefile    |    1 +

 drivers/input/misc/pcspeaker.c |   28 ++++++++++++++++++++++++++++

 7 files changed, 33 insertions(+), 53 deletions(-)

 delete mode 100644 arch/mips/kernel/pcspeaker.c

 delete mode 100644 arch/x86/kernel/pcspeaker.c

 create mode 100644 drivers/input/misc/pcspeaker.c
diff --git a/arch/mips/kernel/Makefile b/arch/mips/kernel/Makefile

index ffa0836..9e78e1a 100644

--- a/arch/mips/kernel/Makefile

+++ b/arch/mips/kernel/Makefile

@@ -76,7 +76,6 @@ obj-$(CONFIG_PROC_FS)		+= proc.o

 obj-$(CONFIG_64BIT)		+= cpu-bugs64.o
 obj-$(CONFIG_I8253)		+= i8253.o

-obj-$(CONFIG_PCSPEAKER)		+= pcspeaker.o
 obj-$(CONFIG_KEXEC)		+= machine_kexec.o relocate_kernel.o

 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o

diff --git a/arch/mi...
[ message continues ]

This patch does not apply cleanly to linux-mips Git tree:
----> snip

patching file arch/x86/kernel/Makefile

Hunk #1 FAILED at 69.

1 out of 1 hunk FAILED -- saving rejects to file arch/x86/kernel/Makefile.rej

----> snip
It seems tempting to include at least Alpha in this patch, because they

have a device initcall identical to the one, which you're removing from

the MIPS and x86 arch code.
Thanks,
Dmitri

--

The main linux-mips git tree is derived of Linus' kernel.org tree and

I usually update it daily.  Patch scheduled for the next kernel release or

later go into a special tree from which akpm pulls.  For significant

patch submissions that means patches should be generated either against

the linux-queue tree from linux-mips.org (which only contains the MIPS

specific stuff) or possibly akpm's -mm tree.
  Ralf

--

The defconfig files do not set anything, they are just .config's you can

use as a start for configuring your kernel.
cu

Adrian
-- 
       "Is there not promise of rain?" Ling Tan asked suddenly out

        of the darkness. There had been need of rain for many days.

       "Only a promise," Lao Er said.

                                       Pearl S. Buck - Dragon Seed
--

Oops, yes! I'll update these patches and post them again as soon as I

get a chance (hopefully soon).
Michael.
--

Michael Opdenacker, Free Electrons

Free Embedded Linux Training Materials

on http://free-electrons.com/training

(More than 1500 pages!)
--

Very nice.  From a UI standpoint, shouldn't disabling the block layer take at

least some of that out?  (Or disabling the block layer and all network

filesystems.  /me tries to remember whether jffs2 depends on the block layer.

Now that qemu can fake an mtd, I really need to start playing with that...)
Rob

--

"One of my most productive days was throwing away 1000 lines of code."

  - Ken Thompson.

--

There are a number of laptops now that ship with solid-state disks.

These things look like normal IDE block devices to the kernel, but have

zero seek time and zero rotational latency. So here, prefetch is a waste

of memory and probably increases latency on average.
This will also be true for using prefetch on a typical embedded board

using compact flash through an IDE interface controller (extremely

common).
--

Mathematics is the supreme nostalgia of our time.
--

btw., are there any pending arch/x86 bits in -tiny? (stupid question:

were can i get the most uptodate version of -tiny from?)
	Ingo

--

It's not a stupid question. I dropped updating the tree regulary some

time ago to focus on merging bits and then got a bit side-tracked by

this little thing called "version control".
Michael is attempting to get the tree started again and has put a quilt

up here:
http://elinux.org/images/3/3c/Tiny-quilt-2.6.23-0.tar.bz2
--

Mathematics is the supreme nostalgia of our time.
--

cool! A few comments about the patches that affect arch/x86:
dmi_blacklist.patch:
  +#ifdef CONFIG_DMI_SCAN

          dmi_scan_machine();

  +#endif
put that #ifdef into the include file and make it an inlined NOP in the

!DMI_SCAN case. That de-#ifdef-ifies the .c files.
cpu-support.patch: ditto.
mtrr.patch: ditto.
inflate-error.patch: please use:
   quilt refresh --diffstat --sort --no-timestamps -p 1
when generating patches, to make them reviewable, uniform and

noise-free.
no-doublefault.patch: looks ok. Please add proper metadata and submit to

lkml.
sbf.patch: looks ok.
sysenter.patch:
+#ifdef CONFIG_SYSENTER

        sysenter_setup();

        enable_sep_cpu();

+#endif
hide these #ifdefs in the include file. (as it already does) Looks ok

otherwise.
threadinfo-ool.patch: doesnt this break the scheduler? Had something

like this in -rt, i turned 'current' into a const actually, but had

trouble with keeping from gcc to do the right thing in sched.c. I guess

we could live with current_non_const() that gets it from assembly. (and

hence gcc wont be able to optimize it away in the middle of the

context-switch) But when done properly, this is both a nice, measurable

speedup on UP, and a nice space-saver.
tiny-cflags.patch: obsolete? Isnt CFLAGS already extendable? Question to

Sam i guess.
tsc.patch: look ok. needs x86.git porting.
	Ingo

--

It didn't when I wrote it, 3+ years ago. But I'm sure it needs to be
Yup.
--

Mathematics is the supreme nostalgia of our time.
--

And what was the question then?
We have today the possibility to say:

make KCFLAGS=-whatever
and we have plenty of kconfig adjustmenst affecting the gcc options.
I do not know if this covers it.
	Sam

--

Basically the idea was you could specify various flags that affected

kernel size, in particular overriding the various bloated alignment

defaults.
If I were to do this today (if they haven't already become the default),

I'd probably add a config var to request minimal alignment instead.
--

Mathematics is the supreme nostalgia of our time.
--

as long as it's arch/x86 stuff i can pick up patches no problem.
[ Sidenote: "too small" and "too insignificant" is not a patch attribute

  that is in my vocabulary - by definition the best patches are very

  small and very insignificant (they just happen to end up doing

  something cool a 1000 steps later ;-) 99% of our problems come from

  'too large' and 'too intrusive' patches. ]
	Ingo

--

I tend to agree, but it's well-established that not everyone here is so

patient.
--

Mathematics is the supreme nostalgia of our time.
--

Have you looked at pcspeaker.o? As far as I can tell, it does *nothing*.
--

Taral <taralx@gmail.com>

"Please let me know if there's any further trouble I can give you."

    -- Unknown

--

Do you mean "almost nothing"? It still allocates and adds a platform

device, and the corresponding function always gets called at boot time.
I know that not compiling this piece of code just reduces the

uncompressed kernel size by just a few bytes (218). However, many small

contributions of this kind can have a significant impact on embedded

systems (or on boot media or on Linux based bootloaders).
As I said earlier, I'm starting to think that this trick should only be

used when CONFIG_EMBEDDED is set. In the non-embedded case, it's

probably not acceptable not to declare a platform device that is always

present in the system (while it's perfectly fine not to load the

corresponding driver).
Your comments and suggestions are more than welcome!
Michael.
--

Michael Opdenacker, Free Electrons

Free Embedded Linux Training Materials

on http://free-electrons.com/training

(More than 1500 pages!)
--

Nothing significant then. I don't see any added functionality from this file.
--

Taral <taralx@gmail.com>

"Please let me know if there's any further trouble I can give you."

    -- Unknown

--

Michael.
--

Michael Opdenacker, Free Electrons

Free Embedded Linux Training Materials

on http://free-electrons.com/training

(More than 1500 pages!)
--