login
Search
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",
+ na...
At Fri, 23 May 2008 14:44:42 +0100,
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
--
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
Ah, good point; I'll take a closer look at that. Thanks.
--
dwmw2--
At Fri, 23 May 2008 15:58:31 +0100,
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
--
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--
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--
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 10...
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
--
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...
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_firmwareif (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--
Makes sense. I'll do that as the first patch of the sequence.
--
dwmw2--
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
--
On Fri, 23 May 2008 17:00:03 +0200
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
--
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--
Could have these in include/linux/sections.h where we
Looks good.
But do we need the firmware after init?(See compiler.h IIRC).
Sam
--
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
--
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
--
If embedded is the main consumer I would consider this a fairly vital
feature though.Rene.
--
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--
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'.If we don't want the firmware in-kernel at all times, stick it an
Done. Thanks.
--
dwmw2--
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
--
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--
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/Makefilediff --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_FIR...
Hi David.
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.
A small comment is justified here.
Do not consider everyone to know od.Assignment to targets i smiisng so generated files are not cleaned
by "make clean".targets := $(FIRMWARE_OBJS)
Sam
--
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--
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
--
Playing with it now. I fixed the mkdir thing by including $(objtree) in
the target.--
dwmw2--
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 +0100firmware: 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/soun...
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
--
Can you explain why it is allowed now? And maybe why the API was
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
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 isThe 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 aThat 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
--
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}
--
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
--
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 itLast I looked Linux supported FAT filesystems. If someone wanted
/lib/firmware to be FAT, why should this not be permitted? What'sYou 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 hierarchicalAnd 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}
--
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
--
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.
--
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-,`; donevoila
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}
--
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.
--
I do, and I stand by that claim, that is not backed by documentation,
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}
--
Ok I see. It seems I misunderstood your mail. Sorry for that.
--
Greetings Michael.
--
ot.
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
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
--
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.
--
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.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 weirdI 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 makesNo 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.
--
this should all be done in userspace and not inside the kernel. Export
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 realAnd 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
--
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.
--
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
--
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'sRight. 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.
--
the reason is pretty simple; the a kernel driver should not do any
kind of policy, namespace or whatever you wanna call it. This shouldAnd some operating system really suffer from this ABI guarantee. I am
not getting into this since it has been discussed so many times andWe 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
--
As I said, kobjects have nothing to do with it, they don't need to have
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.
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
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 sameI 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'tThat 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
--
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 breakBut 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.
--
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
--
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.
--
that is your point of view. Why is changing a config file not
acceptable?Regards
Marcel
--
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.
--
Clearly, you haven't understood the point. It is *NOT* (REPEAT: NOT)
about having the driver name in the firmware key.johannes
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--
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
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 mereBut 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 =3D "b43/pcm5.fw"', nothing needs to know that b43 is a
module name, in fact, it could very well be 'broadcom wlan/pcm5.fw' asThat 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
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
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
--
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 arbitraryThere'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=3Dadd
DEVPATH=3D/devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0
SUBSYSTEM=3Dfirmware
FIRMWARE=3Db43/b0g0bsinitvals5.fw
TIMEOUT=3D60
SEQNUM=3D1376The "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=EF=BB=BF
UEVENT[1211722721.323011] add /devices/pci0001:10/0001:10:12.0/ssb0:0/=
firmware/ssb0:0-1234 (firmware)
ACTION=3Dadd
DEVPATH=3D/devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0-1234
SUBSYSTEM=3Dfirmware
FIRMWARE=3Db43/b0g0bsinitvals5.fw
TIMEOUT=3D60
SEQNUM=3D1376johannes
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
--
Can you explain _why_ we want to have this as bus_id?
I don't really understand that.--
Greetings Michael.
--
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
--
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
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 theI agree that .. and . should probably be avoided.
Depending on the firmware version you have, there are over 30 files
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.
--
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 beAgain, I agree that we wanna have that, but putting a directory prefix
into the driver is wrong.Regards
Marcel
--
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 shouldWhat 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.
--
You add the desired feature to udev, or use symlinks. Hardly a "mess".
Alan
--
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--
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
--
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--
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
--
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--
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
--
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'sOK, thanks.
--
dwmw2--
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 Monit...
At Fri, 23 May 2008 14:50:54 +0100,
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
--
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--
