lkml.org 
[lkml]   [2008]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/3] firmware: Add CONFIG_BUILTIN_FIRMWARE option
Date
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



\
 
 \ /
  Last update: 2008-05-25 13:53    [W:0.617 / U:0.108 seconds]
©2003-2014 Jasper Spaans. Advertise on this site