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

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

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

Dear maintainers & communities,

This patchset is created on top of Matt's patchset:
1.)https://lkml.org/lkml/2014/10/7/390
"[PATCH 1/2] efi: Move efi_status_to_err() to efi.h"
2.)https://lkml.org/lkml/2014/10/7/391
"[PATCH 2/2] efi: Capsule update support"

It expose a misc char interface for user to upload the capsule binary and
calling efi_capsule_update() API to pass the binary to EFI firmware.

The steps to update efi firmware are:
1.) cat firmware.cap > /dev/efi_capsule_loader
2.) reboot

Any failed upload error message will be returned while doing "cat" through
Write() function call.

Tested the code with Intel Quark Galileo platform.

Thanks.

---
changelog v5:
* changed to new design without leveraging firmware_class API
* use misc_char device interface instead of sysfs
* error return through file Write() function call


Kweh, Hock Leong (2):
  efi: export efi_capsule_supported() function symbol
  efi: a misc char interface for user to update efi firmware

 drivers/firmware/efi/Kconfig              |   10 ++
 drivers/firmware/efi/Makefile             |    1
 drivers/firmware/efi/capsule.c            |    1
 drivers/firmware/efi/efi-capsule-loader.c |  218 +++++++++++++++++++++++++++++
 4 files changed, 230 insertions(+)
 create mode 100644 drivers/firmware/efi/efi-capsule-loader.c

-- 
1.7.9.5

[PATCH v5 1/2] efi: export efi_capsule_supported() function symbol

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

This patch export efi_capsule_supported() function symbol for capsule
kernel module to use.

Cc: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/firmware/efi/capsule.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index d8cd75c0..738d437 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -101,6 +101,7 @@ out:
 	kfree(capsule);
 	return rv;
 }
