[PATCH 1/3] firmware: allow firmware files to be built into kernel image

[PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

Some drivers have their own hacks to bypass the kernel's firmware loader
and build their firmware into the kernel; this renders those unnecessary.

Other drivers don't use the firmware loader at all, because they always
want the firmware to be available. This allows them to start using the
firmware loader.

A third set of drivers already use the firmware loader, but can't be
used without help from userspace, which sometimes requires an initrd.
This allows them to work in a static kernel.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 drivers/base/firmware_class.c     |   22 ++++++++++++++++++++--
 include/asm-generic/vmlinux.lds.h |    7 +++++++
 include/linux/firmware.h          |   20 ++++++++++++++++++++
 3 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 9fd4a85..55b9c80 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -49,6 +49,9 @@ struct firmware_priv {
 	struct timer_list timeout;
 };
 
+extern struct builtin_fw __start_builtin_fw[];
+extern struct builtin_fw __end_builtin_fw[];
+
 static void
 fw_load_abort(struct firmware_priv *fw_priv)
 {
@@ -391,13 +394,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	struct device *f_dev;
 	struct firmware_priv *fw_priv;
 	struct firmware *firmware;
+	struct builtin_fw *builtin;
 	int retval;
 
 	if (!firmware_p)
 		return -EINVAL;
 
-	printk(KERN_INFO "firmware: requesting %s\n", name);
-
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
@@ -406,6 +408,22 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
+	for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
+	     builtin++) {
+		if (strcmp(name, builtin->name))
+			continue;
+		printk(KERN_INFO "firmware: using built-in firmware %s\n",
+		       name);
+		firmware->data = vmalloc(builtin->size);
+		if (!firmware->data)
+			return -ENOMEM;
+		firmware->size = builtin->size;
+		memcpy(firmware->data, builtin->data, builtin->size);
+		return 0;
+	}
+	if (uevent)
+		printk(KERN_INFO "firmware: requesting %s\n", name);
+
 	retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
 	if (retval)
 		goto error_kfree_fw;
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index f054778..8d71a40 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -86,6 +86,13 @@
 		VMLINUX_SYMBOL(__end_pci_fixups_resume) = .;		\
 	}								\
 									\
+	/* Built-in firmware blobs */					\
+	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
+		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
+		*(.builtin_fw)						\
+		VMLINUX_SYMBOL(__end_builtin_fw) = .;			\
+	}								\
+									\
 	/* RapidIO route ops */						\
 	.rio_route        : AT(ADDR(.rio_route) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start_rio_route_ops) = .;		\
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 4d10c73..d014589 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -13,6 +13,26 @@ struct firmware {
 
 struct device;
 
+struct builtin_fw {
+	char *name;
+	void *data;
+	unsigned long size;
+};
+
+/* We have to play tricks here much like stringify() to get the
+   __COUNTER__ macro to be expanded as we want it */
+#define __fw_concat1(x,y) x##y
+#define __fw_concat(x,y) __fw_concat1(x,y)
+
+#define DECLARE_BUILTIN_FIRMWARE(name, blob)				\
+	DECLARE_BUILTIN_FIRMWARE_SIZE(name, &(blob), sizeof(blob))
+
+
+#define DECLARE_BUILTIN_FIRMWARE_SIZE(name, blob, size)			     \
+	static const struct builtin_fw __fw_concat(__builtin_fw,__COUNTER__) \
+	__used __attribute__((__section__(".builtin_fw"))) =		     \
+	{ name, blob, size }
+
 #if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
-- 
1.5.4.5


-- 
dwmw2
-- 
dwmw2

[PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

This allows arbitrary firmware files to be included in the static kernel
where the firmware loader can find them without requiring userspace to
be alive.

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 Makefile             |    2 +-
 drivers/base/Kconfig |   12 ++++++++++++
 firmware/Makefile    |   23 +++++++++++++++++++++++
 3 files changed, 36 insertions(+), 1 deletions(-)
 create mode 100644 firmware/Makefile

diff --git a/Makefile b/Makefile
index 20b3235..ac2ab7e 100644
--- a/Makefile
+++ b/Makefile
@@ -450,7 +450,7 @@ scripts: scripts_basic include/config/auto.conf
 
 # Objects we will link into vmlinux / subdirs we need to visit
 init-y		:= init/
-drivers-y	:= drivers/ sound/
+drivers-y	:= drivers/ sound/ firmware/
 net-y		:= net/
 libs-y		:= lib/
 core-y		:= usr/
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index d7da109..687f097 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -34,6 +34,18 @@ config FW_LOADER
 	  require userspace firmware loading support, but a module built outside
 	  the kernel tree does.
 
+config BUILTIN_FIRMWARE
+       string "Firmware blobs to build into the kernel binary"
+       depends on FW_LOADER
+       help
+         This option allows firmware to be built into the kernel, for the cases
+	 where the user either cannot or doesn't want to provide it from
+	 userspace at runtime (for example, when the firmware in question is
+	 required for accessing the boot device, and the user doesn't want to
+	 use an initrd). Multiple files should be separated with spaces, and
+	 the required files should exist under the firmware/ directory in
+	 the source tree.
+
 config DEBUG_DRIVER
 	bool "Driver Core verbose debug messages"
 	depends on DEBUG_KERNEL
diff --git a/firmware/Makefile b/firmware/Makefile
new file mode 100644
index 0000000..184b8ef
--- /dev/null
+++ b/firmware/Makefile
@@ -0,0 +1,23 @@
+#
+# kbuild file for firmware/
+#
+
+FIRMWARE_BINS := $(subst ",,$(CONFIG_BUILTIN_FIRMWARE))
+FIRMWARE_OBJS := $(patsubst %,%.o, $(FIRMWARE_BINS))
+FIRMWARE_SRCS := $(patsubst %,$(obj)/%.c, $(FIRMWARE_BINS))
+
+
+quiet_cmd_fwbin = MK_FW   $@
+      cmd_fwbin = echo '/* File automatically generated */' > $@ ;	 \
+		  echo '\#include <linux/firmware.h>' >> $@ ;		 \
+		  echo 'static const unsigned char fw[] = {' >> $@ ;	 \
+		  od -t x1 -A none -v $(srctree)/$(patsubst %.c,%,$@) | \
+		   sed -e 's/ /, 0x/g' -e 's/^,//' -e 's/$$/,/' >> $@ ; \
+		  echo '};' >> $@ ;					 \
+		  echo 'DECLARE_BUILTIN_FIRMWARE("$(patsubst firmware/%.c,%,$@)",fw);' >> $@
+
+$(FIRMWARE_SRCS): $(obj)/%.c: $(srctree)/$(obj)/%
+	$(call cmd,fwbin)
+
+obj-y := $(FIRMWARE_OBJS)
+
-- 
1.5.4.5


-- 
dwmw2

Re: [PATCH 2/3] firmware: convert korg1212 driver to use firmware loader exclusively

[David Woodhouse]

Now that we can build firmware into the kernel and have the firmware
loader find it, we don't need this kind of special case in drivers...

Signed-off-by: David Woodhouse <dwmw2@infradead.org>
---
 sound/pci/korg1212/Makefile                        |    1 +
 .../{korg1212-firmware.h => korg1212-firmware.c}   |    4 ++++
 sound/pci/korg1212/korg1212.c                      |   18 ------------------
 3 files changed, 5 insertions(+), 18 deletions(-)

 (git diff to keep size down;
  cf. {git,http}://git.infradead.org/users/dwmw2/firmware-2.6.git )

diff --git a/sound/pci/korg1212/Makefile b/sound/pci/korg1212/Makefile
index f11ce1b..7a5ebdf 100644
--- a/sound/pci/korg1212/Makefile
+++ b/sound/pci/korg1212/Makefile
@@ -7,3 +7,4 @@ snd-korg1212-objs := korg1212.o
 
 # Toplevel Module Dependency
 obj-$(CONFIG_SND_KORG1212) += snd-korg1212.o
+obj-$(CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL) += korg1212-firmware.o
diff --git a/sound/pci/korg1212/korg1212-firmware.h b/sound/pci/korg1212/korg1212-firmware.c
similarity index 99%
rename from sound/pci/korg1212/korg1212-firmware.h
rename to sound/pci/korg1212/korg1212-firmware.c
index f6f5b91..2468ee2 100644
--- a/sound/pci/korg1212/korg1212-firmware.h
+++ b/sound/pci/korg1212/korg1212-firmware.c
@@ -1,3 +1,4 @@
+#include <linux/firmware.h>
 static char dspCode [] = {
 0x01,0xff,0x18,0xff,0xf5,0xff,0xcf,0xff,0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,
 0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,
@@ -985,3 +986,6 @@ static char dspCode [] = {
 0x00,0xff,0x40,0xff,0xff,0xff,0xc4,0xff,0x00,0xff,0x0a,0xff,0xff,0xff,0x0f,0xff,
 0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,
 0xff,0xff,0xff,0xff };
+
+DECLARE_BUILTIN_FIRMWARE("korg/k1212.dsp", dspCode);
+
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
index f4c85b5..4a44c0f 100644
--- a/sound/pci/korg1212/korg1212.c
+++ b/sound/pci/korg1212/korg1212.c
@@ -260,14 +260,6 @@ enum MonitorModeSelector {
 #define COMMAND_ACK_DELAY   13         // number of RTC ticks to wait for an acknowledgement
                                        //    from the card after sending a command.
 
-#ifdef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
-#include "korg1212-firmware.h"
-static const struct firmware static_dsp_code = {
-	.data = (u8 *)dspCode,
-	.size = sizeof dspCode
-};
-#endif
-
 enum ClockSourceIndex {
    K1212_CLKIDX_AdatAt44_1K = 0,    // selects source as ADAT at 44.1 kHz
    K1212_CLKIDX_AdatAt48K,          // selects source as ADAT at 48 kHz
@@ -412,9 +404,7 @@ struct snd_korg1212 {
 MODULE_DESCRIPTION("korg1212");
 MODULE_LICENSE("GPL");
 MODULE_SUPPORTED_DEVICE("{{KORG,korg1212}}");
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 MODULE_FIRMWARE("korg/k1212.dsp");
-#endif
 
 static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;     /* Index 0-MAX */
 static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;	   /* ID for this card */
@@ -2348,9 +2338,6 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
         korg1212->AdatTimeCodePhy = korg1212->sharedBufferPhy +
 		offsetof(struct KorgSharedBuffer, AdatTimeCode);
 
-#ifdef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
-	dsp_code = &static_dsp_code;
-#else
 	err = request_firmware(&dsp_code, "korg/k1212.dsp", &pci->dev);
 	if (err < 0) {
 		release_firmware(dsp_code);
@@ -2358,15 +2345,12 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
 		snd_korg1212_free(korg1212);
 		return err;
 	}
-#endif
 
 	if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(pci),
 				dsp_code->size, &korg1212->dma_dsp) < 0) {
 		snd_printk(KERN_ERR "korg1212: cannot allocate dsp code memory (%zd bytes)\n", dsp_code->size);
                 snd_korg1212_free(korg1212);
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 		release_firmware(dsp_code);
-#endif
                 return -ENOMEM;
         }
 
@@ -2376,9 +2360,7 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
 
 	memcpy(korg1212->dma_dsp.area, dsp_code->data, dsp_code->size);
 
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 	release_firmware(dsp_code);
-#endif
 
 	rc = snd_korg1212_Send1212Command(korg1212, K1212_DB_RebootCard, 0, 0, 0, 0);
 


-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Takashi Iwai]

At Fri, 23 May 2008 14:44:42 +0100,
David Woodhouse wrote:
> 
> Some drivers have their own hacks to bypass the kernel's firmware loader
> and build their firmware into the kernel; this renders those unnecessary.
> 
> Other drivers don't use the firmware loader at all, because they always
> want the firmware to be available. This allows them to start using the
> firmware loader.

This looks like a nice cleanup.  It'd be nicer if we can skip vmalloc
and duplication of the image for static binaries.

BTW, is the dependency considered?  FW_LOADER depends on HOTPLUG, and
the static loader shouldn't depend on HOTPLUG.


thanks,

Takashi

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Alan Cox]

> A third set of drivers already use the firmware loader, but can't be
> used without help from userspace, which sometimes requires an initrd.
> This allows them to work in a static kernel.

This makes a lot of sense to me - its small, clean and while I don't
think its relevant to the PC world it has real clear advantages to the
embedded space.

For the embedded case isn't either compressing the firmware or not doing
the vmalloc/copy worth the effort however ?

Re: [PATCH 2/3] firmware: convert korg1212 driver to use firmware loader exclusively

[Takashi Iwai]

At Fri, 23 May 2008 14:50:54 +0100,
David Woodhouse wrote:
> 
> Now that we can build firmware into the kernel and have the firmware
> loader find it, we don't need this kind of special case in drivers...
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>

There are a few more sound drivers doing similar things:
isa/snd-sb16-csp, isa/snd-wavefront, pci/snd-maestro3, and
pci/snd-ymfpci.

I guess you already know but just to be sure...


thanks,

Takashi

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 16:53 +0200, Takashi Iwai wrote:
> This looks like a nice cleanup.  It'd be nicer if we can skip vmalloc
> and duplication of the image for static binaries.

Yeah, I thought about that. I was slightly concerned that drivers might
actually try to _modify_ the firmware they loaded, though. As it is,
there's no fundamental reason why they shouldn't do that.

If we're willing to declare that drivers mustn't write to the firmware
data, then we could do as you suggest.

> BTW, is the dependency considered?  FW_LOADER depends on HOTPLUG, and
> the static loader shouldn't depend on HOTPLUG.

Ah, good point; I'll take a closer look at that. Thanks.

-- 
dwmw2

Re: [PATCH 2/3] firmware: convert korg1212 driver to use firmware loader exclusively

[David Woodhouse]

On Fri, 2008-05-23 at 16:56 +0200, Takashi Iwai wrote:
> 
> There are a few more sound drivers doing similar things:
> isa/snd-sb16-csp, isa/snd-wavefront, pci/snd-maestro3, and
> pci/snd-ymfpci.
>
> I guess you already know but just to be sure...
 
Yeah, thanks -- and then there are the drivers which don't even have the
_option_ of using the firmware loader yet. I just did one, as a proof of
concept...


-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Clemens Ladisch]

David Woodhouse wrote:
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> +	/* Built-in firmware blobs */					\
> +	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> +		*(.builtin_fw)						\
> +		VMLINUX_SYMBOL(__end_builtin_fw) = .;			\
> +	}								\
> +									\

Most drivers, when built as a module, would want their firmware images
to be in the module and not in the kernel image.

This would require some (un)register_builtint_firmware() function.


Regards,
Clemens

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 15:56 +0100, Alan Cox wrote:
> > A third set of drivers already use the firmware loader, but can't be
> > used without help from userspace, which sometimes requires an initrd.
> > This allows them to work in a static kernel.
> 
> This makes a lot of sense to me - its small, clean and while I don't
> think its relevant to the PC world it has real clear advantages to the
> embedded space.
> 
> For the embedded case isn't either compressing the firmware or not doing
> the vmalloc/copy worth the effort however ?

For avoiding the vmalloc/copy see my earlier response.

Adding compression would be cute; I did think about that but figured it
could wait for later (actually I think at the time I meant "later" to be
today too, but then I forgot :)

-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Takashi Iwai]

At Fri, 23 May 2008 15:58:31 +0100,
David Woodhouse wrote:
> 
> On Fri, 2008-05-23 at 16:53 +0200, Takashi Iwai wrote:
> > This looks like a nice cleanup.  It'd be nicer if we can skip vmalloc
> > and duplication of the image for static binaries.
> 
> Yeah, I thought about that. I was slightly concerned that drivers might
> actually try to _modify_ the firmware they loaded, though. As it is,
> there's no fundamental reason why they shouldn't do that.

Hm, but request_firmware() returns the const pointer.  It implies that
the caller shouldn't change the content, doesn't it?

I cannot guarantee that there must be no misbehaving drivers,
though...


thanks,

Takashi

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 17:00 +0200, Clemens Ladisch wrote:
> David Woodhouse wrote:
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > +	/* Built-in firmware blobs */					\
> > +	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
> > +		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> > +		*(.builtin_fw)						\
> > +		VMLINUX_SYMBOL(__end_builtin_fw) = .;			\
> > +	}								\
> > +									\
> 
> Most drivers, when built as a module, would want their firmware images
> to be in the module and not in the kernel image.
> 
> This would require some (un)register_builtint_firmware() function.

If you're willing to build the driver as a module, surely you can use
the proper firmware loader too? We have MODULE_FIRMWARE to make sure
that the firmware ends up in the right place with it, after all.

-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 17:19 +0200, Takashi Iwai wrote:
> Hm, but request_firmware() returns the const pointer.  It implies that
> the caller shouldn't change the content, doesn't it?
> 
> I cannot guarantee that there must be no misbehaving drivers,
> though...

The 'struct firmware' is const. It contains a pointer to data.
That data is not const.

 fw->data = some_other_data; 	// bad.
 fw->data[0] = 0;		// ok.

Of course, we could _make_ it const (if we can remember whether it's
'const u8 *' or 'u8 const *'...)

-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Alan Cox]

On Fri, 23 May 2008 17:00:03 +0200
Clemens Ladisch <clemens@ladisch.de> wrote:

> David Woodhouse wrote:
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > +	/* Built-in firmware blobs */					\
> > +	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
> > +		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> > +		*(.builtin_fw)						\
> > +		VMLINUX_SYMBOL(__end_builtin_fw) = .;			\
> > +	}								\
> > +									\
> 
> Most drivers, when built as a module, would want their firmware images
> to be in the module and not in the kernel image.

When built as a module you must be using an initrd or have a file system
mounted so that case simply doesn't matter or arise.

Alan

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Alan Cox]

> Yeah, I thought about that. I was slightly concerned that drivers might
> actually try to _modify_ the firmware they loaded, though. As it is,
> there's no fundamental reason why they shouldn't do that.

I've not seen any that do and judicious use of "const" should catch
offenders.

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Sam Ravnborg]

Hi David.

> +extern struct builtin_fw __start_builtin_fw[];
> +extern struct builtin_fw __end_builtin_fw[];

Could have these in include/linux/sections.h where we
collect other linker symbols?

> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index f054778..8d71a40 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -86,6 +86,13 @@
>  		VMLINUX_SYMBOL(__end_pci_fixups_resume) = .;		\
>  	}								\
>  									\
> +	/* Built-in firmware blobs */					\
> +	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
> +		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> +		*(.builtin_fw)						\
> +		VMLINUX_SYMBOL(__end_builtin_fw) = .;			\
> +	}								\
> +									\
Looks good.
But do we need the firmware after init?
In other owrds could it be inside a discard after init block?


> +
> +#define DECLARE_BUILTIN_FIRMWARE_SIZE(name, blob, size)			     \
> +	static const struct builtin_fw __fw_concat(__builtin_fw,__COUNTER__) \
> +	__used __attribute__((__section__(".builtin_fw"))) =		     \

Use __section():

> +	__used __section(.builtin_fw))) =		     \
(See compiler.h IIRC).

	Sam

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Dillow]

On Fri, 2008-05-23 at 18:21 +0200, Sam Ravnborg wrote:
> But do we need the firmware after init?
> In other owrds could it be inside a discard after init block?

I think that'll depend on the device. The typhoon NIC driver needs its
firmware on 'ifconfig eth? up', and on resume, so it will need it after
we discard the init block. I expect others will need it at the very
least for resume.

Not that I'm using firmware loader for it right now, but it's been on my
TODO list for ages...

Dave

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Lennart Sorensen]

On Fri, May 23, 2008 at 06:21:01PM +0200, Sam Ravnborg wrote:
> Looks good.
> But do we need the firmware after init?
> In other owrds could it be inside a discard after init block?

What is you can hotplug the device and hence need to initialize the same
device type multiple times?  Wouldn't keeping the firmware around be
useful then?  How about on suspend/resume, do you need to reload
firmware in those cases?

-- 
Len Sorensen

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Sam Ravnborg]

Hi David.

