Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 13, 2015 7:42 AM, "Greg Kroah-Hartman" wrote: > > On Thu, Mar 12, 2015 at 10:47:54PM +, Matt Fleming wrote: > > On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote: > > > > > > I'm not 100% happy with write(2) (which is all we have in sysfs) for > > > two reasons: > > > > > > 1. If we write a file name, eww. That's more complicated, requires > > > temporary files, has annoying mount namespace issues, etc. > > > > > > 2. If we write the full contents, we need to do it in a single call to > > > write. That means that we can't use cat, which mostly defeats the > > > purpose. In fact, using cat could be actively harmful. > > > > At this point I'd really like Greg to chime in. > > > > In principal, I'm not stricly opposed to using a simple char device > > provided that it's not essentially a copy and paste of code from > > drivers/base/firmware_class.c. > > > > Greg? > > Yes, I don't want a character driver here for this if at all possible. > Just stick with the firmware download code, it's there and should work > "as-is" for your stuff. Given the rest of this interminable discussion, it seems pretty clear to me that the firmware download code doesn't work as is for this use case. It will sort of work with lots of changes (to locking, synchronicity, error reporting, enumeration, etc), but I think that the total complexity of doing that will far exceed the complexity if either a new chardev or some straightforward sysfs interface. We don't have ioctls in sysfs, though, and adding that sounds worse than a new character driver, so... --Andy > > thanks, > > 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 13, 2015 7:42 AM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Thu, Mar 12, 2015 at 10:47:54PM +, Matt Fleming wrote: On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote: I'm not 100% happy with write(2) (which is all we have in sysfs) for two reasons: 1. If we write a file name, eww. That's more complicated, requires temporary files, has annoying mount namespace issues, etc. 2. If we write the full contents, we need to do it in a single call to write. That means that we can't use cat, which mostly defeats the purpose. In fact, using cat could be actively harmful. At this point I'd really like Greg to chime in. In principal, I'm not stricly opposed to using a simple char device provided that it's not essentially a copy and paste of code from drivers/base/firmware_class.c. Greg? Yes, I don't want a character driver here for this if at all possible. Just stick with the firmware download code, it's there and should work as-is for your stuff. Given the rest of this interminable discussion, it seems pretty clear to me that the firmware download code doesn't work as is for this use case. It will sort of work with lots of changes (to locking, synchronicity, error reporting, enumeration, etc), but I think that the total complexity of doing that will far exceed the complexity if either a new chardev or some straightforward sysfs interface. We don't have ioctls in sysfs, though, and adding that sounds worse than a new character driver, so... --Andy thanks, 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, Mar 12, 2015 at 10:47:54PM +, Matt Fleming wrote: > On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote: > > > > I'm not 100% happy with write(2) (which is all we have in sysfs) for > > two reasons: > > > > 1. If we write a file name, eww. That's more complicated, requires > > temporary files, has annoying mount namespace issues, etc. > > > > 2. If we write the full contents, we need to do it in a single call to > > write. That means that we can't use cat, which mostly defeats the > > purpose. In fact, using cat could be actively harmful. > > At this point I'd really like Greg to chime in. > > In principal, I'm not stricly opposed to using a simple char device > provided that it's not essentially a copy and paste of code from > drivers/base/firmware_class.c. > > Greg? Yes, I don't want a character driver here for this if at all possible. Just stick with the firmware download code, it's there and should work "as-is" for your stuff. thanks, 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, Mar 12, 2015 at 10:47:54PM +, Matt Fleming wrote: On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote: I'm not 100% happy with write(2) (which is all we have in sysfs) for two reasons: 1. If we write a file name, eww. That's more complicated, requires temporary files, has annoying mount namespace issues, etc. 2. If we write the full contents, we need to do it in a single call to write. That means that we can't use cat, which mostly defeats the purpose. In fact, using cat could be actively harmful. At this point I'd really like Greg to chime in. In principal, I'm not stricly opposed to using a simple char device provided that it's not essentially a copy and paste of code from drivers/base/firmware_class.c. Greg? Yes, I don't want a character driver here for this if at all possible. Just stick with the firmware download code, it's there and should work as-is for your stuff. thanks, 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote: > > I'm not 100% happy with write(2) (which is all we have in sysfs) for > two reasons: > > 1. If we write a file name, eww. That's more complicated, requires > temporary files, has annoying mount namespace issues, etc. > > 2. If we write the full contents, we need to do it in a single call to > write. That means that we can't use cat, which mostly defeats the > purpose. In fact, using cat could be actively harmful. At this point I'd really like Greg to chime in. In principal, I'm not stricly opposed to using a simple char device provided that it's not essentially a copy and paste of code from drivers/base/firmware_class.c. Greg? -- Matt Fleming, Intel Open Source Technology Center -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, 10 Mar, at 08:51:59AM, Andy Lutomirski wrote: I'm not 100% happy with write(2) (which is all we have in sysfs) for two reasons: 1. If we write a file name, eww. That's more complicated, requires temporary files, has annoying mount namespace issues, etc. 2. If we write the full contents, we need to do it in a single call to write. That means that we can't use cat, which mostly defeats the purpose. In fact, using cat could be actively harmful. At this point I'd really like Greg to chime in. In principal, I'm not stricly opposed to using a simple char device provided that it's not essentially a copy and paste of code from drivers/base/firmware_class.c. Greg? -- Matt Fleming, Intel Open Source Technology Center -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 10:26 AM, Peter Jones wrote:
> On Tue, Mar 10, 2015 at 08:51:59AM -0700, Andy Lutomirski wrote:
>> On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones wrote:
>> >
>> >> >> So, for the sysfs interface, let's not allow loading from /lib. Let's
>> >> >> not require a userland tool. Let's just do,
>> >> >>
>> >> >> # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
>> >> >
>> >> >>
>> >> >> and be done with it.
>> >> >>
>> >> >> Hmmm?
>> >> >
>> >> > I assume you're implying a) the capsule header with the guid is embedded
>> >> > in the .bin there already, and b) one contiguous write(2) with error
>> >> > reporting coming through something like vars.c's efi_status_to_err()?
>> >> >
>> >> > If so, yes, I prefer this API.
>> >> >
>> >>
>> >> Is using a char device really so bad? I have a "simple_char" that
>> >> makes this really easy that's pending review.
>> >
>> > As long as there's straightforward propagation of the EFI_STATUS return
>> > from UpdateCapsule() back, sysfs file vs char device makes very little
>> > difference to me. Either way it's open(), write(), close(). Using the
>> > runtime firmware upload interface designed for wifi and scsi devices is
>> > the part I don't really like.
>> >
>>
>> I'm not 100% happy with write(2) (which is all we have in sysfs) for
>> two reasons:
>>
>> 1. If we write a file name, eww. That's more complicated, requires
>> temporary files, has annoying mount namespace issues, etc.
>>
>> 2. If we write the full contents, we need to do it in a single call to
>> write. That means that we can't use cat, which mostly defeats the
>> purpose. In fact, using cat could be actively harmful.
>
> So if what we wind up with is:
>
> fd = open("/sys/.../capsule", O_RDWR);
> write(fd, buf, size/N);
> ...
> write(fd, buf + M*size/N, size/N);
> close(fd);
>
> You're suggesting the error code would post on close()? My worry about
> that is that I imagine a lot less code in the wild checks the error code
> on close() than on write() - though gnu cat does do so on both. But
> there are other questions still - will it post on fdatasync()? On
> fsync()?
Cat, for example, doesn't check any of the above, which is why I don't
like this approach.
--Andy
--
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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 08:51:59AM -0700, Andy Lutomirski wrote:
> On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones wrote:
> >
> >> >> So, for the sysfs interface, let's not allow loading from /lib. Let's
> >> >> not require a userland tool. Let's just do,
> >> >>
> >> >> # echo /path/to/my/awesome/capsule.bin > /sys/../capsule
> >> >
> >> >>
> >> >> and be done with it.
> >> >>
> >> >> Hmmm?
> >> >
> >> > I assume you're implying a) the capsule header with the guid is embedded
> >> > in the .bin there already, and b) one contiguous write(2) with error
> >> > reporting coming through something like vars.c's efi_status_to_err()?
> >> >
> >> > If so, yes, I prefer this API.
> >> >
> >>
> >> Is using a char device really so bad? I have a "simple_char" that
> >> makes this really easy that's pending review.
> >
> > As long as there's straightforward propagation of the EFI_STATUS return
> > from UpdateCapsule() back, sysfs file vs char device makes very little
> > difference to me. Either way it's open(), write(), close(). Using the
> > runtime firmware upload interface designed for wifi and scsi devices is
> > the part I don't really like.
> >
>
> I'm not 100% happy with write(2) (which is all we have in sysfs) for
> two reasons:
>
> 1. If we write a file name, eww. That's more complicated, requires
> temporary files, has annoying mount namespace issues, etc.
>
> 2. If we write the full contents, we need to do it in a single call to
> write. That means that we can't use cat, which mostly defeats the
> purpose. In fact, using cat could be actively harmful.
So if what we wind up with is:
fd = open("/sys/.../capsule", O_RDWR);
write(fd, buf, size/N);
...
write(fd, buf + M*size/N, size/N);
close(fd);
You're suggesting the error code would post on close()? My worry about
that is that I imagine a lot less code in the wild checks the error code
on close() than on write() - though gnu cat does do so on both. But
there are other questions still - will it post on fdatasync()? On
fsync()?
--
Peter
--
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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones wrote: > >> >> So, for the sysfs interface, let's not allow loading from /lib. Let's >> >> not require a userland tool. Let's just do, >> >> >> >> # echo /path/to/my/awesome/capsule.bin > /sys/../capsule >> > >> >> >> >> and be done with it. >> >> >> >> Hmmm? >> > >> > I assume you're implying a) the capsule header with the guid is embedded >> > in the .bin there already, and b) one contiguous write(2) with error >> > reporting coming through something like vars.c's efi_status_to_err()? >> > >> > If so, yes, I prefer this API. >> > >> >> Is using a char device really so bad? I have a "simple_char" that >> makes this really easy that's pending review. > > As long as there's straightforward propagation of the EFI_STATUS return > from UpdateCapsule() back, sysfs file vs char device makes very little > difference to me. Either way it's open(), write(), close(). Using the > runtime firmware upload interface designed for wifi and scsi devices is > the part I don't really like. > I'm not 100% happy with write(2) (which is all we have in sysfs) for two reasons: 1. If we write a file name, eww. That's more complicated, requires temporary files, has annoying mount namespace issues, etc. 2. If we write the full contents, we need to do it in a single call to write. That means that we can't use cat, which mostly defeats the purpose. In fact, using cat could be actively harmful. --Andy -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> >> So, for the sysfs interface, let's not allow loading from /lib. Let's > >> not require a userland tool. Let's just do, > >> > >> # echo /path/to/my/awesome/capsule.bin > /sys/../capsule > > > >> > >> and be done with it. > >> > >> Hmmm? > > > > I assume you're implying a) the capsule header with the guid is embedded > > in the .bin there already, and b) one contiguous write(2) with error > > reporting coming through something like vars.c's efi_status_to_err()? > > > > If so, yes, I prefer this API. > > > > Is using a char device really so bad? I have a "simple_char" that > makes this really easy that's pending review. As long as there's straightforward propagation of the EFI_STATUS return from UpdateCapsule() back, sysfs file vs char device makes very little difference to me. Either way it's open(), write(), close(). Using the runtime firmware upload interface designed for wifi and scsi devices is the part I don't really like. -- Peter -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 8:21 AM, Peter Jones wrote: > On Tue, Mar 10, 2015 at 12:26:52PM +, Matt Fleming wrote: >> On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote: >> > >> > So again: do we really need or want to do this? >> >> One thing that we totally lose the ability to do is use the capsule >> interface for things *other* than firmware updates, e.g. >> >> https://lkml.org/lkml/2013/10/16/327 >> >> Also, requiring embedded or custom OS to include fwupdate into their >> existing boot solutions is a bit heavy handed when literally all they >> want to do is cat a binary blob to a sysfs file. >> >> I don't see why we can't have both solutions. > > Yeah - we clearly need a kernel interface for some embedded devices, and > it'd be better for every vendor to not implement it themselves. > >> Another thing is that ESRT isn't going to be supported by every >> platform. > > Yeah - though I think you're *mostly* talking about the same platforms > as above. > >> So, for the sysfs interface, let's not allow loading from /lib. Let's >> not require a userland tool. Let's just do, >> >> # echo /path/to/my/awesome/capsule.bin > /sys/../capsule > >> >> and be done with it. >> >> Hmmm? > > I assume you're implying a) the capsule header with the guid is embedded > in the .bin there already, and b) one contiguous write(2) with error > reporting coming through something like vars.c's efi_status_to_err()? > > If so, yes, I prefer this API. > Is using a char device really so bad? I have a "simple_char" that makes this really easy that's pending review. --Andy > -- > Peter -- Andy Lutomirski AMA Capital Management, LLC -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 12:26:52PM +, Matt Fleming wrote: > On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote: > > > > So again: do we really need or want to do this? > > One thing that we totally lose the ability to do is use the capsule > interface for things *other* than firmware updates, e.g. > > https://lkml.org/lkml/2013/10/16/327 > > Also, requiring embedded or custom OS to include fwupdate into their > existing boot solutions is a bit heavy handed when literally all they > want to do is cat a binary blob to a sysfs file. > > I don't see why we can't have both solutions. Yeah - we clearly need a kernel interface for some embedded devices, and it'd be better for every vendor to not implement it themselves. > Another thing is that ESRT isn't going to be supported by every > platform. Yeah - though I think you're *mostly* talking about the same platforms as above. > So, for the sysfs interface, let's not allow loading from /lib. Let's > not require a userland tool. Let's just do, > > # echo /path/to/my/awesome/capsule.bin > /sys/../capsule > > and be done with it. > > Hmmm? I assume you're implying a) the capsule header with the guid is embedded in the .bin there already, and b) one contiguous write(2) with error reporting coming through something like vars.c's efi_status_to_err()? If so, yes, I prefer this API. -- Peter -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote: > > So again: do we really need or want to do this? One thing that we totally lose the ability to do is use the capsule interface for things *other* than firmware updates, e.g. https://lkml.org/lkml/2013/10/16/327 Also, requiring embedded or custom OS to include fwupdate into their existing boot solutions is a bit heavy handed when literally all they want to do is cat a binary blob to a sysfs file. I don't see why we can't have both solutions. Another thing is that ESRT isn't going to be supported by every platform. So, for the sysfs interface, let's not allow loading from /lib. Let's not require a userland tool. Let's just do, # echo /path/to/my/awesome/capsule.bin > /sys/../capsule and be done with it. Hmmm? -- Matt Fleming, Intel Open Source Technology Center -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote: So again: do we really need or want to do this? One thing that we totally lose the ability to do is use the capsule interface for things *other* than firmware updates, e.g. https://lkml.org/lkml/2013/10/16/327 Also, requiring embedded or custom OS to include fwupdate into their existing boot solutions is a bit heavy handed when literally all they want to do is cat a binary blob to a sysfs file. I don't see why we can't have both solutions. Another thing is that ESRT isn't going to be supported by every platform. So, for the sysfs interface, let's not allow loading from /lib. Let's not require a userland tool. Let's just do, # echo /path/to/my/awesome/capsule.bin /sys/../capsule and be done with it. Hmmm? -- Matt Fleming, Intel Open Source Technology Center -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 08:51:59AM -0700, Andy Lutomirski wrote: On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones pjo...@redhat.com wrote: So, for the sysfs interface, let's not allow loading from /lib. Let's not require a userland tool. Let's just do, # echo /path/to/my/awesome/capsule.bin /sys/../capsule and be done with it. Hmmm? I assume you're implying a) the capsule header with the guid is embedded in the .bin there already, and b) one contiguous write(2) with error reporting coming through something like vars.c's efi_status_to_err()? If so, yes, I prefer this API. Is using a char device really so bad? I have a simple_char that makes this really easy that's pending review. As long as there's straightforward propagation of the EFI_STATUS return from UpdateCapsule() back, sysfs file vs char device makes very little difference to me. Either way it's open(), write(), close(). Using the runtime firmware upload interface designed for wifi and scsi devices is the part I don't really like. I'm not 100% happy with write(2) (which is all we have in sysfs) for two reasons: 1. If we write a file name, eww. That's more complicated, requires temporary files, has annoying mount namespace issues, etc. 2. If we write the full contents, we need to do it in a single call to write. That means that we can't use cat, which mostly defeats the purpose. In fact, using cat could be actively harmful. So if what we wind up with is: fd = open(/sys/.../capsule, O_RDWR); write(fd, buf, size/N); ... write(fd, buf + M*size/N, size/N); close(fd); You're suggesting the error code would post on close()? My worry about that is that I imagine a lot less code in the wild checks the error code on close() than on write() - though gnu cat does do so on both. But there are other questions still - will it post on fdatasync()? On fsync()? -- Peter -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 10:26 AM, Peter Jones pjo...@redhat.com wrote: On Tue, Mar 10, 2015 at 08:51:59AM -0700, Andy Lutomirski wrote: On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones pjo...@redhat.com wrote: So, for the sysfs interface, let's not allow loading from /lib. Let's not require a userland tool. Let's just do, # echo /path/to/my/awesome/capsule.bin /sys/../capsule and be done with it. Hmmm? I assume you're implying a) the capsule header with the guid is embedded in the .bin there already, and b) one contiguous write(2) with error reporting coming through something like vars.c's efi_status_to_err()? If so, yes, I prefer this API. Is using a char device really so bad? I have a simple_char that makes this really easy that's pending review. As long as there's straightforward propagation of the EFI_STATUS return from UpdateCapsule() back, sysfs file vs char device makes very little difference to me. Either way it's open(), write(), close(). Using the runtime firmware upload interface designed for wifi and scsi devices is the part I don't really like. I'm not 100% happy with write(2) (which is all we have in sysfs) for two reasons: 1. If we write a file name, eww. That's more complicated, requires temporary files, has annoying mount namespace issues, etc. 2. If we write the full contents, we need to do it in a single call to write. That means that we can't use cat, which mostly defeats the purpose. In fact, using cat could be actively harmful. So if what we wind up with is: fd = open(/sys/.../capsule, O_RDWR); write(fd, buf, size/N); ... write(fd, buf + M*size/N, size/N); close(fd); You're suggesting the error code would post on close()? My worry about that is that I imagine a lot less code in the wild checks the error code on close() than on write() - though gnu cat does do so on both. But there are other questions still - will it post on fdatasync()? On fsync()? Cat, for example, doesn't check any of the above, which is why I don't like this approach. --Andy -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 12:26:52PM +, Matt Fleming wrote: On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote: So again: do we really need or want to do this? One thing that we totally lose the ability to do is use the capsule interface for things *other* than firmware updates, e.g. https://lkml.org/lkml/2013/10/16/327 Also, requiring embedded or custom OS to include fwupdate into their existing boot solutions is a bit heavy handed when literally all they want to do is cat a binary blob to a sysfs file. I don't see why we can't have both solutions. Yeah - we clearly need a kernel interface for some embedded devices, and it'd be better for every vendor to not implement it themselves. Another thing is that ESRT isn't going to be supported by every platform. Yeah - though I think you're *mostly* talking about the same platforms as above. So, for the sysfs interface, let's not allow loading from /lib. Let's not require a userland tool. Let's just do, # echo /path/to/my/awesome/capsule.bin /sys/../capsule and be done with it. Hmmm? I assume you're implying a) the capsule header with the guid is embedded in the .bin there already, and b) one contiguous write(2) with error reporting coming through something like vars.c's efi_status_to_err()? If so, yes, I prefer this API. -- Peter -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 8:21 AM, Peter Jones pjo...@redhat.com wrote: On Tue, Mar 10, 2015 at 12:26:52PM +, Matt Fleming wrote: On Fri, 06 Mar, at 04:39:12PM, Peter Jones wrote: So again: do we really need or want to do this? One thing that we totally lose the ability to do is use the capsule interface for things *other* than firmware updates, e.g. https://lkml.org/lkml/2013/10/16/327 Also, requiring embedded or custom OS to include fwupdate into their existing boot solutions is a bit heavy handed when literally all they want to do is cat a binary blob to a sysfs file. I don't see why we can't have both solutions. Yeah - we clearly need a kernel interface for some embedded devices, and it'd be better for every vendor to not implement it themselves. Another thing is that ESRT isn't going to be supported by every platform. Yeah - though I think you're *mostly* talking about the same platforms as above. So, for the sysfs interface, let's not allow loading from /lib. Let's not require a userland tool. Let's just do, # echo /path/to/my/awesome/capsule.bin /sys/../capsule and be done with it. Hmmm? I assume you're implying a) the capsule header with the guid is embedded in the .bin there already, and b) one contiguous write(2) with error reporting coming through something like vars.c's efi_status_to_err()? If so, yes, I prefer this API. Is using a char device really so bad? I have a simple_char that makes this really easy that's pending review. --Andy -- Peter -- Andy Lutomirski AMA Capital Management, LLC -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 10, 2015 at 8:40 AM, Peter Jones pjo...@redhat.com wrote: So, for the sysfs interface, let's not allow loading from /lib. Let's not require a userland tool. Let's just do, # echo /path/to/my/awesome/capsule.bin /sys/../capsule and be done with it. Hmmm? I assume you're implying a) the capsule header with the guid is embedded in the .bin there already, and b) one contiguous write(2) with error reporting coming through something like vars.c's efi_status_to_err()? If so, yes, I prefer this API. Is using a char device really so bad? I have a simple_char that makes this really easy that's pending review. As long as there's straightforward propagation of the EFI_STATUS return from UpdateCapsule() back, sysfs file vs char device makes very little difference to me. Either way it's open(), write(), close(). Using the runtime firmware upload interface designed for wifi and scsi devices is the part I don't really like. I'm not 100% happy with write(2) (which is all we have in sysfs) for two reasons: 1. If we write a file name, eww. That's more complicated, requires temporary files, has annoying mount namespace issues, etc. 2. If we write the full contents, we need to do it in a single call to write. That means that we can't use cat, which mostly defeats the purpose. In fact, using cat could be actively harmful. --Andy -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
So, for the sysfs interface, let's not allow loading from /lib. Let's not require a userland tool. Let's just do, # echo /path/to/my/awesome/capsule.bin /sys/../capsule and be done with it. Hmmm? I assume you're implying a) the capsule header with the guid is embedded in the .bin there already, and b) one contiguous write(2) with error reporting coming through something like vars.c's efi_status_to_err()? If so, yes, I prefer this API. Is using a char device really so bad? I have a simple_char that makes this really easy that's pending review. As long as there's straightforward propagation of the EFI_STATUS return from UpdateCapsule() back, sysfs file vs char device makes very little difference to me. Either way it's open(), write(), close(). Using the runtime firmware upload interface designed for wifi and scsi devices is the part I don't really like. -- Peter -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 06, 2015 at 01:49:20PM -0800, Roy Franz wrote: > On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones wrote: > > On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: > >> Hi All, > >> > >> After some internal discussion and re-design prototyping & testing on > >> this efi capsule interface kernel module, I would like to start a > >> discussion > >> here on the new idea and wish to get input for the implementation and > >> eventually upstream. > > > > So do we actually *need* a kernel interface to UpdateCapsule? We've > > already got an implementation in progress where we use my ESRT patch to > > figure out what firmwares we can update, and we schedule them and do the > > update during boot up before the normal bootloader is run. That means > > never having to call UpdateCapsule() after ExitBootServices() or > > SetVirtualAddressMap() have been called, and only doing it across > > reboots that UEFI is performing itself. It also means never having to > > do it with multiple CPUs running. > > So this does it by writing the capsules to the EFI system partition, and > having > them processed from there during the next boot? > How would this work on diskless systems? (or boot-disk-less systems) One of the things I'm currently writing is making the file we load be specified correctly by UEFI device paths - and that'll include the ability to get it from devices presented by the network drivers. On currently extant test machines that includes tftp support, just like netbooting. On UEFI 2.5 machines, where ESRT is introduced so that we actually know something about the capsules we can apply updates for, it also includes http support. Admittedly that means when you're doing deployment you'll have to have some process for putting the images someplace, but it can be the same device you're net-booting from. Just one example. If we're talking about systems that are booting entirely out of their own firmware volume, there's definitely some legitimate argument that doing this without an ESP could be valuable - so yeah, a kernel API in that case might be worthwhile. That said, the extra complexity combined with the fact that it's running after ExitBootServices() and SetVirtualAddressMap() means I'll probably avoid using it from the userland code except on machines where there are no other options. My faith that we're going to see a lot of machines where that's well tested without our address maps looking exactly like Windows's isn't very high, and we've repeatedly run into a lot of pain with that in the past. That's not the only thing that has to be exactly right, either - for instance there's no guarantee it'll work if you use the ACPI reset vector instead of the EFI runtime services ResetSystem(EfiResetWarm) call. Right now we use the ACPI method preferentially because of SetVirtualAddressMap() not working as documented on so *many* platforms. That means what happens upon reboot when we do UpdateCapsule() is undefined behavior. Of course I'm likely not the only person who will ever implement tools in userland to orchestrate firmware updates, either :) > What are the use cases for capsules that don't require a reboot? I'm > not really clear uses for those, but the kernel interface could be > better for those, as it would allow processing without a reboot. I'm really not sure if we know the answer to that yet. Things like USB devices most often won't ever be registered with firmware's FMP anyway, so they won't have capsules exposed, and they'll use more traditional USB commands to do firmware updates - stuff like hughsie's ColorHug device are in that category, and he's already supporting that with fwupd. Things like peripheral card option ROMs are potentially in that category, though I'm a little skeptical of the idea that I actually want to update the firmware on my video or network card with the kernel already running, and that when I do I'm not going to want to reboot immediately to make sure it worked right anyway. There are almost certainly lots of cases I haven't thought of, though. If we want this interface for those cases, I think we should also be enumerating the cases we think it's supporting, as well, even if only in broad strokes. I don't think we need to say "support this specific device's updates", but keeping track of the basic model of the update we're supporting would go a long way to establishing the value of supporting all the complexity. -- Peter -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones wrote: > On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: >> Hi All, >> >> After some internal discussion and re-design prototyping & testing on >> this efi capsule interface kernel module, I would like to start a discussion >> here on the new idea and wish to get input for the implementation and >> eventually upstream. > > So do we actually *need* a kernel interface to UpdateCapsule? We've > already got an implementation in progress where we use my ESRT patch to > figure out what firmwares we can update, and we schedule them and do the > update during boot up before the normal bootloader is run. That means > never having to call UpdateCapsule() after ExitBootServices() or > SetVirtualAddressMap() have been called, and only doing it across > reboots that UEFI is performing itself. It also means never having to > do it with multiple CPUs running. So this does it by writing the capsules to the EFI system partition, and having them processed from there during the next boot? How would this work on diskless systems? (or boot-disk-less systems) What are the use cases for capsules that don't require a reboot? I'm not really clear uses for those, but the kernel interface could be better for those, as it would allow processing without a reboot. Roy > > I've posted several revisions of the ESRT patch to linux-efi , and right > now we're just waiting on the UEFI 2.5 spec to be finalized before I > send a final copy to Matt. The command line tool and the code to apply > the firmwares during boot are at https://github.com/rhinstaller/fwupdate > and there's a GNOME-based UI in progress at > https://github.com/hughsie/fwupd (yes we apparently stink at naming > things.) > > I'm not sure how making this go through the kernel will make things > better - it introduces a lot more things that can go wrong, and the only > benefit is one reboot where BootNext is set - which actually is > relatively fast even slow-POSTing machines. After that the > UpdateCapsule+reboot to apply is *extremely* fast, because > applying capsule updates have to happen very very early in the boot-up > sequence. (That early processing is /effectively/ a requirement, > since it has to happen before the block write locking on the SPI part is > done.) So even on the machine that takes quite some time to POST, the > time that would be saved saved is less than 1% or so of the total update > time. > > An early version of my code worked to update a machine I got from your > employer, FWIW - right now we're adding API and fixing up implementation > bits I made specifically to that one proof-of-concept scenario, and > there's some API parts that are in UEFI 2.5 draft releases that we're > not quite handling yet. However, there's code there that's already been > checked in which has successfully applied system firmware updates > without having a kernel interface to UpdateCapsule(). > > So again: do we really need or want to do this? > > -- > Peter > -- > To unsubscribe from this list: send the line "unsubscribe linux-efi" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: > Hi All, > > After some internal discussion and re-design prototyping & testing on > this efi capsule interface kernel module, I would like to start a discussion > here on the new idea and wish to get input for the implementation and > eventually upstream. So do we actually *need* a kernel interface to UpdateCapsule? We've already got an implementation in progress where we use my ESRT patch to figure out what firmwares we can update, and we schedule them and do the update during boot up before the normal bootloader is run. That means never having to call UpdateCapsule() after ExitBootServices() or SetVirtualAddressMap() have been called, and only doing it across reboots that UEFI is performing itself. It also means never having to do it with multiple CPUs running. I've posted several revisions of the ESRT patch to linux-efi , and right now we're just waiting on the UEFI 2.5 spec to be finalized before I send a final copy to Matt. The command line tool and the code to apply the firmwares during boot are at https://github.com/rhinstaller/fwupdate and there's a GNOME-based UI in progress at https://github.com/hughsie/fwupd (yes we apparently stink at naming things.) I'm not sure how making this go through the kernel will make things better - it introduces a lot more things that can go wrong, and the only benefit is one reboot where BootNext is set - which actually is relatively fast even slow-POSTing machines. After that the UpdateCapsule+reboot to apply is *extremely* fast, because applying capsule updates have to happen very very early in the boot-up sequence. (That early processing is /effectively/ a requirement, since it has to happen before the block write locking on the SPI part is done.) So even on the machine that takes quite some time to POST, the time that would be saved saved is less than 1% or so of the total update time. An early version of my code worked to update a machine I got from your employer, FWIW - right now we're adding API and fixing up implementation bits I made specifically to that one proof-of-concept scenario, and there's some API parts that are in UEFI 2.5 draft releases that we're not quite handling yet. However, there's code there that's already been checked in which has successfully applied system firmware updates without having a kernel interface to UpdateCapsule(). So again: do we really need or want to do this? -- Peter -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 6, 2015 4:20 AM, "Kweh, Hock Leong" wrote: > > > -Original Message- > > From: Andy Lutomirski [mailto:l...@amacapital.net] > > Sent: Friday, March 06, 2015 7:09 AM > > > > On Mar 5, 2015 1:19 AM, "Kweh, Hock Leong" > > wrote: > > > > > > > > This really is not a big deal. User should cope with it. > > > > > > > > No, it's a big deal, and the user should not cope. > > > > > > > > The user *should not* be required to have write access to anything in > > > > /lib to install a UEFI capsule that they download from their > > > > motherboard vendor's website. /lib belongs to the distro, and UEFI > > > > capsules do not belong to the distro. In this regard, UEFI capsules > > > > are completely unlike your wireless card firmware, your cpu microcode, > > > > etc. > > > > > > > > Imagine systems using NFS root, Atomic-style systems (e.g. ostree), > > > > systems that boot off squashfs, etc. They should still be able to > > > > load capsules. The basic user interface that should work is: > > > > > > > > # uefi-load-capsule /path/to/capsule > > > > > > > > or: > > > > > > > > # uefi-load-capsule - > > > > > > > I don't really care how uefi-load-capsule is implemented, as long as > > > > it's straightforward, because people will screw it up if it isn't > > > > straightforward. > > > > > > > > Why is it so hard to have a file in sysfs that you write the capsule > > > > to using *cat* (not echo) and that will return an error code if cat > > > > fails? Is it because you don't know where the end of the capsule is? > > > > if so, ioctl is designed for exactly this purpose. > > > > > > > > TBH, I find this thread kind of ridiculous. The problem that you're > > > > trying to solve is extremely simple, the functionality that userspace > > > > needs is trivial, and all of these complex proposals for how it should > > > > work are an artifact of the fact that the kernel-internal interfaces > > > > you're using for it are not well suited to the problem at hand. > > > > > > > > --Andy > > > > > > Sorry, I may not catch your point correctly. Are you trying to tell that > > > a "normal" user can perform efi capsule update. But a "normal" user > > > does not have the right to install or copy the capsule binary into > > > "/lib/firmware/". So, there is a need to make this capsule module to > > > allow uploading the capsule binary at any path or location other than > > > "/lib/firmware/". > > > > > > Is this what you mean? > > > > No. Only root should be able to load capsules, but even root may not > > be able to write to /lib. > > > > --Andy > > > > Okay, I accept that only root user can perform the load capsule. It is new > to me that root user may not have the access right to "/lib/firmware". > > But I remember you do mention that CPU micro code and wifi firmware > they are different and able to write in /lib/firmware. I am curious why > efi capsule binary have such a restriction. What is preventing it being > write to that location? > It has to do with where the firmware comes from. When someone prepares Linux fs image, they put user code, a kernel (usually), all modules that might be needed, and all firmware that's likely to be needed into /. This might come from the distro or a company-wide image. If a normal firmware firmware file needs an update, it's just like updating a driver or a library -- / will be updated by whatever mechanism is in use. Nonvolatile firmware is different. The update isn't needed on subsequent boots, and it could be much less generic than a driver. For example, a capsule could contain a boot splash screen image that says "this is computer #27." Updating the system image to do this makes little sense. Instead it'll be a one-time thing done by root. --Andy > I even went to check out the FHS: > http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard > I do not find any restriction calling out for that. > > Would you mind to educate me on that? > Thanks. > > > Regards, > Wilson > -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 06, 2015 at 11:41:57AM +, Kweh, Hock Leong wrote: > # cat /any/path/capsule.bin > /sys/devices/platform/efi_capsule/capsule_load This is straight-forward and clean. > or doing: > # echo "/any/path/capsule.bin" > > /sys/devices/platform/efi_capsule/capsule_load This is strange and clumsy. So do the first one please. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Friday, March 06, 2015 7:09 AM > > On Mar 5, 2015 1:19 AM, "Kweh, Hock Leong" > wrote: > > > > > > This really is not a big deal. User should cope with it. > > > > > > No, it's a big deal, and the user should not cope. > > > > > > The user *should not* be required to have write access to anything in > > > /lib to install a UEFI capsule that they download from their > > > motherboard vendor's website. /lib belongs to the distro, and UEFI > > > capsules do not belong to the distro. In this regard, UEFI capsules > > > are completely unlike your wireless card firmware, your cpu microcode, > > > etc. > > > > > > Imagine systems using NFS root, Atomic-style systems (e.g. ostree), > > > systems that boot off squashfs, etc. They should still be able to > > > load capsules. The basic user interface that should work is: > > > > > > # uefi-load-capsule /path/to/capsule > > > > > > or: > > > > > > # uefi-load-capsule - > > > > > I don't really care how uefi-load-capsule is implemented, as long as > > > it's straightforward, because people will screw it up if it isn't > > > straightforward. > > > > > > Why is it so hard to have a file in sysfs that you write the capsule > > > to using *cat* (not echo) and that will return an error code if cat > > > fails? Is it because you don't know where the end of the capsule is? > > > if so, ioctl is designed for exactly this purpose. > > > > > > TBH, I find this thread kind of ridiculous. The problem that you're > > > trying to solve is extremely simple, the functionality that userspace > > > needs is trivial, and all of these complex proposals for how it should > > > work are an artifact of the fact that the kernel-internal interfaces > > > you're using for it are not well suited to the problem at hand. > > > > > > --Andy > > > > Sorry, I may not catch your point correctly. Are you trying to tell that > > a "normal" user can perform efi capsule update. But a "normal" user > > does not have the right to install or copy the capsule binary into > > "/lib/firmware/". So, there is a need to make this capsule module to > > allow uploading the capsule binary at any path or location other than > > "/lib/firmware/". > > > > Is this what you mean? > > No. Only root should be able to load capsules, but even root may not > be able to write to /lib. > > --Andy > Okay, I accept that only root user can perform the load capsule. It is new to me that root user may not have the access right to "/lib/firmware". But I remember you do mention that CPU micro code and wifi firmware they are different and able to write in /lib/firmware. I am curious why efi capsule binary have such a restriction. What is preventing it being write to that location? I even went to check out the FHS: http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard I do not find any restriction calling out for that. Would you mind to educate me on that? Thanks. Regards, Wilson
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Friday, March 06, 2015 4:14 PM
>
> On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote:
> > No. Only root should be able to load capsules, but even root may not
> > be able to write to /lib.
>
> So basically what we want to do is:
>
> # cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img >
> /sys/firmware/efi/update
>
> Now it can't get any simpler than that and you get error codes too by
> failing the cat if the update fails.
>
> Mind you, I'm using '#' and not '$' as a shell prompt :-)
>
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
I believe you guys are more paying attention to the PATH and
whether doing:
# cat /any/path/capsule.bin > /sys/devices/platform/efi_capsule/capsule_load
or doing:
# echo "/any/path/capsule.bin" > /sys/devices/platform/efi_capsule/capsule_load
is not a big issue right?
Regards,
Wilson
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jÇ«y�m��@A�a���
0��h���i
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote: > No. Only root should be able to load capsules, but even root may not > be able to write to /lib. So basically what we want to do is: # cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img > /sys/firmware/efi/update Now it can't get any simpler than that and you get error codes too by failing the cat if the update fails. Mind you, I'm using '#' and not '$' as a shell prompt :-) -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 06, 2015 at 11:41:57AM +, Kweh, Hock Leong wrote: # cat /any/path/capsule.bin /sys/devices/platform/efi_capsule/capsule_load This is straight-forward and clean. or doing: # echo /any/path/capsule.bin /sys/devices/platform/efi_capsule/capsule_load This is strange and clumsy. So do the first one please. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Friday, March 06, 2015 7:09 AM On Mar 5, 2015 1:19 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: This really is not a big deal. User should cope with it. No, it's a big deal, and the user should not cope. The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Imagine systems using NFS root, Atomic-style systems (e.g. ostree), systems that boot off squashfs, etc. They should still be able to load capsules. The basic user interface that should work is: # uefi-load-capsule /path/to/capsule or: # uefi-load-capsule - /path/to/capsule I don't really care how uefi-load-capsule is implemented, as long as it's straightforward, because people will screw it up if it isn't straightforward. Why is it so hard to have a file in sysfs that you write the capsule to using *cat* (not echo) and that will return an error code if cat fails? Is it because you don't know where the end of the capsule is? if so, ioctl is designed for exactly this purpose. TBH, I find this thread kind of ridiculous. The problem that you're trying to solve is extremely simple, the functionality that userspace needs is trivial, and all of these complex proposals for how it should work are an artifact of the fact that the kernel-internal interfaces you're using for it are not well suited to the problem at hand. --Andy Sorry, I may not catch your point correctly. Are you trying to tell that a normal user can perform efi capsule update. But a normal user does not have the right to install or copy the capsule binary into /lib/firmware/. So, there is a need to make this capsule module to allow uploading the capsule binary at any path or location other than /lib/firmware/. Is this what you mean? No. Only root should be able to load capsules, but even root may not be able to write to /lib. --Andy Okay, I accept that only root user can perform the load capsule. It is new to me that root user may not have the access right to /lib/firmware. But I remember you do mention that CPU micro code and wifi firmware they are different and able to write in /lib/firmware. I am curious why efi capsule binary have such a restriction. What is preventing it being write to that location? I even went to check out the FHS: http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard I do not find any restriction calling out for that. Would you mind to educate me on that? Thanks. Regards, Wilson
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message-
From: Borislav Petkov [mailto:b...@alien8.de]
Sent: Friday, March 06, 2015 4:14 PM
On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote:
No. Only root should be able to load capsules, but even root may not
be able to write to /lib.
So basically what we want to do is:
# cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img
/sys/firmware/efi/update
Now it can't get any simpler than that and you get error codes too by
failing the cat if the update fails.
Mind you, I'm using '#' and not '$' as a shell prompt :-)
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
I believe you guys are more paying attention to the PATH and
whether doing:
# cat /any/path/capsule.bin /sys/devices/platform/efi_capsule/capsule_load
or doing:
# echo /any/path/capsule.bin /sys/devices/platform/efi_capsule/capsule_load
is not a big issue right?
Regards,
Wilson
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jÇ«y�m��@A�a���
0��h���i
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 6, 2015 4:20 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Friday, March 06, 2015 7:09 AM On Mar 5, 2015 1:19 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: This really is not a big deal. User should cope with it. No, it's a big deal, and the user should not cope. The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Imagine systems using NFS root, Atomic-style systems (e.g. ostree), systems that boot off squashfs, etc. They should still be able to load capsules. The basic user interface that should work is: # uefi-load-capsule /path/to/capsule or: # uefi-load-capsule - /path/to/capsule I don't really care how uefi-load-capsule is implemented, as long as it's straightforward, because people will screw it up if it isn't straightforward. Why is it so hard to have a file in sysfs that you write the capsule to using *cat* (not echo) and that will return an error code if cat fails? Is it because you don't know where the end of the capsule is? if so, ioctl is designed for exactly this purpose. TBH, I find this thread kind of ridiculous. The problem that you're trying to solve is extremely simple, the functionality that userspace needs is trivial, and all of these complex proposals for how it should work are an artifact of the fact that the kernel-internal interfaces you're using for it are not well suited to the problem at hand. --Andy Sorry, I may not catch your point correctly. Are you trying to tell that a normal user can perform efi capsule update. But a normal user does not have the right to install or copy the capsule binary into /lib/firmware/. So, there is a need to make this capsule module to allow uploading the capsule binary at any path or location other than /lib/firmware/. Is this what you mean? No. Only root should be able to load capsules, but even root may not be able to write to /lib. --Andy Okay, I accept that only root user can perform the load capsule. It is new to me that root user may not have the access right to /lib/firmware. But I remember you do mention that CPU micro code and wifi firmware they are different and able to write in /lib/firmware. I am curious why efi capsule binary have such a restriction. What is preventing it being write to that location? It has to do with where the firmware comes from. When someone prepares Linux fs image, they put user code, a kernel (usually), all modules that might be needed, and all firmware that's likely to be needed into /. This might come from the distro or a company-wide image. If a normal firmware firmware file needs an update, it's just like updating a driver or a library -- / will be updated by whatever mechanism is in use. Nonvolatile firmware is different. The update isn't needed on subsequent boots, and it could be much less generic than a driver. For example, a capsule could contain a boot splash screen image that says this is computer #27. Updating the system image to do this makes little sense. Instead it'll be a one-time thing done by root. --Andy I even went to check out the FHS: http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard I do not find any restriction calling out for that. Would you mind to educate me on that? Thanks. Regards, Wilson -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: Hi All, After some internal discussion and re-design prototyping testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. So do we actually *need* a kernel interface to UpdateCapsule? We've already got an implementation in progress where we use my ESRT patch to figure out what firmwares we can update, and we schedule them and do the update during boot up before the normal bootloader is run. That means never having to call UpdateCapsule() after ExitBootServices() or SetVirtualAddressMap() have been called, and only doing it across reboots that UEFI is performing itself. It also means never having to do it with multiple CPUs running. I've posted several revisions of the ESRT patch to linux-efi , and right now we're just waiting on the UEFI 2.5 spec to be finalized before I send a final copy to Matt. The command line tool and the code to apply the firmwares during boot are at https://github.com/rhinstaller/fwupdate and there's a GNOME-based UI in progress at https://github.com/hughsie/fwupd (yes we apparently stink at naming things.) I'm not sure how making this go through the kernel will make things better - it introduces a lot more things that can go wrong, and the only benefit is one reboot where BootNext is set - which actually is relatively fast even slow-POSTing machines. After that the UpdateCapsule+reboot to apply is *extremely* fast, because applying capsule updates have to happen very very early in the boot-up sequence. (That early processing is /effectively/ a requirement, since it has to happen before the block write locking on the SPI part is done.) So even on the machine that takes quite some time to POST, the time that would be saved saved is less than 1% or so of the total update time. An early version of my code worked to update a machine I got from your employer, FWIW - right now we're adding API and fixing up implementation bits I made specifically to that one proof-of-concept scenario, and there's some API parts that are in UEFI 2.5 draft releases that we're not quite handling yet. However, there's code there that's already been checked in which has successfully applied system firmware updates without having a kernel interface to UpdateCapsule(). So again: do we really need or want to do this? -- Peter -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones pjo...@redhat.com wrote: On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: Hi All, After some internal discussion and re-design prototyping testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. So do we actually *need* a kernel interface to UpdateCapsule? We've already got an implementation in progress where we use my ESRT patch to figure out what firmwares we can update, and we schedule them and do the update during boot up before the normal bootloader is run. That means never having to call UpdateCapsule() after ExitBootServices() or SetVirtualAddressMap() have been called, and only doing it across reboots that UEFI is performing itself. It also means never having to do it with multiple CPUs running. So this does it by writing the capsules to the EFI system partition, and having them processed from there during the next boot? How would this work on diskless systems? (or boot-disk-less systems) What are the use cases for capsules that don't require a reboot? I'm not really clear uses for those, but the kernel interface could be better for those, as it would allow processing without a reboot. Roy I've posted several revisions of the ESRT patch to linux-efi , and right now we're just waiting on the UEFI 2.5 spec to be finalized before I send a final copy to Matt. The command line tool and the code to apply the firmwares during boot are at https://github.com/rhinstaller/fwupdate and there's a GNOME-based UI in progress at https://github.com/hughsie/fwupd (yes we apparently stink at naming things.) I'm not sure how making this go through the kernel will make things better - it introduces a lot more things that can go wrong, and the only benefit is one reboot where BootNext is set - which actually is relatively fast even slow-POSTing machines. After that the UpdateCapsule+reboot to apply is *extremely* fast, because applying capsule updates have to happen very very early in the boot-up sequence. (That early processing is /effectively/ a requirement, since it has to happen before the block write locking on the SPI part is done.) So even on the machine that takes quite some time to POST, the time that would be saved saved is less than 1% or so of the total update time. An early version of my code worked to update a machine I got from your employer, FWIW - right now we're adding API and fixing up implementation bits I made specifically to that one proof-of-concept scenario, and there's some API parts that are in UEFI 2.5 draft releases that we're not quite handling yet. However, there's code there that's already been checked in which has successfully applied system firmware updates without having a kernel interface to UpdateCapsule(). So again: do we really need or want to do this? -- Peter -- To unsubscribe from this list: send the line unsubscribe linux-efi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Fri, Mar 06, 2015 at 01:49:20PM -0800, Roy Franz wrote: On Fri, Mar 6, 2015 at 1:39 PM, Peter Jones pjo...@redhat.com wrote: On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: Hi All, After some internal discussion and re-design prototyping testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. So do we actually *need* a kernel interface to UpdateCapsule? We've already got an implementation in progress where we use my ESRT patch to figure out what firmwares we can update, and we schedule them and do the update during boot up before the normal bootloader is run. That means never having to call UpdateCapsule() after ExitBootServices() or SetVirtualAddressMap() have been called, and only doing it across reboots that UEFI is performing itself. It also means never having to do it with multiple CPUs running. So this does it by writing the capsules to the EFI system partition, and having them processed from there during the next boot? How would this work on diskless systems? (or boot-disk-less systems) One of the things I'm currently writing is making the file we load be specified correctly by UEFI device paths - and that'll include the ability to get it from devices presented by the network drivers. On currently extant test machines that includes tftp support, just like netbooting. On UEFI 2.5 machines, where ESRT is introduced so that we actually know something about the capsules we can apply updates for, it also includes http support. Admittedly that means when you're doing deployment you'll have to have some process for putting the images someplace, but it can be the same device you're net-booting from. Just one example. If we're talking about systems that are booting entirely out of their own firmware volume, there's definitely some legitimate argument that doing this without an ESP could be valuable - so yeah, a kernel API in that case might be worthwhile. That said, the extra complexity combined with the fact that it's running after ExitBootServices() and SetVirtualAddressMap() means I'll probably avoid using it from the userland code except on machines where there are no other options. My faith that we're going to see a lot of machines where that's well tested without our address maps looking exactly like Windows's isn't very high, and we've repeatedly run into a lot of pain with that in the past. That's not the only thing that has to be exactly right, either - for instance there's no guarantee it'll work if you use the ACPI reset vector instead of the EFI runtime services ResetSystem(EfiResetWarm) call. Right now we use the ACPI method preferentially because of SetVirtualAddressMap() not working as documented on so *many* platforms. That means what happens upon reboot when we do UpdateCapsule() is undefined behavior. Of course I'm likely not the only person who will ever implement tools in userland to orchestrate firmware updates, either :) What are the use cases for capsules that don't require a reboot? I'm not really clear uses for those, but the kernel interface could be better for those, as it would allow processing without a reboot. I'm really not sure if we know the answer to that yet. Things like USB devices most often won't ever be registered with firmware's FMP anyway, so they won't have capsules exposed, and they'll use more traditional USB commands to do firmware updates - stuff like hughsie's ColorHug device are in that category, and he's already supporting that with fwupd. Things like peripheral card option ROMs are potentially in that category, though I'm a little skeptical of the idea that I actually want to update the firmware on my video or network card with the kernel already running, and that when I do I'm not going to want to reboot immediately to make sure it worked right anyway. There are almost certainly lots of cases I haven't thought of, though. If we want this interface for those cases, I think we should also be enumerating the cases we think it's supporting, as well, even if only in broad strokes. I don't think we need to say support this specific device's updates, but keeping track of the basic model of the update we're supporting would go a long way to establishing the value of supporting all the complexity. -- Peter -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote: No. Only root should be able to load capsules, but even root may not be able to write to /lib. So basically what we want to do is: # cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img /sys/firmware/efi/update Now it can't get any simpler than that and you get error codes too by failing the cat if the update fails. Mind you, I'm using '#' and not '$' as a shell prompt :-) -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 5, 2015 1:19 AM, "Kweh, Hock Leong" wrote: > > > -Original Message- > > From: Andy Lutomirski [mailto:l...@amacapital.net] > > Sent: Wednesday, March 04, 2015 4:38 AM > > > > On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong > > wrote: > > > > > > Just to call out that using firmware class auto locate binary feature is > > > limited > > > to locations: > > > - "/lib/firmware/updates/" UTS_RELEASE, > > > - "/lib/firmware/updates", > > > - "/lib/firmware/" UTS_RELEASE, > > > - "/lib/firmware" > > > and one custom path which inputted during firmware_class module load > > > time or kernel boot up time. > > > > > > It is just not like the user helper interface which allow load the binary > > > at > > > any path/location. > > > > > > This really is not a big deal. User should cope with it. > > > > No, it's a big deal, and the user should not cope. > > > > The user *should not* be required to have write access to anything in > > /lib to install a UEFI capsule that they download from their > > motherboard vendor's website. /lib belongs to the distro, and UEFI > > capsules do not belong to the distro. In this regard, UEFI capsules > > are completely unlike your wireless card firmware, your cpu microcode, > > etc. > > > > Imagine systems using NFS root, Atomic-style systems (e.g. ostree), > > systems that boot off squashfs, etc. They should still be able to > > load capsules. The basic user interface that should work is: > > > > # uefi-load-capsule /path/to/capsule > > > > or: > > > > # uefi-load-capsule - > > > I don't really care how uefi-load-capsule is implemented, as long as > > it's straightforward, because people will screw it up if it isn't > > straightforward. > > > > Why is it so hard to have a file in sysfs that you write the capsule > > to using *cat* (not echo) and that will return an error code if cat > > fails? Is it because you don't know where the end of the capsule is? > > if so, ioctl is designed for exactly this purpose. > > > > TBH, I find this thread kind of ridiculous. The problem that you're > > trying to solve is extremely simple, the functionality that userspace > > needs is trivial, and all of these complex proposals for how it should > > work are an artifact of the fact that the kernel-internal interfaces > > you're using for it are not well suited to the problem at hand. > > > > --Andy > > Sorry, I may not catch your point correctly. Are you trying to tell that > a "normal" user can perform efi capsule update. But a "normal" user > does not have the right to install or copy the capsule binary into > "/lib/firmware/". So, there is a need to make this capsule module to > allow uploading the capsule binary at any path or location other than > "/lib/firmware/". > > Is this what you mean? No. Only root should be able to load capsules, but even root may not be able to write to /lib. --Andy > > > Regards, > Wilson -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Wednesday, March 04, 2015 4:38 AM > > On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong > wrote: > > > > Just to call out that using firmware class auto locate binary feature is > > limited > > to locations: > > - "/lib/firmware/updates/" UTS_RELEASE, > > - "/lib/firmware/updates", > > - "/lib/firmware/" UTS_RELEASE, > > - "/lib/firmware" > > and one custom path which inputted during firmware_class module load > > time or kernel boot up time. > > > > It is just not like the user helper interface which allow load the binary at > > any path/location. > > > > This really is not a big deal. User should cope with it. > > No, it's a big deal, and the user should not cope. > > The user *should not* be required to have write access to anything in > /lib to install a UEFI capsule that they download from their > motherboard vendor's website. /lib belongs to the distro, and UEFI > capsules do not belong to the distro. In this regard, UEFI capsules > are completely unlike your wireless card firmware, your cpu microcode, > etc. > > Imagine systems using NFS root, Atomic-style systems (e.g. ostree), > systems that boot off squashfs, etc. They should still be able to > load capsules. The basic user interface that should work is: > > # uefi-load-capsule /path/to/capsule > > or: > > # uefi-load-capsule - > I don't really care how uefi-load-capsule is implemented, as long as > it's straightforward, because people will screw it up if it isn't > straightforward. > > Why is it so hard to have a file in sysfs that you write the capsule > to using *cat* (not echo) and that will return an error code if cat > fails? Is it because you don't know where the end of the capsule is? > if so, ioctl is designed for exactly this purpose. > > TBH, I find this thread kind of ridiculous. The problem that you're > trying to solve is extremely simple, the functionality that userspace > needs is trivial, and all of these complex proposals for how it should > work are an artifact of the fact that the kernel-internal interfaces > you're using for it are not well suited to the problem at hand. > > --Andy Sorry, I may not catch your point correctly. Are you trying to tell that a "normal" user can perform efi capsule update. But a "normal" user does not have the right to install or copy the capsule binary into "/lib/firmware/". So, there is a need to make this capsule module to allow uploading the capsule binary at any path or location other than "/lib/firmware/". Is this what you mean? Regards, Wilson
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 5, 2015 1:19 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Wednesday, March 04, 2015 4:38 AM On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: Just to call out that using firmware class auto locate binary feature is limited to locations: - /lib/firmware/updates/ UTS_RELEASE, - /lib/firmware/updates, - /lib/firmware/ UTS_RELEASE, - /lib/firmware and one custom path which inputted during firmware_class module load time or kernel boot up time. It is just not like the user helper interface which allow load the binary at any path/location. This really is not a big deal. User should cope with it. No, it's a big deal, and the user should not cope. The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Imagine systems using NFS root, Atomic-style systems (e.g. ostree), systems that boot off squashfs, etc. They should still be able to load capsules. The basic user interface that should work is: # uefi-load-capsule /path/to/capsule or: # uefi-load-capsule - /path/to/capsule I don't really care how uefi-load-capsule is implemented, as long as it's straightforward, because people will screw it up if it isn't straightforward. Why is it so hard to have a file in sysfs that you write the capsule to using *cat* (not echo) and that will return an error code if cat fails? Is it because you don't know where the end of the capsule is? if so, ioctl is designed for exactly this purpose. TBH, I find this thread kind of ridiculous. The problem that you're trying to solve is extremely simple, the functionality that userspace needs is trivial, and all of these complex proposals for how it should work are an artifact of the fact that the kernel-internal interfaces you're using for it are not well suited to the problem at hand. --Andy Sorry, I may not catch your point correctly. Are you trying to tell that a normal user can perform efi capsule update. But a normal user does not have the right to install or copy the capsule binary into /lib/firmware/. So, there is a need to make this capsule module to allow uploading the capsule binary at any path or location other than /lib/firmware/. Is this what you mean? No. Only root should be able to load capsules, but even root may not be able to write to /lib. --Andy Regards, Wilson -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Wednesday, March 04, 2015 4:38 AM On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: Just to call out that using firmware class auto locate binary feature is limited to locations: - /lib/firmware/updates/ UTS_RELEASE, - /lib/firmware/updates, - /lib/firmware/ UTS_RELEASE, - /lib/firmware and one custom path which inputted during firmware_class module load time or kernel boot up time. It is just not like the user helper interface which allow load the binary at any path/location. This really is not a big deal. User should cope with it. No, it's a big deal, and the user should not cope. The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Imagine systems using NFS root, Atomic-style systems (e.g. ostree), systems that boot off squashfs, etc. They should still be able to load capsules. The basic user interface that should work is: # uefi-load-capsule /path/to/capsule or: # uefi-load-capsule - /path/to/capsule I don't really care how uefi-load-capsule is implemented, as long as it's straightforward, because people will screw it up if it isn't straightforward. Why is it so hard to have a file in sysfs that you write the capsule to using *cat* (not echo) and that will return an error code if cat fails? Is it because you don't know where the end of the capsule is? if so, ioctl is designed for exactly this purpose. TBH, I find this thread kind of ridiculous. The problem that you're trying to solve is extremely simple, the functionality that userspace needs is trivial, and all of these complex proposals for how it should work are an artifact of the fact that the kernel-internal interfaces you're using for it are not well suited to the problem at hand. --Andy Sorry, I may not catch your point correctly. Are you trying to tell that a normal user can perform efi capsule update. But a normal user does not have the right to install or copy the capsule binary into /lib/firmware/. So, there is a need to make this capsule module to allow uploading the capsule binary at any path or location other than /lib/firmware/. Is this what you mean? Regards, Wilson
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 3, 2015 12:51 PM, "Borislav Petkov" wrote: > > On Tue, Mar 03, 2015 at 12:37:54PM -0800, Andy Lutomirski wrote: > > The user *should not* be required to have write access to anything in > > /lib to install a UEFI capsule that they download from their > > motherboard vendor's website. /lib belongs to the distro, and UEFI > > capsules do not belong to the distro. In this regard, UEFI capsules > > are completely unlike your wireless card firmware, your cpu microcode, > > etc. > > Oh oh but but, if an UEFI capsule can brick the system, a normal user > would be able to brick that system then. I think we should forbid that. Absolutely. That's why I said # uefi-load-capsule and not $ uefi-load-capsule :) > > I agree with the rest of your note that a simple > > cat > /sys/... > > should be enough. > > -- > Regards/Gruss, > Boris. > > ECO tip #101: Trim your mails when you reply. > -- -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 03, 2015 at 12:37:54PM -0800, Andy Lutomirski wrote: > The user *should not* be required to have write access to anything in > /lib to install a UEFI capsule that they download from their > motherboard vendor's website. /lib belongs to the distro, and UEFI > capsules do not belong to the distro. In this regard, UEFI capsules > are completely unlike your wireless card firmware, your cpu microcode, > etc. Oh oh but but, if an UEFI capsule can brick the system, a normal user would be able to brick that system then. I think we should forbid that. I agree with the rest of your note that a simple cat > /sys/... should be enough. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong wrote: >> -Original Message- >> From: Matt Fleming [mailto:m...@console-pimps.org] >> Sent: Monday, March 02, 2015 8:30 PM >> >> On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote: >> > > -Original Message- >> > > From: Borislav Petkov [mailto:b...@alien8.de] >> > > Sent: Wednesday, February 25, 2015 8:49 PM >> > > >> > > On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: >> > > > The reason we use this interface for efi capsule is that efi capsule >> > > > support multi binaries to be uploaded and each binary file name >> > > > can be different. >> > > >> > > So you can write the file path to a second file and reload then, once >> > > per file. >> > >> > Thanks for the suggestion. Did some exploration on this approach and >> > would like to continue the discussion together with this suggested design. >> > >> > Ideal condition: >> > - one file note is enough for load binary and status return (capsule_load) >> > - load steps would be as simple as just: >> >echo [binary file name] > capsule_load >> > - status return in the same command action. If failed, the echo will fail >> >with the failing status code. >> > >> > Trade off: >> > - does not have the run time flexibility to load any firmware binaries at >> >different different path as firmware class only support one custom >> >path inputted during boot time/load time. Unless to add new export >> >function for firmware class. >> >> Could you elaborate here? I don't understand this point. > > Just to call out that using firmware class auto locate binary feature is > limited > to locations: > - "/lib/firmware/updates/" UTS_RELEASE, > - "/lib/firmware/updates", > - "/lib/firmware/" UTS_RELEASE, > - "/lib/firmware" > and one custom path which inputted during firmware_class module load > time or kernel boot up time. > > It is just not like the user helper interface which allow load the binary at > any path/location. > > This really is not a big deal. User should cope with it. No, it's a big deal, and the user should not cope. The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Imagine systems using NFS root, Atomic-style systems (e.g. ostree), systems that boot off squashfs, etc. They should still be able to load capsules. The basic user interface that should work is: # uefi-load-capsule /path/to/capsule or: # uefi-load-capsule - http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Mar 03, 2015 at 12:37:54PM -0800, Andy Lutomirski wrote: The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Oh oh but but, if an UEFI capsule can brick the system, a normal user would be able to brick that system then. I think we should forbid that. I agree with the rest of your note that a simple cat fw_blob /sys/... should be enough. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: Matt Fleming [mailto:m...@console-pimps.org] Sent: Monday, March 02, 2015 8:30 PM On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote: -Original Message- From: Borislav Petkov [mailto:b...@alien8.de] Sent: Wednesday, February 25, 2015 8:49 PM On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: The reason we use this interface for efi capsule is that efi capsule support multi binaries to be uploaded and each binary file name can be different. So you can write the file path to a second file and reload then, once per file. Thanks for the suggestion. Did some exploration on this approach and would like to continue the discussion together with this suggested design. Ideal condition: - one file note is enough for load binary and status return (capsule_load) - load steps would be as simple as just: echo [binary file name] capsule_load - status return in the same command action. If failed, the echo will fail with the failing status code. Trade off: - does not have the run time flexibility to load any firmware binaries at different different path as firmware class only support one custom path inputted during boot time/load time. Unless to add new export function for firmware class. Could you elaborate here? I don't understand this point. Just to call out that using firmware class auto locate binary feature is limited to locations: - /lib/firmware/updates/ UTS_RELEASE, - /lib/firmware/updates, - /lib/firmware/ UTS_RELEASE, - /lib/firmware and one custom path which inputted during firmware_class module load time or kernel boot up time. It is just not like the user helper interface which allow load the binary at any path/location. This really is not a big deal. User should cope with it. No, it's a big deal, and the user should not cope. The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Imagine systems using NFS root, Atomic-style systems (e.g. ostree), systems that boot off squashfs, etc. They should still be able to load capsules. The basic user interface that should work is: # uefi-load-capsule /path/to/capsule or: # uefi-load-capsule - /path/to/capsule I don't really care how uefi-load-capsule is implemented, as long as it's straightforward, because people will screw it up if it isn't straightforward. Why is it so hard to have a file in sysfs that you write the capsule to using *cat* (not echo) and that will return an error code if cat fails? Is it because you don't know where the end of the capsule is? if so, ioctl is designed for exactly this purpose. TBH, I find this thread kind of ridiculous. The problem that you're trying to solve is extremely simple, the functionality that userspace needs is trivial, and all of these complex proposals for how it should work are an artifact of the fact that the kernel-internal interfaces you're using for it are not well suited to the problem at hand. --Andy -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mar 3, 2015 12:51 PM, Borislav Petkov b...@alien8.de wrote: On Tue, Mar 03, 2015 at 12:37:54PM -0800, Andy Lutomirski wrote: The user *should not* be required to have write access to anything in /lib to install a UEFI capsule that they download from their motherboard vendor's website. /lib belongs to the distro, and UEFI capsules do not belong to the distro. In this regard, UEFI capsules are completely unlike your wireless card firmware, your cpu microcode, etc. Oh oh but but, if an UEFI capsule can brick the system, a normal user would be able to brick that system then. I think we should forbid that. Absolutely. That's why I said # uefi-load-capsule and not $ uefi-load-capsule :) I agree with the rest of your note that a simple cat fw_blob /sys/... should be enough. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Matt Fleming [mailto:m...@console-pimps.org] > Sent: Monday, March 02, 2015 8:30 PM > > On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote: > > > -Original Message- > > > From: Borislav Petkov [mailto:b...@alien8.de] > > > Sent: Wednesday, February 25, 2015 8:49 PM > > > > > > On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: > > > > The reason we use this interface for efi capsule is that efi capsule > > > > support multi binaries to be uploaded and each binary file name > > > > can be different. > > > > > > So you can write the file path to a second file and reload then, once > > > per file. > > > > Thanks for the suggestion. Did some exploration on this approach and > > would like to continue the discussion together with this suggested design. > > > > Ideal condition: > > - one file note is enough for load binary and status return (capsule_load) > > - load steps would be as simple as just: > >echo [binary file name] > capsule_load > > - status return in the same command action. If failed, the echo will fail > >with the failing status code. > > > > Trade off: > > - does not have the run time flexibility to load any firmware binaries at > >different different path as firmware class only support one custom > >path inputted during boot time/load time. Unless to add new export > >function for firmware class. > > Could you elaborate here? I don't understand this point. Just to call out that using firmware class auto locate binary feature is limited to locations: - "/lib/firmware/updates/" UTS_RELEASE, - "/lib/firmware/updates", - "/lib/firmware/" UTS_RELEASE, - "/lib/firmware" and one custom path which inputted during firmware_class module load time or kernel boot up time. It is just not like the user helper interface which allow load the binary at any path/location. This really is not a big deal. User should cope with it. > > > - if any typo on user level or file path not found, firmware class falls > > back > >to user helper interface. User is required to open another terminal to > >performs: > >-> echo 1 > loading > >-> cat capsule.bin > data > >-> echo 0 > loading > >Or wait for timeout. > > Not if you use request_firmware_direct(), right? > request_firmware_direct() explicitly does not invoke the user helper. > > > - User has the capability to configure the firmware_class time out value. > >If the infinite value is set, the only method to break the infinite > > waiting > >by manually perform "echo -1 > loading" on the other terminal. > > I'm not sure this is a problem if we use request_firmware_direct(). Oops... I miss out this function. Will rework using this function. > > > Are you guys okay with these trade off? > > Or you guys still okay with the previous 2 design idea? > > I quite like the design that Borislav is proposing. Things become a lot > simpler when we stop using request_firmware_nowait(). > > The next question is whether we want to batch passing capsules to the > firmware. The UpdateCapsule() EFI runtime service allows you to chain > capsule blobs together and pass a scatter-gather list. > > If we do want to batch uploading then we need the equivalent of the > 'reload' file from the microcode loader to signal to the kernel that the > userland process has finished uploading capsules, and the kernel can now > send them to the firmware as one chunk. > > Also, Andy previously raised the point about multiple userland processes > passing capsules to the firmware simultaneously and the races that > occur, so we'd need a way to make that atomic. > > I'd much prefer to send one capsule at a time to the firmware, because > it just makes this whole thing simpler. > > Wilson, I'm really looking for your input here with how capsule > uploading works on Quark. If we can just repeatedly call UpdateCapsule() > with one capsule file at a time, that's fine. If we absolutely must > bundle multiple capsule files together then we probably need to provide > some atomicity. > > Thoughts? Quark does not need batch uploading. Even I tested multiple capsules on Quark by doing repeatedly calling UpdateCapsule() is still working for Quark. So, Quark does not require this bundling. > > > > Alternatively, you can combine all the files into a cpio archive like > > > we do with the microcode loader too, and let the kernel parse the cpio > > > archive. > > > > I believe this will make the implementation complicated compare to above. > > Agreed. The capsule files we're sending to the firmware (via this > interface) already contain all the information we need to stitch > multiple binaries together - there's really no reason to bundle things > any further. > > -- > Matt Fleming, Intel Open Source Technology Center Hi Greg, Ming Lei, Sam & Andy, Do you guys have any others concern on Borislav proposed approach? Thanks. Regards, Wilson -- To unsubscribe from this list: send the
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote: > > -Original Message- > > From: Borislav Petkov [mailto:b...@alien8.de] > > Sent: Wednesday, February 25, 2015 8:49 PM > > > > On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: > > > The reason we use this interface for efi capsule is that efi capsule > > > support multi binaries to be uploaded and each binary file name > > > can be different. > > > > So you can write the file path to a second file and reload then, once > > per file. > > Thanks for the suggestion. Did some exploration on this approach and > would like to continue the discussion together with this suggested design. > > Ideal condition: > - one file note is enough for load binary and status return (capsule_load) > - load steps would be as simple as just: >echo [binary file name] > capsule_load > - status return in the same command action. If failed, the echo will fail >with the failing status code. > > Trade off: > - does not have the run time flexibility to load any firmware binaries at >different different path as firmware class only support one custom >path inputted during boot time/load time. Unless to add new export >function for firmware class. Could you elaborate here? I don't understand this point. > - if any typo on user level or file path not found, firmware class falls back >to user helper interface. User is required to open another terminal to >performs: >-> echo 1 > loading >-> cat capsule.bin > data >-> echo 0 > loading >Or wait for timeout. Not if you use request_firmware_direct(), right? request_firmware_direct() explicitly does not invoke the user helper. > - User has the capability to configure the firmware_class time out value. >If the infinite value is set, the only method to break the infinite waiting >by manually perform "echo -1 > loading" on the other terminal. I'm not sure this is a problem if we use request_firmware_direct(). > Are you guys okay with these trade off? > Or you guys still okay with the previous 2 design idea? I quite like the design that Borislav is proposing. Things become a lot simpler when we stop using request_firmware_nowait(). The next question is whether we want to batch passing capsules to the firmware. The UpdateCapsule() EFI runtime service allows you to chain capsule blobs together and pass a scatter-gather list. If we do want to batch uploading then we need the equivalent of the 'reload' file from the microcode loader to signal to the kernel that the userland process has finished uploading capsules, and the kernel can now send them to the firmware as one chunk. Also, Andy previously raised the point about multiple userland processes passing capsules to the firmware simultaneously and the races that occur, so we'd need a way to make that atomic. I'd much prefer to send one capsule at a time to the firmware, because it just makes this whole thing simpler. Wilson, I'm really looking for your input here with how capsule uploading works on Quark. If we can just repeatedly call UpdateCapsule() with one capsule file at a time, that's fine. If we absolutely must bundle multiple capsule files together then we probably need to provide some atomicity. Thoughts? > > Alternatively, you can combine all the files into a cpio archive like > > we do with the microcode loader too, and let the kernel parse the cpio > > archive. > > I believe this will make the implementation complicated compare to above. Agreed. The capsule files we're sending to the firmware (via this interface) already contain all the information we need to stitch multiple binaries together - there's really no reason to bundle things any further. -- Matt Fleming, Intel Open Source Technology Center -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, 26 Feb, at 04:54:58PM, Borislav Petkov wrote: > On Thu, Feb 26, 2015 at 07:30:54AM -0800, Andy Lutomirski wrote: > > How can the error code be propagated? Would that echo command fail in > > case of error? > > Yeah, either that or we can put the error code in the sysfs file which > userspace can cat. FWIW, I'd prefer to make the echo command fail in case of capsule error. -- Matt Fleming, Intel Open Source Technology Center -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Wednesday, February 25, 2015 8:49 PM > > On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: > > The reason we use this interface for efi capsule is that efi capsule > > support multi binaries to be uploaded and each binary file name > > can be different. > > So you can write the file path to a second file and reload then, once > per file. Thanks for the suggestion. Did some exploration on this approach and would like to continue the discussion together with this suggested design. Ideal condition: - one file note is enough for load binary and status return (capsule_load) - load steps would be as simple as just: echo [binary file name] > capsule_load - status return in the same command action. If failed, the echo will fail with the failing status code. Trade off: - does not have the run time flexibility to load any firmware binaries at different different path as firmware class only support one custom path inputted during boot time/load time. Unless to add new export function for firmware class. - if any typo on user level or file path not found, firmware class falls back to user helper interface. User is required to open another terminal to performs: -> echo 1 > loading -> cat capsule.bin > data -> echo 0 > loading Or wait for timeout. - User has the capability to configure the firmware_class time out value. If the infinite value is set, the only method to break the infinite waiting by manually perform "echo -1 > loading" on the other terminal. Are you guys okay with these trade off? Or you guys still okay with the previous 2 design idea? > > Alternatively, you can combine all the files into a cpio archive like > we do with the microcode loader too, and let the kernel parse the cpio > archive. I believe this will make the implementation complicated compare to above. Regards, Wilson
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Matt Fleming [mailto:m...@console-pimps.org] Sent: Monday, March 02, 2015 8:30 PM On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote: -Original Message- From: Borislav Petkov [mailto:b...@alien8.de] Sent: Wednesday, February 25, 2015 8:49 PM On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: The reason we use this interface for efi capsule is that efi capsule support multi binaries to be uploaded and each binary file name can be different. So you can write the file path to a second file and reload then, once per file. Thanks for the suggestion. Did some exploration on this approach and would like to continue the discussion together with this suggested design. Ideal condition: - one file note is enough for load binary and status return (capsule_load) - load steps would be as simple as just: echo [binary file name] capsule_load - status return in the same command action. If failed, the echo will fail with the failing status code. Trade off: - does not have the run time flexibility to load any firmware binaries at different different path as firmware class only support one custom path inputted during boot time/load time. Unless to add new export function for firmware class. Could you elaborate here? I don't understand this point. Just to call out that using firmware class auto locate binary feature is limited to locations: - /lib/firmware/updates/ UTS_RELEASE, - /lib/firmware/updates, - /lib/firmware/ UTS_RELEASE, - /lib/firmware and one custom path which inputted during firmware_class module load time or kernel boot up time. It is just not like the user helper interface which allow load the binary at any path/location. This really is not a big deal. User should cope with it. - if any typo on user level or file path not found, firmware class falls back to user helper interface. User is required to open another terminal to performs: - echo 1 loading - cat capsule.bin data - echo 0 loading Or wait for timeout. Not if you use request_firmware_direct(), right? request_firmware_direct() explicitly does not invoke the user helper. - User has the capability to configure the firmware_class time out value. If the infinite value is set, the only method to break the infinite waiting by manually perform echo -1 loading on the other terminal. I'm not sure this is a problem if we use request_firmware_direct(). Oops... I miss out this function. Will rework using this function. Are you guys okay with these trade off? Or you guys still okay with the previous 2 design idea? I quite like the design that Borislav is proposing. Things become a lot simpler when we stop using request_firmware_nowait(). The next question is whether we want to batch passing capsules to the firmware. The UpdateCapsule() EFI runtime service allows you to chain capsule blobs together and pass a scatter-gather list. If we do want to batch uploading then we need the equivalent of the 'reload' file from the microcode loader to signal to the kernel that the userland process has finished uploading capsules, and the kernel can now send them to the firmware as one chunk. Also, Andy previously raised the point about multiple userland processes passing capsules to the firmware simultaneously and the races that occur, so we'd need a way to make that atomic. I'd much prefer to send one capsule at a time to the firmware, because it just makes this whole thing simpler. Wilson, I'm really looking for your input here with how capsule uploading works on Quark. If we can just repeatedly call UpdateCapsule() with one capsule file at a time, that's fine. If we absolutely must bundle multiple capsule files together then we probably need to provide some atomicity. Thoughts? Quark does not need batch uploading. Even I tested multiple capsules on Quark by doing repeatedly calling UpdateCapsule() is still working for Quark. So, Quark does not require this bundling. Alternatively, you can combine all the files into a cpio archive like we do with the microcode loader too, and let the kernel parse the cpio archive. I believe this will make the implementation complicated compare to above. Agreed. The capsule files we're sending to the firmware (via this interface) already contain all the information we need to stitch multiple binaries together - there's really no reason to bundle things any further. -- Matt Fleming, Intel Open Source Technology Center Hi Greg, Ming Lei, Sam Andy, Do you guys have any others concern on Borislav proposed approach? Thanks. Regards, Wilson -- 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
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote: -Original Message- From: Borislav Petkov [mailto:b...@alien8.de] Sent: Wednesday, February 25, 2015 8:49 PM On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: The reason we use this interface for efi capsule is that efi capsule support multi binaries to be uploaded and each binary file name can be different. So you can write the file path to a second file and reload then, once per file. Thanks for the suggestion. Did some exploration on this approach and would like to continue the discussion together with this suggested design. Ideal condition: - one file note is enough for load binary and status return (capsule_load) - load steps would be as simple as just: echo [binary file name] capsule_load - status return in the same command action. If failed, the echo will fail with the failing status code. Trade off: - does not have the run time flexibility to load any firmware binaries at different different path as firmware class only support one custom path inputted during boot time/load time. Unless to add new export function for firmware class. Could you elaborate here? I don't understand this point. - if any typo on user level or file path not found, firmware class falls back to user helper interface. User is required to open another terminal to performs: - echo 1 loading - cat capsule.bin data - echo 0 loading Or wait for timeout. Not if you use request_firmware_direct(), right? request_firmware_direct() explicitly does not invoke the user helper. - User has the capability to configure the firmware_class time out value. If the infinite value is set, the only method to break the infinite waiting by manually perform echo -1 loading on the other terminal. I'm not sure this is a problem if we use request_firmware_direct(). Are you guys okay with these trade off? Or you guys still okay with the previous 2 design idea? I quite like the design that Borislav is proposing. Things become a lot simpler when we stop using request_firmware_nowait(). The next question is whether we want to batch passing capsules to the firmware. The UpdateCapsule() EFI runtime service allows you to chain capsule blobs together and pass a scatter-gather list. If we do want to batch uploading then we need the equivalent of the 'reload' file from the microcode loader to signal to the kernel that the userland process has finished uploading capsules, and the kernel can now send them to the firmware as one chunk. Also, Andy previously raised the point about multiple userland processes passing capsules to the firmware simultaneously and the races that occur, so we'd need a way to make that atomic. I'd much prefer to send one capsule at a time to the firmware, because it just makes this whole thing simpler. Wilson, I'm really looking for your input here with how capsule uploading works on Quark. If we can just repeatedly call UpdateCapsule() with one capsule file at a time, that's fine. If we absolutely must bundle multiple capsule files together then we probably need to provide some atomicity. Thoughts? Alternatively, you can combine all the files into a cpio archive like we do with the microcode loader too, and let the kernel parse the cpio archive. I believe this will make the implementation complicated compare to above. Agreed. The capsule files we're sending to the firmware (via this interface) already contain all the information we need to stitch multiple binaries together - there's really no reason to bundle things any further. -- Matt Fleming, Intel Open Source Technology Center -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Borislav Petkov [mailto:b...@alien8.de] Sent: Wednesday, February 25, 2015 8:49 PM On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: The reason we use this interface for efi capsule is that efi capsule support multi binaries to be uploaded and each binary file name can be different. So you can write the file path to a second file and reload then, once per file. Thanks for the suggestion. Did some exploration on this approach and would like to continue the discussion together with this suggested design. Ideal condition: - one file note is enough for load binary and status return (capsule_load) - load steps would be as simple as just: echo [binary file name] capsule_load - status return in the same command action. If failed, the echo will fail with the failing status code. Trade off: - does not have the run time flexibility to load any firmware binaries at different different path as firmware class only support one custom path inputted during boot time/load time. Unless to add new export function for firmware class. - if any typo on user level or file path not found, firmware class falls back to user helper interface. User is required to open another terminal to performs: - echo 1 loading - cat capsule.bin data - echo 0 loading Or wait for timeout. - User has the capability to configure the firmware_class time out value. If the infinite value is set, the only method to break the infinite waiting by manually perform echo -1 loading on the other terminal. Are you guys okay with these trade off? Or you guys still okay with the previous 2 design idea? Alternatively, you can combine all the files into a cpio archive like we do with the microcode loader too, and let the kernel parse the cpio archive. I believe this will make the implementation complicated compare to above. Regards, Wilson
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, 26 Feb, at 04:54:58PM, Borislav Petkov wrote: On Thu, Feb 26, 2015 at 07:30:54AM -0800, Andy Lutomirski wrote: How can the error code be propagated? Would that echo command fail in case of error? Yeah, either that or we can put the error code in the sysfs file which userspace can cat. FWIW, I'd prefer to make the echo command fail in case of capsule error. -- Matt Fleming, Intel Open Source Technology Center -- 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/
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message-
> From: Roy Franz [mailto:roy.fr...@linaro.org]
> Sent: Friday, February 27, 2015 1:07 PM
>
> On Sun, Nov 2, 2014 at 7:07 PM, Kweh Hock Leong
> > +/*
> > + * This function will store the capsule binary and pass it to
> > + * efi_capsule_update() API in capsule.c */ static int
> > +efi_capsule_store(const struct firmware *fw) {
> > + int i;
> > + int ret;
> > + int count = fw->size;
> > + int copy_size = (fw->size > PAGE_SIZE) ? PAGE_SIZE : fw->size;
> > + int pages_needed = ALIGN(fw->size, PAGE_SIZE) >> PAGE_SHIFT;
> > + struct page **pages;
> > + void *page_data;
> > + efi_capsule_header_t *capsule = NULL;
> > +
> > + pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
> > + if (!pages) {
> > + pr_err("%s: kmalloc_array() failed\n", __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + for (i = 0; i < pages_needed; i++) {
> > + pages[i] = alloc_page(GFP_KERNEL);
> > + if (!pages[i]) {
> > + pr_err("%s: alloc_page() failed\n", __func__);
> > + --i;
> > + ret = -ENOMEM;
> > + goto failed;
> > + }
> > + }
> > +
> > + for (i = 0; i < pages_needed; i++) {
> > + page_data = kmap(pages[i]);
> > + memcpy(page_data, fw->data + (i * PAGE_SIZE),
> > + copy_size);
> > +
> > + if (i == 0)
> > + capsule = (efi_capsule_header_t *)page_data;
> > + else
> > + kunmap(pages[i]);
> > +
> > + count -= copy_size;
> > + if (count < PAGE_SIZE)
> > + copy_size = count;
> > + }
> > +
> > + ret = efi_capsule_update(capsule, pages);
> > + if (ret) {
> > + pr_err("%s: efi_capsule_update() failed\n", __func__);
> > + --i;
>
> Hi Hock,
>
> What does the decrement of i do here? I looked at
> efi_capsule_update() and didn't see anything that would account for this. It
> looks like in this failure case one page won't get freed.
>
> As an aside, when I was looking at efi_update_capsule, I see that Matt is
> doing very similar operations (array of struct page pointers), but does it
> like
> this (snipped from his patch):
>
> + struct page **block_pgs;
> ...
> + block_pgs = kzalloc(nr_block_pgs * sizeof(*block_pgs), GFP_KERNEL);
> + if (!block_pgs)
> + return -ENOMEM;
> +
> + for (i = 0; i < nr_block_pgs; i++) {
> + block_pgs[i] = alloc_page(GFP_KERNEL);
> + if (!block_pgs[i])
> + goto fail;
> + }
>
> and then can simply free the pages that are not NULL:
> +fail:
> + for (i = 0; i < nr_block_pgs; i++) {
> + if (block_pgs[i])
> + __free_page(block_pgs[i]);
> + }
>
> I think this way is preferable since it doesn't rely on 'i' being unchanged
> at the
> end of the function. I also think it would be nice if the capsule code stuck
> with one idiom for dealing with arrays of page pointers.
>
> Roy
>
Hi Roy,
The reason "i" there have to perform a decrement is because a full for loop
will end with one extra incremented value if it does not break out in the
middle.
And the efi_capsule_store()'s alloc page is to store the binary content and the
efi_capsule_update() from Matt is to store the efi capsule block descriptor
which will point to the binary blocks content. So, they are different.
Regards,
Wilson
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message-
From: Roy Franz [mailto:roy.fr...@linaro.org]
Sent: Friday, February 27, 2015 1:07 PM
On Sun, Nov 2, 2014 at 7:07 PM, Kweh Hock Leong
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c */ static int
+efi_capsule_store(const struct firmware *fw) {
+ int i;
+ int ret;
+ int count = fw-size;
+ int copy_size = (fw-size PAGE_SIZE) ? PAGE_SIZE : fw-size;
+ int pages_needed = ALIGN(fw-size, PAGE_SIZE) PAGE_SHIFT;
+ struct page **pages;
+ void *page_data;
+ efi_capsule_header_t *capsule = NULL;
+
+ pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
+ if (!pages) {
+ pr_err(%s: kmalloc_array() failed\n, __func__);
+ return -ENOMEM;
+ }
+
+ for (i = 0; i pages_needed; i++) {
+ pages[i] = alloc_page(GFP_KERNEL);
+ if (!pages[i]) {
+ pr_err(%s: alloc_page() failed\n, __func__);
+ --i;
+ ret = -ENOMEM;
+ goto failed;
+ }
+ }
+
+ for (i = 0; i pages_needed; i++) {
+ page_data = kmap(pages[i]);
+ memcpy(page_data, fw-data + (i * PAGE_SIZE),
+ copy_size);
+
+ if (i == 0)
+ capsule = (efi_capsule_header_t *)page_data;
+ else
+ kunmap(pages[i]);
+
+ count -= copy_size;
+ if (count PAGE_SIZE)
+ copy_size = count;
+ }
+
+ ret = efi_capsule_update(capsule, pages);
+ if (ret) {
+ pr_err(%s: efi_capsule_update() failed\n, __func__);
+ --i;
Hi Hock,
What does the decrement of i do here? I looked at
efi_capsule_update() and didn't see anything that would account for this. It
looks like in this failure case one page won't get freed.
As an aside, when I was looking at efi_update_capsule, I see that Matt is
doing very similar operations (array of struct page pointers), but does it
like
this (snipped from his patch):
+ struct page **block_pgs;
...
+ block_pgs = kzalloc(nr_block_pgs * sizeof(*block_pgs), GFP_KERNEL);
+ if (!block_pgs)
+ return -ENOMEM;
+
+ for (i = 0; i nr_block_pgs; i++) {
+ block_pgs[i] = alloc_page(GFP_KERNEL);
+ if (!block_pgs[i])
+ goto fail;
+ }
and then can simply free the pages that are not NULL:
+fail:
+ for (i = 0; i nr_block_pgs; i++) {
+ if (block_pgs[i])
+ __free_page(block_pgs[i]);
+ }
I think this way is preferable since it doesn't rely on 'i' being unchanged
at the
end of the function. I also think it would be nice if the capsule code stuck
with one idiom for dealing with arrays of page pointers.
Roy
Hi Roy,
The reason i there have to perform a decrement is because a full for loop
will end with one extra incremented value if it does not break out in the
middle.
And the efi_capsule_store()'s alloc page is to store the binary content and the
efi_capsule_update() from Matt is to store the efi capsule block descriptor
which will point to the binary blocks content. So, they are different.
Regards,
Wilson
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Sun, Nov 2, 2014 at 7:07 PM, Kweh Hock Leong
wrote:
> From: "Kweh, Hock Leong"
>
> Introducing a kernel module to expose user helper interface for
> user to upload capsule binaries. This module leverage the
> request_firmware_nowait() to expose an interface to user.
>
> Example steps to load the capsule binary:
> 1.) echo 1 > /sys/class/firmware/efi-capsule-file/loading
> 2.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data
> 3.) echo 0 > /sys/class/firmware/efi-capsule-file/loading
>
> Whereas, this module does not allow the capsule binaries to be
> obtained from the request_firmware library search path. If any
> capsule binary loaded from firmware seach path, the module will
> stop functioning.
>
> Besides the request_firmware user helper interface, this module
> also expose a 'capsule_loaded' file note for user to verify
> the number of successfully uploaded capsule binaries. This
> file note has the read only attribute.
>
> Cc: Matt Fleming
> Signed-off-by: Kweh, Hock Leong
> ---
> drivers/firmware/efi/Kconfig | 13 ++
> drivers/firmware/efi/Makefile |1 +
> drivers/firmware/efi/efi-capsule-user-helper.c | 246
>
> 3 files changed, 260 insertions(+)
> create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index f712d47..7dc814e 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
> config EFI_ARMSTUB
> bool
>
> +config EFI_CAPSULE_USER_HELPER
> + tristate "EFI capsule user mode helper"
> + depends on EFI
> + select FW_LOADER
> + select FW_LOADER_USER_HELPER
> + help
> + This option exposes the user mode helper interface for user to load
> + EFI capsule binary and update the EFI firmware after system reboot.
> + This feature does not support auto locating capsule binaries at the
> + firmware lib search path.
> +
> + If unsure, say N.
> +
> endmenu
>
> config UEFI_CPER
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 698846e..63f6910 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
> obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
> obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
> obj-$(CONFIG_EFI_STUB) += libstub/
> +obj-$(CONFIG_EFI_CAPSULE_USER_HELPER) += efi-capsule-user-helper.o
> diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c
> b/drivers/firmware/efi/efi-capsule-user-helper.c
> new file mode 100644
> index 000..84a1628
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-user-helper.c
> @@ -0,0 +1,246 @@
> +/*
> + * EFI capsule user mode helper interface driver.
> + *
> + * Copyright 2014 Intel Corporation
> + *
> + * This file is part of the Linux kernel, and is made available under
> + * the terms of the GNU General Public License version 2.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define CAPSULE_NAME "efi-capsule-file"
> +#define DEV_NAME "efi_capsule_user_helper"
> +#define STRING_INTEGER_MAX_LENGTH 13
> +
> +static DEFINE_MUTEX(user_helper_lock);
> +static int capsule_count;
> +static int stop_request;
> +static struct platform_device *efi_capsule_pdev;
> +
> +/*
> + * This function will store the capsule binary and pass it to
> + * efi_capsule_update() API in capsule.c
> + */
> +static int efi_capsule_store(const struct firmware *fw)
> +{
> + int i;
> + int ret;
> + int count = fw->size;
> + int copy_size = (fw->size > PAGE_SIZE) ? PAGE_SIZE : fw->size;
> + int pages_needed = ALIGN(fw->size, PAGE_SIZE) >> PAGE_SHIFT;
> + struct page **pages;
> + void *page_data;
> + efi_capsule_header_t *capsule = NULL;
> +
> + pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
> + if (!pages) {
> + pr_err("%s: kmalloc_array() failed\n", __func__);
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < pages_needed; i++) {
> + pages[i] = alloc_page(GFP_KERNEL);
> + if (!pages[i]) {
> + pr_err("%s: alloc_page() failed\n", __func__);
> + --i;
> + ret = -ENOMEM;
> + goto failed;
> + }
> + }
> +
> + for (i = 0; i < pages_needed; i++) {
> + page_data = kmap(pages[i]);
> + memcpy(page_data, fw->data + (i * PAGE_SIZE), copy_size);
> +
> + if (i == 0)
> + capsule = (efi_capsule_header_t *)page_data;
> + else
>
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, Feb 26, 2015 at 07:30:54AM -0800, Andy Lutomirski wrote: > How can the error code be propagated? Would that echo command fail in > case of error? Yeah, either that or we can put the error code in the sysfs file which userspace can cat. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Wed, Feb 25, 2015 at 3:47 AM, Borislav Petkov wrote: > On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: >> So the process steps basically look like this: >> 1.) cat capsule_ticket===> acquire a number and lock mutex then >> expose >> firmware_class user helper >> >> interface as well as start timer for timeout >> counting >> 2.) repeat step 1 if obtained a "0" number >> 3.) echo 1 > loading >> 4.) cat bin > data >> 5.) echo 0 > loading===> stop the timeout counting then unlock >> mutex >> at the end of callback routine >> 6.) cat capsule_report ===> grep the number acquired from beginning >> for >> checking succeeded/failed > > So this sounds pretty overengineered for no reason, or maybe I'm missing > the reason. > > If I had to give an example from the microcode loader, what we do there > is put the microcode in /lib/firmware/... and do > > echo 1 > /sys/devices/system/cpu/microcode/reload > > which goes and calls reload_store() in > arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU > hotplug, etc, etc... > > And this mechanism is as simple as it can get. Maybe capsules can be > loaded like that too? > > Error code can be propagated too, if needed, of course. How can the error code be propagated? Would that echo command fail in case of error? --Andy -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Thu, Feb 26, 2015 at 07:30:54AM -0800, Andy Lutomirski wrote: How can the error code be propagated? Would that echo command fail in case of error? Yeah, either that or we can put the error code in the sysfs file which userspace can cat. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Wed, Feb 25, 2015 at 3:47 AM, Borislav Petkov b...@alien8.de wrote: On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: So the process steps basically look like this: 1.) cat capsule_ticket=== acquire a number and lock mutex then expose firmware_class user helper interface as well as start timer for timeout counting 2.) repeat step 1 if obtained a 0 number 3.) echo 1 loading 4.) cat bin data 5.) echo 0 loading=== stop the timeout counting then unlock mutex at the end of callback routine 6.) cat capsule_report === grep the number acquired from beginning for checking succeeded/failed So this sounds pretty overengineered for no reason, or maybe I'm missing the reason. If I had to give an example from the microcode loader, what we do there is put the microcode in /lib/firmware/... and do echo 1 /sys/devices/system/cpu/microcode/reload which goes and calls reload_store() in arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU hotplug, etc, etc... And this mechanism is as simple as it can get. Maybe capsules can be loaded like that too? Error code can be propagated too, if needed, of course. How can the error code be propagated? Would that echo command fail in case of error? --Andy -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Sun, Nov 2, 2014 at 7:07 PM, Kweh Hock Leong
hock.leong.k...@intel.com wrote:
From: Kweh, Hock Leong hock.leong.k...@intel.com
Introducing a kernel module to expose user helper interface for
user to upload capsule binaries. This module leverage the
request_firmware_nowait() to expose an interface to user.
Example steps to load the capsule binary:
1.) echo 1 /sys/class/firmware/efi-capsule-file/loading
2.) cat capsule.bin /sys/class/firmware/efi-capsule-file/data
3.) echo 0 /sys/class/firmware/efi-capsule-file/loading
Whereas, this module does not allow the capsule binaries to be
obtained from the request_firmware library search path. If any
capsule binary loaded from firmware seach path, the module will
stop functioning.
Besides the request_firmware user helper interface, this module
also expose a 'capsule_loaded' file note for user to verify
the number of successfully uploaded capsule binaries. This
file note has the read only attribute.
Cc: Matt Fleming matt.flem...@intel.com
Signed-off-by: Kweh, Hock Leong hock.leong.k...@intel.com
---
drivers/firmware/efi/Kconfig | 13 ++
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/efi-capsule-user-helper.c | 246
3 files changed, 260 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..7dc814e 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_USER_HELPER
+ tristate EFI capsule user mode helper
+ depends on EFI
+ select FW_LOADER
+ select FW_LOADER_USER_HELPER
+ help
+ This option exposes the user mode helper interface for user to load
+ EFI capsule binary and update the EFI firmware after system reboot.
+ This feature does not support auto locating capsule binaries at the
+ firmware lib search path.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..63f6910 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_USER_HELPER) += efi-capsule-user-helper.o
diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c
b/drivers/firmware/efi/efi-capsule-user-helper.c
new file mode 100644
index 000..84a1628
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-user-helper.c
@@ -0,0 +1,246 @@
+/*
+ * EFI capsule user mode helper interface driver.
+ *
+ * Copyright 2014 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME : fmt
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/delay.h
+#include linux/highmem.h
+#include linux/slab.h
+#include linux/mutex.h
+#include linux/reboot.h
+#include linux/efi.h
+#include linux/firmware.h
+
+#define CAPSULE_NAME efi-capsule-file
+#define DEV_NAME efi_capsule_user_helper
+#define STRING_INTEGER_MAX_LENGTH 13
+
+static DEFINE_MUTEX(user_helper_lock);
+static int capsule_count;
+static int stop_request;
+static struct platform_device *efi_capsule_pdev;
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c
+ */
+static int efi_capsule_store(const struct firmware *fw)
+{
+ int i;
+ int ret;
+ int count = fw-size;
+ int copy_size = (fw-size PAGE_SIZE) ? PAGE_SIZE : fw-size;
+ int pages_needed = ALIGN(fw-size, PAGE_SIZE) PAGE_SHIFT;
+ struct page **pages;
+ void *page_data;
+ efi_capsule_header_t *capsule = NULL;
+
+ pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
+ if (!pages) {
+ pr_err(%s: kmalloc_array() failed\n, __func__);
+ return -ENOMEM;
+ }
+
+ for (i = 0; i pages_needed; i++) {
+ pages[i] = alloc_page(GFP_KERNEL);
+ if (!pages[i]) {
+ pr_err(%s: alloc_page() failed\n, __func__);
+ --i;
+ ret = -ENOMEM;
+ goto failed;
+ }
+ }
+
+ for (i = 0; i pages_needed; i++) {
+ page_data = kmap(pages[i]);
+ memcpy(page_data, fw-data + (i * PAGE_SIZE), copy_size);
+
+ if (i == 0)
+
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: > The reason we use this interface for efi capsule is that efi capsule > support multi binaries to be uploaded and each binary file name > can be different. So you can write the file path to a second file and reload then, once per file. Alternatively, you can combine all the files into a cpio archive like we do with the microcode loader too, and let the kernel parse the cpio archive. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Wednesday, February 25, 2015 7:48 PM > > On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: > > So this sounds pretty overengineered for no reason, or maybe I'm missing > the reason. > > If I had to give an example from the microcode loader, what we do there > is put the microcode in /lib/firmware/... and do > > echo 1 > /sys/devices/system/cpu/microcode/reload > > which goes and calls reload_store() in > arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU > hotplug, etc, etc... > > And this mechanism is as simple as it can get. Maybe capsules can be > loaded like that too? > > Error code can be propagated too, if needed, of course. > > -- > Regards/Gruss, > Boris. > > ECO tip #101: Trim your mails when you reply. > -- Hi Boris, The reason we use this interface for efi capsule is that efi capsule support multi binaries to be uploaded and each binary file name can be different. Regards, Wilson
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: > So the process steps basically look like this: > 1.) cat capsule_ticket===> acquire a number and lock mutex then > expose > firmware_class user helper > > interface as well as start timer for timeout > counting > 2.) repeat step 1 if obtained a "0" number > 3.) echo 1 > loading > 4.) cat bin > data > 5.) echo 0 > loading===> stop the timeout counting then unlock > mutex > at the end of callback routine > 6.) cat capsule_report ===> grep the number acquired from beginning > for > checking succeeded/failed So this sounds pretty overengineered for no reason, or maybe I'm missing the reason. If I had to give an example from the microcode loader, what we do there is put the microcode in /lib/firmware/... and do echo 1 > /sys/devices/system/cpu/microcode/reload which goes and calls reload_store() in arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU hotplug, etc, etc... And this mechanism is as simple as it can get. Maybe capsules can be loaded like that too? Error code can be propagated too, if needed, of course. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Borislav Petkov [mailto:b...@alien8.de] Sent: Wednesday, February 25, 2015 7:48 PM On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: So this sounds pretty overengineered for no reason, or maybe I'm missing the reason. If I had to give an example from the microcode loader, what we do there is put the microcode in /lib/firmware/... and do echo 1 /sys/devices/system/cpu/microcode/reload which goes and calls reload_store() in arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU hotplug, etc, etc... And this mechanism is as simple as it can get. Maybe capsules can be loaded like that too? Error code can be propagated too, if needed, of course. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- Hi Boris, The reason we use this interface for efi capsule is that efi capsule support multi binaries to be uploaded and each binary file name can be different. Regards, Wilson
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Feb 24, 2015 at 12:49:09PM +, Kweh, Hock Leong wrote: So the process steps basically look like this: 1.) cat capsule_ticket=== acquire a number and lock mutex then expose firmware_class user helper interface as well as start timer for timeout counting 2.) repeat step 1 if obtained a 0 number 3.) echo 1 loading 4.) cat bin data 5.) echo 0 loading=== stop the timeout counting then unlock mutex at the end of callback routine 6.) cat capsule_report === grep the number acquired from beginning for checking succeeded/failed So this sounds pretty overengineered for no reason, or maybe I'm missing the reason. If I had to give an example from the microcode loader, what we do there is put the microcode in /lib/firmware/... and do echo 1 /sys/devices/system/cpu/microcode/reload which goes and calls reload_store() in arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU hotplug, etc, etc... And this mechanism is as simple as it can get. Maybe capsules can be loaded like that too? Error code can be propagated too, if needed, of course. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
Re: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: The reason we use this interface for efi capsule is that efi capsule support multi binaries to be uploaded and each binary file name can be different. So you can write the file path to a second file and reload then, once per file. Alternatively, you can combine all the files into a cpio archive like we do with the microcode loader too, and let the kernel parse the cpio archive. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- 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/
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Kweh, Hock Leong > Sent: Tuesday, February 24, 2015 6:54 PM > > In callbackfn_efi_capsule, you call request_firmware_nowait. When that > callback is invoked, I think that the /sys/class/firmware/efi-capsule-file > directory doesn't exist at all. > If the callback takes longer than it takes your script to make it through a > full > iteration, then it will try uploading the second capsule before the firmware > class directory is there, so it will fail. > > But I just realized that your script has a loop below to handle that. > It's this: > > oldtime=$(date +%S) > oldtime=$(((time + 2) % 60)) > until [ -f /sys/class/firmware/efi-capsule-file/loading ] > do > newtime=$(date +%S) > if [ $newtime -eq $oldtime ] > then > break > fi > done > > Aside from the fact that this loop itself is racy (it may loop forever if > something goes wrong in the kernel, since $newtime -eq $oldtime may > never happen), it should help, if you're lucky. But there's another bug. > > > Here's the race: > > User: > echo 1 > /sys/class/firmware/efi-capsule-file/loading > cat capsule1 > /sys/class/firmware/efi-capsule-file/data > echo 0 > /sys/class/firmware/efi-capsule-file/loading > Kernel: Be a little slow here due to preemption or whatever. > > User: > -f /sys/class/firmware/efi-capsule-file/loading returns true capsules_loaded > == 0 Assume failure, incorrectly > > Kernel: catch up and increment capsules_loaded. > > If these patches get applied, then I think that the protocol needs to be > documented in Documentation/ABI. It should say something like: > > To upload an EFI capsule, do this: > > Write 1 to /sys/class/firmware/efi-capsule-file/loading > Write the capsule to /sys/class/firmware/efi-capsule-file/data > Write 0 to /sys/class/firmware/efi-capsule-file/loading > > Make sure that /sys/class/firmware/efi-capsule-file disappears and comes > back, perhaps by cd-ing there and waiting for all the files in the directory > to > go away. > > Then, and only then, read capsules_loaded to detect success. > > > Once you've written that doc, please seriously consider whether this > interface is justifiable. I think it sucks. > > --Andy Hi All, After some internal discussion and re-design prototyping & testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. This new idea will expose 2 read-only file notes from the efi capsule kernel module - "capsule_ticket" and "capsule_report". The "capsule_ticket" will let user to obtain a NON-ZERO number and then perform a mutex lock. The obtained number will be used later for "capsule_report" status checking. Unlock mutex is done after "echo 0 > loading" at the end of callback function. So the process steps basically look like this: 1.) cat capsule_ticket===> acquire a number and lock mutex then expose firmware_class user helper interface as well as start timer for timeout counting 2.) repeat step 1 if obtained a "0" number 3.) echo 1 > loading 4.) cat bin > data 5.) echo 0 > loading===> stop the timeout counting then unlock mutex at the end of callback routine 6.) cat capsule_report ===> grep the number acquired from beginning for checking succeeded/failed The ticket numbering is starting from 1 to 999 and then rolls over from 1 to 999 again. The "capsule_report" will show the whole list of the acquired entries and will be limited to 128 entries which is capped within one page buffer. The format of this "capsule_report" output will look like: "Capsule $num uploads successful/failed/aborted" There is a 2mins time out (internal) for each of the number acquired. After that, the interface will be closed/disappeared. I have attached a SAMPLE script and kernel module code in case you are not able to understand my description above. I believe this would really take care of the multi-capsule update concurrently issue and also asynchronous status reporting issue. Besides, this will indirectly take care the module unload issue with "rmmod" and not "rmmod -f". What do you guys think? -- There is another idea during internal discussion. The 2nd idea would require some changes to drivers/base/firmware_class.c user helper interface for the mutex
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Kweh, Hock Leong Sent: Tuesday, February 24, 2015 6:54 PM In callbackfn_efi_capsule, you call request_firmware_nowait. When that callback is invoked, I think that the /sys/class/firmware/efi-capsule-file directory doesn't exist at all. If the callback takes longer than it takes your script to make it through a full iteration, then it will try uploading the second capsule before the firmware class directory is there, so it will fail. But I just realized that your script has a loop below to handle that. It's this: oldtime=$(date +%S) oldtime=$(((time + 2) % 60)) until [ -f /sys/class/firmware/efi-capsule-file/loading ] do newtime=$(date +%S) if [ $newtime -eq $oldtime ] then break fi done Aside from the fact that this loop itself is racy (it may loop forever if something goes wrong in the kernel, since $newtime -eq $oldtime may never happen), it should help, if you're lucky. But there's another bug. Here's the race: User: echo 1 /sys/class/firmware/efi-capsule-file/loading cat capsule1 /sys/class/firmware/efi-capsule-file/data echo 0 /sys/class/firmware/efi-capsule-file/loading Kernel: Be a little slow here due to preemption or whatever. User: -f /sys/class/firmware/efi-capsule-file/loading returns true capsules_loaded == 0 Assume failure, incorrectly Kernel: catch up and increment capsules_loaded. If these patches get applied, then I think that the protocol needs to be documented in Documentation/ABI. It should say something like: To upload an EFI capsule, do this: Write 1 to /sys/class/firmware/efi-capsule-file/loading Write the capsule to /sys/class/firmware/efi-capsule-file/data Write 0 to /sys/class/firmware/efi-capsule-file/loading Make sure that /sys/class/firmware/efi-capsule-file disappears and comes back, perhaps by cd-ing there and waiting for all the files in the directory to go away. Then, and only then, read capsules_loaded to detect success. Once you've written that doc, please seriously consider whether this interface is justifiable. I think it sucks. --Andy Hi All, After some internal discussion and re-design prototyping testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. This new idea will expose 2 read-only file notes from the efi capsule kernel module - capsule_ticket and capsule_report. The capsule_ticket will let user to obtain a NON-ZERO number and then perform a mutex lock. The obtained number will be used later for capsule_report status checking. Unlock mutex is done after echo 0 loading at the end of callback function. So the process steps basically look like this: 1.) cat capsule_ticket=== acquire a number and lock mutex then expose firmware_class user helper interface as well as start timer for timeout counting 2.) repeat step 1 if obtained a 0 number 3.) echo 1 loading 4.) cat bin data 5.) echo 0 loading=== stop the timeout counting then unlock mutex at the end of callback routine 6.) cat capsule_report === grep the number acquired from beginning for checking succeeded/failed The ticket numbering is starting from 1 to 999 and then rolls over from 1 to 999 again. The capsule_report will show the whole list of the acquired entries and will be limited to 128 entries which is capped within one page buffer. The format of this capsule_report output will look like: Capsule $num uploads successful/failed/aborted There is a 2mins time out (internal) for each of the number acquired. After that, the interface will be closed/disappeared. I have attached a SAMPLE script and kernel module code in case you are not able to understand my description above. I believe this would really take care of the multi-capsule update concurrently issue and also asynchronous status reporting issue. Besides, this will indirectly take care the module unload issue with rmmod and not rmmod -f. What do you guys think? -- There is another idea during internal discussion. The 2nd idea would require some changes to drivers/base/firmware_class.c user helper interface for the mutex locking as well as status return. Mutex lock will be performed while doing the 'echo 1 loading'
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Nov 10, 2014 at 12:31 AM, Kweh, Hock Leong wrote: >> -Original Message- >> From: Andy Lutomirski [mailto:l...@amacapital.net] >> > #!/bin/sh >> > >> > old=$(cat >> > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) >> > >> > for arg in "$@" >> > do >> > if [ -f $arg ] >> > then >> > echo 1 > /sys/class/firmware/efi-capsule-file/loading >> > cat $arg > /sys/class/firmware/efi-capsule-file/data >> > echo 0 > /sys/class/firmware/efi-capsule-file/loading >> >> I think you have a race. Try putting msleep(1000) after the >> request_firmware_nowait call, and I bet this will fail on the second try. > > Sorry for the late response. I don't really catch the race condition that > you are referring? Are you trying to tell that the user script could run > faster > before the previous callback function actually end? Will such scenario happen? > In the callback function, after the request_firmware_nowait(), I don't have > any codes will delay the callback function to end. Besides, there is a > mutex_lock > protecting the request_firmware_nowait() calling. Won't that take care of the > issue? In callbackfn_efi_capsule, you call request_firmware_nowait. When that callback is invoked, I think that the /sys/class/firmware/efi-capsule-file directory doesn't exist at all. If the callback takes longer than it takes your script to make it through a full iteration, then it will try uploading the second capsule before the firmware class directory is there, so it will fail. But I just realized that your script has a loop below to handle that. It's this: oldtime=$(date +%S) oldtime=$(((time + 2) % 60)) until [ -f /sys/class/firmware/efi-capsule-file/loading ] do newtime=$(date +%S) if [ $newtime -eq $oldtime ] then break fi done Aside from the fact that this loop itself is racy (it may loop forever if something goes wrong in the kernel, since $newtime -eq $oldtime may never happen), it should help, if you're lucky. But there's another bug. >> >> I think that firmware_class doesn't call the callback until after loading is >> closed >> for the second time. If so, then this is racy. Try inserting msleep(1000) >> at the >> beginning of your callback and uploading a capsule that should load >> successfully -- this will report failure, but a future upload may get very >> confused. Also, what does the firmware class do when simultaneous >> uploads of the same file with different contents are in flight? Is that >> possible? > > Sorry again, I can't really catch you on this race condition statement. Are > you > trying to tell if user is doing this: > > echo 1 > /sys/class/firmware/efi-capsule-file/loading > cat capsule1 > /sys/class/firmware/efi-capsule-file/data > cat capsule2 > /sys/class/firmware/efi-capsule-file/data > echo 0 > /sys/class/firmware/efi-capsule-file/loading > > If so, capsule2 will be the one we will obtain in the callback function. Here's the race: User: echo 1 > /sys/class/firmware/efi-capsule-file/loading cat capsule1 > /sys/class/firmware/efi-capsule-file/data echo 0 > /sys/class/firmware/efi-capsule-file/loading Kernel: Be a little slow here due to preemption or whatever. User: -f /sys/class/firmware/efi-capsule-file/loading returns true capsules_loaded == 0 Assume failure, incorrectly Kernel: catch up and increment capsules_loaded. If these patches get applied, then I think that the protocol needs to be documented in Documentation/ABI. It should say something like: To upload an EFI capsule, do this: Write 1 to /sys/class/firmware/efi-capsule-file/loading Write the capsule to /sys/class/firmware/efi-capsule-file/data Write 0 to /sys/class/firmware/efi-capsule-file/loading Make sure that /sys/class/firmware/efi-capsule-file disappears and comes back, perhaps by cd-ing there and waiting for all the files in the directory to go away. Then, and only then, read capsules_loaded to detect success. Once you've written that doc, please seriously consider whether this interface is justifiable. I think it sucks. --Andy -- 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/
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > > #!/bin/sh > > > > old=$(cat > > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) > > > > for arg in "$@" > > do > > if [ -f $arg ] > > then > > echo 1 > /sys/class/firmware/efi-capsule-file/loading > > cat $arg > /sys/class/firmware/efi-capsule-file/data > > echo 0 > /sys/class/firmware/efi-capsule-file/loading > > I think you have a race. Try putting msleep(1000) after the > request_firmware_nowait call, and I bet this will fail on the second try. Sorry for the late response. I don't really catch the race condition that you are referring? Are you trying to tell that the user script could run faster before the previous callback function actually end? Will such scenario happen? In the callback function, after the request_firmware_nowait(), I don't have any codes will delay the callback function to end. Besides, there is a mutex_lock protecting the request_firmware_nowait() calling. Won't that take care of the issue? > > > > > oldtime=$(date +%S) > > oldtime=$(((time + 2) % 60)) > > until [ -f /sys/class/firmware/efi-capsule-file/loading ] > > do > > newtime=$(date +%S) > > if [ $newtime -eq $oldtime ] > > then > > break > > fi > > done > > > > old=$((old + 1)) > > new=$(cat > > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) > > I think that firmware_class doesn't call the callback until after loading is > closed > for the second time. If so, then this is racy. Try inserting msleep(1000) > at the > beginning of your callback and uploading a capsule that should load > successfully -- this will report failure, but a future upload may get very > confused. Also, what does the firmware class do when simultaneous > uploads of the same file with different contents are in flight? Is that > possible? Sorry again, I can't really catch you on this race condition statement. Are you trying to tell if user is doing this: echo 1 > /sys/class/firmware/efi-capsule-file/loading cat capsule1 > /sys/class/firmware/efi-capsule-file/data cat capsule2 > /sys/class/firmware/efi-capsule-file/data echo 0 > /sys/class/firmware/efi-capsule-file/loading If so, capsule2 will be the one we will obtain in the callback function. Regards, Wilson
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] #!/bin/sh old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) for arg in $@ do if [ -f $arg ] then echo 1 /sys/class/firmware/efi-capsule-file/loading cat $arg /sys/class/firmware/efi-capsule-file/data echo 0 /sys/class/firmware/efi-capsule-file/loading I think you have a race. Try putting msleep(1000) after the request_firmware_nowait call, and I bet this will fail on the second try. Sorry for the late response. I don't really catch the race condition that you are referring? Are you trying to tell that the user script could run faster before the previous callback function actually end? Will such scenario happen? In the callback function, after the request_firmware_nowait(), I don't have any codes will delay the callback function to end. Besides, there is a mutex_lock protecting the request_firmware_nowait() calling. Won't that take care of the issue? oldtime=$(date +%S) oldtime=$(((time + 2) % 60)) until [ -f /sys/class/firmware/efi-capsule-file/loading ] do newtime=$(date +%S) if [ $newtime -eq $oldtime ] then break fi done old=$((old + 1)) new=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) I think that firmware_class doesn't call the callback until after loading is closed for the second time. If so, then this is racy. Try inserting msleep(1000) at the beginning of your callback and uploading a capsule that should load successfully -- this will report failure, but a future upload may get very confused. Also, what does the firmware class do when simultaneous uploads of the same file with different contents are in flight? Is that possible? Sorry again, I can't really catch you on this race condition statement. Are you trying to tell if user is doing this: echo 1 /sys/class/firmware/efi-capsule-file/loading cat capsule1 /sys/class/firmware/efi-capsule-file/data cat capsule2 /sys/class/firmware/efi-capsule-file/data echo 0 /sys/class/firmware/efi-capsule-file/loading If so, capsule2 will be the one we will obtain in the callback function. Regards, Wilson
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Nov 10, 2014 at 12:31 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] #!/bin/sh old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) for arg in $@ do if [ -f $arg ] then echo 1 /sys/class/firmware/efi-capsule-file/loading cat $arg /sys/class/firmware/efi-capsule-file/data echo 0 /sys/class/firmware/efi-capsule-file/loading I think you have a race. Try putting msleep(1000) after the request_firmware_nowait call, and I bet this will fail on the second try. Sorry for the late response. I don't really catch the race condition that you are referring? Are you trying to tell that the user script could run faster before the previous callback function actually end? Will such scenario happen? In the callback function, after the request_firmware_nowait(), I don't have any codes will delay the callback function to end. Besides, there is a mutex_lock protecting the request_firmware_nowait() calling. Won't that take care of the issue? In callbackfn_efi_capsule, you call request_firmware_nowait. When that callback is invoked, I think that the /sys/class/firmware/efi-capsule-file directory doesn't exist at all. If the callback takes longer than it takes your script to make it through a full iteration, then it will try uploading the second capsule before the firmware class directory is there, so it will fail. But I just realized that your script has a loop below to handle that. It's this: oldtime=$(date +%S) oldtime=$(((time + 2) % 60)) until [ -f /sys/class/firmware/efi-capsule-file/loading ] do newtime=$(date +%S) if [ $newtime -eq $oldtime ] then break fi done Aside from the fact that this loop itself is racy (it may loop forever if something goes wrong in the kernel, since $newtime -eq $oldtime may never happen), it should help, if you're lucky. But there's another bug. I think that firmware_class doesn't call the callback until after loading is closed for the second time. If so, then this is racy. Try inserting msleep(1000) at the beginning of your callback and uploading a capsule that should load successfully -- this will report failure, but a future upload may get very confused. Also, what does the firmware class do when simultaneous uploads of the same file with different contents are in flight? Is that possible? Sorry again, I can't really catch you on this race condition statement. Are you trying to tell if user is doing this: echo 1 /sys/class/firmware/efi-capsule-file/loading cat capsule1 /sys/class/firmware/efi-capsule-file/data cat capsule2 /sys/class/firmware/efi-capsule-file/data echo 0 /sys/class/firmware/efi-capsule-file/loading If so, capsule2 will be the one we will obtain in the callback function. Here's the race: User: echo 1 /sys/class/firmware/efi-capsule-file/loading cat capsule1 /sys/class/firmware/efi-capsule-file/data echo 0 /sys/class/firmware/efi-capsule-file/loading Kernel: Be a little slow here due to preemption or whatever. User: -f /sys/class/firmware/efi-capsule-file/loading returns true capsules_loaded == 0 Assume failure, incorrectly Kernel: catch up and increment capsules_loaded. If these patches get applied, then I think that the protocol needs to be documented in Documentation/ABI. It should say something like: To upload an EFI capsule, do this: Write 1 to /sys/class/firmware/efi-capsule-file/loading Write the capsule to /sys/class/firmware/efi-capsule-file/data Write 0 to /sys/class/firmware/efi-capsule-file/loading Make sure that /sys/class/firmware/efi-capsule-file disappears and comes back, perhaps by cd-ing there and waiting for all the files in the directory to go away. Then, and only then, read capsules_loaded to detect success. Once you've written that doc, please seriously consider whether this interface is justifiable. I think it sucks. --Andy -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Nov 8, 2014 5:05 AM, "Matt Fleming" wrote: > > On Tue, 04 Nov, at 08:35:40AM, Andy Lutomirski wrote: > > > > Am I missing something here? The current proposal is missing the > > success/failure part, unless you count the loaded count (in a > > different sysfs directory) as a useful interface for that. > > As Wilson pointed out, you only get the ability to make meaningful > success/failure declarations once you've performed the reboot. > > I know of no firmware that will hot-patch itself when you call > UpdateCapsule(). A reboot is always required. Certainly that's the way > Windows will work from what I've read, which means that for x86 it's > pretty much set in stone. I dunno. If nothing else, efi_capsule_update can fail due to ENOMEM. > > Which means there's only so much info you can return to userspace once > you've handed the blob to the firmware. I don't see a huge problem with > printing things in kernel buffer, since that's how other > firmware-related things work today. I think the kernel log is fine. But if the code is going to report success / failure to userspace at all, shouldn't that indication be reliable? TBH, I find this discussion very strange. In summary: me: This API is really awkward. others: But it's using the subsystem that it should be using. me: Then fix the subsystem? others: The subsystem the correct choice. me: But the API is still really awkward, and, by the way, it probably has at least two races that user code could hit. And, by the way, the sample script written by the author of the patches is subject to *both* races most likely and therefore won't work reliably. you: Common use cases (e.g. Windows-style uses, perhaps) don't need the features that are racy anyway. My only response is that (a) something else might want the full functionality and (b) Wilson's actual example script exercises the racy code. I think I'm done reviewing these patches. I'll probably grumble at the result the first time I actually try to install an EFI capsule, though. --Andy P.S. What happens when a strange UEFI BIOS really wants two capsules, and the second one will brick the machine if the first one isn't there, and the first one failed to load but no one noticed because there's no useful error handling? -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, 04 Nov, at 08:35:40AM, Andy Lutomirski wrote: > > Am I missing something here? The current proposal is missing the > success/failure part, unless you count the loaded count (in a > different sysfs directory) as a useful interface for that. As Wilson pointed out, you only get the ability to make meaningful success/failure declarations once you've performed the reboot. I know of no firmware that will hot-patch itself when you call UpdateCapsule(). A reboot is always required. Certainly that's the way Windows will work from what I've read, which means that for x86 it's pretty much set in stone. Which means there's only so much info you can return to userspace once you've handed the blob to the firmware. I don't see a huge problem with printing things in kernel buffer, since that's how other firmware-related things work today. -- Matt Fleming, Intel Open Source Technology Center -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, 04 Nov, at 08:35:40AM, Andy Lutomirski wrote: Am I missing something here? The current proposal is missing the success/failure part, unless you count the loaded count (in a different sysfs directory) as a useful interface for that. As Wilson pointed out, you only get the ability to make meaningful success/failure declarations once you've performed the reboot. I know of no firmware that will hot-patch itself when you call UpdateCapsule(). A reboot is always required. Certainly that's the way Windows will work from what I've read, which means that for x86 it's pretty much set in stone. Which means there's only so much info you can return to userspace once you've handed the blob to the firmware. I don't see a huge problem with printing things in kernel buffer, since that's how other firmware-related things work today. -- Matt Fleming, Intel Open Source Technology Center -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Nov 8, 2014 5:05 AM, Matt Fleming m...@console-pimps.org wrote: On Tue, 04 Nov, at 08:35:40AM, Andy Lutomirski wrote: Am I missing something here? The current proposal is missing the success/failure part, unless you count the loaded count (in a different sysfs directory) as a useful interface for that. As Wilson pointed out, you only get the ability to make meaningful success/failure declarations once you've performed the reboot. I know of no firmware that will hot-patch itself when you call UpdateCapsule(). A reboot is always required. Certainly that's the way Windows will work from what I've read, which means that for x86 it's pretty much set in stone. I dunno. If nothing else, efi_capsule_update can fail due to ENOMEM. Which means there's only so much info you can return to userspace once you've handed the blob to the firmware. I don't see a huge problem with printing things in kernel buffer, since that's how other firmware-related things work today. I think the kernel log is fine. But if the code is going to report success / failure to userspace at all, shouldn't that indication be reliable? TBH, I find this discussion very strange. In summary: me: This API is really awkward. others: But it's using the subsystem that it should be using. me: Then fix the subsystem? others: The subsystem the correct choice. me: But the API is still really awkward, and, by the way, it probably has at least two races that user code could hit. And, by the way, the sample script written by the author of the patches is subject to *both* races most likely and therefore won't work reliably. you: Common use cases (e.g. Windows-style uses, perhaps) don't need the features that are racy anyway. My only response is that (a) something else might want the full functionality and (b) Wilson's actual example script exercises the racy code. I think I'm done reviewing these patches. I'll probably grumble at the result the first time I actually try to install an EFI capsule, though. --Andy P.S. What happens when a strange UEFI BIOS really wants two capsules, and the second one will brick the machine if the first one isn't there, and the first one failed to load but no one noticed because there's no useful error handling? -- 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/
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Nov 6, 2014 4:56 AM, "Kweh, Hock Leong" wrote: > > > -Original Message- > > From: Andy Lutomirski [mailto:l...@amacapital.net] > > Sent: Wednesday, November 05, 2014 12:36 AM > > > > Am I missing something here? The current proposal is missing the > > success/failure part, unless you count the loaded count (in a different > > sysfs > > directory) as a useful interface for that. > > Here is my sample shell script which allow me to do multi capsule binaries > upload > and obtain error message if error occur: > > #!/bin/sh > > old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) > > for arg in "$@" > do > if [ -f $arg ] > then > echo 1 > /sys/class/firmware/efi-capsule-file/loading > cat $arg > /sys/class/firmware/efi-capsule-file/data > echo 0 > /sys/class/firmware/efi-capsule-file/loading I think you have a race. Try putting msleep(1000) after the request_firmware_nowait call, and I bet this will fail on the second try. > > oldtime=$(date +%S) > oldtime=$(((time + 2) % 60)) > until [ -f /sys/class/firmware/efi-capsule-file/loading ] > do > newtime=$(date +%S) > if [ $newtime -eq $oldtime ] > then > break > fi > done > > old=$((old + 1)) > new=$(cat > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) I think that firmware_class doesn't call the callback until after loading is closed for the second time. If so, then this is racy. Try inserting msleep(1000) at the beginning of your callback and uploading a capsule that should load successfully -- this will report failure, but a future upload may get very confused. Also, what does the firmware class do when simultaneous uploads of the same file with different contents are in flight? Is that possible? --Andy -- 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/
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message-
> From: Andy Lutomirski [mailto:l...@amacapital.net]
> Sent: Wednesday, November 05, 2014 12:36 AM
>
> Am I missing something here? The current proposal is missing the
> success/failure part, unless you count the loaded count (in a different sysfs
> directory) as a useful interface for that.
Here is my sample shell script which allow me to do multi capsule binaries
upload
and obtain error message if error occur:
#!/bin/sh
old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
for arg in "$@"
do
if [ -f $arg ]
then
echo 1 > /sys/class/firmware/efi-capsule-file/loading
cat $arg > /sys/class/firmware/efi-capsule-file/data
echo 0 > /sys/class/firmware/efi-capsule-file/loading
oldtime=$(date +%S)
oldtime=$(((time + 2) % 60))
until [ -f /sys/class/firmware/efi-capsule-file/loading ]
do
newtime=$(date +%S)
if [ $newtime -eq $oldtime ]
then
break
fi
done
old=$((old + 1))
new=$(cat
/sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
if [ ! $new -eq $old ]
then
echo "Capsule binary $arg upload failed"
dmesg | tail | grep -v platform | grep -e efi
exit 1
fi
else
echo "File $arg not found !!"
fi
done
exit 0
Regards,
Wilson
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jÇ«y�m��@A�a���
0��h���i
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Nov 6, 2014 4:56 AM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Wednesday, November 05, 2014 12:36 AM Am I missing something here? The current proposal is missing the success/failure part, unless you count the loaded count (in a different sysfs directory) as a useful interface for that. Here is my sample shell script which allow me to do multi capsule binaries upload and obtain error message if error occur: #!/bin/sh old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) for arg in $@ do if [ -f $arg ] then echo 1 /sys/class/firmware/efi-capsule-file/loading cat $arg /sys/class/firmware/efi-capsule-file/data echo 0 /sys/class/firmware/efi-capsule-file/loading I think you have a race. Try putting msleep(1000) after the request_firmware_nowait call, and I bet this will fail on the second try. oldtime=$(date +%S) oldtime=$(((time + 2) % 60)) until [ -f /sys/class/firmware/efi-capsule-file/loading ] do newtime=$(date +%S) if [ $newtime -eq $oldtime ] then break fi done old=$((old + 1)) new=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) I think that firmware_class doesn't call the callback until after loading is closed for the second time. If so, then this is racy. Try inserting msleep(1000) at the beginning of your callback and uploading a capsule that should load successfully -- this will report failure, but a future upload may get very confused. Also, what does the firmware class do when simultaneous uploads of the same file with different contents are in flight? Is that possible? --Andy -- 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/
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message-
From: Andy Lutomirski [mailto:l...@amacapital.net]
Sent: Wednesday, November 05, 2014 12:36 AM
Am I missing something here? The current proposal is missing the
success/failure part, unless you count the loaded count (in a different sysfs
directory) as a useful interface for that.
Here is my sample shell script which allow me to do multi capsule binaries
upload
and obtain error message if error occur:
#!/bin/sh
old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
for arg in $@
do
if [ -f $arg ]
then
echo 1 /sys/class/firmware/efi-capsule-file/loading
cat $arg /sys/class/firmware/efi-capsule-file/data
echo 0 /sys/class/firmware/efi-capsule-file/loading
oldtime=$(date +%S)
oldtime=$(((time + 2) % 60))
until [ -f /sys/class/firmware/efi-capsule-file/loading ]
do
newtime=$(date +%S)
if [ $newtime -eq $oldtime ]
then
break
fi
done
old=$((old + 1))
new=$(cat
/sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
if [ ! $new -eq $old ]
then
echo Capsule binary $arg upload failed
dmesg | tail | grep -v platform | grep -e efi
exit 1
fi
else
echo File $arg not found !!
fi
done
exit 0
Regards,
Wilson
N�r��yb�X��ǧv�^�)Þº{.n�+{zX����ܨ}ï¿½ï¿½ï¿½Æ z�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jÇ«y�m��@A�a���
0��h���i
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Nov 4, 2014 at 7:40 AM, Greg KH wrote: > On Tue, Nov 04, 2014 at 06:57:21AM -0800, Andy Lutomirski wrote: >> >> 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. Am I missing something here? The current proposal is missing the success/failure part, unless you count the loaded count (in a different sysfs directory) as a useful interface for that. > >> 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. > As I said, I don't object to the use of firmware class. I'd just kind of like the actual result of doing so to not suck. Other things I noticed on brief review of the code: The firmware class has a caching mechanism. I have no idea how it works, but it needs to be reliably disabled for efi-capsule-file. Is it? There's a timeout mechanism. That mechanism should not be invoked for efi-capsule-file. I haven't spotted code to disable it. The count of loaded capsules is in a separate platform device. It is really okay for this driver to have two separate sysfs directories with names that user code needs to hard code? Shouldn't there be just one? Using the NULLness of fw->pages as an indication of where the firmware came from seems extremely unreliable and likely to break in interesting ways in the future. It looks like every firmware request either results in a uevent or a helper invocation. Neither is appropriate here. Should they be turned off (by some new interface in the firmware class)? This might all be nicely resolved by adding a new interface to firmware_class that says "I want userspace to be able to send me firmware zero or more times, and I want to handle each blob individually." --Andy -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Nov 04, 2014 at 06:57:21AM -0800, Andy Lutomirski wrote: > On Nov 4, 2014 6:18 AM, "Matt Fleming" 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Nov 4, 2014 6:18 AM, "Matt Fleming" 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. 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. Reading the count of successful loaded capsules, writing to 'loading', writing the data, writing to loading, reading the count again, and checking that the value is right *works*, but it's quite baroque. And it will never scale well, because AFAICT the "count" file is not even in the same sysfs directory as the rest of the control files. 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. --Andy -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
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.) > > 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. -- 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/
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Tuesday, November 04, 2014 2:32 PM > > 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 did try to upload more than one capsule binaries and I don't observe a long wait is required. In fact, it seem to me the interface is instantly re-created. > > 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. I have not encounter any capsule update that does not require reboot. Base on my understanding, the EFI firmware only do the binary decoding during the next reboot. If the binary is broken/corrupted, you would only know during the next reboot. On kernel driver point of view, it just registers the binary by calling the EFI API UpdateCapsule(). May be Matt you could help out with this? Regarding the EFI call failed message, besides obtains from the dmesg log, you also can verify that through this sysfs: /sys/devices/platform/efi_capsule_user_helper/capsule_loaded I did mention this in the commit message as showed below: Besides the request_firmware user helper interface, this module also exposes a 'capsule_loaded' file note for user to verify the number of successfully uploaded capsule binaries. This file note has the read only attribute. > > What's the benefit of using the firmware interface here? > > --Andy
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Tuesday, November 04, 2014 2:32 PM 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 did try to upload more than one capsule binaries and I don't observe a long wait is required. In fact, it seem to me the interface is instantly re-created. 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. I have not encounter any capsule update that does not require reboot. Base on my understanding, the EFI firmware only do the binary decoding during the next reboot. If the binary is broken/corrupted, you would only know during the next reboot. On kernel driver point of view, it just registers the binary by calling the EFI API UpdateCapsule(). May be Matt you could help out with this? Regarding the EFI call failed message, besides obtains from the dmesg log, you also can verify that through this sysfs: /sys/devices/platform/efi_capsule_user_helper/capsule_loaded I did mention this in the commit message as showed below: Besides the request_firmware user helper interface, this module also exposes a 'capsule_loaded' file note for user to verify the number of successfully uploaded capsule binaries. This file note has the read only attribute. What's the benefit of using the firmware interface here? --Andy
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
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.) 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. -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
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. 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. Reading the count of successful loaded capsules, writing to 'loading', writing the data, writing to loading, reading the count again, and checking that the value is right *works*, but it's quite baroque. And it will never scale well, because AFAICT the count file is not even in the same sysfs directory as the rest of the control files. 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. --Andy -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, Nov 4, 2014 at 7:40 AM, Greg KH gre...@linuxfoundation.org wrote: On Tue, Nov 04, 2014 at 06:57:21AM -0800, Andy Lutomirski wrote: 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. Am I missing something here? The current proposal is missing the success/failure part, unless you count the loaded count (in a different sysfs directory) as a useful interface for that. 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. As I said, I don't object to the use of firmware class. I'd just kind of like the actual result of doing so to not suck. Other things I noticed on brief review of the code: The firmware class has a caching mechanism. I have no idea how it works, but it needs to be reliably disabled for efi-capsule-file. Is it? There's a timeout mechanism. That mechanism should not be invoked for efi-capsule-file. I haven't spotted code to disable it. The count of loaded capsules is in a separate platform device. It is really okay for this driver to have two separate sysfs directories with names that user code needs to hard code? Shouldn't there be just one? Using the NULLness of fw-pages as an indication of where the firmware came from seems extremely unreliable and likely to break in interesting ways in the future. It looks like every firmware request either results in a uevent or a helper invocation. Neither is appropriate here. Should they be turned off (by some new interface in the firmware class)? This might all be nicely resolved by adding a new interface to firmware_class that says I want userspace to be able to send me firmware zero or more times, and I want to handle each blob individually. --Andy -- 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/
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Nov 3, 2014 at 10:04 PM, Kweh, Hock Leong wrote: >> -Original Message- >> From: Andy Lutomirski [mailto:l...@amacapital.net] >> > >> > 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. OK. I missed that you request firmware again after each request finishes. > >> >> 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. > 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.) 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. What's the benefit of using the firmware interface here? --Andy -- 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/
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > > > > 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
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Nov 3, 2014 at 8:32 PM, Greg Kroah-Hartman wrote: > On Mon, Nov 03, 2014 at 11:07:10AM +0800, Kweh Hock Leong wrote: >> From: "Kweh, Hock Leong" >> >> Introducing a kernel module to expose user helper interface for >> user to upload capsule binaries. This module leverage the >> request_firmware_nowait() to expose an interface to user. >> >> Example steps to load the capsule binary: >> 1.) echo 1 > /sys/class/firmware/efi-capsule-file/loading >> 2.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data >> 3.) echo 0 > /sys/class/firmware/efi-capsule-file/loading >> >> Whereas, this module does not allow the capsule binaries to be >> obtained from the request_firmware library search path. If any >> capsule binary loaded from firmware seach path, the module will >> stop functioning. >> >> Besides the request_firmware user helper interface, this module >> also expose a 'capsule_loaded' file note for user to verify >> the number of successfully uploaded capsule binaries. This >> file note has the read only attribute. > > 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. 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? 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 > > Full patch left below... > >> >> Cc: Matt Fleming >> Signed-off-by: Kweh, Hock Leong >> --- >> drivers/firmware/efi/Kconfig | 13 ++ >> drivers/firmware/efi/Makefile |1 + >> drivers/firmware/efi/efi-capsule-user-helper.c | 246 >> >> 3 files changed, 260 insertions(+) >> create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c >> >> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig >> index f712d47..7dc814e 100644 >> --- a/drivers/firmware/efi/Kconfig >> +++ b/drivers/firmware/efi/Kconfig >> @@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS >> config EFI_ARMSTUB >> bool >> >> +config EFI_CAPSULE_USER_HELPER >> + tristate "EFI capsule user mode helper" >> + depends on EFI >> + select FW_LOADER >> + select FW_LOADER_USER_HELPER >> + help >> + This option exposes the user mode helper interface for user to load >> + EFI capsule binary and update the EFI firmware after system reboot. >> + This feature does not support auto locating capsule binaries at the >> + firmware lib search path. >> + >> + If unsure, say N. >> + >> endmenu >> >> config UEFI_CPER >> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile >> index 698846e..63f6910 100644 >> --- a/drivers/firmware/efi/Makefile >> +++ b/drivers/firmware/efi/Makefile >> @@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o >> obj-$(CONFIG_EFI_RUNTIME_MAP)+= runtime-map.o >> obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o >> obj-$(CONFIG_EFI_STUB) += libstub/ >> +obj-$(CONFIG_EFI_CAPSULE_USER_HELPER)+= efi-capsule-user-helper.o >> diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c >> b/drivers/firmware/efi/efi-capsule-user-helper.c >> new file mode 100644 >> index 000..84a1628 >> --- /dev/null >> +++ b/drivers/firmware/efi/efi-capsule-user-helper.c >> @@ -0,0 +1,246 @@ >> +/* >> + * EFI capsule user mode helper interface driver. >> + * >> + * Copyright 2014 Intel Corporation >> + * >> + * This file is part of the Linux kernel, and is made available under >> + * the terms of the GNU General Public License version 2. >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define CAPSULE_NAME
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Nov 03, 2014 at 11:07:10AM +0800, Kweh Hock Leong wrote:
> From: "Kweh, Hock Leong"
>
> Introducing a kernel module to expose user helper interface for
> user to upload capsule binaries. This module leverage the
> request_firmware_nowait() to expose an interface to user.
>
> Example steps to load the capsule binary:
> 1.) echo 1 > /sys/class/firmware/efi-capsule-file/loading
> 2.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data
> 3.) echo 0 > /sys/class/firmware/efi-capsule-file/loading
>
> Whereas, this module does not allow the capsule binaries to be
> obtained from the request_firmware library search path. If any
> capsule binary loaded from firmware seach path, the module will
> stop functioning.
>
> Besides the request_firmware user helper interface, this module
> also expose a 'capsule_loaded' file note for user to verify
> the number of successfully uploaded capsule binaries. This
> file note has the read only attribute.
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.
Full patch left below...
>
> Cc: Matt Fleming
> Signed-off-by: Kweh, Hock Leong
> ---
> drivers/firmware/efi/Kconfig | 13 ++
> drivers/firmware/efi/Makefile |1 +
> drivers/firmware/efi/efi-capsule-user-helper.c | 246
>
> 3 files changed, 260 insertions(+)
> create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index f712d47..7dc814e 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
> config EFI_ARMSTUB
> bool
>
> +config EFI_CAPSULE_USER_HELPER
> + tristate "EFI capsule user mode helper"
> + depends on EFI
> + select FW_LOADER
> + select FW_LOADER_USER_HELPER
> + help
> + This option exposes the user mode helper interface for user to load
> + EFI capsule binary and update the EFI firmware after system reboot.
> + This feature does not support auto locating capsule binaries at the
> + firmware lib search path.
> +
> + If unsure, say N.
> +
> endmenu
>
> config UEFI_CPER
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index 698846e..63f6910 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
> obj-$(CONFIG_EFI_RUNTIME_MAP)+= runtime-map.o
> obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
> obj-$(CONFIG_EFI_STUB) += libstub/
> +obj-$(CONFIG_EFI_CAPSULE_USER_HELPER)+= efi-capsule-user-helper.o
> diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c
> b/drivers/firmware/efi/efi-capsule-user-helper.c
> new file mode 100644
> index 000..84a1628
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-user-helper.c
> @@ -0,0 +1,246 @@
> +/*
> + * EFI capsule user mode helper interface driver.
> + *
> + * Copyright 2014 Intel Corporation
> + *
> + * This file is part of the Linux kernel, and is made available under
> + * the terms of the GNU General Public License version 2.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define CAPSULE_NAME "efi-capsule-file"
> +#define DEV_NAME "efi_capsule_user_helper"
> +#define STRING_INTEGER_MAX_LENGTH 13
> +
> +static DEFINE_MUTEX(user_helper_lock);
> +static int capsule_count;
> +static int stop_request;
> +static struct platform_device *efi_capsule_pdev;
> +
> +/*
> + * This function will store the capsule binary and pass it to
> + * efi_capsule_update() API in capsule.c
> + */
> +static int efi_capsule_store(const struct firmware *fw)
> +{
> + int i;
> + int ret;
> + int count = fw->size;
> + int copy_size = (fw->size > PAGE_SIZE) ? PAGE_SIZE : fw->size;
> + int pages_needed = ALIGN(fw->size, PAGE_SIZE) >> PAGE_SHIFT;
> + struct page **pages;
> + void *page_data;
> + efi_capsule_header_t *capsule = NULL;
> +
> + pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
> + if (!pages) {
> + pr_err("%s: kmalloc_array() failed\n", __func__);
> + return -ENOMEM;
> + }
> +
> + for (i = 0; i < pages_needed; i++) {
> + pages[i] = alloc_page(GFP_KERNEL);
> + if (!pages[i]) {
> + pr_err("%s: alloc_page() failed\n", __func__);
> + --i;
> + ret = -ENOMEM;
> + goto failed;
> + }
> + }
> +
> + for (i = 0; i < pages_needed; i++) {
> + page_data = kmap(pages[i]);
> +
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Nov 03, 2014 at 11:07:10AM +0800, Kweh Hock Leong wrote:
From: Kweh, Hock Leong hock.leong.k...@intel.com
Introducing a kernel module to expose user helper interface for
user to upload capsule binaries. This module leverage the
request_firmware_nowait() to expose an interface to user.
Example steps to load the capsule binary:
1.) echo 1 /sys/class/firmware/efi-capsule-file/loading
2.) cat capsule.bin /sys/class/firmware/efi-capsule-file/data
3.) echo 0 /sys/class/firmware/efi-capsule-file/loading
Whereas, this module does not allow the capsule binaries to be
obtained from the request_firmware library search path. If any
capsule binary loaded from firmware seach path, the module will
stop functioning.
Besides the request_firmware user helper interface, this module
also expose a 'capsule_loaded' file note for user to verify
the number of successfully uploaded capsule binaries. This
file note has the read only attribute.
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.
Full patch left below...
Cc: Matt Fleming matt.flem...@intel.com
Signed-off-by: Kweh, Hock Leong hock.leong.k...@intel.com
---
drivers/firmware/efi/Kconfig | 13 ++
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/efi-capsule-user-helper.c | 246
3 files changed, 260 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..7dc814e 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_USER_HELPER
+ tristate EFI capsule user mode helper
+ depends on EFI
+ select FW_LOADER
+ select FW_LOADER_USER_HELPER
+ help
+ This option exposes the user mode helper interface for user to load
+ EFI capsule binary and update the EFI firmware after system reboot.
+ This feature does not support auto locating capsule binaries at the
+ firmware lib search path.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..63f6910 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP)+= runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_USER_HELPER)+= efi-capsule-user-helper.o
diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c
b/drivers/firmware/efi/efi-capsule-user-helper.c
new file mode 100644
index 000..84a1628
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-user-helper.c
@@ -0,0 +1,246 @@
+/*
+ * EFI capsule user mode helper interface driver.
+ *
+ * Copyright 2014 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME : fmt
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/delay.h
+#include linux/highmem.h
+#include linux/slab.h
+#include linux/mutex.h
+#include linux/reboot.h
+#include linux/efi.h
+#include linux/firmware.h
+
+#define CAPSULE_NAME efi-capsule-file
+#define DEV_NAME efi_capsule_user_helper
+#define STRING_INTEGER_MAX_LENGTH 13
+
+static DEFINE_MUTEX(user_helper_lock);
+static int capsule_count;
+static int stop_request;
+static struct platform_device *efi_capsule_pdev;
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c
+ */
+static int efi_capsule_store(const struct firmware *fw)
+{
+ int i;
+ int ret;
+ int count = fw-size;
+ int copy_size = (fw-size PAGE_SIZE) ? PAGE_SIZE : fw-size;
+ int pages_needed = ALIGN(fw-size, PAGE_SIZE) PAGE_SHIFT;
+ struct page **pages;
+ void *page_data;
+ efi_capsule_header_t *capsule = NULL;
+
+ pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
+ if (!pages) {
+ pr_err(%s: kmalloc_array() failed\n, __func__);
+ return -ENOMEM;
+ }
+
+ for (i = 0; i pages_needed; i++) {
+ pages[i] = alloc_page(GFP_KERNEL);
+ if (!pages[i]) {
+ pr_err(%s: alloc_page() failed\n, __func__);
+ --i;
+ ret = -ENOMEM;
+ goto failed;
+ }
+ }
+
+ for (i = 0; i pages_needed; i++) {
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Nov 3, 2014 at 8:32 PM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Mon, Nov 03, 2014 at 11:07:10AM +0800, Kweh Hock Leong wrote: From: Kweh, Hock Leong hock.leong.k...@intel.com Introducing a kernel module to expose user helper interface for user to upload capsule binaries. This module leverage the request_firmware_nowait() to expose an interface to user. Example steps to load the capsule binary: 1.) echo 1 /sys/class/firmware/efi-capsule-file/loading 2.) cat capsule.bin /sys/class/firmware/efi-capsule-file/data 3.) echo 0 /sys/class/firmware/efi-capsule-file/loading Whereas, this module does not allow the capsule binaries to be obtained from the request_firmware library search path. If any capsule binary loaded from firmware seach path, the module will stop functioning. Besides the request_firmware user helper interface, this module also expose a 'capsule_loaded' file note for user to verify the number of successfully uploaded capsule binaries. This file note has the read only attribute. 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. 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? 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 Full patch left below... Cc: Matt Fleming matt.flem...@intel.com Signed-off-by: Kweh, Hock Leong hock.leong.k...@intel.com --- drivers/firmware/efi/Kconfig | 13 ++ drivers/firmware/efi/Makefile |1 + drivers/firmware/efi/efi-capsule-user-helper.c | 246 3 files changed, 260 insertions(+) create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index f712d47..7dc814e 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS config EFI_ARMSTUB bool +config EFI_CAPSULE_USER_HELPER + tristate EFI capsule user mode helper + depends on EFI + select FW_LOADER + select FW_LOADER_USER_HELPER + help + This option exposes the user mode helper interface for user to load + EFI capsule binary and update the EFI firmware after system reboot. + This feature does not support auto locating capsule binaries at the + firmware lib search path. + + If unsure, say N. + endmenu config UEFI_CPER diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile index 698846e..63f6910 100644 --- a/drivers/firmware/efi/Makefile +++ b/drivers/firmware/efi/Makefile @@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o obj-$(CONFIG_EFI_RUNTIME_MAP)+= runtime-map.o obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o obj-$(CONFIG_EFI_STUB) += libstub/ +obj-$(CONFIG_EFI_CAPSULE_USER_HELPER)+= efi-capsule-user-helper.o diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c b/drivers/firmware/efi/efi-capsule-user-helper.c new file mode 100644 index 000..84a1628 --- /dev/null +++ b/drivers/firmware/efi/efi-capsule-user-helper.c @@ -0,0 +1,246 @@ +/* + * EFI capsule user mode helper interface driver. + * + * Copyright 2014 Intel Corporation + * + * This file is part of the Linux kernel, and is made available under + * the terms of the GNU General Public License version 2. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME : fmt + +#include linux/kernel.h +#include linux/module.h +#include linux/platform_device.h +#include linux/delay.h +#include linux/highmem.h +#include linux/slab.h +#include linux/mutex.h +#include linux/reboot.h +#include linux/efi.h +#include linux/firmware.h + +#define
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
-Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] 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
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Mon, Nov 3, 2014 at 10:04 PM, Kweh, Hock Leong hock.leong.k...@intel.com wrote: -Original Message- From: Andy Lutomirski [mailto:l...@amacapital.net] 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. OK. I missed that you request firmware again after each request finishes. 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. 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.) 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. What's the benefit of using the firmware interface here? --Andy -- 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/
