RE: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@xxxxxxxxxxxxxx]
> >
> > Andy, here's the steps to load a capsule.  I don't have a problem with
> > this, it's userspace driven, no "autoloading" of files in /lib/, and
> > works with sysfs.
> >
> > Have any objection to it, I don't.

Thanks Greg for helping me on the explanation. I would like to apologize if
my cover letter/commit messages did misleading.

> 
> After reading the code, I still have objections.
> 
> The full workflow seems to be, from the user's POV:
> 
> 1. load the module
> 
> 2. hope that there isn't a file called /lib/firmware/efi-capsule-file
> 
> 3. echo 1 >.../loading
> 
> 4. cat firmware >.../data
> 
> 5. echo 0 >.../loading
> 
> 6. efi_update_capsule gets called.  The return value ends up in the kernel
> logs somewhere but doesn't seem to make it to userspace.
> 
> 7. reboot(), but only if the capsule you loaded requires a reboot, which may
> or may not be detectable without the kernel's help (I'm not sure about this
> point).
> 
> If you want to load a second capsule (which seems like a reasonable thing to
> do if the first capsule was the kind that is processed immediately), then
> rmmod -f the module and start over?

You no need to do rmmod in order to upload the 2nd capsule binary. You just need to
do the 3 steps as you mentioned in your 3, 4 and 5 for your 2nd or 3rd capsule binaries.
Then the last, you do the reboot.

> 
> This seems like an unpleasant interface.  I think that ioctl or a dedicated
> custom sysfs file would be considerably nicer.  It would allow you to upload a
> capsule and get an indication of success or failure all at once, and it lets you
> load more than one capsule.
> Also, it gets rid of some of the really weird module refcounting stuff that's
> going on -- the only unusual thing the module would do would be to pin itself
> once a reboot-requiring capsule is loaded.
> 
> --Andy
> 

Regarding the synchronization, it is only required for module unload. The code is designed
in such a way that allow to be built as a kernel module or built into the kernel. If you choose
the built in kernel method, you won't come into the module unload situation which require
the synchronization.


Regards,
Wilson