On Fri, May 23, 2008 at 02:46:14PM +0100, David Woodhouse wrote:
> This allows arbitrary firmware files to be included in the static kernel
> where the firmware loader can find them without requiring userspace to
> be alive.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>  Makefile             |    2 +-
>  drivers/base/Kconfig |   12 ++++++++++++
>  firmware/Makefile    |   23 +++++++++++++++++++++++
>  3 files changed, 36 insertions(+), 1 deletions(-)
>  create mode 100644 firmware/Makefile
> 
> diff --git a/Makefile b/Makefile
> index 20b3235..ac2ab7e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -450,7 +450,7 @@ scripts: scripts_basic include/config/auto.conf
>  
>  # Objects we will link into vmlinux / subdirs we need to visit
>  init-y		:= init/
> -drivers-y	:= drivers/ sound/
> +drivers-y	:= drivers/ sound/ firmware/
>  net-y		:= net/
>  libs-y		:= lib/
>  core-y		:= usr/
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index d7da109..687f097 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -34,6 +34,18 @@ config FW_LOADER
>  	  require userspace firmware loading support, but a module built outside
>  	  the kernel tree does.
>  
> +config BUILTIN_FIRMWARE
> +       string "Firmware blobs to build into the kernel binary"
> +       depends on FW_LOADER
> +       help
> +         This option allows firmware to be built into the kernel, for the cases
> +	 where the user either cannot or doesn't want to provide it from
> +	 userspace at runtime (for example, when the firmware in question is
> +	 required for accessing the boot device, and the user doesn't want to
> +	 use an initrd). Multiple files should be separated with spaces, and
> +	 the required files should exist under the firmware/ directory in
> +	 the source tree.
> +
>  config DEBUG_DRIVER
>  	bool "Driver Core verbose debug messages"
>  	depends on DEBUG_KERNEL
> diff --git a/firmware/Makefile b/firmware/Makefile
> new file mode 100644
> index 0000000..184b8ef
> --- /dev/null
> +++ b/firmware/Makefile
> @@ -0,0 +1,23 @@
> +#
> +# kbuild file for firmware/
> +#
> +
> +FIRMWARE_BINS := $(subst ",,$(CONFIG_BUILTIN_FIRMWARE))
> +FIRMWARE_OBJS := $(patsubst %,%.o, $(FIRMWARE_BINS))
> +FIRMWARE_SRCS := $(patsubst %,$(obj)/%.c, $(FIRMWARE_BINS))

My personal rule-of-thumb is to use lower case for
all local variables and UPPER case for global variables.

I know that outside the kernel everyone use UPPER case
for Makefile variables but that just not readable.
So in this code snippet I would have used lower case.

> +
> +
> +quiet_cmd_fwbin = MK_FW   $@
> +      cmd_fwbin = echo '/* File automatically generated */' > $@ ;	 \
> +		  echo '\#include <linux/firmware.h>' >> $@ ;		 \
> +		  echo 'static const unsigned char fw[] = {' >> $@ ;	 \
> +		  od -t x1 -A none -v $(srctree)/$(patsubst %.c,%,$@) | \
> +		   sed -e 's/ /, 0x/g' -e 's/^,//' -e 's/$$/,/' >> $@ ; \
> +		  echo '};' >> $@ ;					 \
> +		  echo 'DECLARE_BUILTIN_FIRMWARE("$(patsubst firmware/%.c,%,$@)",fw);' >> $@
A small comment is justified here.
Do not consider everyone to know od.
If you choose a deciaml output you do not need to add 0x

> +
> +$(FIRMWARE_SRCS): $(obj)/%.c: $(srctree)/$(obj)/%
> +	$(call cmd,fwbin)
> +
> +obj-y := $(FIRMWARE_OBJS)

Assignment to targets i smiisng so generated files are not cleaned
by "make clean".

targets := $(FIRMWARE_OBJS)

	Sam

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Sam Ravnborg]

On Fri, May 23, 2008 at 12:38:41PM -0400, Lennart Sorensen wrote:
> On Fri, May 23, 2008 at 06:21:01PM +0200, Sam Ravnborg wrote:
> > Looks good.
> > But do we need the firmware after init?
> > In other owrds could it be inside a discard after init block?
> 
> What is you can hotplug the device and hence need to initialize the same
> device type multiple times?  Wouldn't keeping the firmware around be
> useful then?  How about on suspend/resume, do you need to reload
> firmware in those cases?

There are cases where we need it after init.
But if there are cases where we do not need it after init maybe
we should be able to save some memory there?

Let us get it working as is - then we can add such features later.

	Sam

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Rene Herman]

On 23-05-08 18:44, Sam Ravnborg wrote:

> On Fri, May 23, 2008 at 12:38:41PM -0400, Lennart Sorensen wrote:
>> On Fri, May 23, 2008 at 06:21:01PM +0200, Sam Ravnborg wrote:
>>> Looks good.
>>> But do we need the firmware after init?
>>> In other owrds could it be inside a discard after init block?
>> What is you can hotplug the device and hence need to initialize the same
>> device type multiple times?  Wouldn't keeping the firmware around be
>> useful then?  How about on suspend/resume, do you need to reload
>> firmware in those cases?
> 
> There are cases where we need it after init.
> But if there are cases where we do not need it after init maybe
> we should be able to save some memory there?
> 
> Let us get it working as is - then we can add such features later.

If embedded is the main consumer I would consider this a fairly vital 
feature though.

Rene.

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 18:53 +0200, Rene Herman wrote:
> On 23-05-08 18:44, Sam Ravnborg wrote:
> 
> > On Fri, May 23, 2008 at 12:38:41PM -0400, Lennart Sorensen wrote:
> >> On Fri, May 23, 2008 at 06:21:01PM +0200, Sam Ravnborg wrote:
> >>> Looks good.
> >>> But do we need the firmware after init?
> >>> In other owrds could it be inside a discard after init block?
> >> What is you can hotplug the device and hence need to initialize the same
> >> device type multiple times?  Wouldn't keeping the firmware around be
> >> useful then?  How about on suspend/resume, do you need to reload
> >> firmware in those cases?
> > 
> > There are cases where we need it after init.
> > But if there are cases where we do not need it after init maybe
> > we should be able to save some memory there?
> > 
> > Let us get it working as is - then we can add such features later.
> 
> If embedded is the main consumer I would consider this a fairly vital 
> feature though.

Remember, for now this is mostly just replacing the cases where the
firmware used to be unconditionally present in the kernel. If you really
want to avoid taking kernel memory, put it in userspace.

-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

On Fri, 2008-05-23 at 18:41 +0200, Sam Ravnborg wrote:
> > +FIRMWARE_BINS := $(subst ",,$(CONFIG_BUILTIN_FIRMWARE))
> > +FIRMWARE_OBJS := $(patsubst %,%.o, $(FIRMWARE_BINS))
> > +FIRMWARE_SRCS := $(patsubst %,$(obj)/%.c, $(FIRMWARE_BINS))
> 
> My personal rule-of-thumb is to use lower case for
> all local variables and UPPER case for global variables.
> 
> I know that outside the kernel everyone use UPPER case
> for Makefile variables but that just not readable.
> So in this code snippet I would have used lower case.

OK, I'll do that.

> > +
> > +
> > +quiet_cmd_fwbin = MK_FW   $@
> > +      cmd_fwbin = echo '/* File automatically generated */' > $@ ;	 \
> > +		  echo '\#include <linux/firmware.h>' >> $@ ;		 \
> > +		  echo 'static const unsigned char fw[] = {' >> $@ ;	 \
> > +		  od -t x1 -A none -v $(srctree)/$(patsubst %.c,%,$@) | \
> > +		   sed -e 's/ /, 0x/g' -e 's/^,//' -e 's/$$/,/' >> $@ ; \
> > +		  echo '};' >> $@ ;					 \
> > +		  echo 'DECLARE_BUILTIN_FIRMWARE("$(patsubst firmware/%.c,%,$@)",fw);' >> $@

> A small comment is justified here.
> Do not consider everyone to know od.

Isn't the context enough? Some comments are just superfluous :)

> If you choose a deciaml output you do not need to add 0x

Hm, that's true -- force of habit, I suppose. But it doesn't really
matter, and if anyone _does_ have cause to look at the generated file,
it's probably nicer in hex.

Actually, it would be nicer to do it in assembly and use .incbin -- but
then we'd have to deal with potential struct alignment issues for the
'struct builtin_fw'. Again, that's an improvement for later. But it's
why I used 'unsigned long' for the size, instead of 'size_t'.

> > +
> > +$(FIRMWARE_SRCS): $(obj)/%.c: $(srctree)/$(obj)/%
> > +	$(call cmd,fwbin)
> > +
> > +obj-y := $(FIRMWARE_OBJS)
> 
> Assignment to targets i smiisng so generated files are not cleaned
> by "make clean".
> 
> targets := $(FIRMWARE_OBJS)

OK, thanks.

-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 16:33 +0100, Alan Cox wrote:
> > Yeah, I thought about that. I was slightly concerned that drivers might
> > actually try to _modify_ the firmware they loaded, though. As it is,
> > there's no fundamental reason why they shouldn't do that.
> 
> I've not seen any that do and judicious use of "const" should catch
> offenders.

Makes sense. I'll do that as the first patch of the sequence.

-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 18:21 +0200, Sam Ravnborg wrote:
> Hi David.
> 
> > +extern struct builtin_fw __start_builtin_fw[];
> > +extern struct builtin_fw __end_builtin_fw[];
> 
> Could have these in include/linux/sections.h where we
> collect other linker symbols?

Hm. But they're not 'extern char' like the other things in
asm-generic/sections.h; they're 'struct builtin_fw'. So I'd want to
include <linux/firmware.h> from asm-generic/sections.h in order to make
that work... I think the simple answer is 'no'.

We don't do this for PCI fixups either.

> > diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> > index f054778..8d71a40 100644
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -86,6 +86,13 @@
> >  		VMLINUX_SYMBOL(__end_pci_fixups_resume) = .;		\
> >  	}								\
> >  									\
> > +	/* Built-in firmware blobs */					\
> > +	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
> > +		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
> > +		*(.builtin_fw)						\
> > +		VMLINUX_SYMBOL(__end_builtin_fw) = .;			\
> > +	}								\
> > +									\
> Looks good.
> But do we need the firmware after init?
> In other owrds could it be inside a discard after init block?

If we don't want the firmware in-kernel at all times, stick it an
initrd :)

> 
> > +
> > +#define DECLARE_BUILTIN_FIRMWARE_SIZE(name, blob, size)			     \
> > +	static const struct builtin_fw __fw_concat(__builtin_fw,__COUNTER__) \
> > +	__used __attribute__((__section__(".builtin_fw"))) =		     \
> 
> Use __section():
> 
> > +	__used __section(.builtin_fw))) =		     \
> (See compiler.h IIRC).

Done. Thanks.

-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

Er, is this my fault or was it like this already...

pmac /pmac/git/linux-2.6 $ make O=/pmac/git/foo 
  GEN     /pmac/git/foo/Makefile
  HOSTCC  scripts/kconfig/zconf.tab.o
gcc: /pmac/git/linux-2.6/scripts/kconfig/zconf.tab.c: No such file or directory
gcc: no input files

-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Sam Ravnborg]

On Fri, May 23, 2008 at 07:07:30PM +0100, David Woodhouse wrote:
> Er, is this my fault or was it like this already...
> 
> pmac /pmac/git/linux-2.6 $ make O=/pmac/git/foo 
>   GEN     /pmac/git/foo/Makefile
>   HOSTCC  scripts/kconfig/zconf.tab.o
> gcc: /pmac/git/linux-2.6/scripts/kconfig/zconf.tab.c: No such file or directory
> gcc: no input files

In some obscure cases I have seen this.
I never have had the energy to chase it down.

I recall I just deleted scripts/* from my output directory
and then it worked.
But this is maybe 4 months ago so memory is weak.

	Sam

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 16:33 +0100, Alan Cox wrote:
> > Yeah, I thought about that. I was slightly concerned that drivers might
> > actually try to _modify_ the firmware they loaded, though. As it is,
> > there's no fundamental reason why they shouldn't do that.
> 
> I've not seen any that do and judicious use of "const" should catch
> offenders.

Like this one...

static const int dvico_firmware_id_offsets[] = { 6638, 3204 };
static int bluebird_patch_dvico_firmware_download(struct usb_device *udev,
						  const struct firmware *fw)
{
	int pos;

	for (pos = 0; pos < ARRAY_SIZE(dvico_firmware_id_offsets); pos++) {
		int idoff = dvico_firmware_id_offsets[pos];

		if (fw->size < idoff + 4)
			continue;

		if (fw->data[idoff] == (USB_VID_DVICO & 0xff) &&
		    fw->data[idoff + 1] == USB_VID_DVICO >> 8) {
			fw->data[idoff + 2] =
				le16_to_cpu(udev->descriptor.idProduct) + 1;
			fw->data[idoff + 3] =
				le16_to_cpu(udev->descriptor.idProduct) >> 8;

			return usb_cypress_load_firmware(udev, fw, CYPRESS_FX2);
		}
	}

	return -EINVAL;
}

-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 20:14 +0100, David Woodhouse wrote:
> On Fri, 2008-05-23 at 16:33 +0100, Alan Cox wrote:
> > > Yeah, I thought about that. I was slightly concerned that drivers might
> > > actually try to _modify_ the firmware they loaded, though. As it is,
> > > there's no fundamental reason why they shouldn't do that.
> > 
> > I've not seen any that do and judicious use of "const" should catch
> > offenders.
> 
> Like this one...

That's fairly much the kind of thing I was expecting we might see, I'm
not sure it's reasonable to suddenly declare that overwriting the
firmware is forbidden. I'd done the version which avoided the extra
vmalloc and copy, but I've just reverted it.

Here's the full patch in the git tree
(git.infradead.org/users/dwmw2/firmware.git) now. Did I miss something?

I think the only think I haven't addressed is the dependency on HOTPLUG.
I'm comfortable leaving that as-is for now. If/when we want to convert a
driver where that dependency is an issue, we can take a closer look.

diff --git a/Makefile b/Makefile
index 20b3235..ac2ab7e 100644
--- a/Makefile
+++ b/Makefile
@@ -450,7 +450,7 @@ scripts: scripts_basic include/config/auto.conf
 
 # Objects we will link into vmlinux / subdirs we need to visit
 init-y		:= init/
-drivers-y	:= drivers/ sound/
+drivers-y	:= drivers/ sound/ firmware/
 net-y		:= net/
 libs-y		:= lib/
 core-y		:= usr/
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index d7da109..687f097 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -34,6 +34,18 @@ config FW_LOADER
 	  require userspace firmware loading support, but a module built outside
 	  the kernel tree does.
 
+config BUILTIN_FIRMWARE
+       string "Firmware blobs to build into the kernel binary"
+       depends on FW_LOADER
+       help
+         This option allows firmware to be built into the kernel, for the cases
+	 where the user either cannot or doesn't want to provide it from
+	 userspace at runtime (for example, when the firmware in question is
+	 required for accessing the boot device, and the user doesn't want to
+	 use an initrd). Multiple files should be separated with spaces, and
+	 the required files should exist under the firmware/ directory in
+	 the source tree.
+
 config DEBUG_DRIVER
 	bool "Driver Core verbose debug messages"
 	depends on DEBUG_KERNEL
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 9fd4a85..3abf57d 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -49,6 +49,14 @@ struct firmware_priv {
 	struct timer_list timeout;
 };
 
+#ifdef CONFIG_FW_LOADER
+extern struct builtin_fw __start_builtin_fw[];
+extern struct builtin_fw __end_builtin_fw[];
+#else /* Module case. Avoid ifdefs later; it'll all optimise out */
+static struct builtin_fw *__start_builtin_fw = NULL;
+static struct builtin_fw *__end_builtin_fw = NULL;
+#endif
+
 static void
 fw_load_abort(struct firmware_priv *fw_priv)
 {
@@ -391,13 +399,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	struct device *f_dev;
 	struct firmware_priv *fw_priv;
 	struct firmware *firmware;
+	struct builtin_fw *builtin;
 	int retval;
 
 	if (!firmware_p)
 		return -EINVAL;
 
-	printk(KERN_INFO "firmware: requesting %s\n", name);
-
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
@@ -406,6 +413,25 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
+	for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
+	     builtin++) {
+		if (strcmp(name, builtin->name))
+			continue;
+		printk(KERN_INFO "firmware: using built-in firmware %s\n",
+		       name);
+		firmware->data = vmalloc(builtin->size);
+		if (!firmware->data) {
+			retval = -ENOMEM;
+			goto out;
+		}
+		memcpy(firmware->data, builtin->data, builtin->size);
+		firmware->size = builtin->size;
+		return 0;
+	}
+
+	if (uevent)
+		printk(KERN_INFO "firmware: requesting %s\n", name);
+
 	retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
 	if (retval)
 		goto error_kfree_fw;
diff --git a/firmware/Makefile b/firmware/Makefile
new file mode 100644
index 0000000..f6b0c3c
--- /dev/null
+++ b/firmware/Makefile
@@ -0,0 +1,24 @@
+#
+# kbuild file for firmware/
+#
+
+firmware_bins := $(subst ",,$(CONFIG_BUILTIN_FIRMWARE))
+firmware_objs := $(patsubst %,%.o, $(firmware_bins))
+firmware_srcs := $(patsubst %,$(obj)/%.c, $(firmware_bins))
+
+
+quiet_cmd_fwbin = MK_FW   $@
+      cmd_fwbin = echo '/* File automatically generated */' > $@ ;	\
+		  echo '\#include <linux/firmware.h>' >> $@ ;		\
+		  echo 'static const unsigned char fw[] = {' >> $@ ;	\
+		  od -t x1 -A none -v $(srctree)/$(patsubst %.c,%,$@) | \
+		   sed -e 's/ /, 0x/g' -e 's/^,//' -e 's/$$/,/' >> $@ ; \
+		  echo '};' >> $@ ;					\
+		  echo 'DECLARE_BUILTIN_FIRMWARE("$(patsubst firmware/%.c,%,$@)",fw);' >> $@
+
+$(firmware_srcs): $(obj)/%.c: $(srctree)/$(obj)/%
+	$(call cmd,fwbin)
+
+obj-y := $(firmware_objs)
+
+targets := $(firmware_objs)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index f054778..8d71a40 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -86,6 +86,13 @@
 		VMLINUX_SYMBOL(__end_pci_fixups_resume) = .;		\
 	}								\
 									\
+	/* Built-in firmware blobs */					\
+	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
+		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
+		*(.builtin_fw)						\
+		VMLINUX_SYMBOL(__end_builtin_fw) = .;			\
+	}								\
+									\
 	/* RapidIO route ops */						\
 	.rio_route        : AT(ADDR(.rio_route) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start_rio_route_ops) = .;		\
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 4d10c73..a6d5be3 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -1,7 +1,10 @@
 #ifndef _LINUX_FIRMWARE_H
 #define _LINUX_FIRMWARE_H
