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

[Marcel Holtmann <marcel@holtmann.org>]

Hi Johannes,

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

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

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

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

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

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

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

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

Regards

Marcel