* [PATCH v4 0/2] Enable capsule loader interface for efi firmware updating @ 2015-04-14 9:44 Kweh, Hock Leong 2015-04-14 9:44 ` [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() Kweh, Hock Leong 2015-04-14 9:44 ` [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware Kweh, Hock Leong 0 siblings, 2 replies; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-14 9:44 UTC (permalink / raw) To: Ming Lei, Matt Fleming, Greg Kroah-Hartman Cc: Ong Boon Leong, Kweh, Hock Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> Dear maintainers & communities, This patchset is created on top of "efi: Capsule update support" patch: http://permalink.gmane.org/gmane.linux.kernel.efi/4837 It expose a sysfs loader interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. Thanks. --- changelog v4: * rebase the firmware_class to gregkh/driver-core.git#driver-core-next branch and drop the v3 2nd patch as Zahari already fix that in that branch changelog v3: * changes base on the design discussion in the mailing list: https://lkml.org/lkml/2015/2/24/307 * 1st patch introduce a new API request_firmware_direct_full_path() in firmware_class for developer to pass in full path to the firmware file * 2nd patch fix a bug for commit "firmware_loader: handle timeout via wait_for_completion_interruptible_timeout()" * 3rd patch introduce the capsule loader interface kernel module Kweh, Hock Leong (2): firmware_loader: introduce new API - request_firmware_direct_full_path() efi: an sysfs interface for user to update efi firmware drivers/base/firmware_class.c | 46 +++++++- drivers/firmware/efi/Kconfig | 12 ++ drivers/firmware/efi/Makefile | 1 drivers/firmware/efi/efi-capsule-loader.c | 169 +++++++++++++++++++++++++++++ include/linux/firmware.h | 9 ++ 5 files changed, 232 insertions(+), 5 deletions(-) create mode 100644 drivers/firmware/efi/efi-capsule-loader.c -- 1.7.9.5 ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-14 9:44 [PATCH v4 0/2] Enable capsule loader interface for efi firmware updating Kweh, Hock Leong @ 2015-04-14 9:44 ` Kweh, Hock Leong 2015-04-14 14:08 ` Greg Kroah-Hartman 2015-04-15 12:48 ` Matt Fleming 2015-04-14 9:44 ` [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware Kweh, Hock Leong 1 sibling, 2 replies; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-14 9:44 UTC (permalink / raw) To: Ming Lei, Matt Fleming, Greg Kroah-Hartman Cc: Ong Boon Leong, Kweh, Hock Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> Introduce this new API for loading firmware from a specific location instead of /lib/firmware/ by providing a full path to the firmware file. Cc: Ming Lei <ming.lei@canonical.com> Cc: Matt Fleming <matt.fleming@intel.com> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> --- drivers/base/firmware_class.c | 46 ++++++++++++++++++++++++++++++++++++----- include/linux/firmware.h | 9 ++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 171841a..3ab7bb9 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -111,6 +111,7 @@ static inline long firmware_loading_timeout(void) #define FW_OPT_FALLBACK 0 #endif #define FW_OPT_NO_WARN (1U << 3) +#define FW_OPT_FULL_PATH (1U << 4) struct firmware_cache { /* firmware_buf instance will be added into the below list */ @@ -318,20 +319,29 @@ fail: } static int fw_get_filesystem_firmware(struct device *device, - struct firmware_buf *buf) + struct firmware_buf *buf, + unsigned int opt_flags) { int i; int rc = -ENOENT; char *path = __getname(); + int path_array_size = 1; + static const char * const root_path[] = {"/"}; + char **temp_path = (char **)root_path; - for (i = 0; i < ARRAY_SIZE(fw_path); i++) { + if (!(opt_flags & FW_OPT_FULL_PATH)) { + temp_path = (char **)fw_path; + path_array_size = ARRAY_SIZE(fw_path); + } + + for (i = 0; i < path_array_size; i++) { struct file *file; /* skip the unset customized path */ - if (!fw_path[i][0]) + if (!temp_path[i][0]) continue; - snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id); + snprintf(path, PATH_MAX, "%s/%s", temp_path[i], buf->fw_id); file = filp_open(path, O_RDONLY, 0); if (IS_ERR(file)) @@ -1122,7 +1132,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name, } } - ret = fw_get_filesystem_firmware(device, fw->priv); + ret = fw_get_filesystem_firmware(device, fw->priv, opt_flags); if (ret) { if (!(opt_flags & FW_OPT_NO_WARN)) dev_warn(device, @@ -1210,6 +1220,32 @@ int request_firmware_direct(const struct firmware **firmware_p, EXPORT_SYMBOL_GPL(request_firmware_direct); /** + * request_firmware_direct_full_path: - load firmware directly from exact + * full path + * @firmware_p: pointer to firmware image + * @name: full path to the firmware file with file name + * @device: device for which firmware is being loaded + * + * This function works like request_firmware_direct(), but this doesn't + * search the /lib/firmware/ for the firmware file. It support exact full + * path to the firmware file for loading. + **/ +int request_firmware_direct_full_path(const struct firmware **firmware_p, + const char *name, struct device *device) +{ + int ret; + + __module_get(THIS_MODULE); + ret = _request_firmware(firmware_p, name, device, + FW_OPT_UEVENT | FW_OPT_NO_WARN | + FW_OPT_FULL_PATH); + module_put(THIS_MODULE); + + return ret; +} +EXPORT_SYMBOL_GPL(request_firmware_direct_full_path); + +/** * release_firmware: - release the resource associated with a firmware image * @fw: firmware resource to release **/ diff --git a/include/linux/firmware.h b/include/linux/firmware.h index 5c41c5e..b7c6435 100644 --- a/include/linux/firmware.h +++ b/include/linux/firmware.h @@ -47,6 +47,8 @@ int request_firmware_nowait( void (*cont)(const struct firmware *fw, void *context)); int request_firmware_direct(const struct firmware **fw, const char *name, struct device *device); +int request_firmware_direct_full_path(const struct firmware **fw, + const char *name, struct device *device); void release_firmware(const struct firmware *fw); #else @@ -75,5 +77,12 @@ static inline int request_firmware_direct(const struct firmware **fw, return -EINVAL; } +static inline int request_firmware_direct_full_path(const struct firmware **fw, + const char *name, + struct device *device) +{ + return -EINVAL; +} + #endif #endif -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-14 9:44 ` [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() Kweh, Hock Leong @ 2015-04-14 14:08 ` Greg Kroah-Hartman 2015-04-14 15:56 ` Andy Lutomirski 2015-04-15 12:48 ` Matt Fleming 1 sibling, 1 reply; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-14 14:08 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Ming Lei, Matt Fleming, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Tue, Apr 14, 2015 at 05:44:55PM +0800, Kweh, Hock Leong wrote: > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > Introduce this new API for loading firmware from a specific location > instead of /lib/firmware/ by providing a full path to the firmware > file. Ick, why would we want this? ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-14 14:08 ` Greg Kroah-Hartman @ 2015-04-14 15:56 ` Andy Lutomirski 2015-04-14 16:18 ` Borislav Petkov 2015-04-15 13:15 ` Greg Kroah-Hartman 0 siblings, 2 replies; 62+ messages in thread From: Andy Lutomirski @ 2015-04-14 15:56 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov On Tue, Apr 14, 2015 at 10:08 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Tue, Apr 14, 2015 at 05:44:55PM +0800, Kweh, Hock Leong wrote: >> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> >> >> Introduce this new API for loading firmware from a specific location >> instead of /lib/firmware/ by providing a full path to the firmware >> file. > > Ick, why would we want this? > Because this mechanism should still work even if /lib is unwriteable (e.g it's on squashfs or a read-only NFS root). In this regard, UEFI capsules are very much unlike firmware_class firmware. firmware_class firmwise is kind of like device drivers; it generally comes from the same vendor as your kernel image and /lib/modules, and it can be updated by the same mechanism. UEFI capsules, on the other hand, are one-time things that should be loaded at the explicit request of the admin. There is no reason whatsoever that they should exist on persistent storage, and, in fact, there's a very good reason that they should not. On little embedded devices, which will apparently be the initial users of this code, keeping the capsules around is a waste of valuable space. This is why I think that the right approach would be to avoid using firmware_class entirely for this. IMO a simple_char device would be the way to go (hint hint...) but other simple approaches are certainly possible. --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-14 15:56 ` Andy Lutomirski @ 2015-04-14 16:18 ` Borislav Petkov 2015-04-15 10:14 ` Matt Fleming 2015-04-15 13:15 ` Greg Kroah-Hartman 1 sibling, 1 reply; 62+ messages in thread From: Borislav Petkov @ 2015-04-14 16:18 UTC (permalink / raw) To: Andy Lutomirski Cc: Greg Kroah-Hartman, Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz On Tue, Apr 14, 2015 at 11:56:26AM -0400, Andy Lutomirski wrote: > This is why I think that the right approach would be to avoid using > firmware_class entirely for this. IMO a simple_char device would be > the way to go (hint hint...) but other simple approaches are certainly > possible. Btw, didn't mfleming want to try using capsules for other funky stuff like early logging and pstore-like logging in panic time so that we can read out crash data on the next boot? Which would mean that capsules would need a completely different interface, something new for shuffling lotsa binary data in and out of the kernel and in and out of the UEFI... Which would make the firmware_class thing completely inappropriate for that. Hmmm? -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-14 16:18 ` Borislav Petkov @ 2015-04-15 10:14 ` Matt Fleming 2015-04-15 10:18 ` Borislav Petkov 0 siblings, 1 reply; 62+ messages in thread From: Matt Fleming @ 2015-04-15 10:14 UTC (permalink / raw) To: Borislav Petkov Cc: Andy Lutomirski, Greg Kroah-Hartman, Kweh, Hock Leong, Ming Lei, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz On Tue, 14 Apr, at 06:18:33PM, Borislav Petkov wrote: > On Tue, Apr 14, 2015 at 11:56:26AM -0400, Andy Lutomirski wrote: > > This is why I think that the right approach would be to avoid using > > firmware_class entirely for this. IMO a simple_char device would be > > the way to go (hint hint...) but other simple approaches are certainly > > possible. > > Btw, didn't mfleming want to try using capsules for other funky stuff > like early logging and pstore-like logging in panic time so that we can > read out crash data on the next boot? Yeah, exactly. Being able to pass random data blobs to the capsule internals allows the kernel to support cool things we haven't conceived of yet, and where we want to use the capsule's in-memory persistence across reboot to pass data to the next kernel. The panic code paths is just one example. > Which would mean that capsules would need a completely different > interface, something new for shuffling lotsa binary data in and out of > the kernel and in and out of the UEFI... > > Which would make the firmware_class thing completely inappropriate for > that. Well, I haven't come across a scenario where you need a brand new interface for getting it *out* of the kernel again. And I'm happy enough with these patches for passing data in. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-15 10:14 ` Matt Fleming @ 2015-04-15 10:18 ` Borislav Petkov 2015-04-15 11:09 ` Matt Fleming 0 siblings, 1 reply; 62+ messages in thread From: Borislav Petkov @ 2015-04-15 10:18 UTC (permalink / raw) To: Matt Fleming Cc: Andy Lutomirski, Greg Kroah-Hartman, Kweh, Hock Leong, Ming Lei, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz On Wed, Apr 15, 2015 at 11:14:55AM +0100, Matt Fleming wrote: > Well, I haven't come across a scenario where you need a brand new > interface for getting it *out* of the kernel again. Well, how are we going to read crash data on next boot then? EFI var or what? Are we going to have a generic interface like /sys/.../capsule/... or how are we imagining this to look like? Can you give a detailed example please... -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-15 10:18 ` Borislav Petkov @ 2015-04-15 11:09 ` Matt Fleming 0 siblings, 0 replies; 62+ messages in thread From: Matt Fleming @ 2015-04-15 11:09 UTC (permalink / raw) To: Borislav Petkov Cc: Andy Lutomirski, Greg Kroah-Hartman, Kweh, Hock Leong, Ming Lei, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz On Wed, 15 Apr, at 12:18:05PM, Borislav Petkov wrote: > On Wed, Apr 15, 2015 at 11:14:55AM +0100, Matt Fleming wrote: > > Well, I haven't come across a scenario where you need a brand new > > interface for getting it *out* of the kernel again. > > Well, how are we going to read crash data on next boot then? EFI var or > what? Are we going to have a generic interface like > > /sys/.../capsule/... > > or how are we imagining this to look like? You can read it out in userspace using the existing pstorefs code. The last thing we need to do is introduce more userspace APIs ;-) It's possible (and desirable) to separate the input interface from output. I've written patches in the past where the EFI kernel subsystem discovers capsules with a specific GUID reserved for crash data, and then hands that data to the pstore subsystem. Things are then exposed via pstorefs. The capsule code would just be another backend to pstore, similar to how the EFI variable code works today. I am in no way advocating for yet another crash API. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-14 15:56 ` Andy Lutomirski 2015-04-14 16:18 ` Borislav Petkov @ 2015-04-15 13:15 ` Greg Kroah-Hartman 2015-04-15 15:53 ` Andy Lutomirski 1 sibling, 1 reply; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-15 13:15 UTC (permalink / raw) To: Andy Lutomirski Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov On Tue, Apr 14, 2015 at 11:56:26AM -0400, Andy Lutomirski wrote: > On Tue, Apr 14, 2015 at 10:08 AM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 14, 2015 at 05:44:55PM +0800, Kweh, Hock Leong wrote: > >> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > >> > >> Introduce this new API for loading firmware from a specific location > >> instead of /lib/firmware/ by providing a full path to the firmware > >> file. > > > > Ick, why would we want this? > > > > Because this mechanism should still work even if /lib is unwriteable > (e.g it's on squashfs or a read-only NFS root). Why would a filesystem need to be writable to read a firmware blob from? > In this regard, UEFI capsules are very much unlike firmware_class > firmware. firmware_class firmwise is kind of like device drivers; it > generally comes from the same vendor as your kernel image and > /lib/modules, and it can be updated by the same mechanism. UEFI > capsules, on the other hand, are one-time things that should be loaded > at the explicit request of the admin. Just like BIOS updates, which use the firmware interface. > There is no reason whatsoever > that they should exist on persistent storage, and, in fact, there's a > very good reason that they should not. On little embedded devices, > which will apparently be the initial users of this code, keeping the > capsules around is a waste of valuable space. > > This is why I think that the right approach would be to avoid using > firmware_class entirely for this. IMO a simple_char device would be > the way to go (hint hint...) but other simple approaches are certainly > possible. A char device would be present all the time, like a sysfs file to write the firmware to, so I don't see the difference here. For a char device, you would just do the normal open/write/close, just like for the firmware interface, what is the difference? thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-15 13:15 ` Greg Kroah-Hartman @ 2015-04-15 15:53 ` Andy Lutomirski 0 siblings, 0 replies; 62+ messages in thread From: Andy Lutomirski @ 2015-04-15 15:53 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov [Bah, I'm really bad at email today. Trying again.] On Apr 15, 2015 6:15 AM, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 14, 2015 at 11:56:26AM -0400, Andy Lutomirski wrote: > > On Tue, Apr 14, 2015 at 10:08 AM, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Tue, Apr 14, 2015 at 05:44:55PM +0800, Kweh, Hock Leong wrote: > > >> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > >> > > >> Introduce this new API for loading firmware from a specific location > > >> instead of /lib/firmware/ by providing a full path to the firmware > > >> file. > > > > > > Ick, why would we want this? > > > > > > > Because this mechanism should still work even if /lib is unwriteable > > (e.g it's on squashfs or a read-only NFS root). > > Why would a filesystem need to be writable to read a firmware blob from? Because someone would need to temporarily put the image there. In practice, these blobs will come from vendors, signed, online using ESRT magic. Imagine a CoreOS system. When a UEFI update needed on 1% of a deployment's metal is published, no one is going to want to push out a new core CoreOS image. Instead they'll want to run the update on that 1% of nodes and be done with it. To be fair, and for those that didn't follow all the various discussions, it's unclear that this mechanism will ever be useful in the x86 server space. There's some reason to believe that MS will only issue UpdateCapsule before ExitBootServices and that firmware vendors will therefore disallow UpdateCapsule after ExitBootServices. The fwupd crowd is (I think) planning on bypassing this entirely and using the boot loader to update firmware. Regardless, those things aren't going to live in /lib, but they'll have to write *something* to a FAT filesystem because they have no choice. More sensible firmwares will support the runtime stuff, and atomic systems (RHEL Atomic, OSTree, CoreOS, whatever Docker's working on, whatever Sandstorm is working on (?), etc.) should probably be as well supported in the kernel as we can manage. > > > In this regard, UEFI capsules are very much unlike firmware_class > > firmware. firmware_class firmwise is kind of like device drivers; it > > generally comes from the same vendor as your kernel image and > > /lib/modules, and it can be updated by the same mechanism. UEFI > > capsules, on the other hand, are one-time things that should be loaded > > at the explicit request of the admin. > > Just like BIOS updates, which use the firmware interface. What BIOS updates? There's flashrom, which quite sensibly reads its input in user space. The other example I found is dell_rbu, which does a complicated packetized update thing and explicitly says in the docs: "This method makes sure that all the packets get to the driver in a single operation." The mechanism seems quite awkward to me. > > > There is no reason whatsoever > > that they should exist on persistent storage, and, in fact, there's a > > very good reason that they should not. On little embedded devices, > > which will apparently be the initial users of this code, keeping the > > capsules around is a waste of valuable space. > > > > This is why I think that the right approach would be to avoid using > > firmware_class entirely for this. IMO a simple_char device would be > > the way to go (hint hint...) but other simple approaches are certainly > > possible. > > A char device would be present all the time, like a sysfs file to write > the firmware to, so I don't see the difference here. For a char device, > you would just do the normal open/write/close, just like for the > firmware interface, what is the difference? You wouldn't use open/write/close; you'd do it atomically with a single ioctl. That gives userspace an error code. It would also be possible to require a single write(2) call, but that seems to defeat most of the purpose of using open/write/close (namely the ability to script it with cat). --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() 2015-04-14 9:44 ` [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() Kweh, Hock Leong 2015-04-14 14:08 ` Greg Kroah-Hartman @ 2015-04-15 12:48 ` Matt Fleming 1 sibling, 0 replies; 62+ messages in thread From: Matt Fleming @ 2015-04-15 12:48 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Ming Lei, Greg Kroah-Hartman, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Tue, 14 Apr, at 05:44:55PM, Kweh Hock Leong wrote: > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > Introduce this new API for loading firmware from a specific location > instead of /lib/firmware/ by providing a full path to the firmware > file. > > Cc: Ming Lei <ming.lei@canonical.com> > Cc: Matt Fleming <matt.fleming@intel.com> > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> > --- > drivers/base/firmware_class.c | 46 ++++++++++++++++++++++++++++++++++++----- > include/linux/firmware.h | 9 ++++++++ > 2 files changed, 50 insertions(+), 5 deletions(-) This looks good to me. -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-14 9:44 [PATCH v4 0/2] Enable capsule loader interface for efi firmware updating Kweh, Hock Leong 2015-04-14 9:44 ` [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() Kweh, Hock Leong @ 2015-04-14 9:44 ` Kweh, Hock Leong 2015-04-14 14:09 ` Greg Kroah-Hartman 1 sibling, 1 reply; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-14 9:44 UTC (permalink / raw) To: Ming Lei, Matt Fleming, Greg Kroah-Hartman Cc: Ong Boon Leong, Kweh, Hock Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> Introducing a kernel module to expose capsule loader interface for user to upload capsule binaries. This module leverage the request_firmware_direct_full_path() to obtain the binary at a specific path input by user. Example method to load the capsule binary: echo -n "/path/to/capsule/binary" > /sys/devices/platform/efi_capsule_loader/capsule_loader Cc: Matt Fleming <matt.fleming@intel.com> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com> --- drivers/firmware/efi/Kconfig | 12 ++ drivers/firmware/efi/Makefile | 1 + drivers/firmware/efi/efi-capsule-loader.c | 169 +++++++++++++++++++++++++++++ 3 files changed, 182 insertions(+) create mode 100644 drivers/firmware/efi/efi-capsule-loader.c diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig index f712d47..3e84ec0 100644 --- a/drivers/firmware/efi/Kconfig +++ b/drivers/firmware/efi/Kconfig @@ -60,6 +60,18 @@ config EFI_RUNTIME_WRAPPERS config EFI_ARMSTUB bool +config EFI_CAPSULE_LOADER + tristate "EFI capsule loader" + depends on EFI + select FW_LOADER + help + This option exposes a loader interface for user to load EFI + capsule binary and update the EFI firmware through 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..5ab031a 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_LOADER) += efi-capsule-loader.o diff --git a/drivers/firmware/efi/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c new file mode 100644 index 0000000..84b979b --- /dev/null +++ b/drivers/firmware/efi/efi-capsule-loader.c @@ -0,0 +1,169 @@ +/* + * EFI capsule loader driver. + * + * Copyright 2015 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/highmem.h> +#include <linux/slab.h> +#include <linux/mutex.h> +#include <linux/efi.h> +#include <linux/firmware.h> + +#define DEV_NAME "efi_capsule_loader" + +static DEFINE_MUTEX(capsule_loader_lock); +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; + + if (!count) { + pr_err("%s: Received zero binary size\n", __func__); + return -ENOENT; + } + + 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; + goto failed; + } + kunmap(pages[0]); + + /* + * we cannot free the pages here due to reboot need that data + * maintained. + */ + return 0; + +failed: + while (i >= 0) + __free_page(pages[i--]); + kfree(pages); + return ret; +} + +static ssize_t capsule_loader_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int ret = -EBUSY; + const struct firmware *fw; + + pr_debug("%s: Received path = %s\n", __func__, buf); + if (mutex_trylock(&capsule_loader_lock)) { + ret = request_firmware_direct_full_path(&fw, buf, + &efi_capsule_pdev->dev); + if (ret) { + pr_err("%s: request_firmware_direct_full_path() %s\n", + __func__, + "failed"); + mutex_unlock(&capsule_loader_lock); + return ret; + } + + ret = efi_capsule_store(fw); + if (ret) + pr_err("%s: %s, return error code = %d\n", + __func__, + "Failed to store capsule binary", + ret); + release_firmware(fw); + mutex_unlock(&capsule_loader_lock); + } + + return ret ? ret : count; +} +static DEVICE_ATTR_WO(capsule_loader); + +static int __init efi_capsule_loader_init(void) +{ + int ret = 0; + + efi_capsule_pdev = platform_device_register_simple(DEV_NAME, + PLATFORM_DEVID_NONE, + NULL, 0); + if (IS_ERR(efi_capsule_pdev)) { + pr_err("%s: platform_device_register_simple() failed\n", + __func__); + return PTR_ERR(efi_capsule_pdev); + } + + /* + * create this file node for user to pass in the full path of the + * capsule binary + */ + ret = device_create_file(&efi_capsule_pdev->dev, + &dev_attr_capsule_loader); + if (ret) { + pr_err("%s: device_create_file() failed\n", __func__); + platform_device_unregister(efi_capsule_pdev); + } + + return ret; +} +module_init(efi_capsule_loader_init); + +/* + * To remove this kernel module, just perform: + * rmmod efi_capsule_loader.ko + */ +static void __exit efi_capsule_loader_exit(void) +{ + platform_device_unregister(efi_capsule_pdev); +} +module_exit(efi_capsule_loader_exit); + +MODULE_DESCRIPTION("EFI capsule firmware binary loader"); +MODULE_LICENSE("GPL v2"); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-14 9:44 ` [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware Kweh, Hock Leong @ 2015-04-14 14:09 ` Greg Kroah-Hartman 2015-04-14 15:52 ` Andy Lutomirski 2015-04-15 11:32 ` Kweh, Hock Leong 0 siblings, 2 replies; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-14 14:09 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Ming Lei, Matt Fleming, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > Introducing a kernel module to expose capsule loader interface > for user to upload capsule binaries. This module leverage the > request_firmware_direct_full_path() to obtain the binary at a > specific path input by user. > > Example method to load the capsule binary: > echo -n "/path/to/capsule/binary" > /sys/devices/platform/efi_capsule_loader/capsule_loader Ick, why not just have the firmware file location present, and copy it to the sysfs file directly from userspace, instead of this two-step process? > +/* > + * To remove this kernel module, just perform: > + * rmmod efi_capsule_loader.ko This comment is not needed. > + */ > +static void __exit efi_capsule_loader_exit(void) > +{ > + platform_device_unregister(efi_capsule_pdev); This is not a platform device, don't abuse that interface please. greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-14 14:09 ` Greg Kroah-Hartman @ 2015-04-14 15:52 ` Andy Lutomirski 2015-04-15 13:20 ` Greg Kroah-Hartman 2015-04-15 11:32 ` Kweh, Hock Leong 1 sibling, 1 reply; 62+ messages in thread From: Andy Lutomirski @ 2015-04-14 15:52 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: >> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> >> >> Introducing a kernel module to expose capsule loader interface >> for user to upload capsule binaries. This module leverage the >> request_firmware_direct_full_path() to obtain the binary at a >> specific path input by user. >> >> Example method to load the capsule binary: >> echo -n "/path/to/capsule/binary" > /sys/devices/platform/efi_capsule_loader/capsule_loader > > Ick, why not just have the firmware file location present, and copy it > to the sysfs file directly from userspace, instead of this two-step > process? Because it's not at all obvious how error handling should work in that case. --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-14 15:52 ` Andy Lutomirski @ 2015-04-15 13:20 ` Greg Kroah-Hartman 2015-04-15 15:45 ` Andy Lutomirski 0 siblings, 1 reply; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-15 13:20 UTC (permalink / raw) To: Andy Lutomirski Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: > On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > >> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > >> > >> Introducing a kernel module to expose capsule loader interface > >> for user to upload capsule binaries. This module leverage the > >> request_firmware_direct_full_path() to obtain the binary at a > >> specific path input by user. > >> > >> Example method to load the capsule binary: > >> echo -n "/path/to/capsule/binary" > /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > Ick, why not just have the firmware file location present, and copy it > > to the sysfs file directly from userspace, instead of this two-step > > process? > > Because it's not at all obvious how error handling should work in that case. I don't understand how the error handling is any different. The kernel ends up copying the data in through the firmware interface both ways, we just aren't creating yet-another-firmware-path in the system. thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-15 13:20 ` Greg Kroah-Hartman @ 2015-04-15 15:45 ` Andy Lutomirski 2015-04-16 0:19 ` Roy Franz 0 siblings, 1 reply; 62+ messages in thread From: Andy Lutomirski @ 2015-04-15 15:45 UTC (permalink / raw) To: Greg KH Cc: Borislav Petkov, Matt Fleming, Ong Boon Leong, linux-efi, Roy Franz, Sam Protsenko, Kweh, Hock Leong, LKML, Peter Jones, Ming Lei On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> wrote: > > On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: > > On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > >> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > >> > > >> Introducing a kernel module to expose capsule loader interface > > >> for user to upload capsule binaries. This module leverage the > > >> request_firmware_direct_full_path() to obtain the binary at a > > >> specific path input by user. > > >> > > >> Example method to load the capsule binary: > > >> echo -n "/path/to/capsule/binary" > /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > > > Ick, why not just have the firmware file location present, and copy it > > > to the sysfs file directly from userspace, instead of this two-step > > > process? > > > > Because it's not at all obvious how error handling should work in that case. > > I don't understand how the error handling is any different. The kernel > ends up copying the data in through the firmware interface both ways, we > just aren't creating yet-another-firmware-path in the system. If I run uefi-update-capsule foo.bin, I want it to complain if the UEFI call fails. In contrast, if I boot and my ath10k firmware is bad, there's no explicit user interaction that should fail; instead I have no network device and I get to read the logs and figure out why. IOW bad volatile device firmware is just like a bad device driver, whereas bad UEFI capsules are problems that should be reported to whatever tried to send them to UEFI. --Andy > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-15 15:45 ` Andy Lutomirski @ 2015-04-16 0:19 ` Roy Franz 2015-04-17 13:50 ` Greg KH 0 siblings, 1 reply; 62+ messages in thread From: Roy Franz @ 2015-04-16 0:19 UTC (permalink / raw) To: Andy Lutomirski Cc: Greg KH, Borislav Petkov, Matt Fleming, Ong Boon Leong, linux-efi, Sam Protsenko, Kweh, Hock Leong, LKML, Peter Jones, Ming Lei On Wed, Apr 15, 2015 at 8:45 AM, Andy Lutomirski <luto@amacapital.net> wrote: > On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman" > <gregkh@linuxfoundation.org> wrote: >> >> On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: >> > On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman >> > <gregkh@linuxfoundation.org> wrote: >> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: >> > >> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> >> > >> >> > >> Introducing a kernel module to expose capsule loader interface >> > >> for user to upload capsule binaries. This module leverage the >> > >> request_firmware_direct_full_path() to obtain the binary at a >> > >> specific path input by user. >> > >> >> > >> Example method to load the capsule binary: >> > >> echo -n "/path/to/capsule/binary" > /sys/devices/platform/efi_capsule_loader/capsule_loader >> > > >> > > Ick, why not just have the firmware file location present, and copy it >> > > to the sysfs file directly from userspace, instead of this two-step >> > > process? >> > >> > Because it's not at all obvious how error handling should work in that case. >> >> I don't understand how the error handling is any different. The kernel >> ends up copying the data in through the firmware interface both ways, we >> just aren't creating yet-another-firmware-path in the system. > > If I run uefi-update-capsule foo.bin, I want it to complain if the > UEFI call fails. In contrast, if I boot and my ath10k firmware is > bad, there's no explicit user interaction that should fail; instead I > have no network device and I get to read the logs and figure out why. > IOW bad volatile device firmware is just like a bad device driver, > whereas bad UEFI capsules are problems that should be reported to > whatever tried to send them to UEFI. > > --Andy > There are several types of errors that can be returned by UpdateCapsule(), allowing us to distinguish between capsules that are not supported by the platform, capsules that must be updated at boot time, and capsule updates that failed due to a device error. I think we really do want this type of information returned to capsule loader. Roy >> >> thanks, >> >> greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-efi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-16 0:19 ` Roy Franz @ 2015-04-17 13:50 ` Greg KH 0 siblings, 0 replies; 62+ messages in thread From: Greg KH @ 2015-04-17 13:50 UTC (permalink / raw) To: Roy Franz Cc: Andy Lutomirski, Borislav Petkov, Matt Fleming, Ong Boon Leong, linux-efi, Sam Protsenko, Kweh, Hock Leong, LKML, Peter Jones, Ming Lei On Wed, Apr 15, 2015 at 05:19:09PM -0700, Roy Franz wrote: > On Wed, Apr 15, 2015 at 8:45 AM, Andy Lutomirski <luto@amacapital.net> wrote: > > On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman" > > <gregkh@linuxfoundation.org> wrote: > >> > >> On Tue, Apr 14, 2015 at 11:52:48AM -0400, Andy Lutomirski wrote: > >> > On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman > >> > <gregkh@linuxfoundation.org> wrote: > >> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > >> > >> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > >> > >> > >> > >> Introducing a kernel module to expose capsule loader interface > >> > >> for user to upload capsule binaries. This module leverage the > >> > >> request_firmware_direct_full_path() to obtain the binary at a > >> > >> specific path input by user. > >> > >> > >> > >> Example method to load the capsule binary: > >> > >> echo -n "/path/to/capsule/binary" > /sys/devices/platform/efi_capsule_loader/capsule_loader > >> > > > >> > > Ick, why not just have the firmware file location present, and copy it > >> > > to the sysfs file directly from userspace, instead of this two-step > >> > > process? > >> > > >> > Because it's not at all obvious how error handling should work in that case. > >> > >> I don't understand how the error handling is any different. The kernel > >> ends up copying the data in through the firmware interface both ways, we > >> just aren't creating yet-another-firmware-path in the system. > > > > If I run uefi-update-capsule foo.bin, I want it to complain if the > > UEFI call fails. In contrast, if I boot and my ath10k firmware is > > bad, there's no explicit user interaction that should fail; instead I > > have no network device and I get to read the logs and figure out why. > > IOW bad volatile device firmware is just like a bad device driver, > > whereas bad UEFI capsules are problems that should be reported to > > whatever tried to send them to UEFI. > > > > --Andy > > > There are several types of errors that can be returned by > UpdateCapsule(), allowing > us to distinguish between capsules that are not supported by the > platform, capsules > that must be updated at boot time, and capsule updates that failed due > to a device error. > I think we really do want this type of information returned to capsule loader. Ok, all of that sounds like you really want a character device, with an ioctl, to do this. Just use a misc device and your infrastructure will be handled for you (sysfs, character device, etc.) and away you go. thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-14 14:09 ` Greg Kroah-Hartman 2015-04-14 15:52 ` Andy Lutomirski @ 2015-04-15 11:32 ` Kweh, Hock Leong 2015-04-15 13:19 ` Greg Kroah-Hartman 1 sibling, 1 reply; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-15 11:32 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov > -----Original Message----- > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > Sent: Tuesday, April 14, 2015 10:09 PM > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > > > Introducing a kernel module to expose capsule loader interface > > for user to upload capsule binaries. This module leverage the > > request_firmware_direct_full_path() to obtain the binary at a > > specific path input by user. > > > > Example method to load the capsule binary: > > echo -n "/path/to/capsule/binary" > > /sys/devices/platform/efi_capsule_loader/capsule_loader > > Ick, why not just have the firmware file location present, and copy it > to the sysfs file directly from userspace, instead of this two-step > process? Err .... I may not catch your meaning correctly. Are you trying to say that you would prefer the user to perform: cat file.bin > /sys/.../capsule_loader instead of echo -n "/path/to/binary" > /sys/..../capsule_laoder The reason we stick with the firmware_class is because we don't want to replicate a code which already mature and has open API for developer to use. > > > +/* > > + * To remove this kernel module, just perform: > > + * rmmod efi_capsule_loader.ko > > This comment is not needed. Okay, I will remove this. > > > > + */ > > +static void __exit efi_capsule_loader_exit(void) > > +{ > > + platform_device_unregister(efi_capsule_pdev); > > This is not a platform device, don't abuse that interface please. > > greg k-h Okay, so you would recommend to use device_register() for this case? Or you would think that this is more suitable to use class_register()? Thanks & Regards, Wilson ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-15 11:32 ` Kweh, Hock Leong @ 2015-04-15 13:19 ` Greg Kroah-Hartman 2015-04-16 9:42 ` Kweh, Hock Leong 2015-04-22 15:35 ` James Bottomley 0 siblings, 2 replies; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-15 13:19 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > -----Original Message----- > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > > > > > Introducing a kernel module to expose capsule loader interface > > > for user to upload capsule binaries. This module leverage the > > > request_firmware_direct_full_path() to obtain the binary at a > > > specific path input by user. > > > > > > Example method to load the capsule binary: > > > echo -n "/path/to/capsule/binary" > > > /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > Ick, why not just have the firmware file location present, and copy it > > to the sysfs file directly from userspace, instead of this two-step > > process? > > Err .... I may not catch your meaning correctly. Are you trying to say > that you would prefer the user to perform: > > cat file.bin > /sys/.../capsule_loader > > instead of > > echo -n "/path/to/binary" > /sys/..../capsule_laoder Yes. What's the namespace of your /path/to/binary/ and how do you know the kernel has the same one when it does the firmware load call? By just copying the data with 'cat', you don't have to worry about namespace issues at all. > The reason we stick with the firmware_class is because we don't > want to replicate a code which already mature and has open API > for developer to use. That's fine, but adding a new api to the firmware interface seems odd to me, just because you don't like using /lib/ or any of the other "standard" locations for firmware blobs. And note, that path is configurable. > > > + */ > > > +static void __exit efi_capsule_loader_exit(void) > > > +{ > > > + platform_device_unregister(efi_capsule_pdev); > > > > This is not a platform device, don't abuse that interface please. > > > > greg k-h > > Okay, so you would recommend to use device_register() for this case? > Or you would think that this is more suitable to use class_register()? A class isn't needed, you just want a device right? So just use a device, but not a platform device, as that isn't what you have here. thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-15 13:19 ` Greg Kroah-Hartman @ 2015-04-16 9:42 ` Kweh, Hock Leong 2015-04-17 13:49 ` Greg Kroah-Hartman 2015-04-22 15:35 ` James Bottomley 1 sibling, 1 reply; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-16 9:42 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov > -----Original Message----- > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > Sent: Wednesday, April 15, 2015 9:19 PM > > On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > > -----Original Message----- > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > > > > > > > Introducing a kernel module to expose capsule loader interface > > > > for user to upload capsule binaries. This module leverage the > > > > request_firmware_direct_full_path() to obtain the binary at a > > > > specific path input by user. > > > > > > > > Example method to load the capsule binary: > > > > echo -n "/path/to/capsule/binary" > > > > /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > > > Ick, why not just have the firmware file location present, and copy it > > > to the sysfs file directly from userspace, instead of this two-step > > > process? > > > > Err .... I may not catch your meaning correctly. Are you trying to say > > that you would prefer the user to perform: > > > > cat file.bin > /sys/.../capsule_loader > > > > instead of > > > > echo -n "/path/to/binary" > /sys/..../capsule_laoder > > Yes. What's the namespace of your /path/to/binary/ and how do you know > the kernel has the same one when it does the firmware load call? By > just copying the data with 'cat', you don't have to worry about > namespace issues at all. Hi Greg, Let me double confirm that I understand your concern correctly. You are trying to tell that some others module may use a 'same' namespace to request the firmware but never release it. Then when our module trying to request the firmware by passing in the 'same' namespace, I will get the previous data instead of the current binary data from the path I want. Hmm .... I believe this concern also apply to all the current request_firmware APIs right? And I believe the coincidence to have 'same' file name namespace would be higher than full path namespace. I may not able to control other module developers on the namespace naming. But I could ensure each firmware request will be released before the next request firmware happen in this module. This module will return error if it does not receive a real efi capsule binary. Hmm ...... > > > The reason we stick with the firmware_class is because we don't > > want to replicate a code which already mature and has open API > > for developer to use. > > That's fine, but adding a new api to the firmware interface seems odd to > me, just because you don't like using /lib/ or any of the other > "standard" locations for firmware blobs. And note, that path is > configurable. If I am not mistaken, I believe you are referring the module param path. Yes, I do aware of it. If I need a more flexibility method to alter the custom path, the current design would require system reboot or module re-insmod. So, leveraging the request_firmware_direct_full_path() could make things a little bit easier from this point of view. Besides, this new API actually helps to overcome the confusedness of having 2 or more same file name binaries at the firmware search paths (fw_path_para; /lib/firmware/update/; /lib/firmware/). Due to user have the possibility to configure kernel command param / module param for this fw_path_para, it may have chances to point to a path that have same file name the /lib/firmware/ also have. With this API, it can make it specific to the path that developer wants. > > > > > + */ > > > > +static void __exit efi_capsule_loader_exit(void) > > > > +{ > > > > + platform_device_unregister(efi_capsule_pdev); > > > > > > This is not a platform device, don't abuse that interface please. > > > > > > greg k-h > > > > Okay, so you would recommend to use device_register() for this case? > > Or you would think that this is more suitable to use class_register()? > > A class isn't needed, you just want a device right? So just use a > device, but not a platform device, as that isn't what you have here. > > thanks, > > greg k-h Okay, will do this. Thanks. Regards, Wilson ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-16 9:42 ` Kweh, Hock Leong @ 2015-04-17 13:49 ` Greg Kroah-Hartman 2015-04-17 14:36 ` Matt Fleming 2015-04-20 17:59 ` James Bottomley 0 siblings, 2 replies; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-17 13:49 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Thu, Apr 16, 2015 at 09:42:31AM +0000, Kweh, Hock Leong wrote: > > -----Original Message----- > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > Sent: Wednesday, April 15, 2015 9:19 PM > > > > On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > > > -----Original Message----- > > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > > > > > > > > > Introducing a kernel module to expose capsule loader interface > > > > > for user to upload capsule binaries. This module leverage the > > > > > request_firmware_direct_full_path() to obtain the binary at a > > > > > specific path input by user. > > > > > > > > > > Example method to load the capsule binary: > > > > > echo -n "/path/to/capsule/binary" > > > > > /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > > > > > Ick, why not just have the firmware file location present, and copy it > > > > to the sysfs file directly from userspace, instead of this two-step > > > > process? > > > > > > Err .... I may not catch your meaning correctly. Are you trying to say > > > that you would prefer the user to perform: > > > > > > cat file.bin > /sys/.../capsule_loader > > > > > > instead of > > > > > > echo -n "/path/to/binary" > /sys/..../capsule_laoder > > > > Yes. What's the namespace of your /path/to/binary/ and how do you know > > the kernel has the same one when it does the firmware load call? By > > just copying the data with 'cat', you don't have to worry about > > namespace issues at all. > > Hi Greg, > > Let me double confirm that I understand your concern correctly. You are > trying to tell that some others module may use a 'same' namespace to > request the firmware but never release it. Then when our module trying > to request the firmware by passing in the 'same' namespace, I will get the > previous data instead of the current binary data from the path I want. Yes. > Hmm .... I believe this concern also apply to all the current request_firmware > APIs right? And I believe the coincidence to have 'same' file name namespace > would be higher than full path namespace. Not really, the kernel namespace is what matters at that point in time. And maybe it does matter, I haven't thought through all of the issues. But passing a path from userspace, to the kernel, to have the kernel turn around again and use that path is full of nasty consequences at times due to namespaces, let's avoid all of that please. thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-17 13:49 ` Greg Kroah-Hartman @ 2015-04-17 14:36 ` Matt Fleming 2015-04-20 3:28 ` Kweh, Hock Leong 2015-04-20 17:59 ` James Bottomley 1 sibling, 1 reply; 62+ messages in thread From: Matt Fleming @ 2015-04-17 14:36 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Fri, 17 Apr, at 03:49:24PM, Greg KH wrote: > > Not really, the kernel namespace is what matters at that point in time. > > And maybe it does matter, I haven't thought through all of the issues. > But passing a path from userspace, to the kernel, to have the kernel > turn around again and use that path is full of nasty consequences at > times due to namespaces, let's avoid all of that please. Oh crap. The namespace issue is a good point and not something I'd thought of at all. At this point, I think we've really run out of objections to Andy's suggestion of implementing this as a misc device, right? The concern I had about userspace tooling can partly be addressed by including the source in tools/ in the kernel tree. So provided we do that, I'm happy enough to see this implemented as a misc device because the other options we've explored just haven't panned out. Objections? -- Matt Fleming, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-17 14:36 ` Matt Fleming @ 2015-04-20 3:28 ` Kweh, Hock Leong 2015-04-20 14:43 ` Greg Kroah-Hartman 0 siblings, 1 reply; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-20 3:28 UTC (permalink / raw) To: Matt Fleming, Greg Kroah-Hartman, Andy Lutomirski Cc: Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov > -----Original Message----- > From: Matt Fleming [mailto:matt@codeblueprint.co.uk] > Sent: Friday, April 17, 2015 10:37 PM > > On Fri, 17 Apr, at 03:49:24PM, Greg KH wrote: > > > > Not really, the kernel namespace is what matters at that point in time. > > > > And maybe it does matter, I haven't thought through all of the issues. > > But passing a path from userspace, to the kernel, to have the kernel > > turn around again and use that path is full of nasty consequences at > > times due to namespaces, let's avoid all of that please. > > Oh crap. The namespace issue is a good point and not something I'd > thought of at all. > > At this point, I think we've really run out of objections to Andy's > suggestion of implementing this as a misc device, right? > > The concern I had about userspace tooling can partly be addressed by > including the source in tools/ in the kernel tree. So provided we do > that, I'm happy enough to see this implemented as a misc device because > the other options we've explored just haven't panned out. > > Objections? > > -- > Matt Fleming, Intel Open Source Technology Center Hi Greg, Matt & Andy, Wait .... wait a minute. I am lost. I think I have missed something. Let me summarize the message I got from the email threads. ========================================================= Greg:- Recommends to use "cat file.bin > /sys/.../capsule_loader" due to there is concern on kernel namespace with this submitted design which using the request firmware API. Andy:- Prefer to have an interface that could return the error code and suggested char device where the ioctl can meet the purpose. Matt:- Prefer to use kernel interface only as embedded world may not want to include userland tool. ========================================================== I think I have missed somewhere why not using "cat file.bin > /sys/.../loader" and now change to misc device. Is it because the ioctl could return a custom error code (for example: platform not supported, capsule header error) where the "cat file.bin > /sys/.../loader" interface cannot do? Hmm ...... Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match with the capsule upload binary? Is it okay to have one sysfs file note to tell the overall status (for example: 10 capsule binaries uploaded but one require reboot, so the status shows reboot require is yes)? I am not here trying to argue anything. I am just trying to find out what kind of info is needed but the sysfs could not provide. Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB space and you don't even have the gcc compiler included into the package. I believe in this environment, kernel interface + shell command is the only interaction that user could work with. Btw, just to make sure I get it correctly, is misc device refer to the device that created by misc_register() from drivers/char/misc.c and not asked to put this kernel module under drivers/misc/* location, right? And Matt mentioned including the source into tools/* in kernel tree. I have a question: Is this tool can be compiled during kernel compilation and eventually auto included into the rootfs package? Sorry, I am new to OS creation and maybe this is stupid question. Thanks & Regards, Wilson ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-20 3:28 ` Kweh, Hock Leong @ 2015-04-20 14:43 ` Greg Kroah-Hartman 2015-04-21 3:23 ` Kweh, Hock Leong 0 siblings, 1 reply; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-20 14:43 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Matt Fleming, Andy Lutomirski, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov On Mon, Apr 20, 2015 at 03:28:32AM +0000, Kweh, Hock Leong wrote: > Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match > with the capsule upload binary? Is it okay to have one sysfs file note to tell the > overall status (for example: 10 capsule binaries uploaded but one require > reboot, so the status shows reboot require is yes)? I am not here trying to argue > anything. I am just trying to find out what kind of info is needed but the sysfs > could not provide. > > Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB > space and you don't even have the gcc compiler included into the package. > I believe in this environment, kernel interface + shell command is the only > interaction that user could work with. Why would you have to have gcc on such a system? Why is that a requirement for having an ioctl/char interface? And if you only have 6Mb of space, you don't have UEFI, sorry, there's no way that firmware can get that small. > Btw, just to make sure I get it correctly, is misc device refer to the device > that created by misc_register() from drivers/char/misc.c and not asked to > put this kernel module under drivers/misc/* location, right? Yes, use misc_register() > And Matt mentioned including the source into tools/* in kernel tree. I have > a question: Is this tool can be compiled during kernel compilation and > eventually auto included into the rootfs package? Sorry, I am new to OS > creation and maybe this is stupid question. If you ask to build it as part of the configuration, yes, it can be built. See how the tools are build as part of the kernel tree for more information about this. thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-20 14:43 ` Greg Kroah-Hartman @ 2015-04-21 3:23 ` Kweh, Hock Leong 2015-04-21 7:56 ` Greg Kroah-Hartman 0 siblings, 1 reply; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-21 3:23 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Matt Fleming, Andy Lutomirski, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov > -----Original Message----- > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > Sent: Monday, April 20, 2015 10:43 PM > > On Mon, Apr 20, 2015 at 03:28:32AM +0000, Kweh, Hock Leong wrote: > > Regarding the 'reboot require' status, is it critical to have a 1 to 1 status > match > > with the capsule upload binary? Is it okay to have one sysfs file note to tell > the > > overall status (for example: 10 capsule binaries uploaded but one require > > reboot, so the status shows reboot require is yes)? I am not here trying to > argue > > anything. I am just trying to find out what kind of info is needed but the > sysfs > > could not provide. > > > > Please imagine if your whole Linux system (kernel + rootfs) has to fit into > 6MB > > space and you don't even have the gcc compiler included into the package. > > I believe in this environment, kernel interface + shell command is the only > > interaction that user could work with. > > Why would you have to have gcc on such a system? Why is that a > requirement for having an ioctl/char interface? This is my logic: - Besides writing a C program (for example), I am not aware any shell script could perform an ioctl function call. This led me to if I don't have an execution binary then I need a compiler to compile the source to execution binary. - For embedded product as mentioned above, not all vendors willing to carry the userland tool when they are struggling to fit into small memory space. Yet, you may say this tool would not eat up a lot of space compare to others. But when the source of this tool being upstream-ed to the tools/ kernel tree, we cannot stop people to contribute and make the tool more features support, eventually the embedded product may need to drop the tool. > > And if you only have 6Mb of space, you don't have UEFI, sorry, there's > no way that firmware can get that small. Actually there is. Quark is one of the examples. The kernel + rootfs take up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. If you have an Intel Galileo board, you don't need any mass storage (SD & USB), you are able to boot to Linux console. Thanks & Regards, Wilson ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-21 3:23 ` Kweh, Hock Leong @ 2015-04-21 7:56 ` Greg Kroah-Hartman 2015-04-22 1:21 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-21 7:56 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Matt Fleming, Andy Lutomirski, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov On Tue, Apr 21, 2015 at 03:23:55AM +0000, Kweh, Hock Leong wrote: > > -----Original Message----- > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > Sent: Monday, April 20, 2015 10:43 PM > > > > On Mon, Apr 20, 2015 at 03:28:32AM +0000, Kweh, Hock Leong wrote: > > > Regarding the 'reboot require' status, is it critical to have a 1 to 1 status > > match > > > with the capsule upload binary? Is it okay to have one sysfs file note to tell > > the > > > overall status (for example: 10 capsule binaries uploaded but one require > > > reboot, so the status shows reboot require is yes)? I am not here trying to > > argue > > > anything. I am just trying to find out what kind of info is needed but the > > sysfs > > > could not provide. > > > > > > Please imagine if your whole Linux system (kernel + rootfs) has to fit into > > 6MB > > > space and you don't even have the gcc compiler included into the package. > > > I believe in this environment, kernel interface + shell command is the only > > > interaction that user could work with. > > > > Why would you have to have gcc on such a system? Why is that a > > requirement for having an ioctl/char interface? > > This is my logic: > - Besides writing a C program (for example), I am not aware any shell script > could perform an ioctl function call. This led me to if I don't have an execution > binary then I need a compiler to compile the source to execution binary. You would have built it on a separate machine, like the one you used to build your kernel and other binary packages you are running. > - For embedded product as mentioned above, not all vendors willing to carry > the userland tool when they are struggling to fit into small memory space. > Yet, you may say this tool would not eat up a lot of space compare to others. > But when the source of this tool being upstream-ed to the tools/ kernel tree, > we cannot stop people to contribute and make the tool more features support, > eventually the embedded product may need to drop the tool. So because someday in the future someone might make the code that is open source too big for us to use, we are going to reject the idea today? That really doesn't make any sense. > > And if you only have 6Mb of space, you don't have UEFI, sorry, there's > > no way that firmware can get that small. > > Actually there is. Quark is one of the examples. The kernel + rootfs take > up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. > If you have an Intel Galileo board, you don't need any mass storage (SD & USB), > you are able to boot to Linux console. Does Galieleo support this UEFI feature? If so, great, how big is a binary that can read a file and write the data using an ioctl? thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-21 7:56 ` Greg Kroah-Hartman @ 2015-04-22 1:21 ` James Bottomley 2015-04-22 1:58 ` Andy Lutomirski 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-22 1:21 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Matt Fleming, Andy Lutomirski, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov On Tue, 2015-04-21 at 09:56 +0200, Greg Kroah-Hartman wrote: > On Tue, Apr 21, 2015 at 03:23:55AM +0000, Kweh, Hock Leong wrote: > > > -----Original Message----- > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > Sent: Monday, April 20, 2015 10:43 PM > > > > > > On Mon, Apr 20, 2015 at 03:28:32AM +0000, Kweh, Hock Leong wrote: > > > > Regarding the 'reboot require' status, is it critical to have a 1 to 1 status > > > match > > > > with the capsule upload binary? Is it okay to have one sysfs file note to tell > > > the > > > > overall status (for example: 10 capsule binaries uploaded but one require > > > > reboot, so the status shows reboot require is yes)? I am not here trying to > > > argue > > > > anything. I am just trying to find out what kind of info is needed but the > > > sysfs > > > > could not provide. > > > > > > > > Please imagine if your whole Linux system (kernel + rootfs) has to fit into > > > 6MB > > > > space and you don't even have the gcc compiler included into the package. > > > > I believe in this environment, kernel interface + shell command is the only > > > > interaction that user could work with. > > > > > > Why would you have to have gcc on such a system? Why is that a > > > requirement for having an ioctl/char interface? > > > > This is my logic: > > - Besides writing a C program (for example), I am not aware any shell script > > could perform an ioctl function call. This led me to if I don't have an execution > > binary then I need a compiler to compile the source to execution binary. > > You would have built it on a separate machine, like the one you used to > build your kernel and other binary packages you are running. > > > - For embedded product as mentioned above, not all vendors willing to carry > > the userland tool when they are struggling to fit into small memory space. > > Yet, you may say this tool would not eat up a lot of space compare to others. > > But when the source of this tool being upstream-ed to the tools/ kernel tree, > > we cannot stop people to contribute and make the tool more features support, > > eventually the embedded product may need to drop the tool. > > So because someday in the future someone might make the code that is > open source too big for us to use, we are going to reject the idea > today? That really doesn't make any sense. I think we can all agree that interfaces that don't require special purpose tools are easier to use. That doesn't mean that every interface has to not use them, because that would be impossible. However it does mean that if we can get away without using them, we should. > > > And if you only have 6Mb of space, you don't have UEFI, sorry, there's > > > no way that firmware can get that small. > > > > Actually there is. Quark is one of the examples. The kernel + rootfs take > > up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. > > If you have an Intel Galileo board, you don't need any mass storage (SD & USB), > > you are able to boot to Linux console. > > Does Galieleo support this UEFI feature? If so, great, how big is a > binary that can read a file and write the data using an ioctl? The capsule file is usually the same size as the NVRAM for an embedded system (on Galileo Gen I, this is 8MB) it usually includes not only the UEFI but also the OS payload. I actually load UEFI only capsules on Galileo and they're around 2MB. There have been a lot of red herrings in this thread (like namespace confusion and ideas about how big UEFI FW can be), but the problem summary is: We need a way of updating FW including payload via the UEFI capsule mechanism. Since the payload is usually as big as the NVRAM and the NVRAM contains the OS, we can't place the payload at any OS location. Unlike usual firmware, the capsule update is fire and forget (once the update is done we don't need the capsule file anymore). So what we need is a way to load the capsule data from arbitrary storage and then trigger the update. Andy, just on the misc device idea, what about triggering the capsule update from close()? In theory close returns an error code (not sure if most tools actually check this, though). That means we can do the write in chunks but pass it in atomically on the close and cat will work (provided it checks the return code of close). James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 1:21 ` James Bottomley @ 2015-04-22 1:58 ` Andy Lutomirski 2015-04-22 2:20 ` James Bottomley 2015-04-22 13:27 ` Peter Jones 0 siblings, 2 replies; 62+ messages in thread From: Andy Lutomirski @ 2015-04-22 1:58 UTC (permalink / raw) To: James Bottomley Cc: Greg Kroah-Hartman, Kweh, Hock Leong, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Roy Franz, Borislav Petkov On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > Andy, just on the misc device idea, what about triggering the capsule > update from close()? In theory close returns an error code (not sure if > most tools actually check this, though). That means we can do the write > in chunks but pass it in atomically on the close and cat will work > (provided it checks the return code of close). I thought about this but IIRC cat doesn't check the return value from close. --Andy > > James > > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 1:58 ` Andy Lutomirski @ 2015-04-22 2:20 ` James Bottomley 2015-04-22 3:24 ` Andy Lutomirski 2015-04-22 13:27 ` Peter Jones 1 sibling, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-22 2:20 UTC (permalink / raw) To: Andy Lutomirski Cc: Greg Kroah-Hartman, Kweh, Hock Leong, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote: > On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > Andy, just on the misc device idea, what about triggering the capsule > > update from close()? In theory close returns an error code (not sure if > > most tools actually check this, though). That means we can do the write > > in chunks but pass it in atomically on the close and cat will work > > (provided it checks the return code of close). > > I thought about this but IIRC cat doesn't check the return value from close. It does in my copy (coreutils-8.23) : if (!STREQ (infile, "-") && close (input_desc) < 0) { error (0, errno, "%s", infile); ok = false; } [...] if (have_read_stdin && close (STDIN_FILENO) < 0) error (EXIT_FAILURE, errno, _("closing standard input")); James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 2:20 ` James Bottomley @ 2015-04-22 3:24 ` Andy Lutomirski 2015-04-22 4:51 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Andy Lutomirski @ 2015-04-22 3:24 UTC (permalink / raw) To: James Bottomley Cc: Greg Kroah-Hartman, Kweh, Hock Leong, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote: >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley >> <James.Bottomley@hansenpartnership.com> wrote: >> > Andy, just on the misc device idea, what about triggering the capsule >> > update from close()? In theory close returns an error code (not sure if >> > most tools actually check this, though). That means we can do the write >> > in chunks but pass it in atomically on the close and cat will work >> > (provided it checks the return code of close). >> >> I thought about this but IIRC cat doesn't check the return value from close. > > It does in my copy (coreutils-8.23) : > > if (!STREQ (infile, "-") && close (input_desc) < 0) > { > error (0, errno, "%s", infile); > ok = false; > } > [...] > if (have_read_stdin && close (STDIN_FILENO) < 0) > error (EXIT_FAILURE, errno, _("closing standard input")); > True, but it's stdout that we care about, not stdin :( --Andy > > James > > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 3:24 ` Andy Lutomirski @ 2015-04-22 4:51 ` James Bottomley 2015-04-22 16:50 ` Andy Lutomirski 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-22 4:51 UTC (permalink / raw) To: Andy Lutomirski Cc: Greg Kroah-Hartman, Kweh, Hock Leong, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote: > On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote: > >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley > >> <James.Bottomley@hansenpartnership.com> wrote: > >> > Andy, just on the misc device idea, what about triggering the capsule > >> > update from close()? In theory close returns an error code (not sure if > >> > most tools actually check this, though). That means we can do the write > >> > in chunks but pass it in atomically on the close and cat will work > >> > (provided it checks the return code of close). > >> > >> I thought about this but IIRC cat doesn't check the return value from close. > > > > It does in my copy (coreutils-8.23) : > > > > if (!STREQ (infile, "-") && close (input_desc) < 0) > > { > > error (0, errno, "%s", infile); > > ok = false; > > } > > [...] > > if (have_read_stdin && close (STDIN_FILENO) < 0) > > error (EXIT_FAILURE, errno, _("closing standard input")); > > > > True, but it's stdout that we care about, not stdin :( Gosh you're determined to force me to wade through this source code, aren't you? That's handled in lib/closeout.c: /* Close standard output. On error, issue a diagnostic and _exit with status 'exit_failure'. ... The point is that, admittedly much to my surprise, it all looks to be handled by cat ... so we could proceed to have the transaction completed in close in a misc device (or a sysfs file). Unless there are any other rabbits you'd like to pull out of the hat? James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 4:51 ` James Bottomley @ 2015-04-22 16:50 ` Andy Lutomirski 2015-04-22 17:34 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Andy Lutomirski @ 2015-04-22 16:50 UTC (permalink / raw) To: James Bottomley Cc: Greg KH, Kweh Hock Leong, LKML, Matt Fleming, Ong, Boon Leong, linux-efi, Ming Lei On Apr 21, 2015 9:51 PM, "James Bottomley" <James.Bottomley@hansenpartnership.com> wrote: > > On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote: > > On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley > > <James.Bottomley@hansenpartnership.com> wrote: > > > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote: > > >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley > > >> <James.Bottomley@hansenpartnership.com> wrote: > > >> > Andy, just on the misc device idea, what about triggering the capsule > > >> > update from close()? In theory close returns an error code (not sure if > > >> > most tools actually check this, though). That means we can do the write > > >> > in chunks but pass it in atomically on the close and cat will work > > >> > (provided it checks the return code of close). > > >> > > >> I thought about this but IIRC cat doesn't check the return value from close. > > > > > > It does in my copy (coreutils-8.23) : > > > > > > if (!STREQ (infile, "-") && close (input_desc) < 0) > > > { > > > error (0, errno, "%s", infile); > > > ok = false; > > > } > > > [...] > > > if (have_read_stdin && close (STDIN_FILENO) < 0) > > > error (EXIT_FAILURE, errno, _("closing standard input")); > > > > > > > True, but it's stdout that we care about, not stdin :( > > Gosh you're determined to force me to wade through this source code, > aren't you? That's handled in lib/closeout.c: > > /* Close standard output. On error, issue a diagnostic and _exit > with status 'exit_failure'. > > ... > > > The point is that, admittedly much to my surprise, it all looks to be > handled by cat ... so we could proceed to have the transaction completed > in close in a misc device (or a sysfs file). > > Unless there are any other rabbits you'd like to pull out of the hat? No, maybe it's okay, unless there's an issue where the error would only be returned on the close of the last reference of the struct file. After all, 'cat foo >/sys/bar' doesn't fully close /sys/bar until after cat exits. --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 16:50 ` Andy Lutomirski @ 2015-04-22 17:34 ` James Bottomley 2015-04-22 17:45 ` Andy Lutomirski 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-22 17:34 UTC (permalink / raw) To: Andy Lutomirski Cc: Greg KH, Kweh Hock Leong, LKML, Matt Fleming, Ong, Boon Leong, linux-efi, Ming Lei On Wed, 2015-04-22 at 09:50 -0700, Andy Lutomirski wrote: > On Apr 21, 2015 9:51 PM, "James Bottomley" > <James.Bottomley@hansenpartnership.com> wrote: > > > > On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote: > > > On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley > > > <James.Bottomley@hansenpartnership.com> wrote: > > > > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote: > > > >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley > > > >> <James.Bottomley@hansenpartnership.com> wrote: > > > >> > Andy, just on the misc device idea, what about triggering the capsule > > > >> > update from close()? In theory close returns an error code (not sure if > > > >> > most tools actually check this, though). That means we can do the write > > > >> > in chunks but pass it in atomically on the close and cat will work > > > >> > (provided it checks the return code of close). > > > >> > > > >> I thought about this but IIRC cat doesn't check the return value from close. > > > > > > > > It does in my copy (coreutils-8.23) : > > > > > > > > if (!STREQ (infile, "-") && close (input_desc) < 0) > > > > { > > > > error (0, errno, "%s", infile); > > > > ok = false; > > > > } > > > > [...] > > > > if (have_read_stdin && close (STDIN_FILENO) < 0) > > > > error (EXIT_FAILURE, errno, _("closing standard input")); > > > > > > > > > > True, but it's stdout that we care about, not stdin :( > > > > Gosh you're determined to force me to wade through this source code, > > aren't you? That's handled in lib/closeout.c: > > > > /* Close standard output. On error, issue a diagnostic and _exit > > with status 'exit_failure'. > > > > ... > > > > > > The point is that, admittedly much to my surprise, it all looks to be > > handled by cat ... so we could proceed to have the transaction completed > > in close in a misc device (or a sysfs file). > > > > Unless there are any other rabbits you'd like to pull out of the hat? > > No, maybe it's okay, unless there's an issue where the error would > only be returned on the close of the last reference of the struct > file. After all, 'cat foo >/sys/bar' doesn't fully close /sys/bar > until after cat exits. No, cat handles that too. It has an atexit() handler for closing stdout. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 17:34 ` James Bottomley @ 2015-04-22 17:45 ` Andy Lutomirski 0 siblings, 0 replies; 62+ messages in thread From: Andy Lutomirski @ 2015-04-22 17:45 UTC (permalink / raw) To: James Bottomley Cc: Greg KH, Kweh Hock Leong, LKML, Matt Fleming, Ong, Boon Leong, linux-efi, Ming Lei On Wed, Apr 22, 2015 at 10:34 AM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Wed, 2015-04-22 at 09:50 -0700, Andy Lutomirski wrote: >> On Apr 21, 2015 9:51 PM, "James Bottomley" >> <James.Bottomley@hansenpartnership.com> wrote: >> > >> > On Tue, 2015-04-21 at 20:24 -0700, Andy Lutomirski wrote: >> > > On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley >> > > <James.Bottomley@hansenpartnership.com> wrote: >> > > > On Tue, 2015-04-21 at 18:58 -0700, Andy Lutomirski wrote: >> > > >> On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley >> > > >> <James.Bottomley@hansenpartnership.com> wrote: >> > > >> > Andy, just on the misc device idea, what about triggering the capsule >> > > >> > update from close()? In theory close returns an error code (not sure if >> > > >> > most tools actually check this, though). That means we can do the write >> > > >> > in chunks but pass it in atomically on the close and cat will work >> > > >> > (provided it checks the return code of close). >> > > >> >> > > >> I thought about this but IIRC cat doesn't check the return value from close. >> > > > >> > > > It does in my copy (coreutils-8.23) : >> > > > >> > > > if (!STREQ (infile, "-") && close (input_desc) < 0) >> > > > { >> > > > error (0, errno, "%s", infile); >> > > > ok = false; >> > > > } >> > > > [...] >> > > > if (have_read_stdin && close (STDIN_FILENO) < 0) >> > > > error (EXIT_FAILURE, errno, _("closing standard input")); >> > > > >> > > >> > > True, but it's stdout that we care about, not stdin :( >> > >> > Gosh you're determined to force me to wade through this source code, >> > aren't you? That's handled in lib/closeout.c: >> > >> > /* Close standard output. On error, issue a diagnostic and _exit >> > with status 'exit_failure'. >> > >> > ... >> > >> > >> > The point is that, admittedly much to my surprise, it all looks to be >> > handled by cat ... so we could proceed to have the transaction completed >> > in close in a misc device (or a sysfs file). >> > >> > Unless there are any other rabbits you'd like to pull out of the hat? >> >> No, maybe it's okay, unless there's an issue where the error would >> only be returned on the close of the last reference of the struct >> file. After all, 'cat foo >/sys/bar' doesn't fully close /sys/bar >> until after cat exits. > > No, cat handles that too. It has an atexit() handler for closing > stdout. Indeed. Strace tells me that: $ cat foo >bar opens bar and then execs cat, so cat has the only reference to bar. So no more rabbits from me :) --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 1:58 ` Andy Lutomirski 2015-04-22 2:20 ` James Bottomley @ 2015-04-22 13:27 ` Peter Jones 2015-04-22 15:18 ` James Bottomley 1 sibling, 1 reply; 62+ messages in thread From: Peter Jones @ 2015-04-22 13:27 UTC (permalink / raw) To: Andy Lutomirski Cc: James Bottomley, Greg Kroah-Hartman, Kweh, Hock Leong, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov On Tue, Apr 21, 2015 at 06:58:58PM -0700, Andy Lutomirski wrote: > On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > Andy, just on the misc device idea, what about triggering the capsule > > update from close()? In theory close returns an error code (not sure if > > most tools actually check this, though). That means we can do the write > > in chunks but pass it in atomically on the close and cat will work > > (provided it checks the return code of close). > > I thought about this but IIRC cat doesn't check the return value from close. I checked this for the use case we'd talked about before - gnu cat /does/ check the error code, but it's easy to miss how, because coreutils code has some good ole' gnu-code complexity. It'll print the strerror() representation, but it always exits with 1 as the error code. Specifically the close on the output is handled by this: --------------- initialize_main (&argc, &argv); set_program_name (argv[0]); setlocale (LC_ALL, ""); bindtextdomain (PACKAGE, LOCALEDIR); textdomain (PACKAGE); /* Arrange to close stdout if we exit via the case_GETOPT_HELP_CHAR or case_GETOPT_VERSION_CHAR code. Normally STDOUT_FILENO is used rather than stdout, so close_stdout does nothing. */ atexit (close_stdout); /* Parse command line options. */ while ((c = getopt_long (argc, argv, "benstuvAET", long_options, NULL)) --------------- Which in turn does: --------------- void close_stdout (void) { if (close_stream (stdout) != 0 && !(ignore_EPIPE && errno == EPIPE)) { char const *write_error = _("write error"); if (file_name) error (0, errno, "%s: %s", quotearg_colon (file_name), write_error); else error (0, errno, "%s", write_error); _exit (exit_failure); } if (close_stream (stderr) != 0) _exit (exit_failure); } --------------- exit_failure is a global from libcoreutils.a which cat never changes from the default, so it's always 1. (And of course error() is coreutils' own implementation rather than glibc's because hey maybe you're not using glibc, but still, it's there.) So it's /annoying/ to propagate the error from there programatically, but it can work. -- Peter ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 13:27 ` Peter Jones @ 2015-04-22 15:18 ` James Bottomley 2015-04-22 15:24 ` One Thousand Gnomes 2015-04-23 8:30 ` Kweh, Hock Leong 0 siblings, 2 replies; 62+ messages in thread From: James Bottomley @ 2015-04-22 15:18 UTC (permalink / raw) To: Peter Jones Cc: Andy Lutomirski, Greg Kroah-Hartman, Kweh, Hock Leong, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov On Wed, 2015-04-22 at 09:27 -0400, Peter Jones wrote: > On Tue, Apr 21, 2015 at 06:58:58PM -0700, Andy Lutomirski wrote: > > On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley > > <James.Bottomley@hansenpartnership.com> wrote: > > > Andy, just on the misc device idea, what about triggering the capsule > > > update from close()? In theory close returns an error code (not sure if > > > most tools actually check this, though). That means we can do the write > > > in chunks but pass it in atomically on the close and cat will work > > > (provided it checks the return code of close). > > > > I thought about this but IIRC cat doesn't check the return value from close. > > I checked this for the use case we'd talked about before - gnu cat > /does/ check the error code, but it's easy to miss how, because > coreutils code has some good ole' gnu-code complexity. It'll print the > strerror() representation, but it always exits with 1 as the error > code. > > Specifically the close on the output is handled by this: > --------------- > initialize_main (&argc, &argv); > set_program_name (argv[0]); > setlocale (LC_ALL, ""); > bindtextdomain (PACKAGE, LOCALEDIR); > textdomain (PACKAGE); > > /* Arrange to close stdout if we exit via the > case_GETOPT_HELP_CHAR or case_GETOPT_VERSION_CHAR code. > Normally STDOUT_FILENO is used rather than stdout, so > close_stdout does nothing. */ > atexit (close_stdout); > > /* Parse command line options. */ > > while ((c = getopt_long (argc, argv, "benstuvAET", long_options, NULL)) > --------------- > > Which in turn does: > --------------- > void > close_stdout (void) > { > if (close_stream (stdout) != 0 > && !(ignore_EPIPE && errno == EPIPE)) > { > char const *write_error = _("write error"); > if (file_name) > error (0, errno, "%s: %s", quotearg_colon (file_name), > write_error); > else > error (0, errno, "%s", write_error); > > _exit (exit_failure); > } > > if (close_stream (stderr) != 0) > _exit (exit_failure); > } > --------------- > > exit_failure is a global from libcoreutils.a which cat never changes > from the default, so it's always 1. > > (And of course error() is coreutils' own implementation rather than > glibc's because hey maybe you're not using glibc, but still, it's > there.) > > So it's /annoying/ to propagate the error from there programatically, > but it can work. Yes, I think we've all agreed we can do it ... it's now a question of whether we can stomach the ick factor of actually initiating a transaction in close ... I'm still feeling queasy. There are quite a few of these 'transactional blob' problems where we'd like to use a file/device approach because the data is just passed to something but have problems because the something wants all or nothing rather than chunks. I think all of us who work at the coal face on this are not enthused by an ioctl solution because of the need for non-standard tools to effect it. The alternative might be a two file approach (either in sysfs or a mini custom fs), one for load up data and the other for initiate transaction with the data errors (like overflow) being returned on the load up file and the transaction errors being returned on the write that initiates the transaction. My architectural sense is that transaction on close, provided we can make it a more universally accepted idea, has a lot of potential because it's more intuitive than the two file approach. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 15:18 ` James Bottomley @ 2015-04-22 15:24 ` One Thousand Gnomes 2015-04-23 8:30 ` Kweh, Hock Leong 1 sibling, 0 replies; 62+ messages in thread From: One Thousand Gnomes @ 2015-04-22 15:24 UTC (permalink / raw) To: James Bottomley Cc: Peter Jones, Andy Lutomirski, Greg Kroah-Hartman, Kweh, Hock Leong, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov > Yes, I think we've all agreed we can do it ... it's now a question of > whether we can stomach the ick factor of actually initiating a > transaction in close ... I'm still feeling queasy. NFS does transactions - including figuring out if the data will fit - on file close. It does that for real data so I'd relax. With hard disks we don't even complete a set of actions on close they just float around for a bit and get committed (but if there is a media failure you lose if you didnt commit them first via fsync etc) > The alternative might be a two file approach (either in sysfs or a mini > custom fs), one for load up data and the other for initiate transaction > with the data errors (like overflow) being returned on the load up file > and the transaction errors being returned on the write that initiates > the transaction. The two file problem then turns into the "which transaction of the two done in parallel" problem. > My architectural sense is that transaction on close, provided we can > make it a more universally accepted idea, has a lot of potential because > it's more intuitive than the two file approach. Agreed Alan ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 15:18 ` James Bottomley 2015-04-22 15:24 ` One Thousand Gnomes @ 2015-04-23 8:30 ` Kweh, Hock Leong 2015-04-23 14:09 ` James Bottomley 1 sibling, 1 reply; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-23 8:30 UTC (permalink / raw) To: James Bottomley, Peter Jones Cc: Andy Lutomirski, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 877 bytes --] > -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > Sent: Wednesday, April 22, 2015 11:19 PM > > > Yes, I think we've all agreed we can do it ... it's now a question of whether we > can stomach the ick factor of actually initiating a transaction in close ... I'm still > feeling queasy. The file "close" here can I understand that the file system will call the "release" function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops->release() and return the error code if update failed in this function? Thanks & Regards, Wilson ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-23 8:30 ` Kweh, Hock Leong @ 2015-04-23 14:09 ` James Bottomley 2015-04-24 2:14 ` Kweh, Hock Leong 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-23 14:09 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Peter Jones, Andy Lutomirski, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: > > -----Original Message----- > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > > Sent: Wednesday, April 22, 2015 11:19 PM > > > > > > Yes, I think we've all agreed we can do it ... it's now a question of whether we > > can stomach the ick factor of actually initiating a transaction in close ... I'm still > > feeling queasy. > > The file "close" here can I understand that the file system will call the "release" > function at the file_operations struct? > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > > So, James you are meaning that we could initiating the update transaction > inside the f_ops->release() and return the error code if update failed in this > function? Well, that's what I was thinking. However the return value of ->release doesn't get propagated in sys_close (or indeed anywhere ... no idea why it returns an int) thanks to the task work additions, so we'd actually have to use the operation whose value is propagated in sys_close() which turns out to be flush. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-23 14:09 ` James Bottomley @ 2015-04-24 2:14 ` Kweh, Hock Leong 2015-04-24 15:16 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-24 2:14 UTC (permalink / raw) To: James Bottomley Cc: Peter Jones, Andy Lutomirski, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1630 bytes --] > -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > Sent: Thursday, April 23, 2015 10:10 PM > > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: > > > -----Original Message----- > > > From: James Bottomley > [mailto:James.Bottomley@HansenPartnership.com] > > > Sent: Wednesday, April 22, 2015 11:19 PM > > > > > > > > > Yes, I think we've all agreed we can do it ... it's now a question of whether > we > > > can stomach the ick factor of actually initiating a transaction in close ... I'm > still > > > feeling queasy. > > > > The file "close" here can I understand that the file system will call the > "release" > > function at the file_operations struct? > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > > > > So, James you are meaning that we could initiating the update transaction > > inside the f_ops->release() and return the error code if update failed in this > > function? > > Well, that's what I was thinking. However the return value of ->release > doesn't get propagated in sys_close (or indeed anywhere ... no idea why > it returns an int) thanks to the task work additions, so we'd actually > have to use the operation whose value is propagated in sys_close() which > turns out to be flush. > > James > Okay, I think I got you. Just to double check for in case: you are meaning to implement it at f_ops->flush() instead of f_ops->release(). Thanks & Regards, Wilson ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-24 2:14 ` Kweh, Hock Leong @ 2015-04-24 15:16 ` James Bottomley 2015-04-27 21:59 ` Andy Lutomirski 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-24 15:16 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Peter Jones, Andy Lutomirski, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, linux-fsdevel On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote: > > -----Original Message----- > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > > Sent: Thursday, April 23, 2015 10:10 PM > > > > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: > > > > -----Original Message----- > > > > From: James Bottomley > > [mailto:James.Bottomley@HansenPartnership.com] > > > > Sent: Wednesday, April 22, 2015 11:19 PM > > > > > > > > > > > > Yes, I think we've all agreed we can do it ... it's now a question of whether > > we > > > > can stomach the ick factor of actually initiating a transaction in close ... I'm > > still > > > > feeling queasy. > > > > > > The file "close" here can I understand that the file system will call the > > "release" > > > function at the file_operations struct? > > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > > > > > > So, James you are meaning that we could initiating the update transaction > > > inside the f_ops->release() and return the error code if update failed in this > > > function? > > > > Well, that's what I was thinking. However the return value of ->release > > doesn't get propagated in sys_close (or indeed anywhere ... no idea why > > it returns an int) thanks to the task work additions, so we'd actually > > have to use the operation whose value is propagated in sys_close() which > > turns out to be flush. > > > > James > > > > Okay, I think I got you. Just to double check for in case: you are meaning > to implement it at f_ops->flush() instead of f_ops->release(). Well, what I'm saying is that the only way to propagate an error to close is by returning one from the flush file_operation. Let's cc fsdevel to see if they have any brighter ideas. The problem is we need to update firmware (several megabytes of it) via standard system tools. We're thinking cat to a device. The problem is that we need an error code back once the update goes through (which it can't until we've fed all the firmware data into the system). To use standard unix tools, we have to trigger off the standard system calls cat uses and since write() will happen in chunks, the only way to commit the transaction is in close(). We initially through of initiating the transaction in f_ops->release and returning the error code there, but that doesn't work because its value isn't actually propagated, so we're now thinking of initiating the transaction in f_ops->flush instead (this is a device, not a file, so it won't get any other flushers). Are there any other ways for us to propagate error on close? James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-24 15:16 ` James Bottomley @ 2015-04-27 21:59 ` Andy Lutomirski 2015-04-27 22:35 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Andy Lutomirski @ 2015-04-27 21:59 UTC (permalink / raw) To: James Bottomley Cc: Kweh, Hock Leong, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote: >> > -----Original Message----- >> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] >> > Sent: Thursday, April 23, 2015 10:10 PM >> > >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: >> > > > -----Original Message----- >> > > > From: James Bottomley >> > [mailto:James.Bottomley@HansenPartnership.com] >> > > > Sent: Wednesday, April 22, 2015 11:19 PM >> > > > >> > > > >> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether >> > we >> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm >> > still >> > > > feeling queasy. >> > > >> > > The file "close" here can I understand that the file system will call the >> > "release" >> > > function at the file_operations struct? >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 >> > > >> > > So, James you are meaning that we could initiating the update transaction >> > > inside the f_ops->release() and return the error code if update failed in this >> > > function? >> > >> > Well, that's what I was thinking. However the return value of ->release >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why >> > it returns an int) thanks to the task work additions, so we'd actually >> > have to use the operation whose value is propagated in sys_close() which >> > turns out to be flush. >> > >> > James >> > >> >> Okay, I think I got you. Just to double check for in case: you are meaning >> to implement it at f_ops->flush() instead of f_ops->release(). > > Well, what I'm saying is that the only way to propagate an error to > close is by returning one from the flush file_operation. > > Let's cc fsdevel to see if they have any brighter ideas. > > The problem is we need to update firmware (several megabytes of it) via > standard system tools. We're thinking cat to a device. The problem is > that we need an error code back once the update goes through (which it > can't until we've fed all the firmware data into the system). To use > standard unix tools, we have to trigger off the standard system calls > cat uses and since write() will happen in chunks, the only way to commit > the transaction is in close(). > > We initially through of initiating the transaction in f_ops->release and > returning the error code there, but that doesn't work because its value > isn't actually propagated, so we're now thinking of initiating the > transaction in f_ops->flush instead (this is a device, not a file, so it > won't get any other flushers). Are there any other ways for us to > propagate error on close? > I think we may end up wanting to support both UpdateCapsule and QueryCapsuleCapabilities, in which case this gets awkward. Maybe we really should do a misc device + ioctl. --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-27 21:59 ` Andy Lutomirski @ 2015-04-27 22:35 ` James Bottomley 2015-04-27 22:40 ` Andy Lutomirski 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-27 22:35 UTC (permalink / raw) To: Andy Lutomirski Cc: Kweh, Hock Leong, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: > On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote: > >> > -----Original Message----- > >> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > >> > Sent: Thursday, April 23, 2015 10:10 PM > >> > > >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: > >> > > > -----Original Message----- > >> > > > From: James Bottomley > >> > [mailto:James.Bottomley@HansenPartnership.com] > >> > > > Sent: Wednesday, April 22, 2015 11:19 PM > >> > > > > >> > > > > >> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether > >> > we > >> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm > >> > still > >> > > > feeling queasy. > >> > > > >> > > The file "close" here can I understand that the file system will call the > >> > "release" > >> > > function at the file_operations struct? > >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > >> > > > >> > > So, James you are meaning that we could initiating the update transaction > >> > > inside the f_ops->release() and return the error code if update failed in this > >> > > function? > >> > > >> > Well, that's what I was thinking. However the return value of ->release > >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why > >> > it returns an int) thanks to the task work additions, so we'd actually > >> > have to use the operation whose value is propagated in sys_close() which > >> > turns out to be flush. > >> > > >> > James > >> > > >> > >> Okay, I think I got you. Just to double check for in case: you are meaning > >> to implement it at f_ops->flush() instead of f_ops->release(). > > > > Well, what I'm saying is that the only way to propagate an error to > > close is by returning one from the flush file_operation. > > > > Let's cc fsdevel to see if they have any brighter ideas. > > > > The problem is we need to update firmware (several megabytes of it) via > > standard system tools. We're thinking cat to a device. The problem is > > that we need an error code back once the update goes through (which it > > can't until we've fed all the firmware data into the system). To use > > standard unix tools, we have to trigger off the standard system calls > > cat uses and since write() will happen in chunks, the only way to commit > > the transaction is in close(). > > > > We initially through of initiating the transaction in f_ops->release and > > returning the error code there, but that doesn't work because its value > > isn't actually propagated, so we're now thinking of initiating the > > transaction in f_ops->flush instead (this is a device, not a file, so it > > won't get any other flushers). Are there any other ways for us to > > propagate error on close? > > > > I think we may end up wanting to support both UpdateCapsule and > QueryCapsuleCapabilities, in which case this gets awkward. Maybe we > really should do a misc device + ioctl. To be honest, I hate ioctls ... especially the "have to use special tools" part. Would we ever want to support QueryCapsuleUpdate()? The return codes on error are the same as UpdateCapsule() but the query call does nothing on success (and the update call updates, obviously), so it seems a bit pointless if someone's gone to the trouble of getting a capsule ... they obviously want to apply it rather than know if it could be applied. Assuming we do, we could just use the same error on close mechanism, but use sysfs binary attributes ... or probably something new like a binary transaction attribute that does all the transaction on close magic for us. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-27 22:35 ` James Bottomley @ 2015-04-27 22:40 ` Andy Lutomirski 2015-04-27 22:51 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Andy Lutomirski @ 2015-04-27 22:40 UTC (permalink / raw) To: James Bottomley Cc: Kweh, Hock Leong, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley >> <James.Bottomley@hansenpartnership.com> wrote: >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote: >> >> > -----Original Message----- >> >> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] >> >> > Sent: Thursday, April 23, 2015 10:10 PM >> >> > >> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: >> >> > > > -----Original Message----- >> >> > > > From: James Bottomley >> >> > [mailto:James.Bottomley@HansenPartnership.com] >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM >> >> > > > >> >> > > > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether >> >> > we >> >> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm >> >> > still >> >> > > > feeling queasy. >> >> > > >> >> > > The file "close" here can I understand that the file system will call the >> >> > "release" >> >> > > function at the file_operations struct? >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 >> >> > > >> >> > > So, James you are meaning that we could initiating the update transaction >> >> > > inside the f_ops->release() and return the error code if update failed in this >> >> > > function? >> >> > >> >> > Well, that's what I was thinking. However the return value of ->release >> >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why >> >> > it returns an int) thanks to the task work additions, so we'd actually >> >> > have to use the operation whose value is propagated in sys_close() which >> >> > turns out to be flush. >> >> > >> >> > James >> >> > >> >> >> >> Okay, I think I got you. Just to double check for in case: you are meaning >> >> to implement it at f_ops->flush() instead of f_ops->release(). >> > >> > Well, what I'm saying is that the only way to propagate an error to >> > close is by returning one from the flush file_operation. >> > >> > Let's cc fsdevel to see if they have any brighter ideas. >> > >> > The problem is we need to update firmware (several megabytes of it) via >> > standard system tools. We're thinking cat to a device. The problem is >> > that we need an error code back once the update goes through (which it >> > can't until we've fed all the firmware data into the system). To use >> > standard unix tools, we have to trigger off the standard system calls >> > cat uses and since write() will happen in chunks, the only way to commit >> > the transaction is in close(). >> > >> > We initially through of initiating the transaction in f_ops->release and >> > returning the error code there, but that doesn't work because its value >> > isn't actually propagated, so we're now thinking of initiating the >> > transaction in f_ops->flush instead (this is a device, not a file, so it >> > won't get any other flushers). Are there any other ways for us to >> > propagate error on close? >> > >> >> I think we may end up wanting to support both UpdateCapsule and >> QueryCapsuleCapabilities, in which case this gets awkward. Maybe we >> really should do a misc device + ioctl. > > To be honest, I hate ioctls ... especially the "have to use special > tools" part. > > Would we ever want to support QueryCapsuleUpdate()? The return codes on > error are the same as UpdateCapsule() but the query call does nothing on > success (and the update call updates, obviously), so it seems a bit > pointless if someone's gone to the trouble of getting a capsule ... they > obviously want to apply it rather than know if it could be applied. I can imagine a UI that would try to validate a transaction consisting of several of these things, tell the user whether it'll work and whether a reboot is needed, and then do it. > > Assuming we do, we could just use the same error on close mechanism, but > use sysfs binary attributes ... or probably something new like a binary > transaction attribute that does all the transaction on close magic for > us. Yeah, but now we have both input and output, so as ugly as ioctl is, it's a pretty good match. Sigh. This is all more complicated than it deserves to me. --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-27 22:40 ` Andy Lutomirski @ 2015-04-27 22:51 ` James Bottomley 2015-04-29 11:23 ` Kweh, Hock Leong 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-27 22:51 UTC (permalink / raw) To: Andy Lutomirski Cc: Kweh, Hock Leong, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley > >> <James.Bottomley@hansenpartnership.com> wrote: > >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote: > >> >> > -----Original Message----- > >> >> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > >> >> > Sent: Thursday, April 23, 2015 10:10 PM > >> >> > > >> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: > >> >> > > > -----Original Message----- > >> >> > > > From: James Bottomley > >> >> > [mailto:James.Bottomley@HansenPartnership.com] > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM > >> >> > > > > >> >> > > > > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether > >> >> > we > >> >> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm > >> >> > still > >> >> > > > feeling queasy. > >> >> > > > >> >> > > The file "close" here can I understand that the file system will call the > >> >> > "release" > >> >> > > function at the file_operations struct? > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 > >> >> > > > >> >> > > So, James you are meaning that we could initiating the update transaction > >> >> > > inside the f_ops->release() and return the error code if update failed in this > >> >> > > function? > >> >> > > >> >> > Well, that's what I was thinking. However the return value of ->release > >> >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why > >> >> > it returns an int) thanks to the task work additions, so we'd actually > >> >> > have to use the operation whose value is propagated in sys_close() which > >> >> > turns out to be flush. > >> >> > > >> >> > James > >> >> > > >> >> > >> >> Okay, I think I got you. Just to double check for in case: you are meaning > >> >> to implement it at f_ops->flush() instead of f_ops->release(). > >> > > >> > Well, what I'm saying is that the only way to propagate an error to > >> > close is by returning one from the flush file_operation. > >> > > >> > Let's cc fsdevel to see if they have any brighter ideas. > >> > > >> > The problem is we need to update firmware (several megabytes of it) via > >> > standard system tools. We're thinking cat to a device. The problem is > >> > that we need an error code back once the update goes through (which it > >> > can't until we've fed all the firmware data into the system). To use > >> > standard unix tools, we have to trigger off the standard system calls > >> > cat uses and since write() will happen in chunks, the only way to commit > >> > the transaction is in close(). > >> > > >> > We initially through of initiating the transaction in f_ops->release and > >> > returning the error code there, but that doesn't work because its value > >> > isn't actually propagated, so we're now thinking of initiating the > >> > transaction in f_ops->flush instead (this is a device, not a file, so it > >> > won't get any other flushers). Are there any other ways for us to > >> > propagate error on close? > >> > > >> > >> I think we may end up wanting to support both UpdateCapsule and > >> QueryCapsuleCapabilities, in which case this gets awkward. Maybe we > >> really should do a misc device + ioctl. > > > > To be honest, I hate ioctls ... especially the "have to use special > > tools" part. > > > > Would we ever want to support QueryCapsuleUpdate()? The return codes on > > error are the same as UpdateCapsule() but the query call does nothing on > > success (and the update call updates, obviously), so it seems a bit > > pointless if someone's gone to the trouble of getting a capsule ... they > > obviously want to apply it rather than know if it could be applied. > > I can imagine a UI that would try to validate a transaction consisting > of several of these things, tell the user whether it'll work and > whether a reboot is needed, and then do it. You mean for dependent capsules? That's a bit way overthinking the UEFI current use case (which is for firmware update). In theory, the persist across reboot flag can be used for OS persistent information (subject to someone actually coming up with an implementation). I'd code for the simple case: firmware update and let the rest take care of itself if and when we have an implementation. The last thing I want to see landing on the UEFI-SST is some hopelessly complex and nasty capsule spec just "because Linux implements it this way". > > Assuming we do, we could just use the same error on close mechanism, but > > use sysfs binary attributes ... or probably something new like a binary > > transaction attribute that does all the transaction on close magic for > > us. > > Yeah, but now we have both input and output, so as ugly as ioctl is, > it's a pretty good match. No, we'll have read and write, so we can do that. As long as there's no transaction that can't complete in close or any sense of multiple transactions that aren't issued by open read/write close, we're covered. > Sigh. This is all more complicated than it deserves to me. Be fair: it is a new interface and it works in a way that's just different enough from regular firmware to cause all this bother. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-27 22:51 ` James Bottomley @ 2015-04-29 11:23 ` Kweh, Hock Leong 2015-04-29 18:40 ` Andy Lutomirski 2015-04-29 21:35 ` James Bottomley 0 siblings, 2 replies; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-29 11:23 UTC (permalink / raw) To: James Bottomley, Andy Lutomirski Cc: Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7250 bytes --] > -----Original Message----- > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] > Sent: Tuesday, April 28, 2015 6:52 AM > > On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: > > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley > > <James.Bottomley@hansenpartnership.com> wrote: > > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: > > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley > > >> <James.Bottomley@hansenpartnership.com> wrote: > > >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote: > > >> >> > -----Original Message----- > > >> >> > From: James Bottomley > > >> >> > [mailto:James.Bottomley@HansenPartnership.com] > > >> >> > Sent: Thursday, April 23, 2015 10:10 PM > > >> >> > > > >> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: > > >> >> > > > -----Original Message----- > > >> >> > > > From: James Bottomley > > >> >> > [mailto:James.Bottomley@HansenPartnership.com] > > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM > > >> >> > > > > > >> >> > > > > > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a > > >> >> > > > question of whether > > >> >> > we > > >> >> > > > can stomach the ick factor of actually initiating a > > >> >> > > > transaction in close ... I'm > > >> >> > still > > >> >> > > > feeling queasy. > > >> >> > > > > >> >> > > The file "close" here can I understand that the file system > > >> >> > > will call the > > >> >> > "release" > > >> >> > > function at the file_operations struct? > > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L153 > > >> >> > > 8 > > >> >> > > > > >> >> > > So, James you are meaning that we could initiating the > > >> >> > > update transaction inside the f_ops->release() and return > > >> >> > > the error code if update failed in this function? > > >> >> > > > >> >> > Well, that's what I was thinking. However the return value of > > >> >> > ->release doesn't get propagated in sys_close (or indeed > > >> >> > anywhere ... no idea why it returns an int) thanks to the task > > >> >> > work additions, so we'd actually have to use the operation > > >> >> > whose value is propagated in sys_close() which turns out to be > flush. > > >> >> > > > >> >> > James > > >> >> > > > >> >> > > >> >> Okay, I think I got you. Just to double check for in case: you > > >> >> are meaning to implement it at f_ops->flush() instead of f_ops- > >release(). > > >> > > > >> > Well, what I'm saying is that the only way to propagate an error > > >> > to close is by returning one from the flush file_operation. > > >> > > > >> > Let's cc fsdevel to see if they have any brighter ideas. > > >> > > > >> > The problem is we need to update firmware (several megabytes of > > >> > it) via standard system tools. We're thinking cat to a device. > > >> > The problem is that we need an error code back once the update > > >> > goes through (which it can't until we've fed all the firmware > > >> > data into the system). To use standard unix tools, we have to > > >> > trigger off the standard system calls cat uses and since write() > > >> > will happen in chunks, the only way to commit the transaction is in > close(). > > >> > > > >> > We initially through of initiating the transaction in > > >> > f_ops->release and returning the error code there, but that > > >> > doesn't work because its value isn't actually propagated, so > > >> > we're now thinking of initiating the transaction in f_ops->flush > > >> > instead (this is a device, not a file, so it won't get any other > > >> > flushers). Are there any other ways for us to propagate error on close? > > >> > > > >> > > >> I think we may end up wanting to support both UpdateCapsule and > > >> QueryCapsuleCapabilities, in which case this gets awkward. Maybe > > >> we really should do a misc device + ioctl. > > > > > > To be honest, I hate ioctls ... especially the "have to use special > > > tools" part. > > > > > > Would we ever want to support QueryCapsuleUpdate()? The return > > > codes on error are the same as UpdateCapsule() but the query call > > > does nothing on success (and the update call updates, obviously), so > > > it seems a bit pointless if someone's gone to the trouble of getting > > > a capsule ... they obviously want to apply it rather than know if it could be > applied. > > > > I can imagine a UI that would try to validate a transaction consisting > > of several of these things, tell the user whether it'll work and > > whether a reboot is needed, and then do it. > > You mean for dependent capsules? That's a bit way overthinking the UEFI > current use case (which is for firmware update). In theory, the persist across > reboot flag can be used for OS persistent information (subject to someone > actually coming up with an implementation). I'd code for the simple case: > firmware update and let the rest take care of itself if and when we have an > implementation. > > The last thing I want to see landing on the UEFI-SST is some hopelessly > complex and nasty capsule spec just "because Linux implements it this way". > > > > Assuming we do, we could just use the same error on close mechanism, > > > but use sysfs binary attributes ... or probably something new like a > > > binary transaction attribute that does all the transaction on close > > > magic for us. > > > > Yeah, but now we have both input and output, so as ugly as ioctl is, > > it's a pretty good match. > > No, we'll have read and write, so we can do that. As long as there's no > transaction that can't complete in close or any sense of multiple transactions > that aren't issued by open read/write close, we're covered. > > > Sigh. This is all more complicated than it deserves to me. > > Be fair: it is a new interface and it works in a way that's just different enough > from regular firmware to cause all this bother. > > James > Dear communities, I agree with James. Due to different people may have different needs. But from our side, we would just like to have a simple interface for us to upload the efi capsule and perform update. We do not have any use case or need to get info from QueryCapsuleUpdate(). Let me give a suggestion here: please allow me to focus on deliver this simple loading interface and upstream it. Then later whoever has the actual use case or needs on the ioctl implementation, he or she could enhance base on this simple loading interface. What do you guys think? Let me summarize the latest design idea: - No longer leverage on firmware class but use misc device - Do not use platform device but use device_create() - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell - File operation functions include: open(), read(), write() and flush() - Perform mutex lock in open() then release the mutex in flush() for avoiding race condition / concurrent loading - Perform the capsule update and error return at flush() function Is there anything I missed? Any one still have concern with this idea? Thanks for providing the ideas as well as the review. Regards, Wilson ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-29 11:23 ` Kweh, Hock Leong @ 2015-04-29 18:40 ` Andy Lutomirski 2015-04-29 21:37 ` James Bottomley 2015-04-30 9:17 ` Kweh, Hock Leong 2015-04-29 21:35 ` James Bottomley 1 sibling, 2 replies; 62+ messages in thread From: Andy Lutomirski @ 2015-04-29 18:40 UTC (permalink / raw) To: Kweh, Hock Leong Cc: James Bottomley, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong <hock.leong.kweh@intel.com> wrote: >> -----Original Message----- >> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com] >> Sent: Tuesday, April 28, 2015 6:52 AM >> >> On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: >> > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley >> > <James.Bottomley@hansenpartnership.com> wrote: >> > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: >> > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley >> > >> <James.Bottomley@hansenpartnership.com> wrote: >> > >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote: >> > >> >> > -----Original Message----- >> > >> >> > From: James Bottomley >> > >> >> > [mailto:James.Bottomley@HansenPartnership.com] >> > >> >> > Sent: Thursday, April 23, 2015 10:10 PM >> > >> >> > >> > >> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote: >> > >> >> > > > -----Original Message----- >> > >> >> > > > From: James Bottomley >> > >> >> > [mailto:James.Bottomley@HansenPartnership.com] >> > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM >> > >> >> > > > >> > >> >> > > > >> > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a >> > >> >> > > > question of whether >> > >> >> > we >> > >> >> > > > can stomach the ick factor of actually initiating a >> > >> >> > > > transaction in close ... I'm >> > >> >> > still >> > >> >> > > > feeling queasy. >> > >> >> > > >> > >> >> > > The file "close" here can I understand that the file system >> > >> >> > > will call the >> > >> >> > "release" >> > >> >> > > function at the file_operations struct? >> > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L153 >> > >> >> > > 8 >> > >> >> > > >> > >> >> > > So, James you are meaning that we could initiating the >> > >> >> > > update transaction inside the f_ops->release() and return >> > >> >> > > the error code if update failed in this function? >> > >> >> > >> > >> >> > Well, that's what I was thinking. However the return value of >> > >> >> > ->release doesn't get propagated in sys_close (or indeed >> > >> >> > anywhere ... no idea why it returns an int) thanks to the task >> > >> >> > work additions, so we'd actually have to use the operation >> > >> >> > whose value is propagated in sys_close() which turns out to be >> flush. >> > >> >> > >> > >> >> > James >> > >> >> > >> > >> >> >> > >> >> Okay, I think I got you. Just to double check for in case: you >> > >> >> are meaning to implement it at f_ops->flush() instead of f_ops- >> >release(). >> > >> > >> > >> > Well, what I'm saying is that the only way to propagate an error >> > >> > to close is by returning one from the flush file_operation. >> > >> > >> > >> > Let's cc fsdevel to see if they have any brighter ideas. >> > >> > >> > >> > The problem is we need to update firmware (several megabytes of >> > >> > it) via standard system tools. We're thinking cat to a device. >> > >> > The problem is that we need an error code back once the update >> > >> > goes through (which it can't until we've fed all the firmware >> > >> > data into the system). To use standard unix tools, we have to >> > >> > trigger off the standard system calls cat uses and since write() >> > >> > will happen in chunks, the only way to commit the transaction is in >> close(). >> > >> > >> > >> > We initially through of initiating the transaction in >> > >> > f_ops->release and returning the error code there, but that >> > >> > doesn't work because its value isn't actually propagated, so >> > >> > we're now thinking of initiating the transaction in f_ops->flush >> > >> > instead (this is a device, not a file, so it won't get any other >> > >> > flushers). Are there any other ways for us to propagate error on close? >> > >> > >> > >> >> > >> I think we may end up wanting to support both UpdateCapsule and >> > >> QueryCapsuleCapabilities, in which case this gets awkward. Maybe >> > >> we really should do a misc device + ioctl. >> > > >> > > To be honest, I hate ioctls ... especially the "have to use special >> > > tools" part. >> > > >> > > Would we ever want to support QueryCapsuleUpdate()? The return >> > > codes on error are the same as UpdateCapsule() but the query call >> > > does nothing on success (and the update call updates, obviously), so >> > > it seems a bit pointless if someone's gone to the trouble of getting >> > > a capsule ... they obviously want to apply it rather than know if it could be >> applied. >> > >> > I can imagine a UI that would try to validate a transaction consisting >> > of several of these things, tell the user whether it'll work and >> > whether a reboot is needed, and then do it. >> >> You mean for dependent capsules? That's a bit way overthinking the UEFI >> current use case (which is for firmware update). In theory, the persist across >> reboot flag can be used for OS persistent information (subject to someone >> actually coming up with an implementation). I'd code for the simple case: >> firmware update and let the rest take care of itself if and when we have an >> implementation. >> >> The last thing I want to see landing on the UEFI-SST is some hopelessly >> complex and nasty capsule spec just "because Linux implements it this way". >> >> > > Assuming we do, we could just use the same error on close mechanism, >> > > but use sysfs binary attributes ... or probably something new like a >> > > binary transaction attribute that does all the transaction on close >> > > magic for us. >> > >> > Yeah, but now we have both input and output, so as ugly as ioctl is, >> > it's a pretty good match. >> >> No, we'll have read and write, so we can do that. As long as there's no >> transaction that can't complete in close or any sense of multiple transactions >> that aren't issued by open read/write close, we're covered. >> >> > Sigh. This is all more complicated than it deserves to me. >> >> Be fair: it is a new interface and it works in a way that's just different enough >> from regular firmware to cause all this bother. >> >> James >> > > Dear communities, > > I agree with James. Due to different people may have different needs. But > from our side, we would just like to have a simple interface for us to upload > the efi capsule and perform update. We do not have any use case or need > to get info from QueryCapsuleUpdate(). Let me give a suggestion here: > please allow me to focus on deliver this simple loading interface and > upstream it. Then later whoever has the actual use case or needs on the ioctl > implementation, he or she could enhance base on this simple loading interface. > What do you guys think? > > Let me summarize the latest design idea: > - No longer leverage on firmware class but use misc device > - Do not use platform device but use device_create() > - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell If you do this, there's no need for the misc device. > - File operation functions include: open(), read(), write() and flush() > - Perform mutex lock in open() then release the mutex in flush() for avoiding > race condition / concurrent loading Make sure the mutex operation is killable, then, and maybe even interruptable. > - Perform the capsule update and error return at flush() function > > Is there anything I missed? Any one still have concern with this idea? > Thanks for providing the ideas as well as the review. > If it works (and cat really does fail reliably), then it seems okay to me. However, since I like pulling increasing numbers of my hats, someone should verify that the common embedded cat implementations are also okay with this. For example, I haven't yet found any code in busybox's cat implementation that closes stdout. Given that the main targets of this (for now, at least) are embedded, this might be a problem. --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-29 18:40 ` Andy Lutomirski @ 2015-04-29 21:37 ` James Bottomley 2015-04-30 9:17 ` Kweh, Hock Leong 1 sibling, 0 replies; 62+ messages in thread From: James Bottomley @ 2015-04-29 21:37 UTC (permalink / raw) To: Andy Lutomirski Cc: Kweh, Hock Leong, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Wed, 2015-04-29 at 11:40 -0700, Andy Lutomirski wrote: > If it works (and cat really does fail reliably), then it seems okay to me. > > However, since I like pulling increasing numbers of my hats, someone > should verify that the common embedded cat implementations are also > okay with this. For example, I haven't yet found any code in > busybox's cat implementation that closes stdout. > > Given that the main targets of this (for now, at least) are embedded, > this might be a problem. I think that rabbit is a bit mixie: we already demonstrated we *can* collect the error in close. It's up to the fw vendors who want to use this method to make sure they have proper tools (and if busybox cat needs fixing, to fix it before they ship). James ^ permalink raw reply [flat|nested] 62+ messages in thread
* RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-29 18:40 ` Andy Lutomirski 2015-04-29 21:37 ` James Bottomley @ 2015-04-30 9:17 ` Kweh, Hock Leong 2015-04-30 17:55 ` Andy Lutomirski 1 sibling, 1 reply; 62+ messages in thread From: Kweh, Hock Leong @ 2015-04-30 9:17 UTC (permalink / raw) To: Andy Lutomirski Cc: James Bottomley, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2967 bytes --] > -----Original Message----- > From: Andy Lutomirski [mailto:luto@amacapital.net] > Sent: Thursday, April 30, 2015 2:41 AM > > On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong > <hock.leong.kweh@intel.com> wrote: > > > > Dear communities, > > > > I agree with James. Due to different people may have different needs. > > But from our side, we would just like to have a simple interface for > > us to upload the efi capsule and perform update. We do not have any > > use case or need to get info from QueryCapsuleUpdate(). Let me give a > suggestion here: > > please allow me to focus on deliver this simple loading interface and > > upstream it. Then later whoever has the actual use case or needs on > > the ioctl implementation, he or she could enhance base on this simple > loading interface. > > What do you guys think? > > > > Let me summarize the latest design idea: > > - No longer leverage on firmware class but use misc device > > - Do not use platform device but use device_create() > > - User just need to perform "cat file.bin > /sys/.../capsule_loader" > > in the shell > > If you do this, there's no need for the misc device. I do this so that in the future when someone want to implement the Ioctl(), he or she can base on this and expand it. > > > - File operation functions include: open(), read(), write() and > > flush() > > - Perform mutex lock in open() then release the mutex in flush() for > avoiding > > race condition / concurrent loading > > Make sure the mutex operation is killable, then, and maybe even > interruptable. Okay. > > > - Perform the capsule update and error return at flush() function > > > > Is there anything I missed? Any one still have concern with this idea? > > Thanks for providing the ideas as well as the review. > > > > If it works (and cat really does fail reliably), then it seems okay to me. > > However, since I like pulling increasing numbers of my hats, someone should > verify that the common embedded cat implementations are also okay with > this. For example, I haven't yet found any code in busybox's cat > implementation that closes stdout. > > Given that the main targets of this (for now, at least) are embedded, this > might be a problem. > I think we shouldn't focus on the cat implementation for the close issue. My understanding about this action: cat file.bin > /sys/..../capsule_loader It is actually the ">" (IO redirection) who perform the open write & close to this "/sys/..../capsule_loader" file note and not the "cat" do it. So, I think your answer can be found at Shell source code. More info about the IO redirection can be found at: http://www.tldp.org/LDP/abs/html/io-redirection.html Busybox Shell Souce code can be found at: https://lxr.missinglinkelectronics.com/busybox/shell/ash.c Regards, Wilson ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-30 9:17 ` Kweh, Hock Leong @ 2015-04-30 17:55 ` Andy Lutomirski 0 siblings, 0 replies; 62+ messages in thread From: Andy Lutomirski @ 2015-04-30 17:55 UTC (permalink / raw) To: Kweh, Hock Leong Cc: James Bottomley, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Thu, Apr 30, 2015 at 2:17 AM, Kweh, Hock Leong <hock.leong.kweh@intel.com> wrote: >> -----Original Message----- >> From: Andy Lutomirski [mailto:luto@amacapital.net] >> Sent: Thursday, April 30, 2015 2:41 AM >> >> On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong >> <hock.leong.kweh@intel.com> wrote: >> > >> > Dear communities, >> > >> > I agree with James. Due to different people may have different needs. >> > But from our side, we would just like to have a simple interface for >> > us to upload the efi capsule and perform update. We do not have any >> > use case or need to get info from QueryCapsuleUpdate(). Let me give a >> suggestion here: >> > please allow me to focus on deliver this simple loading interface and >> > upstream it. Then later whoever has the actual use case or needs on >> > the ioctl implementation, he or she could enhance base on this simple >> loading interface. >> > What do you guys think? >> > >> > Let me summarize the latest design idea: >> > - No longer leverage on firmware class but use misc device >> > - Do not use platform device but use device_create() >> > - User just need to perform "cat file.bin > /sys/.../capsule_loader" >> > in the shell >> >> If you do this, there's no need for the misc device. > > I do this so that in the future when someone want to implement the > Ioctl(), he or she can base on this and expand it. > >> >> > - File operation functions include: open(), read(), write() and >> > flush() >> > - Perform mutex lock in open() then release the mutex in flush() for >> avoiding >> > race condition / concurrent loading >> >> Make sure the mutex operation is killable, then, and maybe even >> interruptable. > > Okay. > >> >> > - Perform the capsule update and error return at flush() function >> > >> > Is there anything I missed? Any one still have concern with this idea? >> > Thanks for providing the ideas as well as the review. >> > >> >> If it works (and cat really does fail reliably), then it seems okay to me. >> >> However, since I like pulling increasing numbers of my hats, someone should >> verify that the common embedded cat implementations are also okay with >> this. For example, I haven't yet found any code in busybox's cat >> implementation that closes stdout. >> >> Given that the main targets of this (for now, at least) are embedded, this >> might be a problem. >> > > I think we shouldn't focus on the cat implementation for the close issue. > > My understanding about this action: > cat file.bin > /sys/..../capsule_loader > It is actually the ">" (IO redirection) who perform the open write & close > to this "/sys/..../capsule_loader" file note and not the "cat" do it. > So, I think your answer can be found at Shell source code. The shell opens capsule_loader and then execs the command. If you type: (cat file.bin) >/sys/.../captule_loader, then the command is a subshell and the subshell will close the file. (cat might also close it, but there will be two references.) If you type: cat file.bin >/sys/.../capsule_loader then the shell doesn't retain a reference to the file at all. --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-29 11:23 ` Kweh, Hock Leong 2015-04-29 18:40 ` Andy Lutomirski @ 2015-04-29 21:35 ` James Bottomley 2015-04-29 21:36 ` Andy Lutomirski 1 sibling, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-29 21:35 UTC (permalink / raw) To: Kweh, Hock Leong Cc: Andy Lutomirski, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote: > I agree with James. Due to different people may have different needs. But > from our side, we would just like to have a simple interface for us to upload > the efi capsule and perform update. We do not have any use case or need > to get info from QueryCapsuleUpdate(). Let me give a suggestion here: > please allow me to focus on deliver this simple loading interface and > upstream it. Then later whoever has the actual use case or needs on the ioctl > implementation, he or she could enhance base on this simple loading interface. > What do you guys think? > > Let me summarize the latest design idea: > - No longer leverage on firmware class but use misc device > - Do not use platform device but use device_create() > - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell > - File operation functions include: open(), read(), write() and flush() > - Perform mutex lock in open() then release the mutex in flush() for avoiding > race condition / concurrent loading > - Perform the capsule update and error return at flush() function > > Is there anything I missed? Any one still have concern with this idea? > Thanks for providing the ideas as well as the review. I think that's pretty much it. Why don't you let me construct a straw man patch. It's going to be a bit controversial because it involves adding flush operations to sysfs and kernfs, slicing apart firmware_class.c to extract the transaction handling stuff and creating an new efi update capsule file which makes use of it. Once we have code, we at least have something more concrete to argue over. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-29 21:35 ` James Bottomley @ 2015-04-29 21:36 ` Andy Lutomirski 2015-04-29 21:39 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Andy Lutomirski @ 2015-04-29 21:36 UTC (permalink / raw) To: James Bottomley Cc: Kweh, Hock Leong, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote: >> I agree with James. Due to different people may have different needs. But >> from our side, we would just like to have a simple interface for us to upload >> the efi capsule and perform update. We do not have any use case or need >> to get info from QueryCapsuleUpdate(). Let me give a suggestion here: >> please allow me to focus on deliver this simple loading interface and >> upstream it. Then later whoever has the actual use case or needs on the ioctl >> implementation, he or she could enhance base on this simple loading interface. >> What do you guys think? >> >> Let me summarize the latest design idea: >> - No longer leverage on firmware class but use misc device >> - Do not use platform device but use device_create() >> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell >> - File operation functions include: open(), read(), write() and flush() >> - Perform mutex lock in open() then release the mutex in flush() for avoiding >> race condition / concurrent loading >> - Perform the capsule update and error return at flush() function >> >> Is there anything I missed? Any one still have concern with this idea? >> Thanks for providing the ideas as well as the review. > > I think that's pretty much it. > > Why don't you let me construct a straw man patch. It's going to be a > bit controversial because it involves adding flush operations to sysfs > and kernfs, slicing apart firmware_class.c to extract the transaction > handling stuff and creating an new efi update capsule file which makes > use of it. > > Once we have code, we at least have something more concrete to argue > over. Would it be worth checking whether busybox is also okay with it first? (Sorry to be a naysayer.) It would be a shame if we do all this to keep the userspace footprint light and then it doesn't work for non-coreutils userspace. --Andy ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-29 21:36 ` Andy Lutomirski @ 2015-04-29 21:39 ` James Bottomley 2015-04-29 21:42 ` Andy Lutomirski 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-29 21:39 UTC (permalink / raw) To: Andy Lutomirski Cc: Kweh, Hock Leong, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote: > On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote: > >> I agree with James. Due to different people may have different needs. But > >> from our side, we would just like to have a simple interface for us to upload > >> the efi capsule and perform update. We do not have any use case or need > >> to get info from QueryCapsuleUpdate(). Let me give a suggestion here: > >> please allow me to focus on deliver this simple loading interface and > >> upstream it. Then later whoever has the actual use case or needs on the ioctl > >> implementation, he or she could enhance base on this simple loading interface. > >> What do you guys think? > >> > >> Let me summarize the latest design idea: > >> - No longer leverage on firmware class but use misc device > >> - Do not use platform device but use device_create() > >> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell > >> - File operation functions include: open(), read(), write() and flush() > >> - Perform mutex lock in open() then release the mutex in flush() for avoiding > >> race condition / concurrent loading > >> - Perform the capsule update and error return at flush() function > >> > >> Is there anything I missed? Any one still have concern with this idea? > >> Thanks for providing the ideas as well as the review. > > > > I think that's pretty much it. > > > > Why don't you let me construct a straw man patch. It's going to be a > > bit controversial because it involves adding flush operations to sysfs > > and kernfs, slicing apart firmware_class.c to extract the transaction > > handling stuff and creating an new efi update capsule file which makes > > use of it. > > > > Once we have code, we at least have something more concrete to argue > > over. > > Would it be worth checking whether busybox is also okay with it first? > (Sorry to be a naysayer.) > > It would be a shame if we do all this to keep the userspace footprint > light and then it doesn't work for non-coreutils userspace. I don't think so, because we can fix busybox if it's a problem. The embedded people wanting this control the tool space, so they can decide to use the fixed version. So yes, someone should check and fix busybox cat if broken, but no, it's not a blocker. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-29 21:39 ` James Bottomley @ 2015-04-29 21:42 ` Andy Lutomirski 0 siblings, 0 replies; 62+ messages in thread From: Andy Lutomirski @ 2015-04-29 21:42 UTC (permalink / raw) To: James Bottomley Cc: Kweh, Hock Leong, Peter Jones, Greg Kroah-Hartman, Matt Fleming, Ming Lei, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Roy Franz, Borislav Petkov, Al Viro, Linux FS Devel On Wed, Apr 29, 2015 at 2:39 PM, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote: >> On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley >> <James.Bottomley@hansenpartnership.com> wrote: >> > On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote: >> >> I agree with James. Due to different people may have different needs. But >> >> from our side, we would just like to have a simple interface for us to upload >> >> the efi capsule and perform update. We do not have any use case or need >> >> to get info from QueryCapsuleUpdate(). Let me give a suggestion here: >> >> please allow me to focus on deliver this simple loading interface and >> >> upstream it. Then later whoever has the actual use case or needs on the ioctl >> >> implementation, he or she could enhance base on this simple loading interface. >> >> What do you guys think? >> >> >> >> Let me summarize the latest design idea: >> >> - No longer leverage on firmware class but use misc device >> >> - Do not use platform device but use device_create() >> >> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell >> >> - File operation functions include: open(), read(), write() and flush() >> >> - Perform mutex lock in open() then release the mutex in flush() for avoiding >> >> race condition / concurrent loading >> >> - Perform the capsule update and error return at flush() function >> >> >> >> Is there anything I missed? Any one still have concern with this idea? >> >> Thanks for providing the ideas as well as the review. >> > >> > I think that's pretty much it. >> > >> > Why don't you let me construct a straw man patch. It's going to be a >> > bit controversial because it involves adding flush operations to sysfs >> > and kernfs, slicing apart firmware_class.c to extract the transaction >> > handling stuff and creating an new efi update capsule file which makes >> > use of it. >> > >> > Once we have code, we at least have something more concrete to argue >> > over. >> >> Would it be worth checking whether busybox is also okay with it first? >> (Sorry to be a naysayer.) >> >> It would be a shame if we do all this to keep the userspace footprint >> light and then it doesn't work for non-coreutils userspace. > > I don't think so, because we can fix busybox if it's a problem. The > embedded people wanting this control the tool space, so they can decide > to use the fixed version. > > So yes, someone should check and fix busybox cat if broken, but no, it's > not a blocker. It's still a bit unfortunate that: #!/bin/sh cat "$1" >/sys/whatever if [ "$?" != "0" ]; then echo "It didn't work because" ... exit 1 fi echo "It worked! Go reboot if needed." exit 0 will only work sometimes. Will people really test this on their target implementation of cat? I agree that making this possible with just shell is nice, but I'm less happy about it if it'll be unreliable. --Andy -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-17 13:49 ` Greg Kroah-Hartman 2015-04-17 14:36 ` Matt Fleming @ 2015-04-20 17:59 ` James Bottomley 1 sibling, 0 replies; 62+ messages in thread From: James Bottomley @ 2015-04-20 17:59 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Fri, 2015-04-17 at 15:49 +0200, Greg Kroah-Hartman wrote: > On Thu, Apr 16, 2015 at 09:42:31AM +0000, Kweh, Hock Leong wrote: > > > -----Original Message----- > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > Sent: Wednesday, April 15, 2015 9:19 PM > > > > > > On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > > > > -----Original Message----- > > > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > > > From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com> > > > > > > > > > > > > Introducing a kernel module to expose capsule loader interface > > > > > > for user to upload capsule binaries. This module leverage the > > > > > > request_firmware_direct_full_path() to obtain the binary at a > > > > > > specific path input by user. > > > > > > > > > > > > Example method to load the capsule binary: > > > > > > echo -n "/path/to/capsule/binary" > > > > > > /sys/devices/platform/efi_capsule_loader/capsule_loader > > > > > > > > > > Ick, why not just have the firmware file location present, and copy it > > > > > to the sysfs file directly from userspace, instead of this two-step > > > > > process? > > > > > > > > Err .... I may not catch your meaning correctly. Are you trying to say > > > > that you would prefer the user to perform: > > > > > > > > cat file.bin > /sys/.../capsule_loader > > > > > > > > instead of > > > > > > > > echo -n "/path/to/binary" > /sys/..../capsule_laoder > > > > > > Yes. What's the namespace of your /path/to/binary/ and how do you know > > > the kernel has the same one when it does the firmware load call? By > > > just copying the data with 'cat', you don't have to worry about > > > namespace issues at all. > > > > Hi Greg, > > > > Let me double confirm that I understand your concern correctly. You are > > trying to tell that some others module may use a 'same' namespace to > > request the firmware but never release it. Then when our module trying > > to request the firmware by passing in the 'same' namespace, I will get the > > previous data instead of the current binary data from the path I want. > > Yes. > > > Hmm .... I believe this concern also apply to all the current request_firmware > > APIs right? And I believe the coincidence to have 'same' file name namespace > > would be higher than full path namespace. > > Not really, the kernel namespace is what matters at that point in time. > > And maybe it does matter, I haven't thought through all of the issues. > But passing a path from userspace, to the kernel, to have the kernel > turn around again and use that path is full of nasty consequences at > times due to namespaces, let's avoid all of that please. So just to clarify this, namespaces are designed not to cause a problem here, provided the operation is handled correctly (this is key; it is easy do design operations which will screw up no end if done wrongly). The file name to object translation is handled by the mount name space, which is the operative one of the process doing the echo. For a longstanding object (i.e. one which will exist beyond the call to the system of the current process) you need either to convert to the actual underlying object (usually a file descriptor) which has an existence independent of the namespace (and perform all the necessary security validations before returning control back to userspace, so they occur within all the namespace constraints of the calling process), or store sufficient information to redo whatever operation you need to within the namespace (the former is by far preferred for long lived operations). James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-15 13:19 ` Greg Kroah-Hartman 2015-04-16 9:42 ` Kweh, Hock Leong @ 2015-04-22 15:35 ` James Bottomley 2015-04-22 15:46 ` Greg Kroah-Hartman 1 sibling, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-22 15:35 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote: > On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > > -----Original Message----- > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > + */ > > > > +static void __exit efi_capsule_loader_exit(void) > > > > +{ > > > > + platform_device_unregister(efi_capsule_pdev); > > > > > > This is not a platform device, don't abuse that interface please. > > > > > > greg k-h > > > > Okay, so you would recommend to use device_register() for this case? > > Or you would think that this is more suitable to use class_register()? > > A class isn't needed, you just want a device right? So just use a > device, but not a platform device, as that isn't what you have here. Coming back to this, am I the only one confused here? What is a 'platform device' then? Because if it doesn't fit a direct channel to the platform firmware, which seems to be one of the definitions covered in driver-model/platform.txt under devices with minimal infrastructure then perhaps the documentation needs updating. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 15:35 ` James Bottomley @ 2015-04-22 15:46 ` Greg Kroah-Hartman 2015-04-22 16:11 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-22 15:46 UTC (permalink / raw) To: James Bottomley Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote: > On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote: > > On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > > > -----Original Message----- > > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > > + */ > > > > > +static void __exit efi_capsule_loader_exit(void) > > > > > +{ > > > > > + platform_device_unregister(efi_capsule_pdev); > > > > > > > > This is not a platform device, don't abuse that interface please. > > > > > > > > greg k-h > > > > > > Okay, so you would recommend to use device_register() for this case? > > > Or you would think that this is more suitable to use class_register()? > > > > A class isn't needed, you just want a device right? So just use a > > device, but not a platform device, as that isn't what you have here. > > Coming back to this, am I the only one confused here? What is a > 'platform device' then? Because if it doesn't fit a direct channel to > the platform firmware, which seems to be one of the definitions covered > in driver-model/platform.txt under devices with minimal infrastructure > then perhaps the documentation needs updating. I don't remember the original code here at all, sorry. I'm guessing that they were using a class, and a platform device together, which is not a good idea. Just make a "virtual" device, as you don't need/want any of the platform device infrastructure here, you just wanted a device node and/or a way to show up in sysfs somewhere. If you have some kind of "platform resource", then you can be a platform device, otherwise please don't use that api just because it seems simple to use. Use the ones the driver core provides for you that really are just as simple (i.e. device_create()). thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 15:46 ` Greg Kroah-Hartman @ 2015-04-22 16:11 ` James Bottomley 2015-04-23 9:50 ` Greg Kroah-Hartman 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-22 16:11 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote: > On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote: > > On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote: > > > On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > > > > -----Original Message----- > > > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > > > + */ > > > > > > +static void __exit efi_capsule_loader_exit(void) > > > > > > +{ > > > > > > + platform_device_unregister(efi_capsule_pdev); > > > > > > > > > > This is not a platform device, don't abuse that interface please. > > > > > > > > > > greg k-h > > > > > > > > Okay, so you would recommend to use device_register() for this case? > > > > Or you would think that this is more suitable to use class_register()? > > > > > > A class isn't needed, you just want a device right? So just use a > > > device, but not a platform device, as that isn't what you have here. > > > > Coming back to this, am I the only one confused here? What is a > > 'platform device' then? Because if it doesn't fit a direct channel to > > the platform firmware, which seems to be one of the definitions covered > > in driver-model/platform.txt under devices with minimal infrastructure > > then perhaps the documentation needs updating. > > I don't remember the original code here at all, sorry. I'm guessing > that they were using a class, and a platform device together, which is > not a good idea. Just make a "virtual" device, as you don't need/want > any of the platform device infrastructure here, you just wanted a device > node and/or a way to show up in sysfs somewhere. It was a platform device called efi_platform_loader and a single attribute file in that device called capsule_load. I agree that if we're going to use this for other things, we should probably have a uefi directory somewhere (under firmware?) to collect everything together rather than spraying random devices around. > If you have some kind of "platform resource", then you can be a platform > device, otherwise please don't use that api just because it seems simple > to use. Use the ones the driver core provides for you that really are > just as simple (i.e. device_create()). OK, so this is what I'm trying to understand. Why isn't a pipe to firmware for something a "platform resource"? I think UEFI is in the same class as ACPI which uses platform devices all over. James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-22 16:11 ` James Bottomley @ 2015-04-23 9:50 ` Greg Kroah-Hartman 2015-04-23 16:14 ` James Bottomley 0 siblings, 1 reply; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-23 9:50 UTC (permalink / raw) To: James Bottomley Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Wed, Apr 22, 2015 at 09:11:17AM -0700, James Bottomley wrote: > On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote: > > On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote: > > > On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote: > > > > On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > > > > > -----Original Message----- > > > > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > > > > + */ > > > > > > > +static void __exit efi_capsule_loader_exit(void) > > > > > > > +{ > > > > > > > + platform_device_unregister(efi_capsule_pdev); > > > > > > > > > > > > This is not a platform device, don't abuse that interface please. > > > > > > > > > > > > greg k-h > > > > > > > > > > Okay, so you would recommend to use device_register() for this case? > > > > > Or you would think that this is more suitable to use class_register()? > > > > > > > > A class isn't needed, you just want a device right? So just use a > > > > device, but not a platform device, as that isn't what you have here. > > > > > > Coming back to this, am I the only one confused here? What is a > > > 'platform device' then? Because if it doesn't fit a direct channel to > > > the platform firmware, which seems to be one of the definitions covered > > > in driver-model/platform.txt under devices with minimal infrastructure > > > then perhaps the documentation needs updating. > > > > I don't remember the original code here at all, sorry. I'm guessing > > that they were using a class, and a platform device together, which is > > not a good idea. Just make a "virtual" device, as you don't need/want > > any of the platform device infrastructure here, you just wanted a device > > node and/or a way to show up in sysfs somewhere. > > It was a platform device called efi_platform_loader and a single > attribute file in that device called capsule_load. I agree that if > we're going to use this for other things, we should probably have a uefi > directory somewhere (under firmware?) to collect everything together > rather than spraying random devices around. > > > If you have some kind of "platform resource", then you can be a platform > > device, otherwise please don't use that api just because it seems simple > > to use. Use the ones the driver core provides for you that really are > > just as simple (i.e. device_create()). > > OK, so this is what I'm trying to understand. Why isn't a pipe to > firmware for something a "platform resource"? I think UEFI is in the > same class as ACPI which uses platform devices all over. And I hate the fact that ACPI did that, but that ship has sailed a long time ago. It "should" have been it's own bus and device type, but oh well. For a "simple" bus-less device, that has no platform resources needed (i.e from acpi or device tree), so you don't need the infrastructure from the platform core, just use a simple device_create() call, that's what it is there for. thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-23 9:50 ` Greg Kroah-Hartman @ 2015-04-23 16:14 ` James Bottomley 2015-04-23 20:38 ` Greg Kroah-Hartman 0 siblings, 1 reply; 62+ messages in thread From: James Bottomley @ 2015-04-23 16:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Thu, 2015-04-23 at 11:50 +0200, Greg Kroah-Hartman wrote: > On Wed, Apr 22, 2015 at 09:11:17AM -0700, James Bottomley wrote: > > On Wed, 2015-04-22 at 17:46 +0200, Greg Kroah-Hartman wrote: > > > On Wed, Apr 22, 2015 at 08:35:54AM -0700, James Bottomley wrote: > > > > On Wed, 2015-04-15 at 15:19 +0200, Greg Kroah-Hartman wrote: > > > > > On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote: > > > > > > > -----Original Message----- > > > > > > > From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org] > > > > > > > Sent: Tuesday, April 14, 2015 10:09 PM > > > > > > > > > > > > > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote: > > > > > > > > + */ > > > > > > > > +static void __exit efi_capsule_loader_exit(void) > > > > > > > > +{ > > > > > > > > + platform_device_unregister(efi_capsule_pdev); > > > > > > > > > > > > > > This is not a platform device, don't abuse that interface please. > > > > > > > > > > > > > > greg k-h > > > > > > > > > > > > Okay, so you would recommend to use device_register() for this case? > > > > > > Or you would think that this is more suitable to use class_register()? > > > > > > > > > > A class isn't needed, you just want a device right? So just use a > > > > > device, but not a platform device, as that isn't what you have here. > > > > > > > > Coming back to this, am I the only one confused here? What is a > > > > 'platform device' then? Because if it doesn't fit a direct channel to > > > > the platform firmware, which seems to be one of the definitions covered > > > > in driver-model/platform.txt under devices with minimal infrastructure > > > > then perhaps the documentation needs updating. > > > > > > I don't remember the original code here at all, sorry. I'm guessing > > > that they were using a class, and a platform device together, which is > > > not a good idea. Just make a "virtual" device, as you don't need/want > > > any of the platform device infrastructure here, you just wanted a device > > > node and/or a way to show up in sysfs somewhere. > > > > It was a platform device called efi_platform_loader and a single > > attribute file in that device called capsule_load. I agree that if > > we're going to use this for other things, we should probably have a uefi > > directory somewhere (under firmware?) to collect everything together > > rather than spraying random devices around. > > > > > If you have some kind of "platform resource", then you can be a platform > > > device, otherwise please don't use that api just because it seems simple > > > to use. Use the ones the driver core provides for you that really are > > > just as simple (i.e. device_create()). > > > > OK, so this is what I'm trying to understand. Why isn't a pipe to > > firmware for something a "platform resource"? I think UEFI is in the > > same class as ACPI which uses platform devices all over. > > And I hate the fact that ACPI did that, but that ship has sailed a long > time ago. It "should" have been it's own bus and device type, but oh > well. > > For a "simple" bus-less device, that has no platform resources needed > (i.e from acpi or device tree), so you don't need the infrastructure > from the platform core, just use a simple device_create() call, that's > what it is there for. That's not confusing: ACPI shouldn't be a platform device, but something that is should have a platform resource provided by ACPI (or device tree). So I think the problem is that the documentation is wrong? Platform device isn't for "platform resources" like you said initially? Or do we have a more fundamental problem: You don't use the word "platform" the same way we do? A "platform" to most people on this thread is something designed to be delivered with the box that's not amenable to user modification. That's why we think of UEFI (and ACPI) as platform technologies: they come with the box (often they were specially crafted for it) and we use their services to discover stuff and correctly configure the OS. In this definition, almost everything we do via UEFI manipulates "platform resources". James ^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware 2015-04-23 16:14 ` James Bottomley @ 2015-04-23 20:38 ` Greg Kroah-Hartman 0 siblings, 0 replies; 62+ messages in thread From: Greg Kroah-Hartman @ 2015-04-23 20:38 UTC (permalink / raw) To: James Bottomley Cc: Kweh, Hock Leong, Ming Lei, Matt Fleming, Ong, Boon Leong, LKML, linux-efi, Sam Protsenko, Peter Jones, Andy Lutomirski, Roy Franz, Borislav Petkov On Thu, Apr 23, 2015 at 09:14:18AM -0700, James Bottomley wrote: > > > OK, so this is what I'm trying to understand. Why isn't a pipe to > > > firmware for something a "platform resource"? I think UEFI is in the > > > same class as ACPI which uses platform devices all over. > > > > And I hate the fact that ACPI did that, but that ship has sailed a long > > time ago. It "should" have been it's own bus and device type, but oh > > well. > > > > For a "simple" bus-less device, that has no platform resources needed > > (i.e from acpi or device tree), so you don't need the infrastructure > > from the platform core, just use a simple device_create() call, that's > > what it is there for. > > That's not confusing: ACPI shouldn't be a platform device, but something > that is should have a platform resource provided by ACPI (or device tree). > > So I think the problem is that the documentation is wrong? Platform > device isn't for "platform resources" like you said initially? > > Or do we have a more fundamental problem: You don't use the word > "platform" the same way we do? A "platform" to most people on this > thread is something designed to be delivered with the box that's not > amenable to user modification. That's why we think of UEFI (and ACPI) > as platform technologies: they come with the box (often they were > specially crafted for it) and we use their services to discover stuff > and correctly configure the OS. In this definition, almost everything > we do via UEFI manipulates "platform resources". Maybe we aren't using the word "platform" the same way. Platform devices were originally created to handle all of the "cruft" that a system has that used to be only at fixed locations on the CPU/chipset, and were not on any real "bus". Things like timers, keyboard controllers, and all of the odd embedded things that we just "knew" where in memory they were located. Then we got platform data structures, to help know "where" in memory devices were to be able to write to, and board files interacted with them that way. Because of this, and how they were just so easy to create, lots of developers were "lazy" and just put a platform device into their driver instead of having to hook it up to an existing system, or actually write a bus for it (I had an Intel laptop that shipped 1 year ago come with a driver that used a platform device instead of a "real" i2c device like the touchpad really was. Then came device tree, which leveraged platform devices because that's what they needed to control (replacing the board files.) Somewhere in there ACPI also decided to use platform devices and it makes sense now, as drivers just need to get a resource as to how to talk to the hardware, and it doesn't matter if it comes from ACPI or device tree. But for devices that are just "virtual", like this firmware loader, use the virtual bus, which happens automatically when you don't provide a parent to device_create(). That's what it is there for, your device doesn't have to deal with any "resources" that it needs to read from board files, device tree, or acpi, so it shouldn't be a platform device. Hope that helps explain things better. thanks, greg k-h ^ permalink raw reply [flat|nested] 62+ messages in thread
end of thread, other threads:[~2015-04-30 17:55 UTC | newest] Thread overview: 62+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-14 9:44 [PATCH v4 0/2] Enable capsule loader interface for efi firmware updating Kweh, Hock Leong 2015-04-14 9:44 ` [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path() Kweh, Hock Leong 2015-04-14 14:08 ` Greg Kroah-Hartman 2015-04-14 15:56 ` Andy Lutomirski 2015-04-14 16:18 ` Borislav Petkov 2015-04-15 10:14 ` Matt Fleming 2015-04-15 10:18 ` Borislav Petkov 2015-04-15 11:09 ` Matt Fleming 2015-04-15 13:15 ` Greg Kroah-Hartman 2015-04-15 15:53 ` Andy Lutomirski 2015-04-15 12:48 ` Matt Fleming 2015-04-14 9:44 ` [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware Kweh, Hock Leong 2015-04-14 14:09 ` Greg Kroah-Hartman 2015-04-14 15:52 ` Andy Lutomirski 2015-04-15 13:20 ` Greg Kroah-Hartman 2015-04-15 15:45 ` Andy Lutomirski 2015-04-16 0:19 ` Roy Franz 2015-04-17 13:50 ` Greg KH 2015-04-15 11:32 ` Kweh, Hock Leong 2015-04-15 13:19 ` Greg Kroah-Hartman 2015-04-16 9:42 ` Kweh, Hock Leong 2015-04-17 13:49 ` Greg Kroah-Hartman 2015-04-17 14:36 ` Matt Fleming 2015-04-20 3:28 ` Kweh, Hock Leong 2015-04-20 14:43 ` Greg Kroah-Hartman 2015-04-21 3:23 ` Kweh, Hock Leong 2015-04-21 7:56 ` Greg Kroah-Hartman 2015-04-22 1:21 ` James Bottomley 2015-04-22 1:58 ` Andy Lutomirski 2015-04-22 2:20 ` James Bottomley 2015-04-22 3:24 ` Andy Lutomirski 2015-04-22 4:51 ` James Bottomley 2015-04-22 16:50 ` Andy Lutomirski 2015-04-22 17:34 ` James Bottomley 2015-04-22 17:45 ` Andy Lutomirski 2015-04-22 13:27 ` Peter Jones 2015-04-22 15:18 ` James Bottomley 2015-04-22 15:24 ` One Thousand Gnomes 2015-04-23 8:30 ` Kweh, Hock Leong 2015-04-23 14:09 ` James Bottomley 2015-04-24 2:14 ` Kweh, Hock Leong 2015-04-24 15:16 ` James Bottomley 2015-04-27 21:59 ` Andy Lutomirski 2015-04-27 22:35 ` James Bottomley 2015-04-27 22:40 ` Andy Lutomirski 2015-04-27 22:51 ` James Bottomley 2015-04-29 11:23 ` Kweh, Hock Leong 2015-04-29 18:40 ` Andy Lutomirski 2015-04-29 21:37 ` James Bottomley 2015-04-30 9:17 ` Kweh, Hock Leong 2015-04-30 17:55 ` Andy Lutomirski 2015-04-29 21:35 ` James Bottomley 2015-04-29 21:36 ` Andy Lutomirski 2015-04-29 21:39 ` James Bottomley 2015-04-29 21:42 ` Andy Lutomirski 2015-04-20 17:59 ` James Bottomley 2015-04-22 15:35 ` James Bottomley 2015-04-22 15:46 ` Greg Kroah-Hartman 2015-04-22 16:11 ` James Bottomley 2015-04-23 9:50 ` Greg Kroah-Hartman 2015-04-23 16:14 ` James Bottomley 2015-04-23 20:38 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).