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

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

[James Bottomley]

On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> > 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-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> > > > 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?

James

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

[Andy Lutomirski]

On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
>> > -----Original Message-----
>> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
>> > 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-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
>> > > > 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.

--Andy

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

[James Bottomley]

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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
> >> > -----Original Message-----
> >> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> >> > 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-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> >> > > > 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.

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.

James

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

[Andy Lutomirski]

On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
>> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
>> >> > -----Original Message-----
>> >> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
>> >> > 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-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
>> >> > > > 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

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

[James Bottomley]

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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
> >> >> > -----Original Message-----
> >> >> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> >> >> > 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-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> >> >> > > > 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.

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

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@HansenPartnership.com]
> 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@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#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

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

[Andy Lutomirski]

On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
>> -----Original Message-----
>> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
>> 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@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#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

If you do this, there's no need for the misc device.

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

Make sure the mutex operation is killable, then, and maybe even interruptable.

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

If it works (and cat really does fail reliably), then it seems okay to me.

However, since I like pulling increasing numbers of my hats, someone
should verify that the common embedded cat implementations are also
okay with this.  For example, I haven't yet found any code in
busybox's cat implementation that closes stdout.

Given that the main targets of this (for now, at least) are embedded,
this might be a problem.

--Andy

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

[James Bottomley]

On Wed, 2015-04-29 at 11:40 -0700, Andy Lutomirski wrote:
> If it works (and cat really does fail reliably), then it seems okay to me.
> 
> However, since I like pulling increasing numbers of my hats, someone
> should verify that the common embedded cat implementations are also
> okay with this.  For example, I haven't yet found any code in
> busybox's cat implementation that closes stdout.
> 
> Given that the main targets of this (for now, at least) are embedded,
> this might be a problem.

I think that rabbit is a bit mixie: we already demonstrated we *can*
collect the error in close.  It's up to the fw vendors who want to use
this method to make sure they have proper tools (and if busybox cat
needs fixing, to fix it before they ship).

James

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Thursday, April 30, 2015 2:41 AM
> 
> On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong
> <hock.leong.kweh@intel.com> wrote:
> >
> > 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
> 
> If you do this, there's no need for the misc device.

I do this so that in the future when someone want to implement the
Ioctl(), he or she can base on this and expand it.

> 
> > - 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
> 
> Make sure the mutex operation is killable, then, and maybe even
> interruptable.

Okay.

> 
> > - 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.
> >
> 
> If it works (and cat really does fail reliably), then it seems okay to me.
> 
> However, since I like pulling increasing numbers of my hats, someone should
> verify that the common embedded cat implementations are also okay with
> this.  For example, I haven't yet found any code in busybox's cat
> implementation that closes stdout.
> 
> Given that the main targets of this (for now, at least) are embedded, this
> might be a problem.
> 

I think we shouldn't focus on the cat implementation for the close issue.

My understanding about this action:
cat file.bin > /sys/..../capsule_loader
It is actually the ">" (IO redirection) who perform the open write & close
to this "/sys/..../capsule_loader" file note and not the "cat" do it.
So, I think your answer can be found at Shell source code.

More info about the IO redirection can be found at:
http://www.tldp.org/LDP/abs/html/io-redirection.html

Busybox Shell Souce code can be found at:
https://lxr.missinglinkelectronics.com/busybox/shell/ash.c


Regards,
Wilson

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

[Andy Lutomirski]

On Thu, Apr 30, 2015 at 2:17 AM, Kweh, Hock Leong
<hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org]
>> Sent: Thursday, April 30, 2015 2:41 AM
>>
>> On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong
>> <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> > 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
>>
>> If you do this, there's no need for the misc device.
>
> I do this so that in the future when someone want to implement the
> Ioctl(), he or she can base on this and expand it.
>
>>
>> > - 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
>>
>> Make sure the mutex operation is killable, then, and maybe even
>> interruptable.
>
> Okay.
>
>>
>> > - 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.
>> >
>>
>> If it works (and cat really does fail reliably), then it seems okay to me.
>>
>> However, since I like pulling increasing numbers of my hats, someone should
>> verify that the common embedded cat implementations are also okay with
>> this.  For example, I haven't yet found any code in busybox's cat
>> implementation that closes stdout.
>>
>> Given that the main targets of this (for now, at least) are embedded, this
>> might be a problem.
>>
>
> I think we shouldn't focus on the cat implementation for the close issue.
>
> My understanding about this action:
> cat file.bin > /sys/..../capsule_loader
> It is actually the ">" (IO redirection) who perform the open write & close
> to this "/sys/..../capsule_loader" file note and not the "cat" do it.
> So, I think your answer can be found at Shell source code.

