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

From: Andy Lutomirski <luto@amacapital.net>
To: James Bottomley <James.Bottomley@hansenpartnership.com>
Cc: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>,
	Peter Jones <pjones@redhat.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Matt Fleming <matt@codeblueprint.co.uk>,
	Ming Lei <ming.lei@canonical.com>,
	"Ong, Boon Leong" <boon.leong.ong@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
	Sam Protsenko <semen.protsenko@linaro.org>,
	Roy Franz <roy.franz@linaro.org>, Borislav Petkov <bp@alien8.de>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
Date: Mon, 27 Apr 2015 15:40:28 -0700	[thread overview]
Message-ID: <CALCETrXeh+y1XZrdDyhEmy+GK6z7-swZNPYGZybnqzppWAE+Wg@mail.gmail.com> (raw)
In-Reply-To: <1430174136.2314.49.camel@HansenPartnership.com>

On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> 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@hansenpartnership.com> wrote:
>> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
>> >> > -----Original Message-----
>> >> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
>> >> > 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@HansenPartnership.com]
>> >> > > > 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#L1538
>> >> > >
>> >> > > 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.

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

Sigh.  This is all more complicated than it deserves to me.

--Andy