+
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/compiler.h>
+
 #define FIRMWARE_NAME_MAX 30 
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
@@ -13,6 +16,24 @@ struct firmware {
 
 struct device;
 
+struct builtin_fw {
+	char *name;
+	void *data;
+	unsigned long size;
+};
+
+/* We have to play tricks here much like stringify() to get the
+   __COUNTER__ macro to be expanded as we want it */
+#define __fw_concat1(x,y) x##y
+#define __fw_concat(x,y) __fw_concat1(x,y)
+
+#define DECLARE_BUILTIN_FIRMWARE(name, blob)				     \
+	DECLARE_BUILTIN_FIRMWARE_SIZE(name, &(blob), sizeof(blob))
+
+#define DECLARE_BUILTIN_FIRMWARE_SIZE(name, blob, size)			     \
+	static const struct builtin_fw __fw_concat(__builtin_fw,__COUNTER__) \
+	__used __section(.builtin_fw) = { name, blob, size }
+
 #if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
diff --git a/sound/pci/korg1212/Makefile b/sound/pci/korg1212/Makefile
index f11ce1b..7a5ebdf 100644
--- a/sound/pci/korg1212/Makefile
+++ b/sound/pci/korg1212/Makefile
@@ -7,3 +7,4 @@ snd-korg1212-objs := korg1212.o
 
 # Toplevel Module Dependency
 obj-$(CONFIG_SND_KORG1212) += snd-korg1212.o
+obj-$(CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL) += korg1212-firmware.o
diff --git a/sound/pci/korg1212/korg1212-firmware.h b/sound/pci/korg1212/korg1212-firmware.c
similarity index 99%
rename from sound/pci/korg1212/korg1212-firmware.h
rename to sound/pci/korg1212/korg1212-firmware.c
index f6f5b91..2468ee2 100644
--- a/sound/pci/korg1212/korg1212-firmware.h
+++ b/sound/pci/korg1212/korg1212-firmware.c
@@ -1,3 +1,4 @@
+#include <linux/firmware.h>
 static char dspCode [] = {
 0x01,0xff,0x18,0xff,0xf5,0xff,0xcf,0xff,0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,
 0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,
@@ -985,3 +986,6 @@ static char dspCode [] = {
 0x00,0xff,0x40,0xff,0xff,0xff,0xc4,0xff,0x00,0xff,0x0a,0xff,0xff,0xff,0x0f,0xff,
 0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,
 0xff,0xff,0xff,0xff };
+
+DECLARE_BUILTIN_FIRMWARE("korg/k1212.dsp", dspCode);
+
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
index f4c85b5..4a44c0f 100644
--- a/sound/pci/korg1212/korg1212.c
+++ b/sound/pci/korg1212/korg1212.c
@@ -260,14 +260,6 @@ enum MonitorModeSelector {
 #define COMMAND_ACK_DELAY   13         // number of RTC ticks to wait for an acknowledgement
                                        //    from the card after sending a command.
 
-#ifdef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
-#include "korg1212-firmware.h"
-static const struct firmware static_dsp_code = {
-	.data = (u8 *)dspCode,
-	.size = sizeof dspCode
-};
-#endif
-
 enum ClockSourceIndex {
    K1212_CLKIDX_AdatAt44_1K = 0,    // selects source as ADAT at 44.1 kHz
    K1212_CLKIDX_AdatAt48K,          // selects source as ADAT at 48 kHz
@@ -412,9 +404,7 @@ struct snd_korg1212 {
 MODULE_DESCRIPTION("korg1212");
 MODULE_LICENSE("GPL");
 MODULE_SUPPORTED_DEVICE("{{KORG,korg1212}}");
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 MODULE_FIRMWARE("korg/k1212.dsp");
-#endif
 
 static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;     /* Index 0-MAX */
 static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;	   /* ID for this card */
@@ -2348,9 +2338,6 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
         korg1212->AdatTimeCodePhy = korg1212->sharedBufferPhy +
 		offsetof(struct KorgSharedBuffer, AdatTimeCode);
 
-#ifdef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
-	dsp_code = &static_dsp_code;
-#else
 	err = request_firmware(&dsp_code, "korg/k1212.dsp", &pci->dev);
 	if (err < 0) {
 		release_firmware(dsp_code);
@@ -2358,15 +2345,12 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
 		snd_korg1212_free(korg1212);
 		return err;
 	}
-#endif
 
 	if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(pci),
 				dsp_code->size, &korg1212->dma_dsp) < 0) {
 		snd_printk(KERN_ERR "korg1212: cannot allocate dsp code memory (%zd bytes)\n", dsp_code->size);
                 snd_korg1212_free(korg1212);
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 		release_firmware(dsp_code);
-#endif
                 return -ENOMEM;
         }
 
@@ -2376,9 +2360,7 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
 
 	memcpy(korg1212->dma_dsp.area, dsp_code->data, dsp_code->size);
 
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 	release_firmware(dsp_code);
-#endif
 
 	rc = snd_korg1212_Send1212Command(korg1212, K1212_DB_RebootCard, 0, 0, 0, 0);
 

-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[Alan Cox]

> That's fairly much the kind of thing I was expecting we might see, I'm
> not sure it's reasonable to suddenly declare that overwriting the
> firmware is forbidden. I'd done the version which avoided the extra
> vmalloc and copy, but I've just reverted it.

It saves us a lot of memory in several cases where the drivers hang onto
the firmware so I definitely think we should fix the folks assuming they
can widdle on the firmware. That doesn't look hard to do and could save
50K+ on many systems.

Alan

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 21:31 +0100, Alan Cox wrote:
> It saves us a lot of memory in several cases where the drivers hang onto
> the firmware so I definitely think we should fix the folks assuming they
> can widdle on the firmware. That doesn't look hard to do and could save
> 50K+ on many systems.

Hm, ok. Do you see any better fix than this for cxusb.c ?

--- cxusb.c~	2008-04-13 13:38:13.000000000 +0100
+++ cxusb.c	2008-05-23 22:01:31.000000000 +0100
@@ -23,6 +23,8 @@
  *
  * see Documentation/dvb/README.dvb-usb for more information
  */
+#include <linux/vmalloc.h>
+
 #include "cxusb.h"
 
 #include "cx22702.h"
@@ -695,12 +697,26 @@ static int bluebird_patch_dvico_firmware
 
 		if (fw->data[idoff] == (USB_VID_DVICO & 0xff) &&
 		    fw->data[idoff + 1] == USB_VID_DVICO >> 8) {
-			fw->data[idoff + 2] =
+			struct firmware new_fw;
+			u8 *new_fw_data = vmalloc(fw->size);
+			int ret;
+
+			if (!new_fw_data)
+				return -ENOMEM;
+
+			memcpy(new_fw_data, fw->data, fw->size);
+			new_fw.size = fw->size;
+			new_fw.data = fw->data;
+
+			new_fw_data[idoff + 2] =
 				le16_to_cpu(udev->descriptor.idProduct) + 1;
-			fw->data[idoff + 3] =
+			new_fw_data[idoff + 3] =
 				le16_to_cpu(udev->descriptor.idProduct) >> 8;
 
-			return usb_cypress_load_firmware(udev, fw, CYPRESS_FX2);
+			ret = usb_cypress_load_firmware(udev, &new_fw,
+							CYPRESS_FX2);
+			vfree(new_fw_data);
+			return ret;
 		}
 	}
 

-- 
dwmw2

Re: [PATCH 1/3] firmware: allow firmware files to be built into kernel image

[David Woodhouse]

On Fri, 2008-05-23 at 21:31 +0100, Alan Cox wrote:
> > That's fairly much the kind of thing I was expecting we might see, I'm
> > not sure it's reasonable to suddenly declare that overwriting the
> > firmware is forbidden. I'd done the version which avoided the extra
> > vmalloc and copy, but I've just reverted it.
> 
> It saves us a lot of memory in several cases where the drivers hang onto
> the firmware so I definitely think we should fix the folks assuming they
> can widdle on the firmware. That doesn't look hard to do and could save
> 50K+ on many systems.

OK, it's done. The interesting ones were:

 cxusb, for which I posted the patch
 myri10ge, which reads back into fw->data so I made it allocate a new
    buffer
 cx25840, which sends the firmware in 46-byte chunks, each with a 2-byte
    header that gets copied into fw->data. Again solved with a new
    buffer.
 or51211, which passes the buffer in an i2c_msg so I just cast it to u8*

git.infradead.org/users/dwmw2/firmware-2.6.git

diff --git a/Makefile b/Makefile
index 20b3235..ac2ab7e 100644
--- a/Makefile
+++ b/Makefile
@@ -450,7 +450,7 @@ scripts: scripts_basic include/config/auto.conf
 
 # Objects we will link into vmlinux / subdirs we need to visit
 init-y		:= init/
-drivers-y	:= drivers/ sound/
+drivers-y	:= drivers/ sound/ firmware/
 net-y		:= net/
 libs-y		:= lib/
 core-y		:= usr/
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index d7da109..687f097 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -34,6 +34,18 @@ config FW_LOADER
 	  require userspace firmware loading support, but a module built outside
 	  the kernel tree does.
 
+config BUILTIN_FIRMWARE
+       string "Firmware blobs to build into the kernel binary"
+       depends on FW_LOADER
+       help
+         This option allows firmware to be built into the kernel, for the cases
+	 where the user either cannot or doesn't want to provide it from
+	 userspace at runtime (for example, when the firmware in question is
+	 required for accessing the boot device, and the user doesn't want to
+	 use an initrd). Multiple files should be separated with spaces, and
+	 the required files should exist under the firmware/ directory in
+	 the source tree.
+
 config DEBUG_DRIVER
 	bool "Driver Core verbose debug messages"
 	depends on DEBUG_KERNEL
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 9fd4a85..c09f060 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -49,6 +49,14 @@ struct firmware_priv {
 	struct timer_list timeout;
 };
 
+#ifdef CONFIG_FW_LOADER
+extern struct builtin_fw __start_builtin_fw[];
+extern struct builtin_fw __end_builtin_fw[];
+#else /* Module case. Avoid ifdefs later; it'll all optimise out */
+static struct builtin_fw *__start_builtin_fw = NULL;
+static struct builtin_fw *__end_builtin_fw = NULL;
+#endif
+
 static void
 fw_load_abort(struct firmware_priv *fw_priv)
 {
@@ -257,7 +265,7 @@ firmware_data_write(struct kobject *kobj, struct bin_attribute *bin_attr,
 	if (retval)
 		goto out;
 
-	memcpy(fw->data + offset, buffer, count);
+	memcpy((u8 *)fw->data + offset, buffer, count);
 
 	fw->size = max_t(size_t, offset + count, fw->size);
 	retval = count;
@@ -391,13 +399,12 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 	struct device *f_dev;
 	struct firmware_priv *fw_priv;
 	struct firmware *firmware;
+	struct builtin_fw *builtin;
 	int retval;
 
 	if (!firmware_p)
 		return -EINVAL;
 
-	printk(KERN_INFO "firmware: requesting %s\n", name);
-
 	*firmware_p = firmware = kzalloc(sizeof(*firmware), GFP_KERNEL);
 	if (!firmware) {
 		printk(KERN_ERR "%s: kmalloc(struct firmware) failed\n",
@@ -406,6 +413,20 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		goto out;
 	}
 
+	for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
+	     builtin++) {
+		if (strcmp(name, builtin->name))
+			continue;
+		printk(KERN_INFO "firmware: using built-in firmware %s\n",
+		       name);
+		firmware->size = builtin->size;
+		firmware->data = builtin->data;
+		return 0;
+	}
+
+	if (uevent)
+		printk(KERN_INFO "firmware: requesting %s\n", name);
+
 	retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
 	if (retval)
 		goto error_kfree_fw;
@@ -473,8 +494,16 @@ request_firmware(const struct firmware **firmware_p, const char *name,
 void
 release_firmware(const struct firmware *fw)
 {
+	struct builtin_fw *builtin;
+
 	if (fw) {
+		for (builtin = __start_builtin_fw; builtin != __end_builtin_fw;
+		     builtin++) {
+			if (fw->data == builtin->data)
+				goto free_fw;
+		}
 		vfree(fw->data);
+	free_fw:
 		kfree(fw);
 	}
 }
diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index b990805..0c211ad 100644
--- a/drivers/bluetooth/bfusb.c
+++ b/drivers/bluetooth/bfusb.c
@@ -566,7 +566,8 @@ static int bfusb_ioctl(struct hci_dev *hdev, unsigned int cmd, unsigned long arg
 	return -ENOIOCTLCMD;
 }
 
-static int bfusb_load_firmware(struct bfusb_data *data, unsigned char *firmware, int count)
+static int bfusb_load_firmware(struct bfusb_data *data,
+			       const unsigned char *firmware, int count)
 {
 	unsigned char *buf;
 	int err, pipe, len, size, sent = 0;
diff --git a/drivers/bluetooth/bt3c_cs.c b/drivers/bluetooth/bt3c_cs.c
index 7703d6e..593b7c5 100644
--- a/drivers/bluetooth/bt3c_cs.c
+++ b/drivers/bluetooth/bt3c_cs.c
@@ -470,7 +470,8 @@ static int bt3c_hci_ioctl(struct hci_dev *hdev, unsigned int cmd, unsigned long
 /* ======================== Card services HCI interaction ======================== */
 
 
-static int bt3c_load_firmware(bt3c_info_t *info, unsigned char *firmware, int count)
+static int bt3c_load_firmware(bt3c_info_t *info, const unsigned char *firmware,
+			      int count)
 {
 	char *ptr = (char *) firmware;
 	char b[9];
diff --git a/drivers/char/cyclades.c b/drivers/char/cyclades.c
index ef73e72..6bff9d8 100644
--- a/drivers/char/cyclades.c
+++ b/drivers/char/cyclades.c
@@ -4668,7 +4668,7 @@ static inline int __devinit cyc_isfwstr(const char *str, unsigned int size)
 	return 0;
 }
 
-static inline void __devinit cyz_fpga_copy(void __iomem *fpga, u8 *data,
+static inline void __devinit cyz_fpga_copy(void __iomem *fpga, const u8 *data,
 		unsigned int size)
 {
 	for (; size > 0; size--) {
@@ -4701,10 +4701,10 @@ static int __devinit __cyz_load_fw(const struct firmware *fw,
 		const char *name, const u32 mailbox, void __iomem *base,
 		void __iomem *fpga)
 {
-	void *ptr = fw->data;
-	struct zfile_header *h = ptr;
-	struct zfile_config *c, *cs;
-	struct zfile_block *b, *bs;
+	const void *ptr = fw->data;
+	const struct zfile_header *h = ptr;
+	const struct zfile_config *c, *cs;
+	const struct zfile_block *b, *bs;
 	unsigned int a, tmp, len = fw->size;
 #define BAD_FW KERN_ERR "Bad firmware: "
 	if (len < sizeof(*h)) {
diff --git a/drivers/media/common/tuners/tuner-xc2028.c b/drivers/media/common/tuners/tuner-xc2028.c
index 9e9003c..0a6e26a 100644
--- a/drivers/media/common/tuners/tuner-xc2028.c
+++ b/drivers/media/common/tuners/tuner-xc2028.c
@@ -255,7 +255,7 @@ static int load_all_firmwares(struct dvb_frontend *fe)
 {
 	struct xc2028_data    *priv = fe->tuner_priv;
 	const struct firmware *fw   = NULL;
-	unsigned char         *p, *endp;
+	const unsigned char   *p, *endp;
 	int                   rc = 0;
 	int		      n, n_array;
 	char		      name[33];
diff --git a/drivers/media/common/tuners/xc5000.c b/drivers/media/common/tuners/xc5000.c
index ceae6db..d21a5a7 100644
--- a/drivers/media/common/tuners/xc5000.c
+++ b/drivers/media/common/tuners/xc5000.c
@@ -277,7 +277,7 @@ static int xc_read_reg(struct xc5000_priv *priv, u16 regAddr, u16 *i2cData)
 	return result;
 }
 
-static int xc_load_i2c_sequence(struct dvb_frontend *fe, u8 i2c_sequence[])
+static int xc_load_i2c_sequence(struct dvb_frontend *fe, const u8 *i2c_sequence)
 {
 	struct xc5000_priv *priv = fe->tuner_priv;
 
diff --git a/drivers/media/dvb/dvb-usb/cxusb.c b/drivers/media/dvb/dvb-usb/cxusb.c
index 720fcd1..94bb482 100644
--- a/drivers/media/dvb/dvb-usb/cxusb.c
+++ b/drivers/media/dvb/dvb-usb/cxusb.c
@@ -24,6 +24,7 @@
  * see Documentation/dvb/README.dvb-usb for more information
  */
 #include <media/tuner.h>
+#include <linux/vmalloc.h>
 
 #include "cxusb.h"
 
@@ -700,12 +701,26 @@ static int bluebird_patch_dvico_firmware_download(struct usb_device *udev,
 
 		if (fw->data[idoff] == (USB_VID_DVICO & 0xff) &&
 		    fw->data[idoff + 1] == USB_VID_DVICO >> 8) {
-			fw->data[idoff + 2] =
+			struct firmware new_fw;
+			u8 *new_fw_data = vmalloc(fw->size);
+			int ret;
+
+			if (!new_fw_data)
+				return -ENOMEM;
+
+			memcpy(new_fw_data, fw->data, fw->size);
+			new_fw.size = fw->size;
+			new_fw.data = fw->data;
+
+			new_fw_data[idoff + 2] =
 				le16_to_cpu(udev->descriptor.idProduct) + 1;
-			fw->data[idoff + 3] =
+			new_fw_data[idoff + 3] =
 				le16_to_cpu(udev->descriptor.idProduct) >> 8;
 
-			return usb_cypress_load_firmware(udev, fw, CYPRESS_FX2);
+			ret = usb_cypress_load_firmware(udev, &new_fw,
+							CYPRESS_FX2);
+			vfree(new_fw_data);
+			return ret;
 		}
 	}
 
diff --git a/drivers/media/dvb/dvb-usb/gp8psk.c b/drivers/media/dvb/dvb-usb/gp8psk.c
index 9a942af..85836da 100644
--- a/drivers/media/dvb/dvb-usb/gp8psk.c
+++ b/drivers/media/dvb/dvb-usb/gp8psk.c
@@ -86,7 +86,8 @@ static int gp8psk_load_bcm4500fw(struct dvb_usb_device *d)
 {
 	int ret;
 	const struct firmware *fw = NULL;
-	u8 *ptr, *buf;
+	const u8 *ptr;
+	u8 *buf;
 	if ((ret = request_firmware(&fw, bcm4500_firmware,
 					&d->udev->dev)) != 0) {
 		err("did not find the bcm4500 firmware file. (%s) "
diff --git a/drivers/media/dvb/frontends/bcm3510.c b/drivers/media/dvb/frontends/bcm3510.c
index d268e65..cf5e576 100644
--- a/drivers/media/dvb/frontends/bcm3510.c
+++ b/drivers/media/dvb/frontends/bcm3510.c
@@ -590,7 +590,8 @@ static void bcm3510_release(struct dvb_frontend* fe)
  */
 #define BCM3510_DEFAULT_FIRMWARE "dvb-fe-bcm3510-01.fw"
 
-static int bcm3510_write_ram(struct bcm3510_state *st, u16 addr, u8 *b, u16 len)
+static int bcm3510_write_ram(struct bcm3510_state *st, u16 addr, const u8 *b,
+			     u16 len)
 {
 	int ret = 0,i;
 	bcm3510_register_value vH, vL,vD;
@@ -614,7 +615,7 @@ static int bcm3510_download_firmware(struct dvb_frontend* fe)
 	struct bcm3510_state* st = fe->demodulator_priv;
 	const struct firmware *fw;
 	u16 addr,len;
-	u8  *b;
+	const u8 *b;
 	int ret,i;
 
 	deb_info("requesting firmware\n");
diff --git a/drivers/media/dvb/frontends/nxt200x.c b/drivers/media/dvb/frontends/nxt200x.c
index 23d0228..af29835 100644
--- a/drivers/media/dvb/frontends/nxt200x.c
+++ b/drivers/media/dvb/frontends/nxt200x.c
@@ -93,7 +93,8 @@ static u8 i2c_readbytes (struct nxt200x_state* state, u8 addr, u8* buf, u8 len)
 	return 0;
 }
 
-static int nxt200x_writebytes (struct nxt200x_state* state, u8 reg, u8 *buf, u8 len)
+static int nxt200x_writebytes (struct nxt200x_state* state, u8 reg,
+			       const u8 *buf, u8 len)
 {
 	u8 buf2 [len+1];
 	int err;
diff --git a/drivers/media/dvb/frontends/or51211.c b/drivers/media/dvb/frontends/or51211.c
index 7eaa476..6afe12a 100644
--- a/drivers/media/dvb/frontends/or51211.c
+++ b/drivers/media/dvb/frontends/or51211.c
@@ -69,7 +69,7 @@ struct or51211_state {
 	u32 current_frequency;
 };
 
-static int i2c_writebytes (struct or51211_state* state, u8 reg, u8 *buf,
+static int i2c_writebytes (struct or51211_state* state, u8 reg, const u8 *buf,
 			   int len)
 {
 	int err;
@@ -77,7 +77,7 @@ static int i2c_writebytes (struct or51211_state* state, u8 reg, u8 *buf,
 	msg.addr	= reg;
 	msg.flags	= 0;
 	msg.len		= len;
-	msg.buf		= buf;
+	msg.buf		= (u8 *)buf;
 
 	if ((err = i2c_transfer (state->i2c, &msg, 1)) != 1) {
 		printk(KERN_WARNING "or51211: i2c_writebytes error "
diff --git a/drivers/media/dvb/frontends/sp8870.c b/drivers/media/dvb/frontends/sp8870.c
index aa78aa1..1c9a9b4 100644
--- a/drivers/media/dvb/frontends/sp8870.c
+++ b/drivers/media/dvb/frontends/sp8870.c
@@ -98,7 +98,7 @@ static int sp8870_readreg (struct sp8870_state* state, u16 reg)
 static int sp8870_firmware_upload (struct sp8870_state* state, const struct firmware *fw)
 {
 	struct i2c_msg msg;
-	char *fw_buf = fw->data;
+	const char *fw_buf = fw->data;
 	int fw_pos;
 	u8 tx_buf[255];
 	int tx_len;
diff --git a/drivers/media/dvb/frontends/sp887x.c b/drivers/media/dvb/frontends/sp887x.c
index 49f5587..4543609 100644
--- a/drivers/media/dvb/frontends/sp887x.c
+++ b/drivers/media/dvb/frontends/sp887x.c
@@ -140,7 +140,7 @@ static int sp887x_initial_setup (struct dvb_frontend* fe, const struct firmware
 	u8 buf [BLOCKSIZE+2];
 	int i;
 	int fw_size = fw->size;
-	unsigned char *mem = fw->data;
+	const unsigned char *mem = fw->data;
 
 	dprintk("%s\n", __func__);
 
diff --git a/drivers/media/dvb/frontends/tda10048.c b/drivers/media/dvb/frontends/tda10048.c
index 090fb7d..0ab8d86 100644
--- a/drivers/media/dvb/frontends/tda10048.c
+++ b/drivers/media/dvb/frontends/tda10048.c
@@ -233,7 +233,7 @@ static u8 tda10048_readreg(struct tda10048_state *state, u8 reg)
 }
 
 static int tda10048_writeregbulk(struct tda10048_state *state, u8 reg,
-	u8 *data, u16 len)
+				 const u8 *data, u16 len)
 {
 	int ret = -EREMOTEIO;
 	struct i2c_msg msg;
diff --git a/drivers/media/dvb/frontends/tda1004x.c b/drivers/media/dvb/frontends/tda1004x.c
index 4997384..ff6b427 100644
--- a/drivers/media/dvb/frontends/tda1004x.c
+++ b/drivers/media/dvb/frontends/tda1004x.c
@@ -317,7 +317,7 @@ static int tda10046h_set_bandwidth(struct tda1004x_state *state,
 }
 
 static int tda1004x_do_upload(struct tda1004x_state *state,
-			      unsigned char *mem, unsigned int len,
+			      const unsigned char *mem, unsigned int len,
 			      u8 dspCodeCounterReg, u8 dspCodeInReg)
 {
 	u8 buf[65];
diff --git a/drivers/media/dvb/ttusb-dec/ttusb_dec.c b/drivers/media/dvb/ttusb-dec/ttusb_dec.c
index 42eee04..8af88f1 100644
--- a/drivers/media/dvb/ttusb-dec/ttusb_dec.c
+++ b/drivers/media/dvb/ttusb-dec/ttusb_dec.c
@@ -1275,7 +1275,7 @@ static int ttusb_dec_boot_dsp(struct ttusb_dec *dec)
 	u8 b1[] = { 0x61 };
 	u8 *b;
 	char idstring[21];
-	u8 *firmware = NULL;
+	const u8 *firmware = NULL;
 	size_t firmware_size = 0;
 	u16 firmware_csum = 0;
 	u16 firmware_csum_ns;
diff --git a/drivers/media/video/bt8xx/bttv-cards.c b/drivers/media/video/bt8xx/bttv-cards.c
index f20a01c..f93f1ad 100644
--- a/drivers/media/video/bt8xx/bttv-cards.c
+++ b/drivers/media/video/bt8xx/bttv-cards.c
@@ -3767,7 +3767,8 @@ static int terratec_active_radio_upgrade(struct bttv *btv)
 #define BTTV_ALT_DCLK		0x100000
 #define BTTV_ALT_NCONFIG	0x800000
 
-static int __devinit pvr_altera_load(struct bttv *btv, u8 *micro, u32 microlen)
+static int __devinit pvr_altera_load(struct bttv *btv, const u8 *micro,
+				     u32 microlen)
 {
 	u32 n;
 	u8 bits;
diff --git a/drivers/media/video/cx25840/cx25840-firmware.c b/drivers/media/video/cx25840/cx25840-firmware.c
index 620d295..5c08ede 100644
--- a/drivers/media/video/cx25840/cx25840-firmware.c
+++ b/drivers/media/video/cx25840/cx25840-firmware.c
@@ -79,7 +79,7 @@ static int check_fw_load(struct i2c_client *client, int size)
 	return 0;
 }
 
-static int fw_write(struct i2c_client *client, u8 *data, int size)
+static int fw_write(struct i2c_client *client, const u8 *data, int size)
 {
 	if (i2c_master_send(client, data, size) < size) {
 		v4l_err(client, "firmware load i2c failure\n");
@@ -93,7 +93,8 @@ int cx25840_loadfw(struct i2c_client *client)
 {
 	struct cx25840_state *state = i2c_get_clientdata(client);
 	const struct firmware *fw = NULL;
-	u8 buffer[4], *ptr;
+	u8 buffer[FWSEND];
+	const u8 *ptr;
 	int size, retval;
 
 	if (state->is_cx23885)
@@ -108,6 +109,8 @@ int cx25840_loadfw(struct i2c_client *client)
 
 	buffer[0] = 0x08;
 	buffer[1] = 0x02;
+
+	/* Do we really need to do the first two bytes first? */
 	buffer[2] = fw->data[0];
 	buffer[3] = fw->data[1];
 	retval = fw_write(client, buffer, 4);
@@ -118,19 +121,21 @@ int cx25840_loadfw(struct i2c_client *client)
 	}
 
 	size = fw->size - 2;
-	ptr = fw->data;
+	ptr = fw->data + 2;
 	while (size > 0) {
-		ptr[0] = 0x08;
-		ptr[1] = 0x02;
-		retval = fw_write(client, ptr, min(FWSEND, size + 2));
+		int len = min(FWSEND - 2, size);
+
+		memcpy(buffer + 2, ptr, len);
+
+		retval = fw_write(client, ptr, len + 2);
 
 		if (retval < 0) {
 			release_firmware(fw);
 			return retval;
 		}
 
-		size -= FWSEND - 2;
-		ptr += FWSEND - 2;
+		size -= len;
+		ptr += len;
 	}
 
 	end_fw_load(client);
diff --git a/drivers/net/cxgb3/common.h b/drivers/net/cxgb3/common.h
index 579bee4..8e8ebd7 100644
--- a/drivers/net/cxgb3/common.h
+++ b/drivers/net/cxgb3/common.h
@@ -686,8 +686,9 @@ int t3_seeprom_write(struct adapter *adapter, u32 addr, __le32 data);
 int t3_seeprom_wp(struct adapter *adapter, int enable);
 int t3_get_tp_version(struct adapter *adapter, u32 *vers);
 int t3_check_tpsram_version(struct adapter *adapter, int *must_load);
-int t3_check_tpsram(struct adapter *adapter, u8 *tp_ram, unsigned int size);
-int t3_set_proto_sram(struct adapter *adap, u8 *data);
+int t3_check_tpsram(struct adapter *adapter, const u8 *tp_ram,
+		    unsigned int size);
+int t3_set_proto_sram(struct adapter *adap, const u8 *data);
 int t3_read_flash(struct adapter *adapter, unsigned int addr,
 		  unsigned int nwords, u32 *data, int byte_oriented);
 int t3_load_fw(struct adapter *adapter, const u8 * fw_data, unsigned int size);
diff --git a/drivers/net/cxgb3/t3_hw.c b/drivers/net/cxgb3/t3_hw.c
index d405a93..47d5178 100644
--- a/drivers/net/cxgb3/t3_hw.c
+++ b/drivers/net/cxgb3/t3_hw.c
@@ -923,7 +923,8 @@ int t3_check_tpsram_version(struct adapter *adapter, int *must_load)
  *	Checks if an adapter's tp sram is compatible with the driver.
  *	Returns 0 if the versions are compatible, a negative error otherwise.
  */
-int t3_check_tpsram(struct adapter *adapter, u8 *tp_sram, unsigned int size)
+int t3_check_tpsram(struct adapter *adapter, const u8 *tp_sram,
+		    unsigned int size)
 {
 	u32 csum;
 	unsigned int i;
@@ -2875,10 +2876,10 @@ static void ulp_config(struct adapter *adap, const struct tp_params *p)
  *
  *	Write the contents of the protocol SRAM.
  */
-int t3_set_proto_sram(struct adapter *adap, u8 *data)
+int t3_set_proto_sram(struct adapter *adap, const u8 *data)
 {
 	int i;
-	__be32 *buf = (__be32 *)data;
+	const __be32 *buf = (const __be32 *)data;
 
 	for (i = 0; i < PROTO_SRAM_LINES; i++) {
 		t3_write_reg(adap, A_TP_EMBED_OP_FIELD5, be32_to_cpu(*buf++));
diff --git a/drivers/net/irda/irda-usb.c b/drivers/net/irda/irda-usb.c
index 6f50ed7..18b471c 100644
--- a/drivers/net/irda/irda-usb.c
+++ b/drivers/net/irda/irda-usb.c
@@ -1024,7 +1024,7 @@ static int irda_usb_is_receiving(struct irda_usb_cb *self)
  *   Upload firmware code to SigmaTel 421X IRDA-USB dongle
  */
 static int stir421x_fw_upload(struct irda_usb_cb *self,
-			     unsigned char *patch,
+			     const unsigned char *patch,
 			     const unsigned int patch_len)
 {
         int ret = -ENOMEM;
@@ -1073,11 +1073,11 @@ static int stir421x_fw_upload(struct irda_usb_cb *self,
   */
 static int stir421x_patch_device(struct irda_usb_cb *self)
 {
-        unsigned int i;
-        int ret;
-        char stir421x_fw_name[11];
-        const struct firmware *fw;
-        unsigned char *fw_version_ptr; /* pointer to version string */
+	unsigned int i;
+	int ret;
+	char stir421x_fw_name[11];
+	const struct firmware *fw;
+	const unsigned char *fw_version_ptr; /* pointer to version string */
 	unsigned long fw_version = 0;
 
         /*
diff --git a/drivers/net/myri10ge/myri10ge.c b/drivers/net/myri10ge/myri10ge.c
index c91b12e..ab7a80b 100644
--- a/drivers/net/myri10ge/myri10ge.c
+++ b/drivers/net/myri10ge/myri10ge.c
@@ -529,6 +529,7 @@ static int myri10ge_load_hotplug_firmware(struct myri10ge_priv *mgp, u32 * size)
 	unsigned crc, reread_crc;
 	const struct firmware *fw;
 	struct device *dev = &mgp->pdev->dev;
+	unsigned char *fw_readback;
 	struct mcp_gen_header *hdr;
 	size_t hdr_offset;
 	int status;
@@ -571,9 +572,15 @@ static int myri10ge_load_hotplug_firmware(struct myri10ge_priv *mgp, u32 * size)
 		mb();
 		readb(mgp->sram);
 	}
+	fw_readback = vmalloc(fw->size);
+	if (!fw_readback) {
+		status = -ENOMEM;
+		goto abort_with_fw;
+	}
 	/* corruption checking is good for parity recovery and buggy chipset */
-	memcpy_fromio(fw->data, mgp->sram + MYRI10GE_FW_OFFSET, fw->size);
-	reread_crc = crc32(~0, fw->data, fw->size);
+	memcpy_fromio(fw_readback, mgp->sram + MYRI10GE_FW_OFFSET, fw->size);
+	reread_crc = crc32(~0, fw_readback, fw->size);
+	vfree(fw_readback);
 	if (crc != reread_crc) {
 		dev_err(dev, "CRC failed(fw-len=%u), got 0x%x (expect 0x%x)\n",
 			(unsigned)fw->size, reread_crc, crc);
diff --git a/drivers/net/wireless/atmel.c b/drivers/net/wireless/atmel.c
index 438e63e..d1acef7 100644
--- a/drivers/net/wireless/atmel.c
+++ b/drivers/net/wireless/atmel.c
@@ -560,7 +560,7 @@ static const struct {
 static void build_wpa_mib(struct atmel_private *priv);
 static int atmel_ioctl(struct net_device *dev, struct ifreq *rq, int cmd);
 static void atmel_copy_to_card(struct net_device *dev, u16 dest,
-			       unsigned char *src, u16 len);
+			       const unsigned char *src, u16 len);
 static void atmel_copy_to_host(struct net_device *dev, unsigned char *dest,
 			       u16 src, u16 len);
 static void atmel_set_gcr(struct net_device *dev, u16 mask);
@@ -3853,7 +3853,7 @@ static int reset_atmel_card(struct net_device *dev)
 	if (priv->card_type == CARD_TYPE_EEPROM) {
 		/* copy in firmware if needed */
 		const struct firmware *fw_entry = NULL;
-		unsigned char *fw;
+		const unsigned char *fw;
 		int len = priv->firmware_length;
 		if (!(fw = priv->firmware)) {
 			if (priv->firmware_type == ATMEL_FW_TYPE_NONE) {
@@ -4120,7 +4120,7 @@ static void atmel_writeAR(struct net_device *dev, u16 data)
 }
 
 static void atmel_copy_to_card(struct net_device *dev, u16 dest,
-			       unsigned char *src, u16 len)
+			       const unsigned char *src, u16 len)
 {
 	int i;
 	atmel_writeAR(dev, dest);
diff --git a/drivers/net/wireless/libertas/if_cs.c b/drivers/net/wireless/libertas/if_cs.c
index 54280e2..d075b44 100644
--- a/drivers/net/wireless/libertas/if_cs.c
+++ b/drivers/net/wireless/libertas/if_cs.c
@@ -122,7 +122,7 @@ static inline void if_cs_write16(struct if_cs_card *card, uint reg, u16 val)
 static inline void if_cs_write16_rep(
 	struct if_cs_card *card,
 	uint reg,
-	void *buf,
+	const void *buf,
 	unsigned long count)
 {
 	if (debug_output)
diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 51f664b..3dd537b 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -392,7 +392,7 @@ static int if_sdio_prog_helper(struct if_sdio_card *card)
 	unsigned long timeout;
 	u8 *chunk_buffer;
 	u32 chunk_size;
-	u8 *firmware;
+	const u8 *firmware;
 	size_t size;
 
 	lbs_deb_enter(LBS_DEB_SDIO);
@@ -508,7 +508,7 @@ static int if_sdio_prog_real(struct if_sdio_card *card)
 	unsigned long timeout;
 	u8 *chunk_buffer;
 	u32 chunk_size;
-	u8 *firmware;
+	const u8 *firmware;
 	size_t size, req_size;
 
 	lbs_deb_enter(LBS_DEB_SDIO);
diff --git a/drivers/net/wireless/libertas/if_usb.c b/drivers/net/wireless/libertas/if_usb.c
index 8032df7..a25b670 100644
--- a/drivers/net/wireless/libertas/if_usb.c
+++ b/drivers/net/wireless/libertas/if_usb.c
@@ -293,7 +293,7 @@ static void if_usb_disconnect(struct usb_interface *intf)
 static int if_usb_send_fw_pkt(struct if_usb_card *cardp)
 {
 	struct fwdata *fwdata = cardp->ep_out_buf;
-	uint8_t *firmware = cardp->fw->data;
+	const uint8_t *firmware = cardp->fw->data;
 
 	/* If we got a CRC failure on the last block, back
 	   up and retry it */
@@ -746,7 +746,7 @@ static int if_usb_issue_boot_command(struct if_usb_card *cardp, int ivalue)
  *         len               image length
  *  @return     0 or -1
  */
-static int check_fwfile_format(uint8_t *data, uint32_t totlen)
+static int check_fwfile_format(const uint8_t *data, uint32_t totlen)
 {
 	uint32_t bincmd, exit;
 	uint32_t blksize, offset, len;
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 98ddbb3..6728254 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -375,7 +375,8 @@ static int p54u_upload_firmware_3887(struct ieee80211_hw *dev)
 	const struct firmware *fw_entry = NULL;
 	int err, alen;
 	u8 carry = 0;
-	u8 *buf, *tmp, *data;
+	u8 *buf, *tmp;
+	const u8 *data;
 	unsigned int left, remains, block_size;
 	struct x2_header *hdr;
 	unsigned long timeout;
@@ -522,7 +523,7 @@ static int p54u_upload_firmware_net2280(struct ieee80211_hw *dev)
 	void *buf;
 	__le32 reg;
 	unsigned int remains, offset;
-	u8 *data;
+	const u8 *data;
 
 	buf = kmalloc(512, GFP_KERNEL);
 	if (!buf) {
diff --git a/drivers/net/wireless/rt2x00/rt2x00.h b/drivers/net/wireless/rt2x00/rt2x00.h
index 57bdc15..79eabaa 100644
--- a/drivers/net/wireless/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/rt2x00/rt2x00.h
@@ -506,8 +506,8 @@ struct rt2x00lib_ops {
 	 */
 	int (*probe_hw) (struct rt2x00_dev *rt2x00dev);
 	char *(*get_firmware_name) (struct rt2x00_dev *rt2x00dev);
-	u16 (*get_firmware_crc) (void *data, const size_t len);
-	int (*load_firmware) (struct rt2x00_dev *rt2x00dev, void *data,
+	u16 (*get_firmware_crc) (const void *data, const size_t len);
+	int (*load_firmware) (struct rt2x00_dev *rt2x00dev, const void *data,
 			      const size_t len);
 
 	/*
diff --git a/drivers/net/wireless/rt2x00/rt2x00pci.h b/drivers/net/wireless/rt2x00/rt2x00pci.h
index 9d1cdb9..b41967e 100644
--- a/drivers/net/wireless/rt2x00/rt2x00pci.h
+++ b/drivers/net/wireless/rt2x00/rt2x00pci.h
@@ -82,7 +82,7 @@ static inline void rt2x00pci_register_write(struct rt2x00_dev *rt2x00dev,
 static inline void
 rt2x00pci_register_multiwrite(struct rt2x00_dev *rt2x00dev,
 			      const unsigned long offset,
-			      void *value, const u16 length)
+			      const void *value, const u16 length)
 {
 	memcpy_toio(rt2x00dev->csr.base + offset, value, length);
 }
diff --git a/drivers/net/wireless/rt2x00/rt61pci.c b/drivers/net/wireless/rt2x00/rt61pci.c
index 14bc7b2..bb78de5 100644
--- a/drivers/net/wireless/rt2x00/rt61pci.c
+++ b/drivers/net/wireless/rt2x00/rt61pci.c
@@ -915,7 +915,7 @@ static char *rt61pci_get_firmware_name(struct rt2x00_dev *rt2x00dev)
 	return fw_name;
 }
 
-static u16 rt61pci_get_firmware_crc(void *data, const size_t len)
+static u16 rt61pci_get_firmware_crc(const void *data, const size_t len)
 {
 	u16 crc;
 
@@ -932,7 +932,7 @@ static u16 rt61pci_get_firmware_crc(void *data, const size_t len)
 	return crc;
 }
 
-static int rt61pci_load_firmware(struct rt2x00_dev *rt2x00dev, void *data,
+static int rt61pci_load_firmware(struct rt2x00_dev *rt2x00dev, const void *data,
 				 const size_t len)
 {
 	int i;
diff --git a/drivers/net/wireless/rt2x00/rt73usb.c b/drivers/net/wireless/rt2x00/rt73usb.c
index da19a3a..36c2f17 100644
--- a/drivers/net/wireless/rt2x00/rt73usb.c
+++ b/drivers/net/wireless/rt2x00/rt73usb.c
@@ -850,7 +850,7 @@ static char *rt73usb_get_firmware_name(struct rt2x00_dev *rt2x00dev)
 	return FIRMWARE_RT2571;
 }
 
-static u16 rt73usb_get_firmware_crc(void *data, const size_t len)
+static u16 rt73usb_get_firmware_crc(const void *data, const size_t len)
 {
 	u16 crc;
 
@@ -867,13 +867,13 @@ static u16 rt73usb_get_firmware_crc(void *data, const size_t len)
 	return crc;
 }
 
-static int rt73usb_load_firmware(struct rt2x00_dev *rt2x00dev, void *data,
+static int rt73usb_load_firmware(struct rt2x00_dev *rt2x00dev, const void *data,
 				 const size_t len)
 {
 	unsigned int i;
 	int status;
 	u32 reg;
-	char *ptr = data;
+	const char *ptr = data;
 	char *cache;
 	int buflen;
 	int timeout;
diff --git a/drivers/net/wireless/zd1201.c b/drivers/net/wireless/zd1201.c
index d5c0c66..78baa0f 100644
--- a/drivers/net/wireless/zd1201.c
+++ b/drivers/net/wireless/zd1201.c
@@ -49,7 +49,7 @@ MODULE_DEVICE_TABLE(usb, zd1201_table);
 static int zd1201_fw_upload(struct usb_device *dev, int apfw)
 {
 	const struct firmware *fw_entry;
-	char *data;
+	const char *data;
 	unsigned long len;
 	int err;
 	unsigned char ret;
diff --git a/drivers/scsi/aic94xx/aic94xx_sds.c b/drivers/scsi/aic94xx/aic94xx_sds.c
index 4446e3d..8630a75 100644
--- a/drivers/scsi/aic94xx/aic94xx_sds.c
+++ b/drivers/scsi/aic94xx/aic94xx_sds.c
@@ -1093,9 +1093,9 @@ out:
  * @bytes_to_verify: total bytes to verify
  */
 int asd_verify_flash_seg(struct asd_ha_struct *asd_ha,
-		void *src, u32 dest_offset, u32 bytes_to_verify)
+			 const void *src, u32 dest_offset, u32 bytes_to_verify)
 {
-	u8 *src_buf;
+	const u8 *src_buf;
 	u8 flash_char;
 	int err;
 	u32 nv_offset, reg, i;
@@ -1105,7 +1105,7 @@ int asd_verify_flash_seg(struct asd_ha_struct *asd_ha,
 
 	err = FLASH_OK;
 	nv_offset = dest_offset;
-	src_buf = (u8 *)src;
+	src_buf = (const u8 *)src;
 	for (i = 0; i < bytes_to_verify; i++) {
 		flash_char = asd_read_reg_byte(asd_ha, reg + nv_offset + i);
 		if (flash_char != src_buf[i]) {
@@ -1124,9 +1124,9 @@ int asd_verify_flash_seg(struct asd_ha_struct *asd_ha,
  * @bytes_to_write: total bytes to write
  */
 int asd_write_flash_seg(struct asd_ha_struct *asd_ha,
-		void *src, u32 dest_offset, u32 bytes_to_write)
+			const void *src, u32 dest_offset, u32 bytes_to_write)
 {
-	u8 *src_buf;
+	const u8 *src_buf;
 	u32 nv_offset, reg, i;
 	int err;
 
@@ -1153,7 +1153,7 @@ int asd_write_flash_seg(struct asd_ha_struct *asd_ha,
 		return err;
 	}
 
-	src_buf = (u8 *)src;
+	src_buf = (const u8 *)src;
 	for (i = 0; i < bytes_to_write; i++) {
 		/* Setup program command sequence */
 		switch (asd_ha->hw_prof.flash.method) {
diff --git a/drivers/scsi/aic94xx/aic94xx_sds.h b/drivers/scsi/aic94xx/aic94xx_sds.h
index bb9795a..a06dc01 100644
--- a/drivers/scsi/aic94xx/aic94xx_sds.h
+++ b/drivers/scsi/aic94xx/aic94xx_sds.h
@@ -110,9 +110,9 @@ struct bios_file_header {
 };
 
 int asd_verify_flash_seg(struct asd_ha_struct *asd_ha,
-		void *src, u32 dest_offset, u32 bytes_to_verify);
+		const void *src, u32 dest_offset, u32 bytes_to_verify);
 int asd_write_flash_seg(struct asd_ha_struct *asd_ha,
-		void *src, u32 dest_offset, u32 bytes_to_write);
+		const void *src, u32 dest_offset, u32 bytes_to_write);
 int asd_chk_write_status(struct asd_ha_struct *asd_ha,
 		u32 sector_addr, u8 erase_flag);
 int asd_check_flash_type(struct asd_ha_struct *asd_ha);
diff --git a/drivers/scsi/aic94xx/aic94xx_seq.c b/drivers/scsi/aic94xx/aic94xx_seq.c
index f4272ac..8f98e33 100644
--- a/drivers/scsi/aic94xx/aic94xx_seq.c
+++ b/drivers/scsi/aic94xx/aic94xx_seq.c
@@ -46,7 +46,7 @@
 static const struct firmware *sequencer_fw;
 static u16 cseq_vecs[CSEQ_NUM_VECS], lseq_vecs[LSEQ_NUM_VECS], mode2_task,
 	cseq_idle_loop, lseq_idle_loop;
-static u8 *cseq_code, *lseq_code;
+static const u8 *cseq_code, *lseq_code;
 static u32 cseq_code_size, lseq_code_size;
 
 static u16 first_scb_site_no = 0xFFFF;
@@ -1235,7 +1235,8 @@ int asd_release_firmware(void)
 static int asd_request_firmware(struct asd_ha_struct *asd_ha)
 {
 	int err, i;
-	struct sequencer_file_header header, *hdr_ptr;
+	struct sequencer_file_header header;
+	const struct sequencer_file_header *hdr_ptr;
 	u32 csum = 0;
 	u16 *ptr_cseq_vecs, *ptr_lseq_vecs;
 
@@ -1249,7 +1250,7 @@ static int asd_request_firmware(struct asd_ha_struct *asd_ha)
 	if (err)
 		return err;
 
-	hdr_ptr = (struct sequencer_file_header *)sequencer_fw->data;
+	hdr_ptr = (const struct sequencer_file_header *)sequencer_fw->data;
 
 	header.csum = le32_to_cpu(hdr_ptr->csum);
 	header.major = le32_to_cpu(hdr_ptr->major);
diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 5ea3093..90583d6 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -820,7 +820,7 @@ reschedule:
 }
 
 static int cxacru_fw(struct usb_device *usb_dev, enum cxacru_fw_request fw,
-		     u8 code1, u8 code2, u32 addr, u8 *data, int size)
+		     u8 code1, u8 code2, u32 addr, const u8 *data, int size)
 {
 	int ret;
 	u8 *buf;
diff --git a/drivers/usb/atm/ueagle-atm.c b/drivers/usb/atm/ueagle-atm.c
index 5f71ff3..cb01b51 100644
--- a/drivers/usb/atm/ueagle-atm.c
+++ b/drivers/usb/atm/ueagle-atm.c
@@ -579,7 +579,7 @@ MODULE_PARM_DESC(annex,
  * uea_send_modem_cmd - Send a command for pre-firmware devices.
  */
 static int uea_send_modem_cmd(struct usb_device *usb,
-		u16 addr, u16 size, u8 * buff)
+			      u16 addr, u16 size, const u8 *buff)
 {
 	int ret = -ENOMEM;
 	u8 *xfer_buff;
@@ -604,7 +604,8 @@ static int uea_send_modem_cmd(struct usb_device *usb,
 static void uea_upload_pre_firmware(const struct firmware *fw_entry, void *context)
 {
 	struct usb_device *usb = context;
-	u8 *pfw, value;
+	const u8 *pfw;
+	u8 value;
 	u32 crc = 0;
 	int ret, size;
 
@@ -720,7 +721,7 @@ static int uea_load_firmware(struct usb_device *usb, unsigned int ver)
 /*
  * Make sure that the DSP code provided is safe to use.
  */
-static int check_dsp_e1(u8 *dsp, unsigned int len)
+static int check_dsp_e1(const u8 *dsp, unsigned int len)
 {
 	u8 pagecount, blockcount;
 	u16 blocksize;
@@ -771,7 +772,7 @@ static int check_dsp_e1(u8 *dsp, unsigned int len)
 	return 0;
 }
 
-static int check_dsp_e4(u8 *dsp, int len)
+static int check_dsp_e4(const u8 *dsp, int len)
 {
 	int i;
 	struct l1_code *p = (struct l1_code *) dsp;
@@ -819,7 +820,7 @@ static int check_dsp_e4(u8 *dsp, int len)
 /*
  * send data to the idma pipe
  * */
-static int uea_idma_write(struct uea_softc *sc, void *data, u32 size)
+static int uea_idma_write(struct uea_softc *sc, const void *data, u32 size)
 {
 	int ret = -ENOMEM;
 	u8 *xfer_buff;
@@ -903,7 +904,7 @@ static void uea_load_page_e1(struct work_struct *work)
 	u16 ovl = sc->ovl;
 	struct block_info_e1 bi;
 
-	u8 *p;
+	const u8 *p;
 	u8 pagecount, blockcount;
 	u16 blockaddr, blocksize;
 	u32 pageoffset;
@@ -986,7 +987,7 @@ static void __uea_load_page_e4(struct uea_softc *sc, u8 pageno, int boot)
 	bi.wReserved = cpu_to_be16(UEA_RESERVED);
 
 	do {
-		u8 *blockoffset;
+		const u8 *blockoffset;
 		unsigned int blocksize;
 
 		blockidx = &p->page_header[blockno];
@@ -1095,7 +1096,7 @@ static inline int wait_cmv_ack(struct uea_softc *sc)
 #define UCDC_SEND_ENCAPSULATED_COMMAND 0x00
 
 static int uea_request(struct uea_softc *sc,
-		u16 value, u16 index, u16 size, void *data)
+		u16 value, u16 index, u16 size, const void *data)
 {
 	u8 *xfer_buff;
 	int ret = -ENOMEM;
@@ -1891,7 +1892,8 @@ static int load_XILINX_firmware(struct uea_softc *sc)
 {
 	const struct firmware *fw_entry;
 	int ret, size, u, ln;
-	u8 *pfw, value;
+	const u8 *pfw;
+	u8 value;
 	char *fw_name = FW_DIR "930-fpga.bin";
 
 	uea_enters(INS_TO_USBDEV(sc));
diff --git a/firmware/Makefile b/firmware/Makefile
new file mode 100644
index 0000000..f6b0c3c
--- /dev/null
+++ b/firmware/Makefile
@@ -0,0 +1,24 @@
+#
+# kbuild file for firmware/
+#
+
+firmware_bins := $(subst ",,$(CONFIG_BUILTIN_FIRMWARE))
+firmware_objs := $(patsubst %,%.o, $(firmware_bins))
+firmware_srcs := $(patsubst %,$(obj)/%.c, $(firmware_bins))
+
+
+quiet_cmd_fwbin = MK_FW   $@
+      cmd_fwbin = echo '/* File automatically generated */' > $@ ;	\
+		  echo '\#include <linux/firmware.h>' >> $@ ;		\
+		  echo 'static const unsigned char fw[] = {' >> $@ ;	\
+		  od -t x1 -A none -v $(srctree)/$(patsubst %.c,%,$@) | \
+		   sed -e 's/ /, 0x/g' -e 's/^,//' -e 's/$$/,/' >> $@ ; \
+		  echo '};' >> $@ ;					\
+		  echo 'DECLARE_BUILTIN_FIRMWARE("$(patsubst firmware/%.c,%,$@)",fw);' >> $@
+
+$(firmware_srcs): $(obj)/%.c: $(srctree)/$(obj)/%
+	$(call cmd,fwbin)
+
+obj-y := $(firmware_objs)
+
+targets := $(firmware_objs)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index f054778..8d71a40 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -86,6 +86,13 @@
 		VMLINUX_SYMBOL(__end_pci_fixups_resume) = .;		\
 	}								\
 									\
+	/* Built-in firmware blobs */					\
+	.builtin_fw        : AT(ADDR(.builtin_fw) - LOAD_OFFSET) {	\
+		VMLINUX_SYMBOL(__start_builtin_fw) = .;			\
+		*(.builtin_fw)						\
+		VMLINUX_SYMBOL(__end_builtin_fw) = .;			\
+	}								\
+									\
 	/* RapidIO route ops */						\
 	.rio_route        : AT(ADDR(.rio_route) - LOAD_OFFSET) {	\
 		VMLINUX_SYMBOL(__start_rio_route_ops) = .;		\
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 4d10c73..279eaad 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -1,18 +1,39 @@
 #ifndef _LINUX_FIRMWARE_H
 #define _LINUX_FIRMWARE_H
+
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/compiler.h>
+
 #define FIRMWARE_NAME_MAX 30 
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 
 struct firmware {
 	size_t size;
-	u8 *data;
+	const u8 *data;
 };
 
 struct device;
 
+struct builtin_fw {
+	char *name;
+	void *data;
+	unsigned long size;
+};
+
+/* We have to play tricks here much like stringify() to get the
+   __COUNTER__ macro to be expanded as we want it */
+#define __fw_concat1(x,y) x##y
+#define __fw_concat(x,y) __fw_concat1(x,y)
+
+#define DECLARE_BUILTIN_FIRMWARE(name, blob)				     \
+	DECLARE_BUILTIN_FIRMWARE_SIZE(name, &(blob), sizeof(blob))
+
+#define DECLARE_BUILTIN_FIRMWARE_SIZE(name, blob, size)			     \
+	static const struct builtin_fw __fw_concat(__builtin_fw,__COUNTER__) \
+	__used __section(.builtin_fw) = { name, blob, size }
+
 #if defined(CONFIG_FW_LOADER) || defined(CONFIG_FW_LOADER_MODULE)
 int request_firmware(const struct firmware **fw, const char *name,
 		     struct device *device);
diff --git a/sound/drivers/vx/vx_core.c b/sound/drivers/vx/vx_core.c
index 9953886..585af2e 100644
--- a/sound/drivers/vx/vx_core.c
+++ b/sound/drivers/vx/vx_core.c
@@ -453,7 +453,7 @@ int snd_vx_load_boot_image(struct vx_core *chip, const struct firmware *boot)
 			vx_outb(chip, TXM, 0);
 			vx_outb(chip, TXL, 0);
 		} else {
-			unsigned char *image = boot->data + i;
+			const unsigned char *image = boot->data + i;
 			if (vx_wait_isr_bit(chip, ISR_TX_EMPTY) < 0) {
 				snd_printk(KERN_ERR "dsp boot failed at %d\n", i);
 				return -EIO;
@@ -671,7 +671,7 @@ int snd_vx_dsp_load(struct vx_core *chip, const struct firmware *dsp)
 	unsigned int i;
 	int err;
 	unsigned int csum = 0;
-	unsigned char *image, *cptr;
+	const unsigned char *image, *cptr;
 
 	snd_assert(dsp->size % 3 == 0, return -EINVAL);
 
diff --git a/sound/pci/korg1212/Makefile b/sound/pci/korg1212/Makefile
index f11ce1b..7a5ebdf 100644
--- a/sound/pci/korg1212/Makefile
+++ b/sound/pci/korg1212/Makefile
@@ -7,3 +7,4 @@ snd-korg1212-objs := korg1212.o
 
 # Toplevel Module Dependency
 obj-$(CONFIG_SND_KORG1212) += snd-korg1212.o
+obj-$(CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL) += korg1212-firmware.o
diff --git a/sound/pci/korg1212/korg1212-firmware.h b/sound/pci/korg1212/korg1212-firmware.c
similarity index 99%
rename from sound/pci/korg1212/korg1212-firmware.h
rename to sound/pci/korg1212/korg1212-firmware.c
index f6f5b91..2468ee2 100644
--- a/sound/pci/korg1212/korg1212-firmware.h
+++ b/sound/pci/korg1212/korg1212-firmware.c
@@ -1,3 +1,4 @@
+#include <linux/firmware.h>
 static char dspCode [] = {
 0x01,0xff,0x18,0xff,0xf5,0xff,0xcf,0xff,0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,
 0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,0x00,0xff,0x00,0xff,0xff,0xff,0x00,0xff,
@@ -985,3 +986,6 @@ static char dspCode [] = {
 0x00,0xff,0x40,0xff,0xff,0xff,0xc4,0xff,0x00,0xff,0x0a,0xff,0xff,0xff,0x0f,0xff,
 0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,
 0xff,0xff,0xff,0xff };
+
+DECLARE_BUILTIN_FIRMWARE("korg/k1212.dsp", dspCode);
+
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c
index f4c85b5..4a44c0f 100644
--- a/sound/pci/korg1212/korg1212.c
+++ b/sound/pci/korg1212/korg1212.c
@@ -260,14 +260,6 @@ enum MonitorModeSelector {
 #define COMMAND_ACK_DELAY   13         // number of RTC ticks to wait for an acknowledgement
                                        //    from the card after sending a command.
 
-#ifdef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
-#include "korg1212-firmware.h"
-static const struct firmware static_dsp_code = {
-	.data = (u8 *)dspCode,
-	.size = sizeof dspCode
-};
-#endif
-
 enum ClockSourceIndex {
    K1212_CLKIDX_AdatAt44_1K = 0,    // selects source as ADAT at 44.1 kHz
    K1212_CLKIDX_AdatAt48K,          // selects source as ADAT at 48 kHz
@@ -412,9 +404,7 @@ struct snd_korg1212 {
 MODULE_DESCRIPTION("korg1212");
 MODULE_LICENSE("GPL");
 MODULE_SUPPORTED_DEVICE("{{KORG,korg1212}}");
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 MODULE_FIRMWARE("korg/k1212.dsp");
-#endif
 
 static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;     /* Index 0-MAX */
 static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;	   /* ID for this card */
@@ -2348,9 +2338,6 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
         korg1212->AdatTimeCodePhy = korg1212->sharedBufferPhy +
 		offsetof(struct KorgSharedBuffer, AdatTimeCode);
 
-#ifdef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
-	dsp_code = &static_dsp_code;
-#else
 	err = request_firmware(&dsp_code, "korg/k1212.dsp", &pci->dev);
 	if (err < 0) {
 		release_firmware(dsp_code);
@@ -2358,15 +2345,12 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
 		snd_korg1212_free(korg1212);
 		return err;
 	}
-#endif
 
 	if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(pci),
 				dsp_code->size, &korg1212->dma_dsp) < 0) {
 		snd_printk(KERN_ERR "korg1212: cannot allocate dsp code memory (%zd bytes)\n", dsp_code->size);
                 snd_korg1212_free(korg1212);
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 		release_firmware(dsp_code);
-#endif
                 return -ENOMEM;
         }
 
@@ -2376,9 +2360,7 @@ static int __devinit snd_korg1212_create(struct snd_card *card, struct pci_dev *
 
 	memcpy(korg1212->dma_dsp.area, dsp_code->data, dsp_code->size);
 
-#ifndef CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL
 	release_firmware(dsp_code);
-#endif
 
 	rc = snd_korg1212_Send1212Command(korg1212, K1212_DB_RebootCard, 0, 0, 0, 0);
 
diff --git a/sound/pci/pcxhr/pcxhr_core.c b/sound/pci/pcxhr/pcxhr_core.c
index 78aa81f..957e6af 100644
--- a/sound/pci/pcxhr/pcxhr_core.c
+++ b/sound/pci/pcxhr/pcxhr_core.c
@@ -269,7 +269,7 @@ int pcxhr_load_xilinx_binary(struct pcxhr_mgr *mgr, const struct firmware *xilin
 	unsigned int chipsc;
 	unsigned char data;
 	unsigned char mask;
-	unsigned char *image;
+	const unsigned char *image;
 
 	/* test first xilinx */
 	chipsc = PCXHR_INPL(mgr, PCXHR_PLX_CHIPSC);
@@ -316,7 +316,7 @@ static int pcxhr_download_dsp(struct pcxhr_mgr *mgr, const struct firmware *dsp)
 	int err;
 	unsigned int i;
 	unsigned int len;
-	unsigned char *data;
+	const unsigned char *data;
 	unsigned char dummy;
 	/* check the length of boot image */
 	snd_assert(dsp->size > 0, return -EINVAL);
diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c
index 979f7da..6a35962 100644
--- a/sound/pci/riptide/riptide.c
+++ b/sound/pci/riptide/riptide.c
@@ -682,7 +682,7 @@ static union firmware_version firmware_versions[] = {
 	},
 };
 
-static u32 atoh(unsigned char *in, unsigned int len)
+static u32 atoh(const unsigned char *in, unsigned int len)
 {
 	u32 sum = 0;
 	unsigned int mult = 1;
@@ -702,12 +702,12 @@ static u32 atoh(unsigned char *in, unsigned int len)
 	return sum;
 }
 
-static int senddata(struct cmdif *cif, unsigned char *in, u32 offset)
+static int senddata(struct cmdif *cif, const unsigned char *in, u32 offset)
 {
 	u32 addr;
 	u32 data;
 	u32 i;
-	unsigned char *p;
+	const unsigned char *p;
 
 	i = atoh(&in[1], 2);
 	addr = offset + atoh(&in[3], 4);
@@ -726,10 +726,10 @@ static int senddata(struct cmdif *cif, unsigned char *in, u32 offset)
 	return 0;
 }
 
-static int loadfirmware(struct cmdif *cif, unsigned char *img,
+static int loadfirmware(struct cmdif *cif, const unsigned char *img,
 			unsigned int size)
 {
-	unsigned char *in;
+	const unsigned char *in;
 	u32 laddr, saddr, t, val;
 	int err = 0;
 
diff --git a/sound/pci/vx222/vx222_ops.c b/sound/pci/vx222/vx222_ops.c
index b4bfc1a..631f3a6 100644
--- a/sound/pci/vx222/vx222_ops.c
+++ b/sound/pci/vx222/vx222_ops.c
@@ -359,7 +359,7 @@ static int vx2_load_xilinx_binary(struct vx_core *chip, const struct firmware *x
 {
 	unsigned int i;
 	unsigned int port;
-	unsigned char *image;
+	const unsigned char *image;
 
 	/* XILINX reset (wait at least 1 milisecond between reset on and off). */
 	vx_outl(chip, CNTRL, VX_CNTRL_REGISTER_VALUE | VX_XILINX_RESET_MASK);


-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

On Fri, 2008-05-23 at 18:41 +0200, Sam Ravnborg wrote:
> 
> Assignment to targets i smiisng so generated files are not cleaned
> by "make clean".
> 
> targets := $(FIRMWARE_OBJS)

It doesn't seem to have that effect. Whatever I do, on 'make clean'
the .o files are removed and the auto-generated .c files remain. Which
is probably OK.

-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Sam Ravnborg]

On Sat, May 24, 2008 at 03:46:17PM +0100, David Woodhouse wrote:
> On Fri, 2008-05-23 at 18:41 +0200, Sam Ravnborg wrote:
> > 
> > Assignment to targets i smiisng so generated files are not cleaned
> > by "make clean".
> > 
> > targets := $(FIRMWARE_OBJS)
> 
> It doesn't seem to have that effect. Whatever I do, on 'make clean'
> the .o files are removed and the auto-generated .c files remain. Which
> is probably OK.

strange - like 'make clean' does not vist this directory??

The .o files are deleted using a simple find ...
which explain why they are hit.
I have not applied your patches but if you d not see issues
with it I wil not try to chase it further atm.

	Sam

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

On Sat, 2008-05-24 at 17:22 +0200, Sam Ravnborg wrote:
> 
> strange - like 'make clean' does not vist this directory??
> 
> The .o files are deleted using a simple find ...
> which explain why they are hit.
> I have not applied your patches but if you d not see issues
> with it I wil not try to chase it further atm.

Playing with it now. I fixed the mkdir thing by including $(objtree) in
the target.

-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

On Sat, 2008-05-24 at 16:25 +0100, David Woodhouse wrote:
> Playing with it now. I fixed the mkdir thing by including $(objtree)
> in the target.

Here's what I have (on top of the git tree) now. I just need to sort out
the clean/mrproper behaviour -- and possibly should rename the korg
firmware to firmware/korg/k1212.c_shipped?

commit 728aa4212690701823ce3dcf22816f3953923530
Author: David Woodhouse <dwmw2@infradead.org>
Date:   Sat May 24 11:56:32 2008 +0100

    firmware: move firmware files into firmware/
    
    Signed-off-by: David Woodhouse <dwmw2@infradead.org>

diff --git a/firmware/Makefile b/firmware/Makefile
index f6b0c3c..62c2825 100644
--- a/firmware/Makefile
+++ b/firmware/Makefile
@@ -2,10 +2,15 @@
 # kbuild file for firmware/
 #
 
+firmware-$(CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL) += korg/k1212
+
 firmware_bins := $(subst ",,$(CONFIG_BUILTIN_FIRMWARE))
-firmware_objs := $(patsubst %,%.o, $(firmware_bins))
-firmware_srcs := $(patsubst %,$(obj)/%.c, $(firmware_bins))
+firmware_srcs_generated := $(patsubst %,$(obj)/%.c, $(firmware_bins))
+firmware_objs := $(patsubst %,%.o, $(firmware_bins) $(firmware-y)) 
+firmware_dirs := $(patsubst %,$(objtree)/$(obj)/%,$(dir $(firmware_objs)))
 
+quiet_cmd_mkdir = MKDIR   $@
+      cmd_mkdir = mkdir -p $@
 
 quiet_cmd_fwbin = MK_FW   $@
       cmd_fwbin = echo '/* File automatically generated */' > $@ ;	\
@@ -16,9 +21,14 @@ quiet_cmd_fwbin = MK_FW   $@
 		  echo '};' >> $@ ;					\
 		  echo 'DECLARE_BUILTIN_FIRMWARE("$(patsubst firmware/%.c,%,$@)",fw);' >> $@
 
-$(firmware_srcs): $(obj)/%.c: $(srctree)/$(obj)/%
+$(firmware_srcs_generated): $(obj)/%.c: $(srctree)/$(obj)/%
 	$(call cmd,fwbin)
 
+$(firmware_dirs):
+	$(call cmd,mkdir)
+
+$(patsubst %,$(obj)/%,$(firmware_objs)): $(firmware_dirs)
+
 obj-y := $(firmware_objs)
 
-targets := $(firmware_objs)
+targets := $(firmware_objs) $(patsubst $(obj)/%,%, $(firmware_srcs_generated))
diff --git a/sound/pci/korg1212/korg1212-firmware.c b/firmware/korg/k1212.c
similarity index 100%
rename from sound/pci/korg1212/korg1212-firmware.c
rename to firmware/korg/k1212.c
diff --git a/sound/pci/korg1212/Makefile b/sound/pci/korg1212/Makefile
index 7a5ebdf..f11ce1b 100644
--- a/sound/pci/korg1212/Makefile
+++ b/sound/pci/korg1212/Makefile
@@ -7,4 +7,3 @@ snd-korg1212-objs := korg1212.o
 
 # Toplevel Module Dependency
 obj-$(CONFIG_SND_KORG1212) += snd-korg1212.o
-obj-$(CONFIG_SND_KORG1212_FIRMWARE_IN_KERNEL) += korg1212-firmware.o


-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi David,

>> Playing with it now. I fixed the mkdir thing by including $(objtree)
>> in the target.
>
> Here's what I have (on top of the git tree) now. I just need to sort  
> out
> the clean/mrproper behaviour -- and possibly should rename the korg
> firmware to firmware/korg/k1212.c_shipped?

so using "/" within the name parameter for request_firmware() is  
actually forbidden. I know that some driver authers think it is a good  
idea, but it is not.

I am actually working on a patch to make request_firmware() fail/warn  
when the name has a "/" in it.

You don't need to consider any deep nesting of firmware file names. It  
was never meant to be used like this.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

On Sat, 2008-05-24 at 20:18 +0200, Marcel Holtmann wrote:
> Hi David,
> 
> >> Playing with it now. I fixed the mkdir thing by including $(objtree)
> >> in the target.
> >
> > Here's what I have (on top of the git tree) now. I just need to sort  
> > out
> > the clean/mrproper behaviour -- and possibly should rename the korg
> > firmware to firmware/korg/k1212.c_shipped?
> 
> so using "/" within the name parameter for request_firmware() is  
> actually forbidden. I know that some driver authers think it is a good  
> idea, but it is not.
> 
> I am actually working on a patch to make request_firmware() fail/warn  
> when the name has a "/" in it.
> 
> You don't need to consider any deep nesting of firmware file names. It  
> was never meant to be used like this.

Hm. Is there any fundamental reason why we should forbid this? It does
seem to make sense to let people use the namespace for it.

-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi David,

>>>> Playing with it now. I fixed the mkdir thing by including $ 
>>>> (objtree)
>>>> in the target.
>>>
>>> Here's what I have (on top of the git tree) now. I just need to sort
>>> out
>>> the clean/mrproper behaviour -- and possibly should rename the korg
>>> firmware to firmware/korg/k1212.c_shipped?
>>
>> so using "/" within the name parameter for request_firmware() is
>> actually forbidden. I know that some driver authers think it is a  
>> good
>> idea, but it is not.
>>
>> I am actually working on a patch to make request_firmware() fail/warn
>> when the name has a "/" in it.
>>
>> You don't need to consider any deep nesting of firmware file names.  
>> It
>> was never meant to be used like this.
>
> Hm. Is there any fundamental reason why we should forbid this? It does
> seem to make sense to let people use the namespace for it.

I explained this a couple of times. The request_firmware() is an  
abstract mechanism that can request a firmware file. The location of  
the firmware file is up to the userspace. The kernel requests a  
particular file and that is it. All namespacing has to be done by the  
firmware helper script (nowadays udev). That the current  
implementation of the firmware helper maps the filename 1:1 to a file  
under /lib/firmware/ just works, but doesn't have to work all the  
time. It is not the agreed contract between kernel and userspace.

If you wanna do namespacing then you have to do it within the firmware  
helper script. For example the helper script could look for /lib/ 
firmware/<driver>/<filename> instead of /lib/firmware/<filename>.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]


> so using "/" within the name parameter for request_firmware() is  
> actually forbidden. I know that some driver authers think it is a good  
> idea, but it is not.

Can you explain why it is allowed now? And maybe why the API was
designed in a way that easily allows it?

> I explained this a couple of times. The request_firmware() is an  
> abstract mechanism that can request a firmware file. The location of  
> the firmware file is up to the userspace. The kernel requests a  
> particular file and that is it. All namespacing has to be done by the  
> firmware helper script (nowadays udev). That the current  
> implementation of the firmware helper maps the filename 1:1 to a file  
> under /lib/firmware/ just works, but doesn't have to work all the  
> time. It is not the agreed contract between kernel and userspace.

I don't buy this argument. I could agree if you said that the "agreed
contract" between the kernel and userspace is for the kernel to request
a firmware file /keyed by an arbitrary, null-terminated string/.

The fact that it is usually stored on a filesystem where / means a
directory (and thus grouping) can be seen as a nice convenience of the
filesystem storage, but if firmware was stored elsewhere then you could
degrade to the simple key-based lookup that happens to allow "/" as a
character in the keys.

And because the kernel is nice, it allows userspace to use a filesystem
storage by not using paths like "../../lib/firmware/asdf". But
fundamentally, I don't even see anything wrong with that.

Put another way, you can have pretty arbitrary firmware firmware names
(though since humans need to handle them you want printable characters),
and I don't see why now all the sudden you would treat "/" specially by
*explicitly* disallowing it.

b43 comes with 22 firmware files for a single driver, and groups them
using "b43/<name>". What you're proposing will make firmware fail
*again* for all users, and we got a *LOT* of flak from all kinds of
stakeholders (not just the users) when firmware upgrades were required,
doing it again for such a petty reason is ridiculous.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 11:30:37 Johannes Berg wrote:
> > I explained this a couple of times. The request_firmware() is an  
> > abstract mechanism that can request a firmware file. The location of  
> > the firmware file is up to the userspace. The kernel requests a  
> > particular file and that is it. All namespacing has to be done by the  
> > firmware helper script (nowadays udev). That the current  
> > implementation of the firmware helper maps the filename 1:1 to a file  
> > under /lib/firmware/ just works, but doesn't have to work all the  
> > time. It is not the agreed contract between kernel and userspace.
> 
> I don't buy this argument. I could agree if you said that the "agreed
> contract" between the kernel and userspace is for the kernel to request
> a firmware file /keyed by an arbitrary, null-terminated string/.

I fully agree with johannes, that using / as a grouping-separator is a
_nice_ idea and userspace must support it, regardless of the actual
storage system it is using.

Currently using a single directory for one driver really enhances the way
firmware can be handled. For example, we can have several different versions
cleanly installed in /lib/firmware. For example, on my development machine
I have
/lib/firmware/b43
/lib/firmware/b43-old
/lib/firmware/b43-open
/lib/firmware/b43legacy
Putting all these files into plain /lib/firmware would require changing the
actual filenames and that would be a real pain. (This can be up to about 100 files).

> The fact that it is usually stored on a filesystem where / means a
> directory (and thus grouping) can be seen as a nice convenience of the
> filesystem storage, but if firmware was stored elsewhere then you could
> degrade to the simple key-based lookup that happens to allow "/" as a
> character in the keys.
> 
> And because the kernel is nice, it allows userspace to use a filesystem
> storage by not using paths like "../../lib/firmware/asdf". But
> fundamentally, I don't even see anything wrong with that.

I agree that .. and . should probably be avoided.
And I also don't see a good reason to use them.

> Put another way, you can have pretty arbitrary firmware firmware names
> (though since humans need to handle them you want printable characters),
> and I don't see why now all the sudden you would treat "/" specially by
> *explicitly* disallowing it.
> 
> b43 comes with 22 firmware files for a single driver, and groups them

Depending on the firmware version you have, there are over 30 files
for b43 and b43legacy each.

> using "b43/<name>". What you're proposing will make firmware fail
> *again* for all users, and we got a *LOT* of flak from all kinds of
> stakeholders (not just the users) when firmware upgrades were required,
> doing it again for such a petty reason is ridiculous.

Yeah. I'm not going to change that. The users are going to kill us.
Besides that, there were very good reasons to start grouping the files
in b43 (bcm43xx didn't do it), as it simply was unmaintainable in bcm43xx.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Johannes,

>> so using "/" within the name parameter for request_firmware() is
>> actually forbidden. I know that some driver authers think it is a  
>> good
>> idea, but it is not.
>
> Can you explain why it is allowed now? And maybe why the API was
> designed in a way that easily allows it?

in the early days we had something like three drivers using the  
request_firmware() and it was understood between the authors what the  
filename was meant for. And to be quite honest it was an oversight on  
our side to not explicitly fail when the filename contains a "/". So  
it happened that driver authors exploited the fact that they can group  
firmware files under a subdirectory from within the kernel. Nobody  
made the effort and proposed changes to udev.

Personally I think it is fine to have _ALL_ firmware files in one  
directory and not namespace them at all, but it seems that this is  
important for some driver authors.

>> I explained this a couple of times. The request_firmware() is an
>> abstract mechanism that can request a firmware file. The location of
>> the firmware file is up to the userspace. The kernel requests a
>> particular file and that is it. All namespacing has to be done by the
>> firmware helper script (nowadays udev). That the current
>> implementation of the firmware helper maps the filename 1:1 to a file
>> under /lib/firmware/ just works, but doesn't have to work all the
>> time. It is not the agreed contract between kernel and userspace.
>
> I don't buy this argument. I could agree if you said that the "agreed
> contract" between the kernel and userspace is for the kernel to  
> request
> a firmware file /keyed by an arbitrary, null-terminated string/.
>
> The fact that it is usually stored on a filesystem where / means a
> directory (and thus grouping) can be seen as a nice convenience of the
> filesystem storage, but if firmware was stored elsewhere then you  
> could
> degrade to the simple key-based lookup that happens to allow "/" as a
> character in the keys.

The kernel should not in any case have knowledge about directories or  
subdirectories where the firmware files are stored. That is fully  
irrelevant for the kernel.

Especially with the case of built-in firmwares now, it because more  
important to do it right. The one reason why we have to handover the  
struct device to request_firmware() is that we can give the helper  
script full access to the device and driver information of the caller.  
Hence adding for example b43/ as prefix simply duplicates everything  
since the struct device has a link to the driver that is requesting a  
firmware file.

> b43 comes with 22 firmware files for a single driver, and groups them
> using "b43/<name>". What you're proposing will make firmware fail
> *again* for all users, and we got a *LOT* of flak from all kinds of
> stakeholders (not just the users) when firmware upgrades were  
> required,
> doing it again for such a petty reason is ridiculous.

That is not what I am proposing. What I am proposing is that we do  
this the right way. Meaning that we fix udev to do the namespacing. I  
am working on a way to have this change in a backward compatible way.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Michael,

>>> I explained this a couple of times. The request_firmware() is an
>>> abstract mechanism that can request a firmware file. The location of
>>> the firmware file is up to the userspace. The kernel requests a
>>> particular file and that is it. All namespacing has to be done by  
>>> the
>>> firmware helper script (nowadays udev). That the current
>>> implementation of the firmware helper maps the filename 1:1 to a  
>>> file
>>> under /lib/firmware/ just works, but doesn't have to work all the
>>> time. It is not the agreed contract between kernel and userspace.
>>
>> I don't buy this argument. I could agree if you said that the "agreed
>> contract" between the kernel and userspace is for the kernel to  
>> request
>> a firmware file /keyed by an arbitrary, null-terminated string/.
>
> I fully agree with johannes, that using / as a grouping-separator is a
> _nice_ idea and userspace must support it, regardless of the actual
> storage system it is using.
>
> Currently using a single directory for one driver really enhances  
> the way
> firmware can be handled. For example, we can have several different  
> versions
> cleanly installed in /lib/firmware. For example, on my development  
> machine
> I have
> /lib/firmware/b43
> /lib/firmware/b43-old
> /lib/firmware/b43-open
> /lib/firmware/b43legacy
> Putting all these files into plain /lib/firmware would require  
> changing the
> actual filenames and that would be a real pain. (This can be up to  
> about 100 files).

I am okay with userspace supporting namespacing with subdirectories  
and see the point why it helps, but the important here is that  
userspace must support this. It should not be done inside the kernel.

So we have to change udev to look for /lib/firmware/<driver>/ 
<filename> which perfectly fine, but the <driver> part needs to be  
derived from struct device and not from the <filename> part.

>> using "b43/<name>". What you're proposing will make firmware fail
>> *again* for all users, and we got a *LOT* of flak from all kinds of
>> stakeholders (not just the users) when firmware upgrades were  
>> required,
>> doing it again for such a petty reason is ridiculous.
>
> Yeah. I'm not going to change that. The users are going to kill us.
> Besides that, there were very good reasons to start grouping the files
> in b43 (bcm43xx didn't do it), as it simply was unmaintainable in  
> bcm43xx.

Again, I agree that we wanna have that, but putting a directory prefix  
into the driver is wrong.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Marcel,

> The kernel should not in any case have knowledge about directories or  
> subdirectories where the firmware files are stored. That is fully  
> irrelevant for the kernel.

Exactly my point! Hence, the kernel just gives it an arbitrary name. The
fact that userspace uses this as a filename could be regarded as a bug,
but it works fine. Therefore, the kernel simply assigns an arbitrary,
NULL-terminated string as the name. It happens to include a / because it
is convenient with the current userspace implementation, but that's mere
detail, the ABI/API here is to allow any arbitrary names.

> Especially with the case of built-in firmwares now, it because more  
> important to do it right. The one reason why we have to handover the  
> struct device to request_firmware() is that we can give the helper  
> script full access to the device and driver information of the caller.  
> Hence adding for example b43/ as prefix simply duplicates everything  
> since the struct device has a link to the driver that is requesting a  
> firmware file.

But that doesn't matter at all! Dave's work to build firmware files into
the kernel will simply result in an entry in the kernel firmware table
that has a '.name = "b43/pcm5.fw"', nothing needs to know that b43 is a
module name, in fact, it could very well be 'broadcom wlan/pcm5.fw' as
well.

> That is not what I am proposing. What I am proposing is that we do  
> this the right way. Meaning that we fix udev to do the namespacing. I  
> am working on a way to have this change in a backward compatible way.

That will introduce a "flag-day" where you have to upgrade userspace
with the kernel, and vice versa, OR userspace will have to guess. Not a
good solution either.

Why are you so fixated on special-casing the single character '/'?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 13:49:48 Marcel Holtmann wrote:
> Hi Johannes,
> 
> >> so using "/" within the name parameter for request_firmware() is
> >> actually forbidden. I know that some driver authers think it is a  
> >> good
> >> idea, but it is not.
> >
> > Can you explain why it is allowed now? And maybe why the API was
> > designed in a way that easily allows it?
> 
> in the early days we had something like three drivers using the  
> request_firmware() and it was understood between the authors what the  
> filename was meant for. And to be quite honest it was an oversight on  
> our side to not explicitly fail when the filename contains a "/". So  
> it happened that driver authors exploited the fact that they can group  
> firmware files under a subdirectory from within the kernel. Nobody  
> made the effort and proposed changes to udev.

There is absolutely _no_ reason to _fail_ on /
The only thing that make sense is:
Treat the firmware name in the kernel as an opaque key.
Userspace can then make policy decisions on that key.
The current policy decisions are to treat / as a directory separator.
(Which is a good thing, as it makes firmware development a lot easier).
That policy decision is a userspace decision made in udev.
Besides that it's ABI that should not be changed all the time.

> Personally I think it is fine to have _ALL_ firmware files in one  
> directory and not namespace them at all, but it seems that this is  
> important for some driver authors.

It is important, if you have to use several different versions of firmware
for one driver. If there are no directories, you'll have so use prefixes
and so on. That will make the firmware directory rather unmaintainable.
You can also move the directories easily around without using weird
sed scripts to rename the file prefixes. a simple mv will do.

> The kernel should not in any case have knowledge about directories or  
> subdirectories where the firmware files are stored. That is fully  
> irrelevant for the kernel.

I completely agree.
But: It should _also_ not enforce any "this and that char is forbidden"
rules.
If a database decides to use / as a separator, it's fine. If it doesn't,
it's also fine.
Currently we use / as a directory separator in udev. We shouldn't change
that for stable-ABI reasons.

If you want to create some other database (built-in into the kernel or whatever),
feel free to not specialcase the slash. That's perfectly fine and even makes
sense for built-in stuff.

> Especially with the case of built-in firmwares now, it because more  
> important to do it right. The one reason why we have to handover the  
> struct device to request_firmware() is that we can give the helper  
> script full access to the device and driver information of the caller.  
> Hence adding for example b43/ as prefix simply duplicates everything  
> since the struct device has a link to the driver that is requesting a  
> firmware file.

No it doesn't duplicate it.
in b43 we support postfixes. A module parameter can be used to
postfix a string to the directory name. So one can fetch firmware
from b43-test/ for testing purposes. This is needed for firmware development,
for example.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 13:54:32 Marcel Holtmann wrote:
> I am okay with userspace supporting namespacing with subdirectories  
> and see the point why it helps, but the important here is that  
> userspace must support this. It should not be done inside the kernel.

The decision is currently not made in the kernel. It's a udev
decision to use / as directory separator. We exploit that udev
feature to use a convenient dir structure. But that is unrelated
to how any new database will handle it. The in-kernel database should
treat / like any other char.

> So we have to change udev to look for /lib/firmware/<driver>/ 
> <filename> which perfectly fine, but the <driver> part needs to be  
> derived from struct device and not from the <filename> part.

What if I want 2 or 3 different firmware versions installed at once?
I'll have to play dirty tricks with the filenames then and put several
unrelated firmware versions into one directory. That's going to be a mess.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Johannes,

>> The kernel should not in any case have knowledge about directories or
>> subdirectories where the firmware files are stored. That is fully
>> irrelevant for the kernel.
>
> Exactly my point! Hence, the kernel just gives it an arbitrary name.  
> The
> fact that userspace uses this as a filename could be regarded as a  
> bug,
> but it works fine. Therefore, the kernel simply assigns an arbitrary,
> NULL-terminated string as the name. It happens to include a /  
> because it
> is convenient with the current userspace implementation, but that's  
> mere
> detail, the ABI/API here is to allow any arbitrary names.
>
>> Especially with the case of built-in firmwares now, it because more
>> important to do it right. The one reason why we have to handover the
>> struct device to request_firmware() is that we can give the helper
>> script full access to the device and driver information of the  
>> caller.
>> Hence adding for example b43/ as prefix simply duplicates everything
>> since the struct device has a link to the driver that is requesting a
>> firmware file.
>
> But that doesn't matter at all! Dave's work to build firmware files  
> into
> the kernel will simply result in an entry in the kernel firmware table
> that has a '.name = "b43/pcm5.fw"', nothing needs to know that b43  
> is a
> module name, in fact, it could very well be 'broadcom wlan/pcm5.fw' as
> well.
>
>> That is not what I am proposing. What I am proposing is that we do
>> this the right way. Meaning that we fix udev to do the namespacing. I
>> am working on a way to have this change in a backward compatible way.
>
> That will introduce a "flag-day" where you have to upgrade userspace
> with the kernel, and vice versa, OR userspace will have to guess.  
> Not a
> good solution either.

not it does not. I have a fix for udev that will allow a smooth  
transition and keep full backward compatibility. Need to do some more  
testing.

And bumping the required udev version after 12 month is perfectly fine  
since we have the future removal document.

> Why are you so fixated on special-casing the single character '/'?

It is not the "/" character. It is the directory separator. Just  
happened to be "/" on Unix operating systems. You are really missing  
my point here. The kernel should not be involved in enforcing this  
kind of namespaces.

Look at the device nodes. The kernel has mouse0 for example and udev  
will translate this into /dev/input/mouse0. Nobody expects the kernel  
to use input/mouse0 and actually you even can't do that at all since  
the device model forbids "/" as bus id. Same applies for the firmware  
filenames.

Also at some point we might change the actual implementation of  
request_firmware() to allow running multiple request_firmware() at the  
same time to improve the init time of devices (if that makes sense).  
In that case the filename would become a kobject and then the  
directory separator would become illegal.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Alan Cox]

> What if I want 2 or 3 different firmware versions installed at once?
> I'll have to play dirty tricks with the filenames then and put several
> unrelated firmware versions into one directory. That's going to be a mess.

You add the desired feature to udev, or use symlinks. Hardly a "mess".

Alan

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Michael,

>>>> so using "/" within the name parameter for request_firmware() is
>>>> actually forbidden. I know that some driver authers think it is a
>>>> good
>>>> idea, but it is not.
>>>
>>> Can you explain why it is allowed now? And maybe why the API was
>>> designed in a way that easily allows it?
>>
>> in the early days we had something like three drivers using the
>> request_firmware() and it was understood between the authors what the
>> filename was meant for. And to be quite honest it was an oversight on
>> our side to not explicitly fail when the filename contains a "/". So
>> it happened that driver authors exploited the fact that they can  
>> group
>> firmware files under a subdirectory from within the kernel. Nobody
>> made the effort and proposed changes to udev.
>
> There is absolutely _no_ reason to _fail_ on /
> The only thing that make sense is:
> Treat the firmware name in the kernel as an opaque key.
> Userspace can then make policy decisions on that key.
> The current policy decisions are to treat / as a directory separator.
> (Which is a good thing, as it makes firmware development a lot  
> easier).
> That policy decision is a userspace decision made in udev.
> Besides that it's ABI that should not be changed all the time.
>
>> Personally I think it is fine to have _ALL_ firmware files in one
>> directory and not namespace them at all, but it seems that this is
>> important for some driver authors.
>
> It is important, if you have to use several different versions of  
> firmware
> for one driver. If there are no directories, you'll have so use  
> prefixes
> and so on. That will make the firmware directory rather  
> unmaintainable.
> You can also move the directories easily around without using weird
> sed scripts to rename the file prefixes. a simple mv will do.

this should all be done in userspace and not inside the kernel. Export  
the special device versions via struct device and stop being lazy.

>> The kernel should not in any case have knowledge about directories or
>> subdirectories where the firmware files are stored. That is fully
>> irrelevant for the kernel.
>
> I completely agree.
> But: It should _also_ not enforce any "this and that char is  
> forbidden"
> rules.
> If a database decides to use / as a separator, it's fine. If it  
> doesn't,
> it's also fine.
> Currently we use / as a directory separator in udev. We shouldn't  
> change
> that for stable-ABI reasons.

No it is not. The kobject doesn't allow "/" and why should  
request_firmware() be an exception. Also see my other comment on how  
the kernel handles device nodes and on how udev maps them to real  
device nodes on the filesystem.

>> Especially with the case of built-in firmwares now, it because more
>> important to do it right. The one reason why we have to handover the
>> struct device to request_firmware() is that we can give the helper
>> script full access to the device and driver information of the  
>> caller.
>> Hence adding for example b43/ as prefix simply duplicates everything
>> since the struct device has a link to the driver that is requesting a
>> firmware file.
>
> No it doesn't duplicate it.
> in b43 we support postfixes. A module parameter can be used to
> postfix a string to the directory name. So one can fetch firmware
> from b43-test/ for testing purposes. This is needed for firmware  
> development,
> for example.

And again. This is up to the userspace to handle and not the kernel.  
In userspace you could do a general approach to support these kind of  
testing, but you decide to only do this for your driver. You are fully  
exploiting the request_firmware() interface and making any kind of  
userspace policy impossible. This in return actually means that if we  
would improve the request_firmware(), we have to maintain special  
cases for the b43 drivers, because your driver does some hacking of  
firmware files inside the kernel. You actually proved my point that we  
should not allow this inside the kernel.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]


> > Why are you so fixated on special-casing the single character '/'?
> 
> It is not the "/" character. It is the directory separator. Just  
> happened to be "/" on Unix operating systems. You are really missing  
> my point here. The kernel should not be involved in enforcing this  
> kind of namespaces.

Why are you thinking of it as namespaces then? It isn't. The "firmware
key" lives in a flat namespace where each key is an arbitrary
nul-terminated string.

> Look at the device nodes. The kernel has mouse0 for example and udev  
> will translate this into /dev/input/mouse0. Nobody expects the kernel  
> to use input/mouse0 and actually you even can't do that at all since  
> the device model forbids "/" as bus id. Same applies for the firmware  
> filenames.

No, it doesn't.

> Also at some point we might change the actual implementation of  
> request_firmware() to allow running multiple request_firmware() at the  
> same time to improve the init time of devices (if that makes sense).  
> In that case the filename would become a kobject and then the  
> directory separator would become illegal.

There's no need to think of it that way. Look at a uevent now:

UEVENT[1211722721.323011] add      /devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0 (firmware)
ACTION=add
DEVPATH=/devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0
SUBSYSTEM=firmware
FIRMWARE=b43/b0g0bsinitvals5.fw
TIMEOUT=60
SEQNUM=1376


The "firmware key" is contained in the FIRMWARE environment variable. If
you want to allow loading multiple firmwares at the same time, you
wouldn't have to make the key part of the device name, you would only
have to add a unique ID to the firmware device name, say


UEVENT[1211722721.323011] add      /devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0-1234 (firmware)
ACTION=add
DEVPATH=/devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0-1234
SUBSYSTEM=firmware
FIRMWARE=b43/b0g0bsinitvals5.fw
TIMEOUT=60
SEQNUM=1376

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 15:19:37 Marcel Holtmann wrote:
> this should all be done in userspace and not inside the kernel. Export  
> the special device versions via struct device and stop being lazy.

No. Wait. It's not exactly going to work this way.
You're not going to break my ABI and then tell me I'm going to work around
that, right? Answer: No.

b43 will use the slash in request_firmware. That is not going to change.

This is _exactly_ what users are so tired about. Every now and then people
decide to change some ABI.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

On Sun, 2008-05-25 at 14:16 +0100, Alan Cox wrote:
> > What if I want 2 or 3 different firmware versions installed at once?
> > I'll have to play dirty tricks with the filenames then and put several
> > unrelated firmware versions into one directory. That's going to be a mess.
> 
> You add the desired feature to udev, or use symlinks. Hardly a "mess".

It's a bit of a pain for the in-kernel firmware loading that I'm working
on now, if we can no longer count on the requested string to be the
single match criterion.

-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 2950 bytes --]


> No it is not. The kobject doesn't allow "/" and why should  
> request_firmware() be an exception. Also see my other comment on how  
> the kernel handles device nodes and on how udev maps them to real  
> device nodes on the filesystem.

As I said, kobjects have nothing to do with it, they don't need to have
a filename based on the firmware key.

> And again. This is up to the userspace to handle and not the kernel.  
> In userspace you could do a general approach to support these kind of  
> testing, but you decide to only do this for your driver. You are fully  
> exploiting the request_firmware() interface and making any kind of  
> userspace policy impossible. This in return actually means that if we  
> would improve the request_firmware(), we have to maintain special  
> cases for the b43 drivers, because your driver does some hacking of  
> firmware files inside the kernel. You actually proved my point that we  
> should not allow this inside the kernel.

That makes no sense at all.

Michael is exploiting the firmware API that you are suggesting to
change, and so far I don't see a technical reason for changing it. In
kernel space, he's simply requesting varying firmware blobs based on
different keys, which happen to be "b43%s/%s" because that's making
things simpler for him on the other end.

On the other hand, you're saying that we have all kinds of policy about
this in userspace, so why are you saying that directory separators
(slashes, but you seem to distinguish those on some level I do not
understand) should be special in the firmware key?

The firmware key is just that, a *key*. It's only exposed to userspace
via an environment variable in a kobject named
"/devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0" or similar.

The fact that userspace uses the key as a filename is maybe unfortunate,
maybe fortunate, but shouldn't have anything to do with what sort of
keys the kernel allows.

If Michael wants to serve his firmware blobs from an SQL database, he'd
use a simple table like this:

CREATE TABLE firmware (
	ID INTEGER,
	name VARCHAR(100),
	data BLOB
);

I don't see any problem with that.

Also, you said above (quoting again):

> You are fully  
> exploiting the request_firmware() interface and making any kind of  
> userspace policy impossible.

That's not true at all. If you decide that the userspace policy should
be to load $modulename/$firmwarekey then you'd maybe have something
like /lib/firmware/b43/b43-test/ucode5.fw
and /lib/firmware/b43/b43-osfw/ucode5.fw
and /lib/firmware/b43/b43/ucode5.fw, this doesn't preclude the use.

Now, if it had been like that from the beginning, Michael probably
wouldn't have used the string "b43" (or "b43-*") but rather requested
"broadcom/ucode5.fw" by default and "osfw/ucode5.fw" for the open source
firmware, but since it's just a key that doesn't matter.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[David Woodhouse]

On Sun, 2008-05-25 at 16:13 +0200, Johannes Berg wrote:
> If Michael wants to serve his firmware blobs from an SQL database,
> he'd use a simple table like this:
> 
> CREATE TABLE firmware (
>         ID INTEGER,
>         name VARCHAR(100),
>         data BLOB
> );

That's fairly much what the in-kernel firmware support does (just
serving firmware from a flat table using the string as a key, that is.
If I tried using SQL in-kernel you should take me out back and shoot me,
of course).

-- 
dwmw2

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On Sun, 2008-05-25 at 15:18 +0100, David Woodhouse wrote:
> On Sun, 2008-05-25 at 16:13 +0200, Johannes Berg wrote:
> > If Michael wants to serve his firmware blobs from an SQL database,
> > he'd use a simple table like this:
> > 
> > CREATE TABLE firmware (
> >         ID INTEGER,
> >         name VARCHAR(100),
> >         data BLOB
> > );
> 
> That's fairly much what the in-kernel firmware support does (just
> serving firmware from a flat table using the string as a key, that is.
> If I tried using SQL in-kernel you should take me out back and shoot me,
> of course).

Yeah, and only that makes sense, if you ask me. And as you said in the
other mail, adding a module name disambiguation policy in userspace
would mean that two different modules would be allowed to both request
the firmware key "foobar" and not have a problem, while it would be
pretty hard to support it with the built-in firmware option.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Alexandre Oliva]

On May 25, 2008, Marcel Holtmann <marcel@holtmann.org> wrote:

> in the early days we had something like three drivers using the
> request_firmware() and it was understood between the authors what the
> filename was meant for.

You're contradicting yourself.  Is it a filename, or is it not?
Earlier, you said it wasn't, it was just a name that userspace was
supposed to map to a filename.  Now, you're saying it is a filename.

Clearly (to me) your wish to prohibit '/'s in the firmware name has to
do with an attempt to force a distiction, to make the firmware a
filename rather than a pathname.  But, as you said yourself, the
mapping from firmware name is supposed to be entirely handled in
userland, therefore it doesn't even begin to make sense to distinguish
between filenames and pathnames.  You'd have to make assumptions that
(i) the firmware name names files (with built-in firmware, it
doesn't), and, if it is about filenames, (ii) what the pathname
separator character is.  Should '\\' be ruled out as well, because
someone might want /lib/firmware to be in a FAT filesystem?

nWouldn't it be better to leave the resolution of firmware names to
content *entirely* up to userland?  Say, if userland wants to
implement something very similar to the key-to-data map in-kernel
built-in firmware, this would work just fine, without any artificial
constraints?

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}
FSFLA Board Member       ¡Sé Libre! => http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi David,

>>> What if I want 2 or 3 different firmware versions installed at once?
>>> I'll have to play dirty tricks with the filenames then and put  
>>> several
>>> unrelated firmware versions into one directory. That's going to be  
>>> a mess.
>>
>> You add the desired feature to udev, or use symlinks. Hardly a  
>> "mess".
>
> It's a bit of a pain for the in-kernel firmware loading that I'm  
> working
> on now, if we can no longer count on the requested string to be the
> single match criterion.

the in-kernel firmware loading can use a total different policy on how  
to match the firmware filenames and driver names to a directory/ 
filename.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Johannes,

>> Look at the device nodes. The kernel has mouse0 for example and udev
>> will translate this into /dev/input/mouse0. Nobody expects the kernel
>> to use input/mouse0 and actually you even can't do that at all since
>> the device model forbids "/" as bus id. Same applies for the firmware
>> filenames.
>
> No, it doesn't.

please enlighten me how you can use "/" within bus ids.

>> Also at some point we might change the actual implementation of
>> request_firmware() to allow running multiple request_firmware() at  
>> the
>> same time to improve the init time of devices (if that makes sense).
>> In that case the filename would become a kobject and then the
>> directory separator would become illegal.
>
> There's no need to think of it that way. Look at a uevent now:
>
> UEVENT[1211722721.323011] add      /devices/pci0001:10/0001:10:12.0/ 
> ssb0:0/firmware/ssb0:0 (firmware)
> ACTION=add
> DEVPATH=/devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0
> SUBSYSTEM=firmware
> FIRMWARE=b43/b0g0bsinitvals5.fw
> TIMEOUT=60
> SEQNUM=1376
>
>
> The "firmware key" is contained in the FIRMWARE environment  
> variable. If
> you want to allow loading multiple firmwares at the same time, you
> wouldn't have to make the key part of the device name, you would only
> have to add a unique ID to the firmware device name, say

So you actually do know how request_firmware() actually works right  
now? You need to change the firmware_class implementation and API to  
give it an extra parameter to allow any kind if simultaneous loading  
within one driver. Having the FIRMWARE as environment variable is  
actually suboptimal. You want to have the FIRMWARE environment  
variable as bus_id for the firmware struct device object.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Michael,

>> this should all be done in userspace and not inside the kernel.  
>> Export
>> the special device versions via struct device and stop being lazy.
>
> No. Wait. It's not exactly going to work this way.
> You're not going to break my ABI and then tell me I'm going to work  
> around
> that, right? Answer: No.
>
> b43 will use the slash in request_firmware. That is not going to  
> change.
>
> This is _exactly_ what users are so tired about. Every now and then  
> people
> decide to change some ABI.

don't give me that crap. Nobody plans to break everything just right  
now and leave people hanging in between. We will do a smooth  
transition. Your users won't even notice it.

And we change the API/ABI all the time. Get used to it.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 20:12:41 Marcel Holtmann wrote:
> So you actually do know how request_firmware() actually works right  
> now? You need to change the firmware_class implementation and API to  
> give it an extra parameter to allow any kind if simultaneous loading  
> within one driver. Having the FIRMWARE as environment variable is  
> actually suboptimal. You want to have the FIRMWARE environment  
> variable as bus_id for the firmware struct device object.

Can you explain _why_ we want to have this as bus_id?
I don't really understand that.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Johannes,


>> No it is not. The kobject doesn't allow "/" and why should
>> request_firmware() be an exception. Also see my other comment on how
>> the kernel handles device nodes and on how udev maps them to real
>> device nodes on the filesystem.
>
> As I said, kobjects have nothing to do with it, they don't need to  
> have
> a filename based on the firmware key.

our current usage is suboptimal and we should use kobjects for  
representing the loading entity. Please keep in mind that when we  
created request_firmware() most of the driver model developers still  
thought that having class devices was a good idea. That has changed  
and to represent firmware loading in a clean and race free environment  
where you could load multipl firmwares at the same time for the same  
driver, we need changes here.

>> And again. This is up to the userspace to handle and not the kernel.
>> In userspace you could do a general approach to support these kind of
>> testing, but you decide to only do this for your driver. You are  
>> fully
>> exploiting the request_firmware() interface and making any kind of
>> userspace policy impossible. This in return actually means that if we
>> would improve the request_firmware(), we have to maintain special
>> cases for the b43 drivers, because your driver does some hacking of
>> firmware files inside the kernel. You actually proved my point that  
>> we
>> should not allow this inside the kernel.
>
> That makes no sense at all.
>
> Michael is exploiting the firmware API that you are suggesting to
> change, and so far I don't see a technical reason for changing it. In
> kernel space, he's simply requesting varying firmware blobs based on
> different keys, which happen to be "b43%s/%s" because that's making
> things simpler for him on the other end.
>
> On the other hand, you're saying that we have all kinds of policy  
> about
> this in userspace, so why are you saying that directory separators
> (slashes, but you seem to distinguish those on some level I do not
> understand) should be special in the firmware key?
>
> The firmware key is just that, a *key*. It's only exposed to userspace
> via an environment variable in a kobject named
> "/devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0" or similar.
>
> The fact that userspace uses the key as a filename is maybe  
> unfortunate,
> maybe fortunate, but shouldn't have anything to do with what sort of
> keys the kernel allows.

I disagree with you. The kernel should be free of these kind of  
subdirectory stuff. We saw devfs failing and we have a flat device  
node names in the kernel. Why do we have to duplicate information in  
the firmware filenames where we have all the information already  
present in the driver model. The reason that people are lazy doesn't  
work for me.

> Also, you said above (quoting again):
>
>> You are fully
>> exploiting the request_firmware() interface and making any kind of
>> userspace policy impossible.
>
> That's not true at all. If you decide that the userspace policy should
> be to load $modulename/$firmwarekey then you'd maybe have something
> like /lib/firmware/b43/b43-test/ucode5.fw
> and /lib/firmware/b43/b43-osfw/ucode5.fw
> and /lib/firmware/b43/b43/ucode5.fw, this doesn't preclude the use.
>
> Now, if it had been like that from the beginning, Michael probably
> wouldn't have used the string "b43" (or "b43-*") but rather requested
> "broadcom/ucode5.fw" by default and "osfw/ucode5.fw" for the open  
> source
> firmware, but since it's just a key that doesn't matter.

That something works at the moment is not a reason for me not to fix  
it and improve the current framework around firmware loading. I have  
been a lot of times saying that the request_firmware() should not  
include "/" in the filename and driver authors followed it. Some of  
them did it anyway and so these need fixing now.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 20:15:13 Marcel Holtmann wrote:
> don't give me that crap. Nobody plans to break everything just right  
> now and leave people hanging in between. We will do a smooth  
> transition. Your users won't even notice it.

Right. I will forward any complain mail to you then.
This is not the first time we (need to) change the firmware ABI, so I
pretty much know what I'm talking about.

I still didn't see a single valid reason in the whole thread that explains
why we suddenly have to forbid the use of the "/" character. (and that's
really what my problem only is about)

> And we change the API/ABI all the time. Get used to it.

Right. And endusers are really scared by it.
Other operating systems out there can live without ABI breakage for 10 years.

Breakage example? I have a server running Ubuntu Dapper. I'm running a 2.6.23 kernel on
it and it complains that several features used by the dapper udev will go away in
a future kernel release. So if I want to update the kernel (security update or
for whatever reason) I need to upgrade the whole distribution, basically.
That is OK and I will do this. But this just shows that we really must try hard
to avoid breaking the udev ABI.
And I don't see this happening here.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Michael,

>> So you actually do know how request_firmware() actually works right
>> now? You need to change the firmware_class implementation and API to
>> give it an extra parameter to allow any kind if simultaneous loading
>> within one driver. Having the FIRMWARE as environment variable is
>> actually suboptimal. You want to have the FIRMWARE environment
>> variable as bus_id for the firmware struct device object.
>
> Can you explain _why_ we want to have this as bus_id?
> I don't really understand that.

we want to avoid duplicate sysfs entries. And with multi-function  
devices like in SDIO this can happen that the actual sysfs device  
entry becomes the same and then the struct device creation for the  
second firmware loading fails (if the first one hasn't finished).  
Putting the actually firmware filename as bus_id allows us to  
distinguish between the different firmware loading tasks.

With the remove of class devices and the more complex multi-function  
devices we have to make the firmware loading fully race free. If not  
we end up with sleep(1) hacks around everything and we don't want that.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Michael,

>> don't give me that crap. Nobody plans to break everything just right
>> now and leave people hanging in between. We will do a smooth
>> transition. Your users won't even notice it.
>
> Right. I will forward any complain mail to you then.
> This is not the first time we (need to) change the firmware ABI, so I
> pretty much know what I'm talking about.
>
> I still didn't see a single valid reason in the whole thread that  
> explains
> why we suddenly have to forbid the use of the "/" character. (and  
> that's
> really what my problem only is about)

the reason is pretty simple; the a kernel driver should not do any  
kind of policy, namespace or whatever you wanna call it. This should  
be done in userspace.

>> And we change the API/ABI all the time. Get used to it.
>
> Right. And endusers are really scared by it.
> Other operating systems out there can live without ABI breakage for  
> 10 years.

And some operating system really suffer from this ABI guarantee. I am  
not getting into this since it has been discussed so many times and  
the kernel source contains full documentation why we are not doing it.

> Breakage example? I have a server running Ubuntu Dapper. I'm running  
> a 2.6.23 kernel on
> it and it complains that several features used by the dapper udev  
> will go away in
> a future kernel release. So if I want to update the kernel (security  
> update or
> for whatever reason) I need to upgrade the whole distribution,  
> basically.
> That is OK and I will do this. But this just shows that we really  
> must try hard
> to avoid breaking the udev ABI.
> And I don't see this happening here.

We actually do and we the future remove document and the requirements  
document within the kernel source this is fully documented all of the  
time.

However you have to understand that at some point we have to make sure  
that kernel and userspace are recent. But again, this will be all  
documented and if a distro than decides to bluntly ignore it, then go  
ahead an blame the distro. The kernel depends on the "plumbing" and  
vise versa.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 20:23:46 Marcel Holtmann wrote:
> > The fact that userspace uses the key as a filename is maybe  
> > unfortunate,
> > maybe fortunate, but shouldn't have anything to do with what sort of
> > keys the kernel allows.
> 
> I disagree with you. The kernel should be free of these kind of  
> subdirectory stuff. We saw devfs failing and we have a flat device  
> node names in the kernel. Why do we have to duplicate information in  
> the firmware filenames where we have all the information already  
> present in the driver model. The reason that people are lazy doesn't  
> work for me.

I think you don't really understand what we are trying to explain.
So I'll try it once again.

We are _not_ saying that having hierarchy policy decisions in the kernel
is a good thing. It's just the case that we _currently_ have this kind
of firmware filename, that happens to _map_ to a hierarchy policy
currently made by udev.

That's either unfortunate (to you) or fortunate (to me).
In either case we have to live with it and we can _not_ break it.
By introducing a policy that forbids the use of the slash, we do break
this.

> > Also, you said above (quoting again):
> >
> >> You are fully
> >> exploiting the request_firmware() interface and making any kind of
> >> userspace policy impossible.
> >
> > That's not true at all. If you decide that the userspace policy should
> > be to load $modulename/$firmwarekey then you'd maybe have something
> > like /lib/firmware/b43/b43-test/ucode5.fw
> > and /lib/firmware/b43/b43-osfw/ucode5.fw
> > and /lib/firmware/b43/b43/ucode5.fw, this doesn't preclude the use.
> >
> > Now, if it had been like that from the beginning, Michael probably
> > wouldn't have used the string "b43" (or "b43-*") but rather requested
> > "broadcom/ucode5.fw" by default and "osfw/ucode5.fw" for the open  
> > source
> > firmware, but since it's just a key that doesn't matter.
> 
> That something works at the moment is not a reason for me not to fix  
> it and improve the current framework around firmware loading. I have  
> been a lot of times saying that the request_firmware() should not  
> include "/" in the filename and driver authors followed it. Some of  
> them did it anyway and so these need fixing now.

But to forbid usage of "/" is the _wrong_ way to go, as it breaks
existing setups.

b43 users are not going to accept re-installing or moving the firmware
files to another place. We had that in the past. It will result in a _lot_
of angry complaints like "How dare can you break my setup!".

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Michael,

>>> The fact that userspace uses the key as a filename is maybe
>>> unfortunate,
>>> maybe fortunate, but shouldn't have anything to do with what sort of
>>> keys the kernel allows.
>>
>> I disagree with you. The kernel should be free of these kind of
>> subdirectory stuff. We saw devfs failing and we have a flat device
>> node names in the kernel. Why do we have to duplicate information in
>> the firmware filenames where we have all the information already
>> present in the driver model. The reason that people are lazy doesn't
>> work for me.
>
> I think you don't really understand what we are trying to explain.
> So I'll try it once again.
>
> We are _not_ saying that having hierarchy policy decisions in the  
> kernel
> is a good thing. It's just the case that we _currently_ have this kind
> of firmware filename, that happens to _map_ to a hierarchy policy
> currently made by udev.
>
> That's either unfortunate (to you) or fortunate (to me).
> In either case we have to live with it and we can _not_ break it.
> By introducing a policy that forbids the use of the slash, we do break
> this.

we can change these things. We do it all the time.

>>> Also, you said above (quoting again):
>>>
>>>> You are fully
>>>> exploiting the request_firmware() interface and making any kind of
>>>> userspace policy impossible.
>>>
>>> That's not true at all. If you decide that the userspace policy  
>>> should
>>> be to load $modulename/$firmwarekey then you'd maybe have something
>>> like /lib/firmware/b43/b43-test/ucode5.fw
>>> and /lib/firmware/b43/b43-osfw/ucode5.fw
>>> and /lib/firmware/b43/b43/ucode5.fw, this doesn't preclude the use.
>>>
>>> Now, if it had been like that from the beginning, Michael probably
>>> wouldn't have used the string "b43" (or "b43-*") but rather  
>>> requested
>>> "broadcom/ucode5.fw" by default and "osfw/ucode5.fw" for the open
>>> source
>>> firmware, but since it's just a key that doesn't matter.
>>
>> That something works at the moment is not a reason for me not to fix
>> it and improve the current framework around firmware loading. I have
>> been a lot of times saying that the request_firmware() should not
>> include "/" in the filename and driver authors followed it. Some of
>> them did it anyway and so these need fixing now.
>
> But to forbid usage of "/" is the _wrong_ way to go, as it breaks
> existing setups.
>
> b43 users are not going to accept re-installing or moving the firmware
> files to another place. We had that in the past. It will result in a  
> _lot_
> of angry complaints like "How dare can you break my setup!".

Again, I don't plan to break any setup. I actually do think it is a  
good idea to group firmware files in subdirectories in /lib/firmware,  
but this subdirectory name doesn't belong in the kernel.

We will update udev to read the driver name and look into /lib/ 
firmware/<driver>/<filename> and /lib/firmware/<filename> for the  
firmware file.

Then we will set a date and note it in the future remove document.  
Something like 12 month after an updated udev has been released. This  
gives the distribution two generations time to update udev and kernel  
packages.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Alexandra,

>> in the early days we had something like three drivers using the
>> request_firmware() and it was understood between the authors what the
>> filename was meant for.
>
> You're contradicting yourself.  Is it a filename, or is it not?
> Earlier, you said it wasn't, it was just a name that userspace was
> supposed to map to a filename.  Now, you're saying it is a filename.

it is a filename with any directory components.

> Clearly (to me) your wish to prohibit '/'s in the firmware name has to
> do with an attempt to force a distiction, to make the firmware a
> filename rather than a pathname.  But, as you said yourself, the
> mapping from firmware name is supposed to be entirely handled in
> userland, therefore it doesn't even begin to make sense to distinguish
> between filenames and pathnames.  You'd have to make assumptions that
> (i) the firmware name names files (with built-in firmware, it
> doesn't), and, if it is about filenames, (ii) what the pathname
> separator character is.  Should '\\' be ruled out as well, because
> someone might want /lib/firmware to be in a FAT filesystem?

Actually the request_firmware() is Linux specific :)

> nWouldn't it be better to leave the resolution of firmware names to
> content *entirely* up to userland?  Say, if userland wants to
> implement something very similar to the key-to-data map in-kernel
> built-in firmware, this would work just fine, without any artificial
> constraints?

And again the grouping into subdirectories should have been fixed in  
userspace by reading the driver name. The kernel should not do this at  
all.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 19:17:57 Alexandre Oliva wrote:
> On May 25, 2008, Marcel Holtmann <marcel@holtmann.org> wrote:
> 
> > in the early days we had something like three drivers using the
> > request_firmware() and it was understood between the authors what the
> > filename was meant for.
> 
> You're contradicting yourself.  Is it a filename, or is it not?
> Earlier, you said it wasn't, it was just a name that userspace was
> supposed to map to a filename.  Now, you're saying it is a filename.
> 
> Clearly (to me) your wish to prohibit '/'s in the firmware name has to
> do with an attempt to force a distiction, to make the firmware a
> filename rather than a pathname.  But, as you said yourself, the
> mapping from firmware name is supposed to be entirely handled in
> userland, therefore it doesn't even begin to make sense to distinguish
> between filenames and pathnames.  You'd have to make assumptions that
> (i) the firmware name names files (with built-in firmware, it
> doesn't), and, if it is about filenames, (ii) what the pathname
> separator character is.  Should '\\' be ruled out as well, because
> someone might want /lib/firmware to be in a FAT filesystem?
> 
> nWouldn't it be better to leave the resolution of firmware names to
> content *entirely* up to userland?  Say, if userland wants to
> implement something very similar to the key-to-data map in-kernel
> built-in firmware, this would work just fine, without any artificial
> constraints?

One additional thing is to make sure the usability of the whole stuff
is not reduded. Currently I can do:

modprobe b43 fwpostfix=-open
# work with opensource firmware in b43-open/
rmmod b43
modprobe b43
# work with standard firmware in b43/

So it is really simple to switch between different flavours of firmware.
It is _not_ acceptable to change an udev configuration file all the time,
if you want to use another firmware. One needs to frequently switch
between firmware versions when developing firmware code.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 20:46:00 Marcel Holtmann wrote:
> we can change these things. We do it all the time.

Doing something stupid all the time is not that of a good idea.

> Again, I don't plan to break any setup. I actually do think it is a  
> good idea to group firmware files in subdirectories in /lib/firmware,  
> but this subdirectory name doesn't belong in the kernel.
> 
> We will update udev to read the driver name and look into /lib/ 
> firmware/<driver>/<filename> and /lib/firmware/<filename> for the  
> firmware file.
> 
> Then we will set a date and note it in the future remove document.  
> Something like 12 month after an updated udev has been released. This  
> gives the distribution two generations time to update udev and kernel  
> packages.

How are you going to handle multiple versions of the firmware for one driver?
How does one switch between versions (changing config files is not acceptable)?

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Michael,

>>> in the early days we had something like three drivers using the
>>> request_firmware() and it was understood between the authors what  
>>> the
>>> filename was meant for.
>>
>> You're contradicting yourself.  Is it a filename, or is it not?
>> Earlier, you said it wasn't, it was just a name that userspace was
>> supposed to map to a filename.  Now, you're saying it is a filename.
>>
>> Clearly (to me) your wish to prohibit '/'s in the firmware name has  
>> to
>> do with an attempt to force a distiction, to make the firmware a
>> filename rather than a pathname.  But, as you said yourself, the
>> mapping from firmware name is supposed to be entirely handled in
>> userland, therefore it doesn't even begin to make sense to  
>> distinguish
>> between filenames and pathnames.  You'd have to make assumptions that
>> (i) the firmware name names files (with built-in firmware, it
>> doesn't), and, if it is about filenames, (ii) what the pathname
>> separator character is.  Should '\\' be ruled out as well, because
>> someone might want /lib/firmware to be in a FAT filesystem?
>>
>> nWouldn't it be better to leave the resolution of firmware names to
>> content *entirely* up to userland?  Say, if userland wants to
>> implement something very similar to the key-to-data map in-kernel
>> built-in firmware, this would work just fine, without any artificial
>> constraints?
>
> One additional thing is to make sure the usability of the whole stuff
> is not reduded. Currently I can do:
>
> modprobe b43 fwpostfix=-open
> # work with opensource firmware in b43-open/
> rmmod b43
> modprobe b43
> # work with standard firmware in b43/
>
> So it is really simple to switch between different flavours of  
> firmware.
> It is _not_ acceptable to change an udev configuration file all the  
> time,
> if you want to use another firmware. One needs to frequently switch
> between firmware versions when developing firmware code.

we might should write down what everybody expects from a firmware  
loading mechanism.

I would like to see generic support for these kind of things. Not  
duplicated functionality in every driver.

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Marcel Holtmann]

Hi Michael,

>> Again, I don't plan to break any setup. I actually do think it is a
>> good idea to group firmware files in subdirectories in /lib/firmware,
>> but this subdirectory name doesn't belong in the kernel.
>>
>> We will update udev to read the driver name and look into /lib/
>> firmware/<driver>/<filename> and /lib/firmware/<filename> for the
>> firmware file.
>>
>> Then we will set a date and note it in the future remove document.
>> Something like 12 month after an updated udev has been released. This
>> gives the distribution two generations time to update udev and kernel
>> packages.
>
> How are you going to handle multiple versions of the firmware for  
> one driver?
> How does one switch between versions (changing config files is not  
> acceptable)?

that is your point of view. Why is changing a config file not  
acceptable?

Regards

Marcel

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 21:01:44 Marcel Holtmann wrote:
> Hi Michael,
> 
> >>> in the early days we had something like three drivers using the
> >>> request_firmware() and it was understood between the authors what  
> >>> the
> >>> filename was meant for.
> >>
> >> You're contradicting yourself.  Is it a filename, or is it not?
> >> Earlier, you said it wasn't, it was just a name that userspace was
> >> supposed to map to a filename.  Now, you're saying it is a filename.
> >>
> >> Clearly (to me) your wish to prohibit '/'s in the firmware name has  
> >> to
> >> do with an attempt to force a distiction, to make the firmware a
> >> filename rather than a pathname.  But, as you said yourself, the
> >> mapping from firmware name is supposed to be entirely handled in
> >> userland, therefore it doesn't even begin to make sense to  
> >> distinguish
> >> between filenames and pathnames.  You'd have to make assumptions that
> >> (i) the firmware name names files (with built-in firmware, it
> >> doesn't), and, if it is about filenames, (ii) what the pathname
> >> separator character is.  Should '\\' be ruled out as well, because
> >> someone might want /lib/firmware to be in a FAT filesystem?
> >>
> >> nWouldn't it be better to leave the resolution of firmware names to
> >> content *entirely* up to userland?  Say, if userland wants to
> >> implement something very similar to the key-to-data map in-kernel
> >> built-in firmware, this would work just fine, without any artificial
> >> constraints?
> >
> > One additional thing is to make sure the usability of the whole stuff
> > is not reduded. Currently I can do:
> >
> > modprobe b43 fwpostfix=-open
> > # work with opensource firmware in b43-open/
> > rmmod b43
> > modprobe b43
> > # work with standard firmware in b43/
> >
> > So it is really simple to switch between different flavours of  
> > firmware.
> > It is _not_ acceptable to change an udev configuration file all the  
> > time,
> > if you want to use another firmware. One needs to frequently switch
> > between firmware versions when developing firmware code.
> 
> we might should write down what everybody expects from a firmware  
> loading mechanism.
> 
> I would like to see generic support for these kind of things. Not  
> duplicated functionality in every driver.

Additionally the driver must be able to use different versions of firmware.
I'm going to implement fw probing in b43 like:
first probe propietary firmware (in b43/)
if that doesn't exist probe open firmware (in b43-open/)

These different versions of one firmware _must_ live within different
directories in userspace. I don't care about where this policy decision is made.
But the new policy default must match the current policy in any case.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Sunday 25 May 2008 21:03:56 Marcel Holtmann wrote:
> > How are you going to handle multiple versions of the firmware for  
> > one driver?
> > How does one switch between versions (changing config files is not  
> > acceptable)?
> 
> that is your point of view. Why is changing a config file not  
> acceptable?

Because I need to change between different versions of firmware
every few minutes while developing on firmware level.
Besides that the driver must be allowed to tell whether it wants to
have one or the other version of the firmware (see my previous mail).
So this must not be a userspace-only policy.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Alan Cox]

> And again the grouping into subdirectories should have been fixed in  
> userspace by reading the driver name. The kernel should not do this at  
> all.

The kernel should carry on doing what it does at the moment. The amount
of gain from semi-pointless screwing about with the namespace and
compatibility is vastly less than the gain from leaving things in their
current perfectly happy and adequate form.

Alan

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Alexandre Oliva]

On May 25, 2008, Michael Buesch <mb@bu3sch.de> wrote:

> One additional thing is to make sure the usability of the whole stuff
> is not reduded.

Removing support for slashes wouldn't reduce usability.  It would be
no more than silly policy (and ego?) enforcement.

cd /lib/firmware
for f in test/*; do ln -s $f `echo $f | sed s,^test/,test-,`; done

voila

Removing support for slashes just makes things more cumbersome,
placing artificial restrictions where none make sense, to impose a
specific mindset and prohibit very natural uses for no real gain.  But
it's not like anything really significant is lost.  It is still a flat
namespace, this change would just be taking out the '/' from the
available alphabet.

No big loss, but no real advance in whatever rule one is trying to
impose with this.  Simply demanding additional work arounds to do
things that can be done in perfectly simple and natural ways today,
for no actual advantage.

Reminds me of the additional mailing list recently suggested by Al
Viro ;-)

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}
FSFLA Board Member       ¡Sé Libre! => http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Alexandre Oliva]

On May 25, 2008, Marcel Holtmann <marcel@holtmann.org> wrote:

> Hi Alexandra,
>>> in the early days we had something like three drivers using the
>>> request_firmware() and it was understood between the authors what the
>>> filename was meant for.
>> 
>> You're contradicting yourself.  Is it a filename, or is it not?
>> Earlier, you said it wasn't, it was just a name that userspace was
>> supposed to map to a filename.  Now, you're saying it is a filename.

> it is a filename with any directory components.

You can't have it both ways.  Either it is a firmware name in a flat
namespace that userland is supposed to map however it wants to data,
or it is a filename that userland can map in more limited ways
*because* you declared it to be a filename.

Unless...

Are we talking of different things?

I thought you were talking about the string presented to userland to
request firmware to be loaded, but if you're talking about say
pathnames within /sys, I can see how supporting directories et al
could make things more complex and undesirable.

AFAICT by perusing the code for a few seconds, the name within /sys
that is created to receive the firmware code and the string that
userland is requested to resolve to the firmware code are currently
the same, but I don't quite see that there's a reason that requires it
to be so.

>> Should '\\' be ruled out as well, because someone might want
>> /lib/firmware to be in a FAT filesystem?

> Actually the request_firmware() is Linux specific :)

Last I looked Linux supported FAT filesystems.  If someone wanted
/lib/firmware to be FAT, why should this not be permitted?  What's
your point?

> And again the grouping into subdirectories should have been fixed in
> userspace by reading the driver name.

You appear to be assuming that driver name is the only reason and
criterium for grouping firmware files.  Any particular reason to force
this one-level grouping model, rather than permitting hierarchical
grouping, that might make more sense?

> The kernel should not do this at all.

And AFAICT ATM it doesn't, and that's how it should be.  Let's not
conflate userland implementation with in-kernel naming of firmwares.
These are orthogonal issues.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}
FSFLA Board Member       ¡Sé Libre! => http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]


> Again, I don't plan to break any setup. I actually do think it is a  
> good idea to group firmware files in subdirectories in /lib/firmware,  
> but this subdirectory name doesn't belong in the kernel.
> 
> We will update udev to read the driver name and look into /lib/ 
> firmware/<driver>/<filename> and /lib/firmware/<filename> for the  
> firmware file.

Clearly, you haven't understood the point. It is *NOT* (REPEAT: NOT)
about having the driver name in the firmware key.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1576 bytes --]


> >> Look at the device nodes. The kernel has mouse0 for example and udev
> >> will translate this into /dev/input/mouse0. Nobody expects the kernel
> >> to use input/mouse0 and actually you even can't do that at all since
> >> the device model forbids "/" as bus id. Same applies for the firmware
> >> filenames.
> >
> > No, it doesn't.
> 
> please enlighten me how you can use "/" within bus ids.

Please enlighten me why you need to use the firmware key in the bus ID.

> So you actually do know how request_firmware() actually works right  
> now? You need to change the firmware_class implementation and API to  
> give it an extra parameter to allow any kind if simultaneous loading  
> within one driver. 

Yeah, the API change required is internal and rather trivial.

> Having the FIRMWARE as environment variable is  
> actually suboptimal. 

Why? You still haven't given any reason for this.

> You want to have the FIRMWARE environment  
> variable as bus_id for the firmware struct device object.

Why?

The way I see it, there's nothing stopping you from creating firmware
"devices" using a simple increasing number as the bus_id, and having the
firmware key be presented in a 'FIRMWARE' property of those devices.
Then, the userspace loader realises such a device is created, looks at
the firmware property, takes the firmware, and stuffs it into the 'data'
property. It doesn't matter how many there are outstanding at the same
time because one userspace process is invoked per firmware device, hence
per ID.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Monday 26 May 2008 05:13:18 Alexandre Oliva wrote:
> On May 25, 2008, Michael Buesch <mb@bu3sch.de> wrote:
> 
> > One additional thing is to make sure the usability of the whole stuff
> > is not reduded.
> 
> Removing support for slashes wouldn't reduce usability.  It would be
> no more than silly policy (and ego?) enforcement.

I the other mail you say that the string is _not_ a filename.
So why do we want to remove the slash from the allowed charset, if
the string is not a filename?
This doesn't make any sense. Either it is a filename/pathname, or it is not.

-- 
Greetings Michael.

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]


> This doesn't make any sense. Either it is a filename/pathname, or it is not.

I still think that in the kernel it shouldn't be thought of as a
filename and I have not yet seen any real reason why it has to be.
Whether your userspace then uses it as a filename or not is pretty much
orthogonal, it currently does for great effect, and since the only
firmware loader currently is userspace, we tend to think of it as a
filename in the kernel too.

However, when building firmware into the kernel, this will no longer be
true, it'll be a simple key in an array.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Alexandre Oliva]

On May 26, 2008, Michael Buesch <mb@bu3sch.de> wrote:

> On Monday 26 May 2008 05:13:18 Alexandre Oliva wrote:
>> On May 25, 2008, Michael Buesch <mb@bu3sch.de> wrote:
>> 
>> > One additional thing is to make sure the usability of the whole stuff
>> > is not reduded.
>> 
>> Removing support for slashes wouldn't reduce usability.  It would be
>> no more than silly policy (and ego?) enforcement.

> I the other mail you say that the string is _not_ a filename.

I do, and I stand by that claim, that is not backed by documentation,
but rather by peeking at the implementation.

> So why do we want to remove the slash from the allowed charset, if
> the string is not a filename?

Exactly my point.  I don't know any reason to do that.  It appears to
be just silly policy (and ego) enforcement.

-- 
Alexandre Oliva         http://www.lsd.ic.unicamp.br/~oliva/
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}
FSFLA Board Member       ¡Sé Libre! => http://www.fsfla.org/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}

Re: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option

[Michael Buesch]

On Monday 26 May 2008 19:09:12 Alexandre Oliva wrote:
> On May 26, 2008, Michael Buesch <mb@bu3sch.de> wrote:
> 
> > On Monday 26 May 2008 05:13:18 Alexandre Oliva wrote:
> >> On May 25, 2008, Michael Buesch <mb@bu3sch.de> wrote:
> >> 
> >> > One additional thing is to make sure the usability of the whole stuff
> >> > is not reduded.
> >> 
> >> Removing support for slashes wouldn't reduce usability.  It would be
> >> no more than silly policy (and ego?) enforcement.
> 
> > I the other mail you say that the string is _not_ a filename.
> 
> I do, and I stand by that claim, that is not backed by documentation,
> but rather by peeking at the implementation.

Right.

> > So why do we want to remove the slash from the allowed charset, if
> > the string is not a filename?
> 
> Exactly my point.  I don't know any reason to do that.  It appears to
> be just silly policy (and ego) enforcement.

Ok I see. It seems I misunderstood your mail. Sorry for that.

-- 
Greetings Michael.