[PATCH v4 0/2] Enable capsule loader interface for efi firmware updating

[PATCH v4 0/2] Enable capsule loader interface for efi firmware updating

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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

[PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Greg Kroah-Hartman]

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?

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Andy Lutomirski]

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

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Borislav Petkov]

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

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Matt Fleming]

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

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Borislav Petkov]

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

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Matt Fleming]

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

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Greg Kroah-Hartman]

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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > On Tue, Apr 14, 2015 at 05:44:55PM +0800, Kweh, Hock Leong wrote:
> >> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>
> >> 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

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Andy Lutomirski]

[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

Re: [PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()

[Matt Fleming]

On Tue, 14 Apr, at 05:44:55PM, Kweh Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Cc: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  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

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

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 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

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

[Greg Kroah-Hartman]

On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> 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

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

[Andy Lutomirski]

On Tue, Apr 14, 2015 at 10:09 AM, Greg Kroah-Hartman
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
>> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> 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

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

[Greg Kroah-Hartman]

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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> >> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >>
> >> 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

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

[Andy Lutomirski]

On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman"
<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > >> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >>
> > >> 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

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

[Roy Franz]

On Wed, Apr 15, 2015 at 8:45 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman"
> <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
>> > >> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> > >>
>> > >> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Greg KH]

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-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> > On Apr 15, 2015 6:20 AM, "Greg Kroah-Hartman"
> > <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
> >> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> >> > >> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> > >>
> >> > >> 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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> > 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

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

[Greg Kroah-Hartman]

On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote:
> > -----Original Message-----
> > From: Greg Kroah-Hartman [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >
> > > 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

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

[Kweh, Hock Leong]

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

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

[Greg Kroah-Hartman]

On Thu, Apr 16, 2015 at 09:42:31AM +0000, Kweh, Hock Leong wrote:
> > -----Original Message-----
> > From: Greg Kroah-Hartman [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > >
> > > > > 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

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

[Matt Fleming]

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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Matt Fleming [mailto:matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org]
> 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

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

[Greg Kroah-Hartman]

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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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

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

[Greg Kroah-Hartman]

On Tue, Apr 21, 2015 at 03:23:55AM +0000, Kweh, Hock Leong wrote:
> > -----Original Message-----
> > From: Greg Kroah-Hartman [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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

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

[James Bottomley]

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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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

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

[Andy Lutomirski]

On Tue, Apr 21, 2015 at 6:21 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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

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

[James Bottomley]

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

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

[Andy Lutomirski]

On Tue, Apr 21, 2015 at 7:20 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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

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

[James Bottomley]

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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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

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

[Andy Lutomirski]

On Apr 21, 2015 9:51 PM, "James Bottomley"
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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

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

[James Bottomley]

On Wed, 2015-04-22 at 09:50 -0700, Andy Lutomirski wrote:
> On Apr 21, 2015 9:51 PM, "James Bottomley"
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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

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

[Andy Lutomirski]

On Wed, Apr 22, 2015 at 10:34 AM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Wed, 2015-04-22 at 09:50 -0700, Andy Lutomirski wrote:
>> On Apr 21, 2015 9:51 PM, "James Bottomley"
>> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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

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

[Peter Jones]

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-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> 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

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

[James Bottomley]

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

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

[One Thousand Gnomes]

> 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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: 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

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

[James Bottomley]

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

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: 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

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

[James Bottomley]

On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> > Sent: Thursday, April 23, 2015 10:10 PM
> > 
> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
> > > > -----Original Message-----
> > > > From: James Bottomley
> > [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> > > > Sent: Wednesday, April 22, 2015 11:19 PM
> > > >
> > > >
> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether
> > we
> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm
> > still
> > > > feeling queasy.
> > >
> > > The file "close" here can I understand that the file system will call the
> > "release"
> > > function at the file_operations struct?
> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538
> > >
> > > So, James you are meaning that we could initiating the update transaction
> > > inside the f_ops->release() and return the error code if update failed in this
> > > function?
> > 
> > Well, that's what I was thinking.  However the return value of ->release
> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why
> > it returns an int) thanks to the task work additions, so we'd actually
> > have to use the operation whose value is propagated in sys_close() which
> > turns out to be flush.
> > 
> > James
> > 
> 
> Okay, I think I got you. Just to double check for in case: you are meaning
> to implement it at f_ops->flush() instead of f_ops->release().

Well, what I'm saying is that the only way to propagate an error to
close is by returning one from the flush file_operation.

Let's cc fsdevel to see if they have any brighter ideas.

The problem is we need to update firmware (several megabytes of it) via
standard system tools.  We're thinking cat to a device.  The problem is
that we need an error code back once the update goes through (which it
can't until we've fed all the firmware data into the system).  To use
standard unix tools, we have to trigger off the standard system calls
cat uses and since write() will happen in chunks, the only way to commit
the transaction is in close().

We initially through of initiating the transaction in f_ops->release and
returning the error code there, but that doesn't work because its value
isn't actually propagated, so we're now thinking of initiating the
transaction in f_ops->flush instead (this is a device, not a file, so it
won't get any other flushers).  Are there any other ways for us to
propagate error on close?

James

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

[Andy Lutomirski]

On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
>> > -----Original Message-----
>> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
>> > Sent: Thursday, April 23, 2015 10:10 PM
>> >
>> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
>> > > > -----Original Message-----
>> > > > From: James Bottomley
>> > [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
>> > > > Sent: Wednesday, April 22, 2015 11:19 PM
>> > > >
>> > > >
>> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether
>> > we
>> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm
>> > still
>> > > > feeling queasy.
>> > >
>> > > The file "close" here can I understand that the file system will call the
>> > "release"
>> > > function at the file_operations struct?
>> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538
>> > >
>> > > So, James you are meaning that we could initiating the update transaction
>> > > inside the f_ops->release() and return the error code if update failed in this
>> > > function?
>> >
>> > Well, that's what I was thinking.  However the return value of ->release
>> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why
>> > it returns an int) thanks to the task work additions, so we'd actually
>> > have to use the operation whose value is propagated in sys_close() which
>> > turns out to be flush.
>> >
>> > James
>> >
>>
>> Okay, I think I got you. Just to double check for in case: you are meaning
>> to implement it at f_ops->flush() instead of f_ops->release().
>
> Well, what I'm saying is that the only way to propagate an error to
> close is by returning one from the flush file_operation.
>
> Let's cc fsdevel to see if they have any brighter ideas.
>
> The problem is we need to update firmware (several megabytes of it) via
> standard system tools.  We're thinking cat to a device.  The problem is
> that we need an error code back once the update goes through (which it
> can't until we've fed all the firmware data into the system).  To use
> standard unix tools, we have to trigger off the standard system calls
> cat uses and since write() will happen in chunks, the only way to commit
> the transaction is in close().
>
> We initially through of initiating the transaction in f_ops->release and
> returning the error code there, but that doesn't work because its value
> isn't actually propagated, so we're now thinking of initiating the
> transaction in f_ops->flush instead (this is a device, not a file, so it
> won't get any other flushers).  Are there any other ways for us to
> propagate error on close?
>

I think we may end up wanting to support both UpdateCapsule and
QueryCapsuleCapabilities, in which case this gets awkward.  Maybe we
really should do a misc device + ioctl.

--Andy

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

[James Bottomley]

On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote:
> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
> >> > -----Original Message-----
> >> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> >> > Sent: Thursday, April 23, 2015 10:10 PM
> >> >
> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
> >> > > > -----Original Message-----
> >> > > > From: James Bottomley
> >> > [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM
> >> > > >
> >> > > >
> >> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether
> >> > we
> >> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm
> >> > still
> >> > > > feeling queasy.
> >> > >
> >> > > The file "close" here can I understand that the file system will call the
> >> > "release"
> >> > > function at the file_operations struct?
> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538
> >> > >
> >> > > So, James you are meaning that we could initiating the update transaction
> >> > > inside the f_ops->release() and return the error code if update failed in this
> >> > > function?
> >> >
> >> > Well, that's what I was thinking.  However the return value of ->release
> >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why
> >> > it returns an int) thanks to the task work additions, so we'd actually
> >> > have to use the operation whose value is propagated in sys_close() which
> >> > turns out to be flush.
> >> >
> >> > James
> >> >
> >>
> >> Okay, I think I got you. Just to double check for in case: you are meaning
> >> to implement it at f_ops->flush() instead of f_ops->release().
> >
> > Well, what I'm saying is that the only way to propagate an error to
> > close is by returning one from the flush file_operation.
> >
> > Let's cc fsdevel to see if they have any brighter ideas.
> >
> > The problem is we need to update firmware (several megabytes of it) via
> > standard system tools.  We're thinking cat to a device.  The problem is
> > that we need an error code back once the update goes through (which it
> > can't until we've fed all the firmware data into the system).  To use
> > standard unix tools, we have to trigger off the standard system calls
> > cat uses and since write() will happen in chunks, the only way to commit
> > the transaction is in close().
> >
> > We initially through of initiating the transaction in f_ops->release and
> > returning the error code there, but that doesn't work because its value
> > isn't actually propagated, so we're now thinking of initiating the
> > transaction in f_ops->flush instead (this is a device, not a file, so it
> > won't get any other flushers).  Are there any other ways for us to
> > propagate error on close?
> >
> 
> I think we may end up wanting to support both UpdateCapsule and
> QueryCapsuleCapabilities, in which case this gets awkward.  Maybe we
> really should do a misc device + ioctl.

To be honest, I hate ioctls ... especially the "have to use special
tools" part.

Would we ever want to support QueryCapsuleUpdate()?  The return codes on
error are the same as UpdateCapsule() but the query call does nothing on
success (and the update call updates, obviously), so it seems a bit
pointless if someone's gone to the trouble of getting a capsule ... they
obviously want to apply it rather than know if it could be applied.

Assuming we do, we could just use the same error on close mechanism, but
use sysfs binary attributes ... or probably something new like a binary
transaction attribute that does all the transaction on close magic for
us.

James

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

[Andy Lutomirski]

On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote:
>> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley
>> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
>> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
>> >> > -----Original Message-----
>> >> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
>> >> > Sent: Thursday, April 23, 2015 10:10 PM
>> >> >
>> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
>> >> > > > -----Original Message-----
>> >> > > > From: James Bottomley
>> >> > [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
>> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM
>> >> > > >
>> >> > > >
>> >> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether
>> >> > we
>> >> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm
>> >> > still
>> >> > > > feeling queasy.
>> >> > >
>> >> > > The file "close" here can I understand that the file system will call the
>> >> > "release"
>> >> > > function at the file_operations struct?
>> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538
>> >> > >
>> >> > > So, James you are meaning that we could initiating the update transaction
>> >> > > inside the f_ops->release() and return the error code if update failed in this
>> >> > > function?
>> >> >
>> >> > Well, that's what I was thinking.  However the return value of ->release
>> >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why
>> >> > it returns an int) thanks to the task work additions, so we'd actually
>> >> > have to use the operation whose value is propagated in sys_close() which
>> >> > turns out to be flush.
>> >> >
>> >> > James
>> >> >
>> >>
>> >> Okay, I think I got you. Just to double check for in case: you are meaning
>> >> to implement it at f_ops->flush() instead of f_ops->release().
>> >
>> > Well, what I'm saying is that the only way to propagate an error to
>> > close is by returning one from the flush file_operation.
>> >
>> > Let's cc fsdevel to see if they have any brighter ideas.
>> >
>> > The problem is we need to update firmware (several megabytes of it) via
>> > standard system tools.  We're thinking cat to a device.  The problem is
>> > that we need an error code back once the update goes through (which it
>> > can't until we've fed all the firmware data into the system).  To use
>> > standard unix tools, we have to trigger off the standard system calls
>> > cat uses and since write() will happen in chunks, the only way to commit
>> > the transaction is in close().
>> >
>> > We initially through of initiating the transaction in f_ops->release and
>> > returning the error code there, but that doesn't work because its value
>> > isn't actually propagated, so we're now thinking of initiating the
>> > transaction in f_ops->flush instead (this is a device, not a file, so it
>> > won't get any other flushers).  Are there any other ways for us to
>> > propagate error on close?
>> >
>>
>> I think we may end up wanting to support both UpdateCapsule and
>> QueryCapsuleCapabilities, in which case this gets awkward.  Maybe we
>> really should do a misc device + ioctl.
>
> To be honest, I hate ioctls ... especially the "have to use special
> tools" part.
>
> Would we ever want to support QueryCapsuleUpdate()?  The return codes on
> error are the same as UpdateCapsule() but the query call does nothing on
> success (and the update call updates, obviously), so it seems a bit
> pointless if someone's gone to the trouble of getting a capsule ... they
> obviously want to apply it rather than know if it could be applied.

I can imagine a UI that would try to validate a transaction consisting
of several of these things, tell the user whether it'll work and
whether a reboot is needed, and then do it.

>
> Assuming we do, we could just use the same error on close mechanism, but
> use sysfs binary attributes ... or probably something new like a binary
> transaction attribute that does all the transaction on close magic for
> us.

Yeah, but now we have both input and output, so as ugly as ioctl is,
it's a pretty good match.

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

--Andy

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

[James Bottomley]

On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote:
> On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley
> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote:
> >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley
> >> <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
> >> >> > -----Original Message-----
> >> >> > From: James Bottomley [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> >> >> > Sent: Thursday, April 23, 2015 10:10 PM
> >> >> >
> >> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
> >> >> > > > -----Original Message-----
> >> >> > > > From: James Bottomley
> >> >> > [mailto:James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org]
> >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM
> >> >> > > >
> >> >> > > >
> >> >> > > > Yes, I think we've all agreed we can do it ... it's now a question of whether
> >> >> > we
> >> >> > > > can stomach the ick factor of actually initiating a transaction in close ... I'm
> >> >> > still
> >> >> > > > feeling queasy.
> >> >> > >
> >> >> > > The file "close" here can I understand that the file system will call the
> >> >> > "release"
> >> >> > > function at the file_operations struct?
> >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538
> >> >> > >
> >> >> > > So, James you are meaning that we could initiating the update transaction
> >> >> > > inside the f_ops->release() and return the error code if update failed in this
> >> >> > > function?
> >> >> >
> >> >> > Well, that's what I was thinking.  However the return value of ->release
> >> >> > doesn't get propagated in sys_close (or indeed anywhere ... no idea why
> >> >> > it returns an int) thanks to the task work additions, so we'd actually
> >> >> > have to use the operation whose value is propagated in sys_close() which
> >> >> > turns out to be flush.
> >> >> >
> >> >> > James
> >> >> >
> >> >>
> >> >> Okay, I think I got you. Just to double check for in case: you are meaning
> >> >> to implement it at f_ops->flush() instead of f_ops->release().
> >> >
> >> > Well, what I'm saying is that the only way to propagate an error to
> >> > close is by returning one from the flush file_operation.
> >> >
> >> > Let's cc fsdevel to see if they have any brighter ideas.
> >> >
> >> > The problem is we need to update firmware (several megabytes of it) via
> >> > standard system tools.  We're thinking cat to a device.  The problem is
> >> > that we need an error code back once the update goes through (which it
> >> > can't until we've fed all the firmware data into the system).  To use
> >> > standard unix tools, we have to trigger off the standard system calls
> >> > cat uses and since write() will happen in chunks, the only way to commit
> >> > the transaction is in close().
> >> >
> >> > We initially through of initiating the transaction in f_ops->release and
> >> > returning the error code there, but that doesn't work because its value
> >> > isn't actually propagated, so we're now thinking of initiating the
> >> > transaction in f_ops->flush instead (this is a device, not a file, so it
> >> > won't get any other flushers).  Are there any other ways for us to
> >> > propagate error on close?
> >> >
> >>
> >> I think we may end up wanting to support both UpdateCapsule and
> >> QueryCapsuleCapabilities, in which case this gets awkward.  Maybe we
> >> really should do a misc device + ioctl.
> >
> > To be honest, I hate ioctls ... especially the "have to use special
> > tools" part.
> >
> > Would we ever want to support QueryCapsuleUpdate()?  The return codes on
> > error are the same as UpdateCapsule() but the query call does nothing on
> > success (and the update call updates, obviously), so it seems a bit
> > pointless if someone's gone to the trouble of getting a capsule ... they
> > obviously want to apply it rather than know if it could be applied.
> 
> I can imagine a UI that would try to validate a transaction consisting
> of several of these things, tell the user whether it'll work and
> whether a reboot is needed, and then do it.

You mean for dependent capsules?  That's a bit way overthinking the UEFI
current use case (which is for firmware update).  In theory, the persist
across reboot flag can be used for OS persistent information (subject to
someone actually coming up with an implementation).  I'd code for the
simple case: firmware update and let the rest take care of itself if and
when we have an implementation.

The last thing I want to see landing on the UEFI-SST is some hopelessly
complex and nasty capsule spec just "because Linux implements it this
way".

> > Assuming we do, we could just use the same error on close mechanism, but
> > use sysfs binary attributes ... or probably something new like a binary
> > transaction attribute that does all the transaction on close magic for
> > us.
> 
> Yeah, but now we have both input and output, so as ugly as ioctl is,
> it's a pretty good match.

No, we'll have read and write, so we can do that.  As long as there's no
transaction that can't complete in close or any sense of multiple
transactions that aren't issued by open read/write close, we're covered.

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

Be fair: it is a new interface and it works in a way that's just
different enough from regular firmware to cause all this bother.

James

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Tuesday, April 28, 2015 6:52 AM
> 
> On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote:
> > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote:
> > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley
> > >> <James.Bottomley@hansenpartnership.com> wrote:
> > >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
> > >> >> > -----Original Message-----
> > >> >> > From: James Bottomley
> > >> >> > [mailto:James.Bottomley@HansenPartnership.com]
> > >> >> > Sent: Thursday, April 23, 2015 10:10 PM
> > >> >> >
> > >> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
> > >> >> > > > -----Original Message-----
> > >> >> > > > From: James Bottomley
> > >> >> > [mailto:James.Bottomley@HansenPartnership.com]
> > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM
> > >> >> > > >
> > >> >> > > >
> > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a
> > >> >> > > > question of whether
> > >> >> > we
> > >> >> > > > can stomach the ick factor of actually initiating a
> > >> >> > > > transaction in close ... I'm
> > >> >> > still
> > >> >> > > > feeling queasy.
> > >> >> > >
> > >> >> > > The file "close" here can I understand that the file system
> > >> >> > > will call the
> > >> >> > "release"
> > >> >> > > function at the file_operations struct?
> > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L153
> > >> >> > > 8
> > >> >> > >
> > >> >> > > So, James you are meaning that we could initiating the
> > >> >> > > update transaction inside the f_ops->release() and return
> > >> >> > > the error code if update failed in this function?
> > >> >> >
> > >> >> > Well, that's what I was thinking.  However the return value of
> > >> >> > ->release doesn't get propagated in sys_close (or indeed
> > >> >> > anywhere ... no idea why it returns an int) thanks to the task
> > >> >> > work additions, so we'd actually have to use the operation
> > >> >> > whose value is propagated in sys_close() which turns out to be
> flush.
> > >> >> >
> > >> >> > James
> > >> >> >
> > >> >>
> > >> >> Okay, I think I got you. Just to double check for in case: you
> > >> >> are meaning to implement it at f_ops->flush() instead of f_ops-
> >release().
> > >> >
> > >> > Well, what I'm saying is that the only way to propagate an error
> > >> > to close is by returning one from the flush file_operation.
> > >> >
> > >> > Let's cc fsdevel to see if they have any brighter ideas.
> > >> >
> > >> > The problem is we need to update firmware (several megabytes of
> > >> > it) via standard system tools.  We're thinking cat to a device.
> > >> > The problem is that we need an error code back once the update
> > >> > goes through (which it can't until we've fed all the firmware
> > >> > data into the system).  To use standard unix tools, we have to
> > >> > trigger off the standard system calls cat uses and since write()
> > >> > will happen in chunks, the only way to commit the transaction is in
> close().
> > >> >
> > >> > We initially through of initiating the transaction in
> > >> > f_ops->release and returning the error code there, but that
> > >> > doesn't work because its value isn't actually propagated, so
> > >> > we're now thinking of initiating the transaction in f_ops->flush
> > >> > instead (this is a device, not a file, so it won't get any other
> > >> > flushers).  Are there any other ways for us to propagate error on close?
> > >> >
> > >>
> > >> I think we may end up wanting to support both UpdateCapsule and
> > >> QueryCapsuleCapabilities, in which case this gets awkward.  Maybe
> > >> we really should do a misc device + ioctl.
> > >
> > > To be honest, I hate ioctls ... especially the "have to use special
> > > tools" part.
> > >
> > > Would we ever want to support QueryCapsuleUpdate()?  The return
> > > codes on error are the same as UpdateCapsule() but the query call
> > > does nothing on success (and the update call updates, obviously), so
> > > it seems a bit pointless if someone's gone to the trouble of getting
> > > a capsule ... they obviously want to apply it rather than know if it could be
> applied.
> >
> > I can imagine a UI that would try to validate a transaction consisting
> > of several of these things, tell the user whether it'll work and
> > whether a reboot is needed, and then do it.
> 
> You mean for dependent capsules?  That's a bit way overthinking the UEFI
> current use case (which is for firmware update).  In theory, the persist across
> reboot flag can be used for OS persistent information (subject to someone
> actually coming up with an implementation).  I'd code for the simple case:
> firmware update and let the rest take care of itself if and when we have an
> implementation.
> 
> The last thing I want to see landing on the UEFI-SST is some hopelessly
> complex and nasty capsule spec just "because Linux implements it this way".
> 
> > > Assuming we do, we could just use the same error on close mechanism,
> > > but use sysfs binary attributes ... or probably something new like a
> > > binary transaction attribute that does all the transaction on close
> > > magic for us.
> >
> > Yeah, but now we have both input and output, so as ugly as ioctl is,
> > it's a pretty good match.
> 
> No, we'll have read and write, so we can do that.  As long as there's no
> transaction that can't complete in close or any sense of multiple transactions
> that aren't issued by open read/write close, we're covered.
> 
> > Sigh.  This is all more complicated than it deserves to me.
> 
> Be fair: it is a new interface and it works in a way that's just different enough
> from regular firmware to cause all this bother.
> 
> James
> 

Dear communities,

I agree with James. Due to different people may have different needs. But
from our side, we would just like to have a simple interface for us to upload
the efi capsule and perform update. We do not have any use case or need
to get info from QueryCapsuleUpdate(). Let me give a suggestion here:
please allow me to focus on deliver this simple loading interface and
upstream it. Then later whoever has the actual use case or needs on the ioctl
implementation, he or she could enhance base on this simple loading interface.
What do you guys think?

Let me summarize the latest design idea:
- No longer leverage on firmware class but use misc device
- Do not use platform device but use device_create()
- User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell
- File operation functions include: open(), read(), write() and flush()
- Perform mutex lock in open() then release the mutex in flush() for avoiding
   race condition / concurrent loading
- Perform the capsule update and error return at flush() function

Is there anything I missed? Any one still have concern with this idea?
Thanks for providing the ideas as well as the review.


Regards,
Wilson

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

[Andy Lutomirski]

On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
>> -----Original Message-----
>> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
>> Sent: Tuesday, April 28, 2015 6:52 AM
>>
>> On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote:
>> > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley
>> > <James.Bottomley@hansenpartnership.com> wrote:
>> > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote:
>> > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley
>> > >> <James.Bottomley@hansenpartnership.com> wrote:
>> > >> > On Fri, 2015-04-24 at 02:14 +0000, Kweh, Hock Leong wrote:
>> > >> >> > -----Original Message-----
>> > >> >> > From: James Bottomley
>> > >> >> > [mailto:James.Bottomley@HansenPartnership.com]
>> > >> >> > Sent: Thursday, April 23, 2015 10:10 PM
>> > >> >> >
>> > >> >> > On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
>> > >> >> > > > -----Original Message-----
>> > >> >> > > > From: James Bottomley
>> > >> >> > [mailto:James.Bottomley@HansenPartnership.com]
>> > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM
>> > >> >> > > >
>> > >> >> > > >
>> > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a
>> > >> >> > > > question of whether
>> > >> >> > we
>> > >> >> > > > can stomach the ick factor of actually initiating a
>> > >> >> > > > transaction in close ... I'm
>> > >> >> > still
>> > >> >> > > > feeling queasy.
>> > >> >> > >
>> > >> >> > > The file "close" here can I understand that the file system
>> > >> >> > > will call the
>> > >> >> > "release"
>> > >> >> > > function at the file_operations struct?
>> > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L153
>> > >> >> > > 8
>> > >> >> > >
>> > >> >> > > So, James you are meaning that we could initiating the
>> > >> >> > > update transaction inside the f_ops->release() and return
>> > >> >> > > the error code if update failed in this function?
>> > >> >> >
>> > >> >> > Well, that's what I was thinking.  However the return value of
>> > >> >> > ->release doesn't get propagated in sys_close (or indeed
>> > >> >> > anywhere ... no idea why it returns an int) thanks to the task
>> > >> >> > work additions, so we'd actually have to use the operation
>> > >> >> > whose value is propagated in sys_close() which turns out to be
>> flush.
>> > >> >> >
>> > >> >> > James
>> > >> >> >
>> > >> >>
>> > >> >> Okay, I think I got you. Just to double check for in case: you
>> > >> >> are meaning to implement it at f_ops->flush() instead of f_ops-
>> >release().
>> > >> >
>> > >> > Well, what I'm saying is that the only way to propagate an error
>> > >> > to close is by returning one from the flush file_operation.
>> > >> >
>> > >> > Let's cc fsdevel to see if they have any brighter ideas.
>> > >> >
>> > >> > The problem is we need to update firmware (several megabytes of
>> > >> > it) via standard system tools.  We're thinking cat to a device.
>> > >> > The problem is that we need an error code back once the update
>> > >> > goes through (which it can't until we've fed all the firmware
>> > >> > data into the system).  To use standard unix tools, we have to
>> > >> > trigger off the standard system calls cat uses and since write()
>> > >> > will happen in chunks, the only way to commit the transaction is in
>> close().
>> > >> >
>> > >> > We initially through of initiating the transaction in
>> > >> > f_ops->release and returning the error code there, but that
>> > >> > doesn't work because its value isn't actually propagated, so
>> > >> > we're now thinking of initiating the transaction in f_ops->flush
>> > >> > instead (this is a device, not a file, so it won't get any other
>> > >> > flushers).  Are there any other ways for us to propagate error on close?
>> > >> >
>> > >>
>> > >> I think we may end up wanting to support both UpdateCapsule and
>> > >> QueryCapsuleCapabilities, in which case this gets awkward.  Maybe
>> > >> we really should do a misc device + ioctl.
>> > >
>> > > To be honest, I hate ioctls ... especially the "have to use special
>> > > tools" part.
>> > >
>> > > Would we ever want to support QueryCapsuleUpdate()?  The return
>> > > codes on error are the same as UpdateCapsule() but the query call
>> > > does nothing on success (and the update call updates, obviously), so
>> > > it seems a bit pointless if someone's gone to the trouble of getting
>> > > a capsule ... they obviously want to apply it rather than know if it could be
>> applied.
>> >
>> > I can imagine a UI that would try to validate a transaction consisting
>> > of several of these things, tell the user whether it'll work and
>> > whether a reboot is needed, and then do it.
>>
>> You mean for dependent capsules?  That's a bit way overthinking the UEFI
>> current use case (which is for firmware update).  In theory, the persist across
>> reboot flag can be used for OS persistent information (subject to someone
>> actually coming up with an implementation).  I'd code for the simple case:
>> firmware update and let the rest take care of itself if and when we have an
>> implementation.
>>
>> The last thing I want to see landing on the UEFI-SST is some hopelessly
>> complex and nasty capsule spec just "because Linux implements it this way".
>>
>> > > Assuming we do, we could just use the same error on close mechanism,
>> > > but use sysfs binary attributes ... or probably something new like a
>> > > binary transaction attribute that does all the transaction on close
>> > > magic for us.
>> >
>> > Yeah, but now we have both input and output, so as ugly as ioctl is,
>> > it's a pretty good match.
>>
>> No, we'll have read and write, so we can do that.  As long as there's no
>> transaction that can't complete in close or any sense of multiple transactions
>> that aren't issued by open read/write close, we're covered.
>>
>> > Sigh.  This is all more complicated than it deserves to me.
>>
>> Be fair: it is a new interface and it works in a way that's just different enough
>> from regular firmware to cause all this bother.
>>
>> James
>>
>
> Dear communities,
>
> I agree with James. Due to different people may have different needs. But
> from our side, we would just like to have a simple interface for us to upload
> the efi capsule and perform update. We do not have any use case or need
> to get info from QueryCapsuleUpdate(). Let me give a suggestion here:
> please allow me to focus on deliver this simple loading interface and
> upstream it. Then later whoever has the actual use case or needs on the ioctl
> implementation, he or she could enhance base on this simple loading interface.
> What do you guys think?
>
> Let me summarize the latest design idea:
> - No longer leverage on firmware class but use misc device
> - Do not use platform device but use device_create()
> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell

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

> - File operation functions include: open(), read(), write() and flush()
> - Perform mutex lock in open() then release the mutex in flush() for avoiding
>    race condition / concurrent loading

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

> - Perform the capsule update and error return at flush() function
>
> Is there anything I missed? Any one still have concern with this idea?
> Thanks for providing the ideas as well as the review.
>

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

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

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

--Andy

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

[James Bottomley]

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

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

James

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Thursday, April 30, 2015 2:41 AM
> 
> On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong
> <hock.leong.kweh@intel.com> wrote:
> >
> > Dear communities,
> >
> > I agree with James. Due to different people may have different needs.
> > But from our side, we would just like to have a simple interface for
> > us to upload the efi capsule and perform update. We do not have any
> > use case or need to get info from QueryCapsuleUpdate(). Let me give a
> suggestion here:
> > please allow me to focus on deliver this simple loading interface and
> > upstream it. Then later whoever has the actual use case or needs on
> > the ioctl implementation, he or she could enhance base on this simple
> loading interface.
> > What do you guys think?
> >
> > Let me summarize the latest design idea:
> > - No longer leverage on firmware class but use misc device
> > - Do not use platform device but use device_create()
> > - User just need to perform "cat file.bin > /sys/.../capsule_loader"
> > in the shell
> 
> If you do this, there's no need for the misc device.

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

> 
> > - File operation functions include: open(), read(), write() and
> > flush()
> > - Perform mutex lock in open() then release the mutex in flush() for
> avoiding
> >    race condition / concurrent loading
> 
> Make sure the mutex operation is killable, then, and maybe even
> interruptable.

Okay.

> 
> > - Perform the capsule update and error return at flush() function
> >
> > Is there anything I missed? Any one still have concern with this idea?
> > Thanks for providing the ideas as well as the review.
> >
> 
> If it works (and cat really does fail reliably), then it seems okay to me.
> 
> However, since I like pulling increasing numbers of my hats, someone should
> verify that the common embedded cat implementations are also okay with
> this.  For example, I haven't yet found any code in busybox's cat
> implementation that closes stdout.
> 
> Given that the main targets of this (for now, at least) are embedded, this
> might be a problem.
> 

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

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

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

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


Regards,
Wilson

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

[Andy Lutomirski]

On Thu, Apr 30, 2015 at 2:17 AM, Kweh, Hock Leong
<hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org]
>> Sent: Thursday, April 30, 2015 2:41 AM
>>
>> On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong
>> <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> > Dear communities,
>> >
>> > I agree with James. Due to different people may have different needs.
>> > But from our side, we would just like to have a simple interface for
>> > us to upload the efi capsule and perform update. We do not have any
>> > use case or need to get info from QueryCapsuleUpdate(). Let me give a
>> suggestion here:
>> > please allow me to focus on deliver this simple loading interface and
>> > upstream it. Then later whoever has the actual use case or needs on
>> > the ioctl implementation, he or she could enhance base on this simple
>> loading interface.
>> > What do you guys think?
>> >
>> > Let me summarize the latest design idea:
>> > - No longer leverage on firmware class but use misc device
>> > - Do not use platform device but use device_create()
>> > - User just need to perform "cat file.bin > /sys/.../capsule_loader"
>> > in the shell
>>
>> If you do this, there's no need for the misc device.
>
> I do this so that in the future when someone want to implement the
> Ioctl(), he or she can base on this and expand it.
>
>>
>> > - File operation functions include: open(), read(), write() and
>> > flush()
>> > - Perform mutex lock in open() then release the mutex in flush() for
>> avoiding
>> >    race condition / concurrent loading
>>
>> Make sure the mutex operation is killable, then, and maybe even
>> interruptable.
>
> Okay.
>
>>
>> > - Perform the capsule update and error return at flush() function
>> >
>> > Is there anything I missed? Any one still have concern with this idea?
>> > Thanks for providing the ideas as well as the review.
>> >
>>
>> If it works (and cat really does fail reliably), then it seems okay to me.
>>
>> However, since I like pulling increasing numbers of my hats, someone should
>> verify that the common embedded cat implementations are also okay with
>> this.  For example, I haven't yet found any code in busybox's cat
>> implementation that closes stdout.
>>
>> Given that the main targets of this (for now, at least) are embedded, this
>> might be a problem.
>>
>
> I think we shouldn't focus on the cat implementation for the close issue.
>
> My understanding about this action:
> cat file.bin > /sys/..../capsule_loader
> It is actually the ">" (IO redirection) who perform the open write & close
> to this "/sys/..../capsule_loader" file note and not the "cat" do it.
> So, I think your answer can be found at Shell source code.

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

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

If you type:

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

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

--Andy

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

[James Bottomley]

On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote:
> I agree with James. Due to different people may have different needs. But
> from our side, we would just like to have a simple interface for us to upload
> the efi capsule and perform update. We do not have any use case or need
> to get info from QueryCapsuleUpdate(). Let me give a suggestion here:
> please allow me to focus on deliver this simple loading interface and
> upstream it. Then later whoever has the actual use case or needs on the ioctl
> implementation, he or she could enhance base on this simple loading interface.
> What do you guys think?
> 
> Let me summarize the latest design idea:
> - No longer leverage on firmware class but use misc device
> - Do not use platform device but use device_create()
> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell
> - File operation functions include: open(), read(), write() and flush()
> - Perform mutex lock in open() then release the mutex in flush() for avoiding
>    race condition / concurrent loading
> - Perform the capsule update and error return at flush() function
> 
> Is there anything I missed? Any one still have concern with this idea?
> Thanks for providing the ideas as well as the review.

I think that's pretty much it.

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

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

James

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

[Andy Lutomirski]

On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote:
>> I agree with James. Due to different people may have different needs. But
>> from our side, we would just like to have a simple interface for us to upload
>> the efi capsule and perform update. We do not have any use case or need
>> to get info from QueryCapsuleUpdate(). Let me give a suggestion here:
>> please allow me to focus on deliver this simple loading interface and
>> upstream it. Then later whoever has the actual use case or needs on the ioctl
>> implementation, he or she could enhance base on this simple loading interface.
>> What do you guys think?
>>
>> Let me summarize the latest design idea:
>> - No longer leverage on firmware class but use misc device
>> - Do not use platform device but use device_create()
>> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell
>> - File operation functions include: open(), read(), write() and flush()
>> - Perform mutex lock in open() then release the mutex in flush() for avoiding
>>    race condition / concurrent loading
>> - Perform the capsule update and error return at flush() function
>>
>> Is there anything I missed? Any one still have concern with this idea?
>> Thanks for providing the ideas as well as the review.
>
> I think that's pretty much it.
>
> Why don't you let me construct a straw man patch.  It's going to be a
> bit controversial because it involves adding flush operations to sysfs
> and kernfs, slicing apart firmware_class.c to extract the transaction
> handling stuff and creating an new efi update capsule file which makes
> use of it.
>
> Once we have code, we at least have something more concrete to argue
> over.

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

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

--Andy

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

[James Bottomley]

On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote:
> On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote:
> >> I agree with James. Due to different people may have different needs. But
> >> from our side, we would just like to have a simple interface for us to upload
> >> the efi capsule and perform update. We do not have any use case or need
> >> to get info from QueryCapsuleUpdate(). Let me give a suggestion here:
> >> please allow me to focus on deliver this simple loading interface and
> >> upstream it. Then later whoever has the actual use case or needs on the ioctl
> >> implementation, he or she could enhance base on this simple loading interface.
> >> What do you guys think?
> >>
> >> Let me summarize the latest design idea:
> >> - No longer leverage on firmware class but use misc device
> >> - Do not use platform device but use device_create()
> >> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell
> >> - File operation functions include: open(), read(), write() and flush()
> >> - Perform mutex lock in open() then release the mutex in flush() for avoiding
> >>    race condition / concurrent loading
> >> - Perform the capsule update and error return at flush() function
> >>
> >> Is there anything I missed? Any one still have concern with this idea?
> >> Thanks for providing the ideas as well as the review.
> >
> > I think that's pretty much it.
> >
> > Why don't you let me construct a straw man patch.  It's going to be a
> > bit controversial because it involves adding flush operations to sysfs
> > and kernfs, slicing apart firmware_class.c to extract the transaction
> > handling stuff and creating an new efi update capsule file which makes
> > use of it.
> >
> > Once we have code, we at least have something more concrete to argue
> > over.
> 
> Would it be worth checking whether busybox is also okay with it first?
> (Sorry to be a naysayer.)
> 
> It would be a shame if we do all this to keep the userspace footprint
> light and then it doesn't work for non-coreutils userspace.

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

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

James

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

[Andy Lutomirski]

On Wed, Apr 29, 2015 at 2:39 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Wed, 2015-04-29 at 14:36 -0700, Andy Lutomirski wrote:
>> On Wed, Apr 29, 2015 at 2:35 PM, James Bottomley
>> <James.Bottomley@hansenpartnership.com> wrote:
>> > On Wed, 2015-04-29 at 11:23 +0000, Kweh, Hock Leong wrote:
>> >> I agree with James. Due to different people may have different needs. But
>> >> from our side, we would just like to have a simple interface for us to upload
>> >> the efi capsule and perform update. We do not have any use case or need
>> >> to get info from QueryCapsuleUpdate(). Let me give a suggestion here:
>> >> please allow me to focus on deliver this simple loading interface and
>> >> upstream it. Then later whoever has the actual use case or needs on the ioctl
>> >> implementation, he or she could enhance base on this simple loading interface.
>> >> What do you guys think?
>> >>
>> >> Let me summarize the latest design idea:
>> >> - No longer leverage on firmware class but use misc device
>> >> - Do not use platform device but use device_create()
>> >> - User just need to perform "cat file.bin > /sys/.../capsule_loader" in the shell
>> >> - File operation functions include: open(), read(), write() and flush()
>> >> - Perform mutex lock in open() then release the mutex in flush() for avoiding
>> >>    race condition / concurrent loading
>> >> - Perform the capsule update and error return at flush() function
>> >>
>> >> Is there anything I missed? Any one still have concern with this idea?
>> >> Thanks for providing the ideas as well as the review.
>> >
>> > I think that's pretty much it.
>> >
>> > Why don't you let me construct a straw man patch.  It's going to be a
>> > bit controversial because it involves adding flush operations to sysfs
>> > and kernfs, slicing apart firmware_class.c to extract the transaction
>> > handling stuff and creating an new efi update capsule file which makes
>> > use of it.
>> >
>> > Once we have code, we at least have something more concrete to argue
>> > over.
>>
>> Would it be worth checking whether busybox is also okay with it first?
>> (Sorry to be a naysayer.)
>>
>> It would be a shame if we do all this to keep the userspace footprint
>> light and then it doesn't work for non-coreutils userspace.
>
> I don't think so, because we can fix busybox if it's a problem.  The
> embedded people wanting this control the tool space, so they can decide
> to use the fixed version.
>
> So yes, someone should check and fix busybox cat if broken, but no, it's
> not a blocker.

It's still a bit unfortunate that:

#!/bin/sh

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

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

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

--Andy


-- 
Andy Lutomirski
AMA Capital Management, LLC

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

[James Bottomley]

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

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

[James Bottomley]

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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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

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

[Greg Kroah-Hartman]

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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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

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

[James Bottomley]

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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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

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

[Greg Kroah-Hartman]

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-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.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

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

[James Bottomley]

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

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

[Greg Kroah-Hartman]

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