Re: [REVIEW] NVM Express driver

[Greg KH <greg@kroah.com>]

On Fri, Mar 04, 2011 at 09:59:15PM +0000, Alan Cox wrote:
> > And non-automated loading of firmware as well.  Dell uses this for
> > updating their BIOSes just fine, with a userspace tool that initiates
> > the loading of the firmware.
> 
> Try using the Dell tool with namespaces.

Heck, try using _any_ tool that talks to sysfs with namespaces, don't
try to claim that this driver using its own custom firmware loader is a
solution to the namespace/sysfs issue please.  That's very disingenuous.

> > How does Dell do it?
> 
> How do most other apps do it.

They write to the sysfs file with the firmware when they feel like it.

> > So, what could be changed in the current firmware interface to fix this
> > problem in a manner which would solve these perceived issues?
> 
> I'm not sure it can. The basis of the interface is driver requests
> firmware. That is done by using a pathname which in a namespaced
> environment isn't global and has races

Of course, but those races don't show up in real systems, right?

> (Several things break quite spectacularly if you request_firmware while
> updating a package, but of course nobody considered such details even for
> automatic stuff in many cases. Really there is some locking needed).

I've never heard of this race before as you usually do firmware upload
either at boot time, or when a user specifically asks for it.  Neither
of those times is when packages are usually getting updated.

> For manual updating of a block of firmware the interface most stuff wants
> is
> 
> 	copy_from_user()
> 
> or if you wanted to wrap it up nicely
> 
> 	x = vmalloc_from_user(void __user *ptr, ssize_t len);
> 
> The app knows which firmware it is talking about and can inspect and
> compare it while holding an open file handle on the deivce. That protects
> against hotplug races and getting the wrong device second access, and
> ensures that the firmware, device and userspace are all talking about
> exactly the same thing.

But you do get this type of buffer from the firware interface to your
driver today, right?

> It would nice to say "its an obscure corner case that will just error",
> but far too much hardware still gets semi permanently (or permanently)
> converted into junk if you goof a permanent flash firmware update.

One would hope that the hardware was a bit more resiliant than that, but
I know how hardware is designed :(

Still, I don't want this to all of a sudden be "open season" for every
individual driver deciding to want to create an ioctl call for firmware
updating just because the authors don't like the existing in-kernel
interface.  Please fix up the in-kernel one instead to meet your needs.

thanks,

greg k-h