On Tue, Nov 04, 2014 at 06:57:21AM -0800, Andy Lutomirski wrote: > On Nov 4, 2014 6:18 AM, "Matt Fleming" <matt.flem...@intel.com> wrote: > > > > On Mon, 2014-11-03 at 22:32 -0800, Andy Lutomirski wrote: > > > > > > It seems like a large fraction of the code in this module exists just > > > to work around the fact that request_firmware doesn't do what you want > > > it to do. You have code to: > > > > > > - Prevent the /lib/firmware mechanism from working. > > > - Avoid a deadlock by doing strange things in the unload code. > > > - Allow more than one capsule per module load. (Isn't this hard to > > > use? User code will have to wait for the next firmware request before > > > sending a second capsule.) > > I think that the firmware directory goes away and comes back. Try cd > /sys/firmware/efi-capsule-file and upload one capsule. I bet that ls > will show an empty directory. > > > > > > > All of this is for dubious gain. You have to do three separate opens > > > in sysfs to upload a capsule, and there's no way to report back to > > > userspace whether the EFI call worked and whether a reboot is needed. > > > > Whether or not a reboot is required is indicated in the capsule image > > itself, i.e. the capsule tells the firmware whether an immediate reboot > > is required not the other way around. > > > > The firmware does tell the kernel what *kind* of a reboot is required, > > but that doesn't need reporting to userspace. > > > > > What's the benefit of using the firmware interface here? > > > > I originally implemented something to send capsules to the firmware via > > sysfs files back in 2013 and I basically ended up duplicating 25% of the > > code that's already in drivers/base/firmware_class.c. > > > > If you're objecting to the lack of modularity in firmware_class.c, then > > we could probably carve up the functionality we require a little more > > neatly (like not having to do the /lib/firmware avoidance hacks), but > > firmware_class.c should definitely be used as the foundation. > > > > I don't particularly care what the foundation is, but given that a > misc char device currently looks like it would be considerably less > code for a nicer interface, using the firmware class in its current > state doesn't look so great.
Then fix the firmware class code. Please, don't create random /dev nodes for firmware images like this, use the firmware code, that is what it is there for, and it is correct to use it for this type of interface. > If I were to write user code for this, I'd really want a single > transaction that uploads a capsule and gets a success/failure > indication. Using ioctl or a single big write sound fine. That's what you are using with the firmware interface, so this patch is currect. > Basically, I agree that EFI capsules are firmware, and using the > "firmware" mechanism makes logical sense. But the firmware class is > designed for kernel drivers that request firmware, user code that just > blindly supplies an existing file, user code that doesn't care at all > whether the firmware loaded correctly (because, for many drivers, > there is probably no synchronous way to figure out whether the upload > worked anyway), and firmware loading being idempotent. > > For EFI capsules and other flash-my-device mechanisms, we want user > code to send firmware independently of any driver request, user code > to actively think about what it wants to send, user code to find out > what happened as a result of the request, and user code to actively > think about whether it should send another update. > > If someone wants to make firmware_class work very nicely for this > interface, that sounds great. I'd recommend using a non-overlapping > set of filenames in the sysfs directory to avoid confusing existing > tools, especially since it's not obvious to be that the kernel has any > way at all to detect that what it thinks is an intentional capsule > load request is actually an old version of udev mindlessly responding > to a firmware loading request (via udevadm trigger if nothing else). > I kind of suspect that it will end up sharing very little code with > the normal firmware mechanism, though. > > This thing really does sound like it'll be 20-30 lines of code *total* > using a misc device, and the earlier patches in the series could > possibly be dropped entirely. I don't want to see the misc interface used for this, sorry you don't like it, but I feel the firmware interface is correct. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