+EXPORT_SYMBOL_GPL(efi_capsule_supported);
 
 /**
  * efi_capsule_update - send a capsule to the firmware
-- 
1.7.9.5

[PATCH v5 2/2] efi: a misc char interface for user to update efi firmware

[Kweh, Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

Introducing a kernel module to expose capsule loader interface
(misc char device file note) for user to upload capsule binaries.

Example method to load the capsule binary:
cat firmware.bin > /dev/efi_capsule_loader

Cc: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/firmware/efi/Kconfig              |   10 ++
 drivers/firmware/efi/Makefile             |    1 +
 drivers/firmware/efi/efi-capsule-loader.c |  218 +++++++++++++++++++++++++++++
 3 files changed, 229 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..0be8ee3 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_ARMSTUB
 	bool
 
+config EFI_CAPSULE_LOADER
+	tristate "EFI capsule loader"
+	depends on EFI
+	help
+	  This option exposes a loader interface "/dev/efi_capsule_loader" for
+	  user to load EFI capsule binary and update the EFI firmware through
+	  system reboot.
+
+	  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..1da8608
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,218 @@
+/*
+ * 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/miscdevice.h>
+#include <linux/highmem.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/efi.h>
+
+#define DEV_NAME "efi_capsule_loader"
+
+struct capsule_info {
+	int		curr_index;
+	int		curr_size;
+	int		total_size;
+	struct page	**pages;
+};
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c
+ */
+static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
+				 size_t count, loff_t *offp)
+{
+	int ret = 0;
+	struct capsule_info *cap_info = file->private_data;
+	struct page *kbuff_page;
+	void *kbuff;
+
+	pr_debug("%s: Entering Write()\n", __func__);
+	if (count == 0)
+		return 0;
+
+	if (cap_info->curr_index == -1)
+		return count;
+
+	kbuff_page = alloc_page(GFP_KERNEL);
+	if (!kbuff_page) {
+		pr_err("%s: alloc_page() failed\n", __func__);
+		if (!cap_info->curr_index)
+			return -ENOMEM;
+		ret = -ENOMEM;
+		goto failed;
+	}
+
+	kbuff = kmap(kbuff_page);
+	if (!kbuff) {
+		pr_err("%s: kmap() failed\n", __func__);
+		if (cap_info->curr_index)
+			cap_info->pages[cap_info->curr_index++] = kbuff_page;
+		ret = -EFAULT;
+		goto failed;
+	}
+
+	/* copy capsule binary data from user space to kernel space buffer */
+	if (copy_from_user(kbuff, buff, min_t(size_t, count, PAGE_SIZE))) {
+		pr_err("%s: copy_from_user() failed\n", __func__);
+		if (cap_info->curr_index)
+			cap_info->pages[cap_info->curr_index++] = kbuff_page;
+		kunmap(kbuff_page);
+		ret = -EFAULT;
+		goto failed;
+	}
+
+	/* setup capsule binary info structure */
+	if (cap_info->curr_index == 0) {
+		efi_capsule_header_t *cap_hdr = kbuff;
+		int reset_type;
+		int pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+					PAGE_SHIFT;
+
+		if (pages_needed <= 0) {
+			pr_err("%s: pages count invalid\n", __func__);
+			ret = -EINVAL;
+			kunmap(kbuff_page);
+			goto failed;
+		}
+
+		/* check if the capsule binary supported */
+		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+					    cap_hdr->imagesize, &reset_type);
+		if (ret) {
+			pr_err("%s: efi_capsule_supported() failed\n",
+			       __func__);
+			kunmap(kbuff_page);
+			goto failed;
+		}
+
+		cap_info->total_size = cap_hdr->imagesize;
+		cap_info->pages = kmalloc_array(pages_needed, sizeof(void *),
+						GFP_KERNEL);
+		if (!cap_info->pages) {
+			pr_err("%s: kmalloc_array() failed\n", __func__);
+			ret = -ENOMEM;
+			kunmap(kbuff_page);
+			goto failed;
+		}
+	}
+
+	cap_info->pages[cap_info->curr_index++] = kbuff_page;
+	cap_info->curr_size += count;
+	kunmap(kbuff_page);
+
+	/* submit the full binary to efi_capsule_update() API */
+	if (cap_info->curr_size >= cap_info->total_size) {
+		ret = efi_capsule_update(kmap(cap_info->pages[0]),
+					 cap_info->pages);
+		kunmap(cap_info->pages[0]);
+		if (ret) {
+			pr_err("%s: efi_capsule_update() failed\n", __func__);
+			goto failed;
+		}
+		/* indicate capsule binary uploading is done */
+		cap_info->curr_index = -1;
+	}
+
+	/*
+	 * we cannot free the pages here due to reboot need that data
+	 * maintained.
+	 */
+	return count;
+
+failed:
+	if (!cap_info->curr_index) {
+		__free_page(kbuff_page);
+	} else {
+		while (cap_info->curr_index > 0)
+			__free_page(cap_info->pages[--cap_info->curr_index]);
+		kfree(cap_info->pages);
+	}
+
+	return ret;
+}
+
+static int efi_capsule_release(struct inode *inode, struct file *file)
+{
+	int ret = 0;
+	struct capsule_info *cap_info = file->private_data;
+
+	pr_debug("%s: Exit in Release()\n", __func__);
+	/* release those uncompleted uploaded block */
+	if (cap_info->curr_index > 0) {
+		pr_err("%s: capsule upload not complete\n", __func__);
+		while (cap_info->curr_index > 0)
+			__free_page(cap_info->pages[--cap_info->curr_index]);
+		kfree(cap_info->pages);
+		ret = -ECANCELED;
+	}
+	kfree(file->private_data);
+	file->private_data = NULL;
+	mutex_unlock(&capsule_loader_lock);
+	return ret;
+}
+
+static int efi_capsule_open(struct inode *inode, struct file *file)
+{
+	int ret = -EBUSY;
+	struct capsule_info *cap_info;
+
+	pr_debug("%s: Entering Open()\n", __func__);
+	if (mutex_trylock(&capsule_loader_lock)) {
+		cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
+		file->private_data = cap_info;
+		ret = 0;
+	}
+	return ret;
+}
+
+static const struct file_operations efi_capsule_fops = {
+	.owner = THIS_MODULE,
+	.open = efi_capsule_open,
+	.write = efi_capsule_write,
+	.release = efi_capsule_release,
+	.llseek = no_llseek,
+};
+
+static struct miscdevice efi_capsule_misc = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = DEV_NAME,
+	.fops = &efi_capsule_fops,
+};
+
+static int __init efi_capsule_loader_init(void)
+{
+	int ret;
+
+	mutex_init(&capsule_loader_lock);
+
+	ret = misc_register(&efi_capsule_misc);
+	if (ret)
+		pr_err("%s: misc_register() failed\n", __func__);
+
+	return ret;
+}
+module_init(efi_capsule_loader_init);
+
+static void __exit efi_capsule_loader_exit(void)
+{
+	mutex_destroy(&capsule_loader_lock);
+	misc_deregister(&efi_capsule_misc);
+}
+module_exit(efi_capsule_loader_exit);
+
+MODULE_DESCRIPTION("EFI capsule firmware binary loader");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v5 2/2] efi: a misc char interface for user to update efi firmware

