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

[Marcel Holtmann]

Hi Michael,

> > > > 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.

this should all be done in userspace and not inside the kernel. Export  
the special device versions via struct device and stop being lazy.

> > 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.

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  
device nodes on the filesystem.

> > 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.

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

--
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/