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",
+ ...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 ...
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 ...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 --
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 --
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 OK, thanks. -- dwmw2 --
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 --
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 +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 ...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 --
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 --
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
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 I 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 be Again, 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 should 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. --
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 --
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 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 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 --
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 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 =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' as 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
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 arbitrary 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=3Dadd DEVPATH=3D/devices/pci0001:10/0001:10:12.0/ssb0:0/firmware/ssb0:0 SUBSYSTEM=3Dfirmware FIRMWARE=3Db43/b0g0bsinitvals5.fw TIMEOUT=3D60 SEQNUM=3D1376 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 =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=3D1376 johannes
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 --
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
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 --
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 weird 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 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. --
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 real 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 --
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
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
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 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 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 --
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 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. --
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 --
Clearly, you haven't understood the point. It is *NOT* (REPEAT: NOT) about having the driver name in the firmware key. johannes
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. --
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's 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. --
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 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 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 --
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} --
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. --
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. --
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} --
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. --
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
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. --
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 --
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 --
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 Last I looked Linux supported FAT filesystems. If someone wanted /lib/firmware to be FAT, why should this not be permitted? What's 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 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} --
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 --
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 --
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 --
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 --
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 --
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 --
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 --
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 --
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 --
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 --
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 ...
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 --
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
--
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 ...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 --