[Matt Fleming]

On Fri, 21 Aug, at 07:34:48PM, Kweh Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> 
> Introducing a kernel module to expose capsule loader interface
> (misc char device file note) for user to upload capsule binaries.
> 
> Example method to load the capsule binary:
> cat firmware.bin > /dev/efi_capsule_loader
 
OK interesting, we're going down the misc char device route - Andy
might be happier, even if there is no ioctl(2) support.

> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/Kconfig              |   10 ++
>  drivers/firmware/efi/Makefile             |    1 +
>  drivers/firmware/efi/efi-capsule-loader.c |  218 +++++++++++++++++++++++++++++
>  3 files changed, 229 insertions(+)
>  create mode 100644 drivers/firmware/efi/efi-capsule-loader.c

[...]

> diff --git a/drivers/firmware/efi/efi-capsule-loader.c b/drivers/firmware/efi/efi-capsule-loader.c
> new file mode 100644
> index 0000000..1da8608
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-loader.c
> @@ -0,0 +1,218 @@
> +/*
> + * 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/miscdevice.h>
> +#include <linux/highmem.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/efi.h>
> +
> +#define DEV_NAME "efi_capsule_loader"
> +
> +struct capsule_info {
> +	int		curr_index;
> +	int		curr_size;
> +	int		total_size;

It's totally conceivable that a capsule might be greater than 2GB in
size. In which case, 'int' is the wrong data type for these size
fields. Perhaps 'unsigned long' ?

I'd suggest swapping 'curr_index' for simply 'index' and 'curr_size'
for 'count' or 'bytes'.

> +	struct page	**pages;
> +};
> +
> +static DEFINE_MUTEX(capsule_loader_lock);
> +
> +/*
> + * This function will store the capsule binary and pass it to
> + * efi_capsule_update() API in capsule.c
> + */
> +static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
> +				 size_t count, loff_t *offp)
> +{
> +	int ret = 0;
> +	struct capsule_info *cap_info = file->private_data;
> +	struct page *kbuff_page;
> +	void *kbuff;
> +
> +	pr_debug("%s: Entering Write()\n", __func__);
> +	if (count == 0)
> +		return 0;
> +
> +	if (cap_info->curr_index == -1)
> +		return count;

Shouldn't we be returning an error here to signal that the driver
wasn't expecting any more data to be sent? Otherwise the caller will
think everything worked out fine, but it didn't. See my comments below
about the success/failure design.

> +
> +	kbuff_page = alloc_page(GFP_KERNEL);
> +	if (!kbuff_page) {
> +		pr_err("%s: alloc_page() failed\n", __func__);
> +		if (!cap_info->curr_index)
> +			return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto failed;
> +	}

If the caller writes less than PAGE_SIZE bytes at a time this could be
incredibly wasteful. We should use the remaining space from any
previous page allocations.

> +
> +	kbuff = kmap(kbuff_page);
> +	if (!kbuff) {
> +		pr_err("%s: kmap() failed\n", __func__);
> +		if (cap_info->curr_index)
> +			cap_info->pages[cap_info->curr_index++] = kbuff_page;
> +		ret = -EFAULT;
> +		goto failed;
> +	}
> +
> +	/* copy capsule binary data from user space to kernel space buffer */
> +	if (copy_from_user(kbuff, buff, min_t(size_t, count, PAGE_SIZE))) {
> +		pr_err("%s: copy_from_user() failed\n", __func__);
> +		if (cap_info->curr_index)
> +			cap_info->pages[cap_info->curr_index++] = kbuff_page;

Huh? Is this to satisfy the requirements of the code at the 'failed'
label? The error handling could do with some cleanup because it's very
difficult to follow the code flow.

Please try and get rid of the housekeeping code where you add
kbuff_page to the pages array just to make the 'failed' code work.

I'd also ditch the pr_err() calls for all but the most fatal of error
conditions because they make the code harder to read.

> +		kunmap(kbuff_page);
> +		ret = -EFAULT;
> +		goto failed;
> +	}
> +
> +	/* setup capsule binary info structure */
> +	if (cap_info->curr_index == 0) {
> +		efi_capsule_header_t *cap_hdr = kbuff;
> +		int reset_type;
> +		int pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
> +					PAGE_SHIFT;
> +
> +		if (pages_needed <= 0) {

Can ALIGN() even return a genuine negative value? 'pages_needed'
should be 'size_t'.

> +			pr_err("%s: pages count invalid\n", __func__);
> +			ret = -EINVAL;
> +			kunmap(kbuff_page);
> +			goto failed;
> +		}
> +
> +		/* check if the capsule binary supported */
> +		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> +					    cap_hdr->imagesize, &reset_type);
> +		if (ret) {
> +			pr_err("%s: efi_capsule_supported() failed\n",
> +			       __func__);
> +			kunmap(kbuff_page);
> +			goto failed;
> +		}
> +
> +		cap_info->total_size = cap_hdr->imagesize;
> +		cap_info->pages = kmalloc_array(pages_needed, sizeof(void *),
> +						GFP_KERNEL);
> +		if (!cap_info->pages) {
> +			pr_err("%s: kmalloc_array() failed\n", __func__);
> +			ret = -ENOMEM;
> +			kunmap(kbuff_page);
> +			goto failed;
> +		}
> +	}
> +
> +	cap_info->pages[cap_info->curr_index++] = kbuff_page;
> +	cap_info->curr_size += count;
> +	kunmap(kbuff_page);
> +
> +	/* submit the full binary to efi_capsule_update() API */
> +	if (cap_info->curr_size >= cap_info->total_size) {
> +		ret = efi_capsule_update(kmap(cap_info->pages[0]),
> +					 cap_info->pages);

But kmap() can fail, so you need to handle that.

> +		kunmap(cap_info->pages[0]);
> +		if (ret) {
> +			pr_err("%s: efi_capsule_update() failed\n", __func__);
> +			goto failed;
> +		}
> +		/* indicate capsule binary uploading is done */
> +		cap_info->curr_index = -1;
> +	}
> +
> +	/*
> +	 * we cannot free the pages here due to reboot need that data
> +	 * maintained.
> +	 */
> +	return count;
> +
> +failed:
> +	if (!cap_info->curr_index) {
> +		__free_page(kbuff_page);
> +	} else {
> +		while (cap_info->curr_index > 0)
> +			__free_page(cap_info->pages[--cap_info->curr_index]);
> +		kfree(cap_info->pages);
> +	}
> +
> +	return ret;

