RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware

[Kweh, Hock Leong]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx]
> Sent: Tuesday, April 28, 2015 6:52 AM
> 
> On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote:
> > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley
> > <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote:
> > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley
> > >> <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
> > >> >> > -----Original Message-----
> > >> >> > From: James Bottomley
> > >> >> > [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx]
> > >> >> > Sent: Thursday, April 23, 2015 10:10 PM
> > >> >> >
> > >> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
> > >> >> > > > -----Original Message-----
> > >> >> > > > From: James Bottomley
> > >> >> > [mailto:James.Bottomley@xxxxxxxxxxxxxxxxxxxxx]
> > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM
> > >> >> > > >
> > >> >> > > >
> > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a
> > >> >> > > > question of whether
> > >> >> > we
> > >> >> > > > can stomach the ick factor of actually initiating a
> > >> >> > > > transaction in close ... I'm
> > >> >> > still
> > >> >> > > > feeling queasy.
> > >> >> > >
> > >> >> > > The file "close" here can I understand that the file system
> > >> >> > > will call the
> > >> >> > "release"
> > >> >> > > function at the file_operations struct?
> > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L153
> > >> >> > > 8
> > >> >> > >
> > >> >> > > So, James you are meaning that we could initiating the
> > >> >> > > update transaction inside the f_ops->release() and return
> > >> >> > > the error code if update failed in this function?
> > >> >> >
> > >> >> > Well, that's what I was thinking.  However the return value of
> > >> >> > ->release doesn't get propagated in sys_close (or indeed
> > >> >> > anywhere ... no idea why it returns an int) thanks to the task
> > >> >> > work additions, so we'd actually have to use the operation
> > >> >> > whose value is propagated in sys_close() which turns out to be
> flush.
> > >> >> >
> > >> >> > James
> > >> >> >
> > >> >>
> > >> >> Okay, I think I got you. Just to double check for in case: you
> > >> >> are meaning to implement it at f_ops->flush() instead of f_ops-
> >release().
> > >> >
> > >> > Well, what I'm saying is that the only way to propagate an error
> > >> > to close is by returning one from the flush file_operation.
> > >> >
> > >> > Let's cc fsdevel to see if they have any brighter ideas.
> > >> >
> > >> > The problem is we need to update firmware (several megabytes of
> > >> > it) via standard system tools.  We're thinking cat to a device.
> > >> > The problem is that we need an error code back once the update
> > >> > goes through (which it can't until we've fed all the firmware
> > >> > data into the system).  To use standard unix tools, we have to
> > >> > trigger off the standard system calls cat uses and since write()
> > >> > will happen in chunks, the only way to commit the transaction is in
> close().
> > >> >
> > >> > We initially through of initiating the transaction in
> > >> > f_ops->release and returning the error code there, but that
> > >> > doesn't work because its value isn't actually propagated, so
> > >> > we're now thinking of initiating the transaction in f_ops->flush
> > >> > instead (this is a device, not a file, so it won't get any other
> > >> > flushers).  Are there any other ways for us to propagate error on close?
> > >> >
> > >>
> > >> I think we may end up wanting to support both UpdateCapsule and
> > >> QueryCapsuleCapabilities, in which case this gets awkward.  Maybe
> > >> we really should do a misc device + ioctl.
> > >
> > > To be honest, I hate ioctls ... especially the "have to use special
> > > tools" part.
> > >
> > > Would we ever want to support QueryCapsuleUpdate()?  The return
> > > codes on error are the same as UpdateCapsule() but the query call
> > > does nothing on success (and the update call updates, obviously), so
> > > it seems a bit pointless if someone's gone to the trouble of getting
> > > a capsule ... they obviously want to apply it rather than know if it could be
> applied.
> >
> > I can imagine a UI that would try to validate a transaction consisting
> > of several of these things, tell the user whether it'll work and
> > whether a reboot is needed, and then do it.
> 
> You mean for dependent capsules?  That's a bit way overthinking the UEFI
> current use case (which is for firmware update).  In theory, the persist across
> reboot flag can be used for OS persistent information (subject to someone
> actually coming up with an implementation).  I'd code for the simple case:
> firmware update and let the rest take care of itself if and when we have an
> implementation.
> 
> The last thing I want to see landing on the UEFI-SST is some hopelessly
> complex and nasty capsule spec just "because Linux implements it this way".
> 
> > > Assuming we do, we could just use the same error on close mechanism,
> > > but use sysfs binary attributes ... or probably something new like a
> > > binary transaction attribute that does all the transaction on close
> > > magic for us.
> >
> > Yeah, but now we have both input and output, so as ugly as ioctl is,
> > it's a pretty good match.
> 
> No, we'll have read and write, so we can do that.  As long as there's no
> transaction that can't complete in close or any sense of multiple transactions
> that aren't issued by open read/write close, we're covered.
> 
> > Sigh.  This is all more complicated than it deserves to me.
> 
> Be fair: it is a new interface and it works in a way that's just different enough
> from regular firmware to cause all this bother.
> 
> James
> 

Dear communities,

I agree with James. Due to different people may have different needs. But
from our side, we would just like to have a simple interface for us to upload
the efi capsule and perform update. We do not have any use case or need
to get info from QueryCapsuleUpdate(). Let me give a suggestion here:
please allow me to focus on deliver this simple loading interface and
upstream it. Then later whoever has the actual use case or needs on the ioctl
implementation, he or she could enhance base on this simple loading interface.
What do you guys think?

Let me summarize the latest design idea:
- No longer leverage on firmware class but use misc device
- Do not use platform device but use device_create()
- User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell
- File operation functions include: open(), read(), write() and flush()
- Perform mutex lock in open() then release the mutex in flush() for avoiding
   race condition / concurrent loading
- Perform the capsule update and error return at flush() function

Is there anything I missed? Any one still have concern with this idea?
Thanks for providing the ideas as well as the review.


Regards,
Wilson