Re: [PATCH v2 00/10] arm64 EFI patches for 3.19
On 28 October 2014 17:18, Ard Biesheuvel wrote: > Version v2 of arm64/efi patches for 3.19. > Changes since previous version: > - dropped patch #6 'arm64/efi: use UEFI memory map unconditionally if > available' > (will be revisited later, but due to other changes in the pipeline regarding > the virtual mapping of UEFI Runtime Services, it makes sense to defer this > to 3.20) > - added patch #10 'efi: efi-stub: notify on DTB absence' > > Note that Matt Fleming has given his approval for taking all of these through > the arm64 tree. > > @Matt: may we have your ack on patches #6, #7 and #10 please? Patch #7 covers > DMI not EFI but get_maintainer.pl is not very helpful here. (The patch is > cc'ed > to akpm and Tony Luck [ia64 maintainer]) > > Patches #1 - #3 are fixes for compliance with the UEFI and PE/COFF specs. > No issues are known that require these patches, so there is no reason to > pull them into a stable release. > > Patch #4 fixes is_reserve_region() to correctly identify UEFI memory regions > that need to be reserved by the OS. This patch supersedes the patch sent as > part of the v1 series: 'arm64/efi: reserve regions of type ACPI_MEMORY_NVS' > > Patch #5 removes a redundant set_bit(EFI_CONFIG_TABLES) call. > > Patches #6 - #9 implement DMI/SMBIOS for arm64, both the existing 32-bit > version and the upcoming 3.0 version that allows the SMBIOS structure table > to reside at a physical offset that cannot be encoded in 32-bits. It also > installs a 'Hardware name: xxx' string that is printed along with oopses > and kernel call stack dumps on systems that implement DMI/SMBIOS. > > Patch #10 is a patch from Mark Rutland I picked up that improves the > diagnostic output of the EFI stub regarding the origin of the device > tree blob. > > Please refer to the patches themselves for version history. Acks and/or > comments appreciated. > OK, it appears we're good to go with this series. @Will: would you like me to repost one final time? Or would you prefer a pull request instead? The only changes are added acks and a single code change where an open-coded constant 127 is replaced with its symbolic name DMI_ENTRY_END_OF_TABLE. Regards, Ard. -- 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
Re: [PATCH v2 10/10] efi: efi-stub: notify on DTB absence
On Tue, 28 Oct, at 05:18:43PM, Ard Biesheuvel wrote: > From: Mark Rutland > > In the absence of a DTB configuration table, the EFI stub will happily > continue attempting to boot a kernel, despite the fact that this kernel > may not function without a description of the hardware. In this case, as > with a typo'd "dtb=" option (e.g. "dbt=") or many other possible > failures, the only output seen by the user will be the rather terse > output from the EFI stub: > > EFI stub: Booting Linux Kernel... > > To aid those attempting to debug such failures, this patch adds a notice > when no DTB is found, making the output more helpful: > > EFI stub: Booting Linux Kernel... > EFI stub: Generating empty DTB > > Additionally, a positive acknowledgement is added when a user-specified > DTB is in use: > > EFI stub: Booting Linux Kernel... > EFI stub: Using DTB from command line > > Similarly, a positive acknowledgement is added when a DTB from a > configuration table is in use: > > EFI stub: Booting Linux Kernel... > EFI stub: Using DTB from configuration table > > Signed-off-by: Mark Rutland > Acked-by: Leif Lindholm > Acked-by: Ard Biesheuvel > Cc: Mark Salter > Cc: Matt Fleming > Acked-by: Roy Franz > Signed-off-by: Ard Biesheuvel > --- > drivers/firmware/efi/libstub/arm-stub.c | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) Can't really argue with this. Acked-by: Matt Fleming -- Matt Fleming, Intel Open Source Technology Center -- 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
Re: [PATCH v2 07/10] dmi: add support for SMBIOS 3.0 64-bit entry point
On Tue, 28 Oct, at 05:18:40PM, Ard Biesheuvel wrote: > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index 17afc51f3054..4139ef0bd51d 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -93,6 +93,12 @@ static void dmi_table(u8 *buf, int len, int num, > const struct dmi_header *dm = (const struct dmi_header *)data; > > /* > + * 7.45 End-of-Table (Type 127) [SMBIOS reference spec v3.0.0] > + */ > + if (dm->type == 127) > + break; > + > + /* Hmm.. tiny nit, but s/127/DMI_ENTRY_END_OF_TABLE/ we already have a symbol for this constant. But other than that, Acked-by: Matt Fleming -- Matt Fleming, Intel Open Source Technology Center -- 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
Re: [PATCH v2 06/10] efi: dmi: add support for SMBIOS 3.0 UEFI configuration table
On Tue, 28 Oct, at 05:18:39PM, Ard Biesheuvel wrote: > This adds support to the UEFI side for detecting the presence of > a SMBIOS 3.0 64-bit entry point. This allows the actual SMBIOS > structure table to reside at a physical offset over 4 GB, which > cannot be supported by the legacy SMBIOS 32-bit entry point. > > Since the firmware can legally provide both entry points, store > the SMBIOS 3.0 entry point in a separate variable, and let the > DMI decoding layer decide which one will be used. > > Tested-by: Suravee Suthikulpanit > Acked-by: Leif Lindholm > Signed-off-by: Ard Biesheuvel > --- > drivers/firmware/efi/efi.c | 4 > drivers/xen/efi.c | 1 + > include/linux/efi.h| 6 +- > 3 files changed, 10 insertions(+), 1 deletion(-) Acked-by: Matt Fleming -- Matt Fleming, Intel Open Source Technology Center -- 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
Re: [PATCH] efivarfs: allow unloading when build as module
On Thu, 23 Oct, at 11:20:37PM, Mathias Krause wrote: > There is no need to keep the module loaded when it serves no function in > case the EFI runtime services are disabled. Return an error in this case > so loading the module will fail. > > Also supply a module_exit function to allow unloading the module. > > Last, but not least, set the owner of the file_system_type struct. > > Cc: Jeremy Kerr > Cc: Matthew Garrett > Signed-off-by: Mathias Krause > --- > fs/efivarfs/super.c | 11 +-- > 1 file changed, 9 insertions(+), 2 deletions(-) Thanks, applied. -- Matt Fleming, Intel Open Source Technology Center -- 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
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-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update
On Mon, 2014-11-03 at 16:38 -0800, Greg Kroah-Hartman wrote: > > Good point, I don't know. > > Who ever wrote this code should know this, can someone please provide a > use-case for how this is all supposed to work? That would be Wilson. He's wanting to use this to send updates to the firmware on the Intel Quark, I think. -- 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
Re: [PATCH 2/2] efi: Capsule update support
Matt,
I've tested your patch with zero image size (no image passed, only headers)
and it crashes because there is no check for image size there.
This case (zero image size) seems to be legit according to specification
and also can be useful in real life. So I developed a little fix for your patch:
<<< cut here >
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index ca29bad..597b363 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -169,13 +169,17 @@ static int
efi_update_capsule(efi_capsule_header_t *capsule,
struct page **pages, size_t size, int reset)
{
efi_capsule_block_desc_t *block = NULL;
-struct page **block_pgs;
+struct page **block_pgs = NULL;
efi_status_t status;
-unsigned int nr_data_pgs, nr_block_pgs;
+unsigned int nr_data_pgs = 0, nr_block_pgs = 0;
+unsigned long sg_list = 0;
int i, j, err = -ENOMEM;
lockdep_assert_held(&capsule_mutex);
+if (size == 0)
+goto update_caps;
+
nr_data_pgs = DIV_ROUND_UP(size, PAGE_SIZE);
nr_block_pgs = num_block_pages(nr_data_pgs);
@@ -215,7 +219,10 @@ static int
efi_update_capsule(efi_capsule_header_t *capsule,
kunmap(block_pgs[i]);
}
-status = efi.update_capsule(&capsule, 1, page_to_phys(block_pgs[0]));
+sg_list = page_to_phys(block_pgs[0]);
+
+update_caps:
+status = efi.update_capsule(&capsule, 1, sg_list);
if (status != EFI_SUCCESS) {
pr_err("update_capsule fail: 0x%lx\n", status);
err = efi_status_to_err(status);
--
2.1.1
<<< cut here >
I'm planning to use your API for our UpdateCapsule test module so
it would be really helpful if you can include this fix to your patch.
On 16 October 2014 19:15, Matt Fleming wrote:
> On Tue, 14 Oct, at 06:30:22PM, Sam Protsenko wrote:
>> Matt,
>>
>> I tried to play with your code and now I have some extra notes about this
>> patch:
>>
>> 1. As it was proposed earlier, I support thought that it would be nice to
>>rename function names in next way:
>>
>> efi_update_capsule -> __efi_update_capsule
>> efi_capsule_update -> efi_update_capsule
>>
>>because it's quite confusing to have both efi_update_capsule() and
>>efi_capsule_update(). Besides, EFI function called UpdateCapsule, so it's
>>good idea to stick to this name in kernel API (I mean exporting
>>efi_update_capsule() instead of efi_capsule_update()).
>
> I'm not so convinced by that argument. Remember, we're building a kernel
> API here, so we've got functions like,
>
> efi_capsule_supported()
> efi_capsule_pending()
>
> I've stuck with efi_capsule_update() and __efi_capsule_update() for now,
> to continue the efi_capsule* theme (avoiding both efi_capsule_update()
> and efi_update_capsule() was a good point though).
>
>> 2. UEFI's UpdateCapsule() runtime service supports passing more than one
>>capsule to it (we can pass CapsuleCount argument to it for this purpose).
>>But your particular kernel implementation allows us only to provide one
>>capsule at a time. Is that was done for a reason? Can it be consider as
>>shortcoming?
>
> Yeah, the reason is simply that it makes the capsule management more
> complicated if you have more than one capsule, and when testing the
> patches (and experimenting with the features in the capsule-* branches
> in my git tree) I didn't come across a scenario where sending multiple
> capsules at one time was required.
>
> Doesn't mean we couldn't extend the kernel API in the future, though.
> We'd just need an in-kernel user first.
>
>> 3. I noticed that you dropped efi_capsule_build() in this patch (w.r.t.
>>https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/
>>implementation). BTW, it should be declared in header there.
>>Anyway, how do we suppose to build capsule to pass to
>> efi_capsule_update()?
>>I mean, it can take a quite large code to build a capsule (allocating
>> pages
>>etc). Wouldn't it be easier to user to use your API if it has something
>>ready to use? Anyway, if it should be done like this, it would be nice
>>to have a decent example code (use-case) how to use this API (maybe in
>>Documentation/, idk), because it looks quite non-intuitive (for me at
>> least).
>
> The two patches that I sent are only preparatory patches for EFI capsule
> support, and Kweh (Cc'd) is working on patches that implement a userland
> interface.
>
> Wilson, do you think you could post your patches by the beginning of
> next week? They just need to give an idea of how we can use this API.
>
>> 4. Tedious stuff: I checked your patch with "checkpatch.pl" and it shows
>> some warnings, please fix them if possible.
>
> Will do.
>
>> I will try to test and verify this patch further, will notify you if
>> notice any issues.
>
> Great, thanks.
>
> --
> Matt Fleming, Intel Open So
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