The shell opens capsule_loader and then execs the command.  If you type:

(cat file.bin) >/sys/.../captule_loader, then the command is a
subshell and the subshell will close the file.  (cat might also close
it, but there will be two references.)

If you type:

cat file.bin >/sys/.../capsule_loader

then the shell doesn't retain a reference to the file at all.

--Andy

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

[James Bottomley]

On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote:
> 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.

I think that's pretty much it.

Why don't you let me construct a straw man patch.  It's going to be a
bit controversial because it involves adding flush operations to sysfs
and kernfs, slicing apart firmware_class.c to extract the transaction
handling stuff and creating an new efi update capsule file which makes
use of it.

Once we have code, we at least have something more concrete to argue
over.

James

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

[Andy Lutomirski]

On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote:
>> 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.
>
> I think that's pretty much it.
>
> Why don't you let me construct a straw man patch.  It's going to be a
> bit controversial because it involves adding flush operations to sysfs
> and kernfs, slicing apart firmware_class.c to extract the transaction
> handling stuff and creating an new efi update capsule file which makes
> use of it.
>
> Once we have code, we at least have something more concrete to argue
> over.

Would it be worth checking whether busybox is also okay with it first?
(Sorry to be a naysayer.)

It would be a shame if we do all this to keep the userspace footprint
light and then it doesn't work for non-coreutils userspace.

--Andy

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

[James Bottomley]

On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote:
> On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote:
> >> 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.
> >
> > I think that's pretty much it.
> >
> > Why don't you let me construct a straw man patch.  It's going to be a
> > bit controversial because it involves adding flush operations to sysfs
> > and kernfs, slicing apart firmware_class.c to extract the transaction
> > handling stuff and creating an new efi update capsule file which makes
> > use of it.
> >
> > Once we have code, we at least have something more concrete to argue
> > over.
> 
> Would it be worth checking whether busybox is also okay with it first?
> (Sorry to be a naysayer.)
> 
> It would be a shame if we do all this to keep the userspace footprint
> light and then it doesn't work for non-coreutils userspace.

I don't think so, because we can fix busybox if it's a problem.  The
embedded people wanting this control the tool space, so they can decide
to use the fixed version.

So yes, someone should check and fix busybox cat if broken, but no, it's
not a blocker.

James

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

[Andy Lutomirski]

On Wed, Apr 29, 2015 at 2:39 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote:
>> On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>> > On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote:
>> >> 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.
>> >
>> > I think that's pretty much it.
>> >
>> > Why don't you let me construct a straw man patch.  It's going to be a
>> > bit controversial because it involves adding flush operations to sysfs
>> > and kernfs, slicing apart firmware_class.c to extract the transaction
>> > handling stuff and creating an new efi update capsule file which makes
>> > use of it.
>> >
>> > Once we have code, we at least have something more concrete to argue
>> > over.
>>
>> Would it be worth checking whether busybox is also okay with it first?
>> (Sorry to be a naysayer.)
>>
>> It would be a shame if we do all this to keep the userspace footprint
>> light and then it doesn't work for non-coreutils userspace.
>
> I don't think so, because we can fix busybox if it's a problem.  The
> embedded people wanting this control the tool space, so they can decide
> to use the fixed version.
>
> So yes, someone should check and fix busybox cat if broken, but no, it's
> not a blocker.

It's still a bit unfortunate that:

#!/bin/sh

cat "$1" >/sys/whatever
if [ "$?" != "0" ]; then
   echo "It didn't work because" ...
   exit 1
fi

echo "It worked!  Go reboot if needed."
exit 0

will only work sometimes.  Will people really test this on their
target implementation of cat?  I agree that making this possible with
just shell is nice, but I'm less happy about it if it'll be
unreliable.

--Andy


-- 
Andy Lutomirski
AMA Capital Management, LLC