Let's explicitly discuss the failure mode. If we fail in this function
we need to decide what happens if the userspace tool continues writing
data instead of closing the file.

It looks like we expect the userspace tool to start at the beginning
of the capsule data again, but you explicitly prohibit calling
lseek(2) by using the 'no_llseek' file_operations function. That makes
it difficult to verify that the app is starting at the beginning of
the file (I also notice that 'ppos' isn't being updated).

In the success case we expect the user to close/open this file to
write more capsules.

Personally, I think these two approaches are backwards. You should be
able to continue writing capsule data as long as write(2) returns
success, but should have to close/re-open the device file as soon as
an error is encountered. If you don't close/re-open on failure I'd
expect write(2) to return -EIO or somesuch error until you do.

It's worth explicitly documenting these two scenarios in the code.

> +}
> +
> +static int efi_capsule_release(struct inode *inode, struct file *file)
> +{
> +	int ret = 0;
> +	struct capsule_info *cap_info = file->private_data;
> +
> +	pr_debug("%s: Exit in Release()\n", __func__);
> +	/* release those uncompleted uploaded block */
> +	if (cap_info->curr_index > 0) {
> +		pr_err("%s: capsule upload not complete\n", __func__);
> +		while (cap_info->curr_index > 0)
> +			__free_page(cap_info->pages[--cap_info->curr_index]);
> +		kfree(cap_info->pages);
> +		ret = -ECANCELED;

Interestingly the return value from ->release() isn't propagated to
userspace, look at __fput(). This is because the actual put'ing of the
file is delayed.

Notice how James used ->flush() in his patch series, which *does*
propagate the return value and most importantly, does so at close(2)
time (and also if the task exits for any reason, like being killed by
a fatal signal).

> +	}
> +	kfree(file->private_data);
> +	file->private_data = NULL;
> +	mutex_unlock(&capsule_loader_lock);
> +	return ret;
> +}
> +
> +static int efi_capsule_open(struct inode *inode, struct file *file)
> +{
> +	int ret = -EBUSY;
> +	struct capsule_info *cap_info;
> +
> +	pr_debug("%s: Entering Open()\n", __func__);
> +	if (mutex_trylock(&capsule_loader_lock)) {
> +		cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
> +		file->private_data = cap_info;
> +		ret = 0;
> +	}
> +	return ret;
> +}

