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

[Michael Buesch]

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

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

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

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

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

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

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

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

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

-- 
Greetings Michael.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/