I think this would be more readable like this,

	if (mutex_trylock(&capsule_loader_lock))
		return -EBUSY;

	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
	file->private_data = cap_info;

	return 0;

Also, if we only allow one open at a time, why not make 'cap_info'
statically allocated so that you don't need to handle the kzalloc()
failure in this function?

-- 
Matt Fleming, Intel Open Source Technology Center

RE: [PATCH v5 2/2] efi: a misc char interface for user to update efi firmware

[Ong, Boon Leong]

>OK, that's interesting. Which cat tool is included with the Quark BSP?
>
Quark is using busybox 1.22.1

RE: [PATCH v5 2/2] efi: a misc char interface for user to update efi firmware

[Ong, Boon Leong]

>OK, that's interesting. Which cat tool is included with the Quark BSP?
>
Quark is using busybox 1.22.1

Re: [PATCH v5 2/2] efi: a misc char interface for user to update efi firmware

['Matt Fleming']

On Wed, 02 Sep, at 06:31:36AM, Kweh Hock Leong wrote:
> 
> I have done an experiment on that by using the misc char device file note.
> I included the flush() callback function to the fops. In flush(), I put a printk()
> and return -EINVAL. When I perform "cat XXX > /dev/XXX" on Intel Quark
> Galileo platform, I found that the flush() is not just being called at file
> close, it will also being called before the file write. And the error return, that I
> forced in the flush(), does not show up at shell terminal. This is the reason that
> I did not follow James recommendation and figure out to do it at write().

OK, that's interesting. Which cat tool is included with the Quark BSP?
 
-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v5 2/2] efi: a misc char interface for user to update efi firmware

['Matt Fleming']

On Wed, 02 Sep, at 06:31:36AM, Kweh Hock Leong wrote:
> 
> I have done an experiment on that by using the misc char device file note.
> I included the flush() callback function to the fops. In flush(), I put a printk()
> and return -EINVAL. When I perform "cat XXX > /dev/XXX" on Intel Quark
> Galileo platform, I found that the flush() is not just being called at file
> close, it will also being called before the file write. And the error return, that I
> forced in the flush(), does not show up at shell terminal. This is the reason that
> I did not follow James recommendation and figure out to do it at write().

OK, that's interesting. Which cat tool is included with the Quark BSP?
 
-- 
Matt Fleming, Intel Open Source Technology Center

RE: [PATCH v5 2/2] efi: a misc char interface for user to update efi firmware

[Kweh, Hock Leong]

> -----Original Message-----
> From: Matt Fleming [mailto:matt@codeblueprint.co.uk]
> Sent: Thursday, August 27, 2015 10:43 PM
> >
> > Introducing a kernel module to expose capsule loader interface
> > (misc char device file note) for user to upload capsule binaries.
> >
> > Example method to load the capsule binary:
> > cat firmware.bin > /dev/efi_capsule_loader
> 
> OK interesting, we're going down the misc char device route - Andy
> might be happier, even if there is no ioctl(2) support.
> 
> > Cc: Matt Fleming <matt.fleming@intel.com>
> > Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> > ---
> >  drivers/firmware/efi/Kconfig              |   10 ++
> >  drivers/firmware/efi/Makefile             |    1 +
> >  drivers/firmware/efi/efi-capsule-loader.c |  218
> +++++++++++++++++++++++++++++
> >  3 files changed, 229 insertions(+)
> >  create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
> 
> [...]
> 
> > diff --git a/drivers/firmware/efi/efi-capsule-loader.c
> b/drivers/firmware/efi/efi-capsule-loader.c
> > new file mode 100644
> > index 0000000..1da8608
> > --- /dev/null
> > +++ b/drivers/firmware/efi/efi-capsule-loader.c
> > @@ -0,0 +1,218 @@
> > +/*
> > + * 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/miscdevice.h>
> > +#include <linux/highmem.h>
> > +#include <linux/slab.h>
> > +#include <linux/mutex.h>
> > +#include <linux/efi.h>
> > +
> > +#define DEV_NAME "efi_capsule_loader"
> > +
> > +struct capsule_info {
> > +	int		curr_index;
> > +	int		curr_size;
> > +	int		total_size;
> 
> It's totally conceivable that a capsule might be greater than 2GB in
> size. In which case, 'int' is the wrong data type for these size
> fields. Perhaps 'unsigned long' ?
> 
> I'd suggest swapping 'curr_index' for simply 'index' and 'curr_size'
> for 'count' or 'bytes'.
> 

Will do.

> > +	struct page	**pages;
> > +};
> > +
> > +static DEFINE_MUTEX(capsule_loader_lock);
> > +
> > +/*
> > + * This function will store the capsule binary and pass it to
> > + * efi_capsule_update() API in capsule.c
> > + */
> > +static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
> > +				 size_t count, loff_t *offp)
> > +{
> > +	int ret = 0;
> > +	struct capsule_info *cap_info = file->private_data;
> > +	struct page *kbuff_page;
> > +	void *kbuff;
> > +
> > +	pr_debug("%s: Entering Write()\n", __func__);
> > +	if (count == 0)
> > +		return 0;
> > +
> > +	if (cap_info->curr_index == -1)
> > +		return count;
> 
> Shouldn't we be returning an error here to signal that the driver
> wasn't expecting any more data to be sent? Otherwise the caller will
> think everything worked out fine, but it didn't. See my comments below
> about the success/failure design.
> 

Ok, not a problem. 

> > +
> > +	kbuff_page = alloc_page(GFP_KERNEL);
> > +	if (!kbuff_page) {
> > +		pr_err("%s: alloc_page() failed\n", __func__);
> > +		if (!cap_info->curr_index)
> > +			return -ENOMEM;
> > +		ret = -ENOMEM;
> > +		goto failed;
> > +	}
> 
> If the caller writes less than PAGE_SIZE bytes at a time this could be
> incredibly wasteful. We should use the remaining space from any
> previous page allocations.
> 

Ok, will re-think about this part.

> > +
> > +	kbuff = kmap(kbuff_page);
> > +	if (!kbuff) {
> > +		pr_err("%s: kmap() failed\n", __func__);
> > +		if (cap_info->curr_index)
> > +			cap_info->pages[cap_info->curr_index++] =
> kbuff_page;
> > +		ret = -EFAULT;
> > +		goto failed;
> > +	}
> > +
> > +	/* copy capsule binary data from user space to kernel space buffer */
> > +	if (copy_from_user(kbuff, buff, min_t(size_t, count, PAGE_SIZE))) {
> > +		pr_err("%s: copy_from_user() failed\n", __func__);
> > +		if (cap_info->curr_index)
> > +			cap_info->pages[cap_info->curr_index++] =
> kbuff_page;
> 
> Huh? Is this to satisfy the requirements of the code at the 'failed'
> label? The error handling could do with some cleanup because it's very
> difficult to follow the code flow.
> 
> Please try and get rid of the housekeeping code where you add
> kbuff_page to the pages array just to make the 'failed' code work.
> 
> I'd also ditch the pr_err() calls for all but the most fatal of error
> conditions because they make the code harder to read.
> 

Hmm..... Let me think how to make it better. Thx.

> > +		kunmap(kbuff_page);
> > +		ret = -EFAULT;
> > +		goto failed;
> > +	}
> > +
> > +	/* setup capsule binary info structure */
> > +	if (cap_info->curr_index == 0) {
> > +		efi_capsule_header_t *cap_hdr = kbuff;
> > +		int reset_type;
> > +		int pages_needed = ALIGN(cap_hdr->imagesize,
> PAGE_SIZE) >>
> > +					PAGE_SHIFT;
> > +
> > +		if (pages_needed <= 0) {
> 
> Can ALIGN() even return a genuine negative value? 'pages_needed'
> should be 'size_t'.
> 

Ok, will change.

> > +			pr_err("%s: pages count invalid\n", __func__);
> > +			ret = -EINVAL;
> > +			kunmap(kbuff_page);
> > +			goto failed;
> > +		}
> > +
> > +		/* check if the capsule binary supported */
> > +		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> > +					    cap_hdr->imagesize, &reset_type);
> > +		if (ret) {
> > +			pr_err("%s: efi_capsule_supported() failed\n",
> > +			       __func__);
> > +			kunmap(kbuff_page);
> > +			goto failed;
> > +		}
> > +
> > +		cap_info->total_size = cap_hdr->imagesize;
> > +		cap_info->pages = kmalloc_array(pages_needed, sizeof(void
> *),
> > +						GFP_KERNEL);
> > +		if (!cap_info->pages) {
> > +			pr_err("%s: kmalloc_array() failed\n", __func__);
> > +			ret = -ENOMEM;
> > +			kunmap(kbuff_page);
> > +			goto failed;
> > +		}
> > +	}
> > +
> > +	cap_info->pages[cap_info->curr_index++] = kbuff_page;
> > +	cap_info->curr_size += count;
> > +	kunmap(kbuff_page);
> > +
> > +	/* submit the full binary to efi_capsule_update() API */
> > +	if (cap_info->curr_size >= cap_info->total_size) {
> > +		ret = efi_capsule_update(kmap(cap_info->pages[0]),
> > +					 cap_info->pages);
> 
> But kmap() can fail, so you need to handle that.
> 

Ok, will take care this.

> > +		kunmap(cap_info->pages[0]);
> > +		if (ret) {
> > +			pr_err("%s: efi_capsule_update() failed\n",
> __func__);
> > +			goto failed;
> > +		}
> > +		/* indicate capsule binary uploading is done */
> > +		cap_info->curr_index = -1;
> > +	}
> > +
> > +	/*
> > +	 * we cannot free the pages here due to reboot need that data
> > +	 * maintained.
> > +	 */
> > +	return count;
> > +
> > +failed:
> > +	if (!cap_info->curr_index) {
> > +		__free_page(kbuff_page);
> > +	} else {
> > +		while (cap_info->curr_index > 0)
> > +			__free_page(cap_info->pages[--cap_info-
> >curr_index]);
> > +		kfree(cap_info->pages);
> > +	}
> > +
> > +	return ret;
> 
> Let's explicitly discuss the failure mode. If we fail in this function
> we need to decide what happens if the userspace tool continues writing
> data instead of closing the file.
> 
> It looks like we expect the userspace tool to start at the beginning
> of the capsule data again, but you explicitly prohibit calling
> lseek(2) by using the 'no_llseek' file_operations function. That makes
> it difficult to verify that the app is starting at the beginning of
> the file (I also notice that 'ppos' isn't being updated).
> 
> In the success case we expect the user to close/open this file to
> write more capsules.
> 
> Personally, I think these two approaches are backwards. You should be
> able to continue writing capsule data as long as write(2) returns
> success, but should have to close/re-open the device file as soon as
> an error is encountered. If you don't close/re-open on failure I'd
> expect write(2) to return -EIO or somesuch error until you do.
> 
> It's worth explicitly documenting these two scenarios in the code.
> 

Hmm .... will think about it and capture the scenarios as a comment
into code. Thx.

> > +}
> > +
> > +static int efi_capsule_release(struct inode *inode, struct file *file)
> > +{
> > +	int ret = 0;
> > +	struct capsule_info *cap_info = file->private_data;
> > +
> > +	pr_debug("%s: Exit in Release()\n", __func__);
> > +	/* release those uncompleted uploaded block */
> > +	if (cap_info->curr_index > 0) {
> > +		pr_err("%s: capsule upload not complete\n", __func__);
> > +		while (cap_info->curr_index > 0)
> > +			__free_page(cap_info->pages[--cap_info-
> >curr_index]);
> > +		kfree(cap_info->pages);
> > +		ret = -ECANCELED;
> 
> Interestingly the return value from ->release() isn't propagated to
> userspace, look at __fput(). This is because the actual put'ing of the
> file is delayed.
> 
> Notice how James used ->flush() in his patch series, which *does*
> propagate the return value and most importantly, does so at close(2)
> time (and also if the task exits for any reason, like being killed by
> a fatal signal).
> 

I have done an experiment on that by using the misc char device file note.
I included the flush() callback function to the fops. In flush(), I put a printk()
and return -EINVAL. When I perform "cat XXX > /dev/XXX" on Intel Quark
Galileo platform, I found that the flush() is not just being called at file
close, it will also being called before the file write. And the error return, that I
forced in the flush(), does not show up at shell terminal. This is the reason that
I did not follow James recommendation and figure out to do it at write().

> > +	}
> > +	kfree(file->private_data);
> > +	file->private_data = NULL;
> > +	mutex_unlock(&capsule_loader_lock);
> > +	return ret;
> > +}
> > +
> > +static int efi_capsule_open(struct inode *inode, struct file *file)
> > +{
> > +	int ret = -EBUSY;
> > +	struct capsule_info *cap_info;
> > +
> > +	pr_debug("%s: Entering Open()\n", __func__);
> > +	if (mutex_trylock(&capsule_loader_lock)) {
> > +		cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
> > +		file->private_data = cap_info;
> > +		ret = 0;
> > +	}
> > +	return ret;
> > +}
> 
> I think this would be more readable like this,
> 
> 	if (mutex_trylock(&capsule_loader_lock))
> 		return -EBUSY;
> 
> 	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
> 	file->private_data = cap_info;
> 
> 	return 0;
> 
> Also, if we only allow one open at a time, why not make 'cap_info'
> statically allocated so that you don't need to handle the kzalloc()
> failure in this function?
> 

Yes, true. Will change it.

> --
> Matt Fleming, Intel Open Source Technology Center