This is a straw man implementation. The three patches firstly thread
the needed ->flush() file op through sysfs and kernfs. The next one
extracts transaction buffer handling from firmware_class.c and makes it
generic in a lib helper and the third patch adds a bare bones capsule
update (I suspect the latter needs more work, since it doesn't implement
the scatterlist).
James Bottomley (3):
sysfs,kernfs: add flush operation
firmware_class: split out transaction helpers
efi: add capsule update capability via sysfs
drivers/base/firmware_class.c | 117 ++++---------------------------
drivers/firmware/efi/Makefile | 2 +-
drivers/firmware/efi/capsule.c | 78 +++++++++++++++++++++
drivers/firmware/efi/capsule.h | 2 +
drivers/firmware/efi/efi.c | 8 +++
fs/kernfs/file.c | 16 +++++
fs/sysfs/file.c | 16 +++++
include/linux/kernfs.h | 2 +
include/linux/sysfs.h | 2 +
include/linux/transaction_helper.h | 26 +++++++
lib/Makefile | 2 +-
lib/transaction_helper.c | 137 +++++++++++++++++++++++++++++++++++++
12 files changed, 304 insertions(+), 104 deletions(-)
create mode 100644 drivers/firmware/efi/capsule.c
create mode 100644 drivers/firmware/efi/capsule.h
create mode 100644 include/linux/transaction_helper.h
create mode 100644 lib/transaction_helper.c
James
From: James Bottomley <[email protected]>
This is necessary to allow sysfs operations to return error in close (we are
using close to initiate and return a possible error from a transaction).
Signed-off-by: James Bottomley <[email protected]>
---
fs/kernfs/file.c | 16 ++++++++++++++++
fs/sysfs/file.c | 16 ++++++++++++++++
include/linux/kernfs.h | 2 ++
include/linux/sysfs.h | 2 ++
4 files changed, 36 insertions(+)
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index 2bacb99..d9bc8d7 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -802,6 +802,21 @@ static unsigned int kernfs_fop_poll(struct file *filp, poll_table *wait)
return DEFAULT_POLLMASK|POLLERR|POLLPRI;
}
+static int kernfs_fop_flush(struct file *file, fl_owner_t id)
+{
+ struct kernfs_open_file *of = kernfs_of(file);
+ const struct kernfs_ops *ops;
+ int rc = 0;
+
+ mutex_lock(&of->mutex);
+ ops = kernfs_ops(of->kn);
+ if (ops->flush)
+ rc = ops->flush(of, id);
+ mutex_unlock(&of->mutex);
+
+ return rc;
+}
+
static void kernfs_notify_workfn(struct work_struct *work)
{
struct kernfs_node *kn;
@@ -891,6 +906,7 @@ const struct file_operations kernfs_file_fops = {
.open = kernfs_fop_open,
.release = kernfs_fop_release,
.poll = kernfs_fop_poll,
+ .flush = kernfs_fop_flush,
};
/**
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 7c2867b..188639f 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -162,6 +162,19 @@ static int sysfs_kf_bin_mmap(struct kernfs_open_file *of,
return battr->mmap(of->file, kobj, battr, vma);
}
+static int sysfs_kf_bin_flush(struct kernfs_open_file *of,
+ fl_owner_t id)
+{
+ struct bin_attribute *battr = of->kn->priv;
+ struct kobject *kobj = of->kn->parent->priv;
+ int rc = 0;
+
+ if (battr->flush)
+ rc = battr->flush(of->file, kobj, battr, id);
+
+ return rc;
+}
+
void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr)
{
struct kernfs_node *kn = kobj->sd, *tmp;
@@ -222,17 +235,20 @@ static const struct kernfs_ops sysfs_bin_kfops_ro = {
static const struct kernfs_ops sysfs_bin_kfops_wo = {
.write = sysfs_kf_bin_write,
+ .flush = sysfs_kf_bin_flush,
};
static const struct kernfs_ops sysfs_bin_kfops_rw = {
.read = sysfs_kf_bin_read,
.write = sysfs_kf_bin_write,
+ .flush = sysfs_kf_bin_flush,
};
static const struct kernfs_ops sysfs_bin_kfops_mmap = {
.read = sysfs_kf_bin_read,
.write = sysfs_kf_bin_write,
.mmap = sysfs_kf_bin_mmap,
+ .flush = sysfs_kf_bin_flush,
};
int sysfs_add_file_mode_ns(struct kernfs_node *parent,
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 71ecdab..ead781c 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -11,6 +11,7 @@
#include <linux/err.h>
#include <linux/list.h>
#include <linux/mutex.h>
+#include <linux/fs.h>
#include <linux/idr.h>
#include <linux/lockdep.h>
#include <linux/rbtree.h>
@@ -225,6 +226,7 @@ struct kernfs_ops {
loff_t off);
int (*mmap)(struct kernfs_open_file *of, struct vm_area_struct *vma);
+ int (*flush) (struct kernfs_open_file *of, fl_owner_t id);
#ifdef CONFIG_DEBUG_LOCK_ALLOC
struct lock_class_key lockdep_key;
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 99382c0..391da13 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -152,6 +152,8 @@ struct bin_attribute {
char *, loff_t, size_t);
int (*mmap)(struct file *, struct kobject *, struct bin_attribute *attr,
struct vm_area_struct *vma);
+ int (*flush)(struct file *, struct kobject *,
+ struct bin_attribute *attr, fl_owner_t id);
};
/**
--
2.1.4
From: James Bottomley <[email protected]>
The firmware class contains code to manage an arbitrary sized buffer for
discrete read and write operations. We need precisely this ability to update
firmware capsule files (and likely for other transactions as well), so split
out the capability into a library helper
Signed-off-by: James Bottomley <[email protected]>
---
drivers/base/firmware_class.c | 117 ++++---------------------------
include/linux/transaction_helper.h | 26 +++++++
lib/Makefile | 2 +-
lib/transaction_helper.c | 137 +++++++++++++++++++++++++++++++++++++
4 files changed, 179 insertions(+), 103 deletions(-)
create mode 100644 include/linux/transaction_helper.h
create mode 100644 lib/transaction_helper.c
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841a..7d4c9d0 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
#include <linux/syscore_ops.h>
#include <linux/reboot.h>
#include <linux/security.h>
+#include <linux/transaction_helper.h>
#include <generated/utsrelease.h>
@@ -144,10 +145,8 @@ struct firmware_buf {
size_t size;
#ifdef CONFIG_FW_LOADER_USER_HELPER
bool is_paged_buf;
+ struct transaction_buf *tb;
bool need_uevent;
- struct page **pages;
- int nr_pages;
- int page_array_size;
struct list_head pending_list;
#endif
char fw_id[];
@@ -248,13 +247,9 @@ static void __fw_free_buf(struct kref *ref)
spin_unlock(&fwc->lock);
#ifdef CONFIG_FW_LOADER_USER_HELPER
- if (buf->is_paged_buf) {
- int i;
- vunmap(buf->data);
- for (i = 0; i < buf->nr_pages; i++)
- __free_page(buf->pages[i]);
- kfree(buf->pages);
- } else
+ if (buf->is_paged_buf)
+ transaction_free(buf->tb);
+ else
#endif
vfree(buf->data);
kfree(buf);
@@ -374,7 +369,7 @@ static void fw_set_page_data(struct firmware_buf *buf, struct firmware *fw)
{
fw->priv = buf;
#ifdef CONFIG_FW_LOADER_USER_HELPER
- fw->pages = buf->pages;
+ fw->pages = buf->tb->pages;
#endif
fw->size = buf->size;
fw->data = buf->data;
@@ -591,7 +586,7 @@ static int fw_map_pages_buf(struct firmware_buf *buf)
return 0;
vunmap(buf->data);
- buf->data = vmap(buf->pages, buf->nr_pages, 0, PAGE_KERNEL_RO);
+ buf->data = transaction_map(buf->tb, PAGE_KERNEL_RO);
if (!buf->data)
return -ENOMEM;
return 0;
@@ -618,7 +613,6 @@ static ssize_t firmware_loading_store(struct device *dev,
struct firmware_buf *fw_buf;
ssize_t written = count;
int loading = simple_strtol(buf, NULL, 10);
- int i;
mutex_lock(&fw_lock);
fw_buf = fw_priv->buf;
@@ -629,12 +623,8 @@ static ssize_t firmware_loading_store(struct device *dev,
case 1:
/* discarding any previous partial load */
if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
- for (i = 0; i < fw_buf->nr_pages; i++)
- __free_page(fw_buf->pages[i]);
- kfree(fw_buf->pages);
- fw_buf->pages = NULL;
- fw_buf->page_array_size = 0;
- fw_buf->nr_pages = 0;
+ transaction_free(fw_buf->tb);
+ transaction_init(fw_buf->tb);
set_bit(FW_STATUS_LOADING, &fw_buf->status);
}
break;
@@ -701,74 +691,14 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
ret_count = -ENODEV;
goto out;
}
- if (offset > buf->size) {
- ret_count = 0;
- goto out;
- }
- if (count > buf->size - offset)
- count = buf->size - offset;
-
- ret_count = count;
-
- while (count) {
- void *page_data;
- int page_nr = offset >> PAGE_SHIFT;
- int page_ofs = offset & (PAGE_SIZE-1);
- int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
-
- page_data = kmap(buf->pages[page_nr]);
-
- memcpy(buffer, page_data + page_ofs, page_cnt);
- kunmap(buf->pages[page_nr]);
- buffer += page_cnt;
- offset += page_cnt;
- count -= page_cnt;
- }
+ ret_count = transaction_read(buf->tb, buffer, offset, count);
+
out:
mutex_unlock(&fw_lock);
return ret_count;
}
-static int fw_realloc_buffer(struct firmware_priv *fw_priv, int min_size)
-{
- struct firmware_buf *buf = fw_priv->buf;
- int pages_needed = PAGE_ALIGN(min_size) >> PAGE_SHIFT;
-
- /* If the array of pages is too small, grow it... */
- if (buf->page_array_size < pages_needed) {
- int new_array_size = max(pages_needed,
- buf->page_array_size * 2);
- struct page **new_pages;
-
- new_pages = kmalloc(new_array_size * sizeof(void *),
- GFP_KERNEL);
- if (!new_pages) {
- fw_load_abort(fw_priv);
- return -ENOMEM;
- }
- memcpy(new_pages, buf->pages,
- buf->page_array_size * sizeof(void *));
- memset(&new_pages[buf->page_array_size], 0, sizeof(void *) *
- (new_array_size - buf->page_array_size));
- kfree(buf->pages);
- buf->pages = new_pages;
- buf->page_array_size = new_array_size;
- }
-
- while (buf->nr_pages < pages_needed) {
- buf->pages[buf->nr_pages] =
- alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
-
- if (!buf->pages[buf->nr_pages]) {
- fw_load_abort(fw_priv);
- return -ENOMEM;
- }
- buf->nr_pages++;
- }
- return 0;
-}
-
/**
* firmware_data_write - write method for firmware
* @filp: open sysfs file
@@ -800,29 +730,12 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
goto out;
}
- retval = fw_realloc_buffer(fw_priv, offset + count);
- if (retval)
- goto out;
-
- retval = count;
-
- while (count) {
- void *page_data;
- int page_nr = offset >> PAGE_SHIFT;
- int page_ofs = offset & (PAGE_SIZE - 1);
- int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
-
- page_data = kmap(buf->pages[page_nr]);
-
- memcpy(page_data + page_ofs, buffer, page_cnt);
+ retval = transaction_write(buf->tb, buffer, offset, count);
+ if (retval < 0)
+ fw_load_abort(fw_priv);
- kunmap(buf->pages[page_nr]);
- buffer += page_cnt;
- offset += page_cnt;
- count -= page_cnt;
- }
+ buf->size = buf->tb->size;
- buf->size = max_t(size_t, offset, buf->size);
out:
mutex_unlock(&fw_lock);
return retval;
diff --git a/include/linux/transaction_helper.h b/include/linux/transaction_helper.h
new file mode 100644
index 0000000..009181b
--- /dev/null
+++ b/include/linux/transaction_helper.h
@@ -0,0 +1,26 @@
+/*
+ * transaction_helper.h - headers and defines for lib/transaction_helper.c
+ */
+
+#ifndef _TRANSACTION_HELPER_H_
+#define _TRANSACTION_HELPER_H_
+
+#include <linux/vmalloc.h> /* pgprot_t */
+
+struct transaction_buf {
+ void *vaddr;
+ size_t size;
+ struct page **pages;
+ int nr_pages;
+ int page_array_size;
+};
+
+void transaction_free(struct transaction_buf *buf);
+void *transaction_map(struct transaction_buf *buf, pgprot_t prot);
+void transaction_init(struct transaction_buf *buf);
+ssize_t transaction_write(struct transaction_buf *buf, char *data,
+ loff_t offset, size_t count);
+ssize_t transaction_read(struct transaction_buf *buf, char *data, loff_t offset,
+ size_t count);
+
+#endif /* _TRANSACTION_HELPER_H_ */
diff --git a/lib/Makefile b/lib/Makefile
index 6c37933..fac1534 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -13,7 +13,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
sha1.o md5.o irq_regs.o argv_split.o \
proportions.o flex_proportions.o ratelimit.o show_mem.o \
is_single_threaded.o plist.o decompress.o kobject_uevent.o \
- earlycpio.o seq_buf.o
+ earlycpio.o seq_buf.o transaction_helper.o
obj-$(CONFIG_ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS) += usercopy.o
lib-$(CONFIG_MMU) += ioremap.o
diff --git a/lib/transaction_helper.c b/lib/transaction_helper.c
new file mode 100644
index 0000000..2407512
--- /dev/null
+++ b/lib/transaction_helper.c
@@ -0,0 +1,137 @@
+/*
+ * transaction_helper.c - helper functions for sysfs binary file transaction
+ *
+ * Most of this file is split out of firmware_class.c
+ */
+
+#include <linux/highmem.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <linux/transaction_helper.h>
+
+void transaction_free(struct transaction_buf *buf)
+{
+ int i;
+
+ if (buf->vaddr)
+ vunmap(buf->vaddr);
+ for (i = 0; i < buf->nr_pages; i++)
+ __free_page(buf->pages[i]);
+ kfree(buf->pages);
+}
+
+void *transaction_map(struct transaction_buf *buf, pgprot_t prot)
+{
+ if (buf->vaddr)
+ vunmap(buf->vaddr);
+ buf->vaddr = vmap(buf->pages, buf->nr_pages, 0, prot);
+
+ return buf->vaddr;
+}
+
+void transaction_init(struct transaction_buf *buf)
+{
+ memset(buf, 0, sizeof(*buf));
+}
+
+static int transaction_realloc_buffer(struct transaction_buf *buf, int min_size)
+{
+ int pages_needed = PAGE_ALIGN(min_size) >> PAGE_SHIFT;
+
+ /* If the array of pages is too small, grow it... */
+ if (buf->page_array_size < pages_needed) {
+ int new_array_size = max(pages_needed,
+ buf->page_array_size * 2);
+ struct page **new_pages;
+
+ new_pages = kmalloc(new_array_size * sizeof(void *),
+ GFP_KERNEL);
+ if (!new_pages) {
+ return -ENOMEM;
+ }
+ memcpy(new_pages, buf->pages,
+ buf->page_array_size * sizeof(void *));
+ memset(&new_pages[buf->page_array_size], 0, sizeof(void *) *
+ (new_array_size - buf->page_array_size));
+ kfree(buf->pages);
+ buf->pages = new_pages;
+ buf->page_array_size = new_array_size;
+ }
+
+ while (buf->nr_pages < pages_needed) {
+ buf->pages[buf->nr_pages] =
+ alloc_page(GFP_KERNEL | __GFP_HIGHMEM);
+
+ if (!buf->pages[buf->nr_pages])
+ return -ENOMEM;
+
+ buf->nr_pages++;
+ }
+ return 0;
+}
+
+ssize_t transaction_write(struct transaction_buf *buf, char *data,
+ loff_t offset, size_t count)
+{
+ int retval;
+
+ retval = transaction_realloc_buffer(buf, offset + count);
+ if (retval)
+ return retval;
+
+ retval = count;
+
+ while (count) {
+ void *page_data;
+ int page_nr = offset >> PAGE_SHIFT;
+ int page_ofs = offset & (PAGE_SIZE - 1);
+ int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
+
+ page_data = kmap(buf->pages[page_nr]);
+
+ memcpy(page_data + page_ofs, data, page_cnt);
+
+ kunmap(buf->pages[page_nr]);
+ data += page_cnt;
+ offset += page_cnt;
+ count -= page_cnt;
+ }
+
+ buf->size = max_t(size_t, offset, buf->size);
+
+ return retval;
+}
+
+ssize_t transaction_read(struct transaction_buf *buf, char *data, loff_t offset,
+ size_t count)
+{
+ int retval = 0;
+
+ if (offset > buf->size)
+ goto out;
+
+ if (count > buf->size - offset)
+ count = buf->size - offset;
+
+ retval = count;
+
+ while (count) {
+ void *page_data;
+ int page_nr = offset >> PAGE_SHIFT;
+ int page_ofs = offset & (PAGE_SIZE-1);
+ int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
+
+ page_data = kmap(buf->pages[page_nr]);
+
+ memcpy(data, page_data + page_ofs, page_cnt);
+
+ kunmap(buf->pages[page_nr]);
+ data += page_cnt;
+ offset += page_cnt;
+ count -= page_cnt;
+ }
+
+ out:
+ return retval;
+}
--
2.1.4
From: James Bottomley <[email protected]>
The firmware update should be applied simply by doing
cat fw_file > /sys/firmware/capsule/update
With a properly formatted fw_file. Any error will be returned on close of
stdout. util-linux returns errors correctly from closing stdout, but firmware
shippers should check whatever utilities package they use correctly captures
the error return on close.
Signed-off-by: James Bottomley <[email protected]>
---
drivers/firmware/efi/Makefile | 2 +-
drivers/firmware/efi/capsule.c | 78 ++++++++++++++++++++++++++++++++++++++++++
drivers/firmware/efi/capsule.h | 2 ++
drivers/firmware/efi/efi.c | 8 +++++
4 files changed, 89 insertions(+), 1 deletion(-)
create mode 100644 drivers/firmware/efi/capsule.c
create mode 100644 drivers/firmware/efi/capsule.h
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index d8be608..698846e 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -1,7 +1,7 @@
#
# Makefile for linux kernel
#
-obj-$(CONFIG_EFI) += efi.o vars.o reboot.o
+obj-$(CONFIG_EFI) += efi.o vars.o reboot.o capsule.o
obj-$(CONFIG_EFI_VARS) += efivars.o
obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
obj-$(CONFIG_UEFI_CPER) += cper.o
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
new file mode 100644
index 0000000..1fd78e7
--- /dev/null
+++ b/drivers/firmware/efi/capsule.c
@@ -0,0 +1,78 @@
+#include <linux/efi.h>
+#include <linux/slab.h>
+#include <linux/transaction_helper.h>
+
+#include "capsule.h"
+
+static struct kset *capsule_kset;
+static struct transaction_buf *capsule_buf;
+
+static int capsule_data_write(struct file *file, struct kobject *kobj,
+ struct bin_attribute *attr,
+ char *buffer, loff_t offset, size_t count)
+{
+ if (!capsule_buf) {
+ capsule_buf = kmalloc(sizeof(*capsule_buf), GFP_KERNEL);
+ if (!capsule_buf)
+ return -ENOMEM;
+ transaction_init(capsule_buf);
+ }
+
+ return transaction_write(capsule_buf, buffer, offset, count);
+}
+
+static int capsule_data_flush(struct file *file, struct kobject *kobj,
+ struct bin_attribute *attr, fl_owner_t id)
+{
+ efi_capsule_header_t *hdr[1];
+ int retval = -EINVAL;
+
+ void *data = transaction_map(capsule_buf, PAGE_KERNEL_RO);
+
+ hdr[0] = data;
+ if (hdr[0]->headersize > capsule_buf->size)
+ goto out;
+
+ retval = efi.update_capsule(hdr, 1, 0);
+
+ out:
+ transaction_free(capsule_buf);
+ kfree(capsule_buf);
+ capsule_buf = NULL;
+
+ return retval;
+}
+
+
+static const struct bin_attribute capsule_update_attr = {
+ .attr = { .name = "update", .mode = 0600 },
+ .size = 0,
+ .write = capsule_data_write,
+ .flush = capsule_data_flush,
+};
+
+__init int efi_capsule_init(struct kobject *efi_kobj)
+{
+
+ int err;
+
+ /* FIXME: check that UEFI actually supports capsule here */
+
+ capsule_kset = kset_create_and_add("capsule", NULL, efi_kobj);
+ if (!capsule_kset) {
+ printk(KERN_ERR "UEFI capsule: failed to register subsystem\n");
+ return -ENOMEM;
+ }
+
+ err = sysfs_create_bin_file(&capsule_kset->kobj, &capsule_update_attr);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+void efi_capsule_remove(struct kobject *efi_kobj)
+{
+ sysfs_remove_bin_file(&capsule_kset->kobj, &capsule_update_attr);
+ kset_unregister(capsule_kset);
+}
diff --git a/drivers/firmware/efi/capsule.h b/drivers/firmware/efi/capsule.h
new file mode 100644
index 0000000..cc38f7d
--- /dev/null
+++ b/drivers/firmware/efi/capsule.h
@@ -0,0 +1,2 @@
+int efi_capsule_init(struct kobject *efi_kobj);
+void efi_capsule_remove(struct kobject *efi_kobj);
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3061bb8..92da61e 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -25,6 +25,8 @@
#include <linux/io.h>
#include <linux/platform_device.h>
+#include "capsule.h"
+
struct efi __read_mostly efi = {
.mps = EFI_INVALID_TABLE_ADDR,
.acpi = EFI_INVALID_TABLE_ADDR,
@@ -219,8 +221,14 @@ static int __init efisubsys_init(void)
goto err_remove_group;
}
+ error = efi_capsule_init(efi_kobj);
+ if (error)
+ goto err_remove_capsule;
+
return 0;
+err_remove_capsule:
+ efi_capsule_remove(efi_kobj);
err_remove_group:
sysfs_remove_group(efi_kobj, &efi_subsys_attr_group);
err_unregister:
--
2.1.4
On Wed, Apr 29, 2015 at 4:12 PM, James Bottomley
<[email protected]> wrote:
> From: James Bottomley <[email protected]>
>
> The firmware update should be applied simply by doing
>
> cat fw_file > /sys/firmware/capsule/update
>
> With a properly formatted fw_file. Any error will be returned on close of
> stdout. util-linux returns errors correctly from closing stdout, but firmware
> shippers should check whatever utilities package they use correctly captures
> the error return on close.
s/util-linux/coreutils/
This still makes my API sense itch. It's kind of an abuse of open/write/close.
>
> Signed-off-by: James Bottomley <[email protected]>
> ---
> drivers/firmware/efi/Makefile | 2 +-
> drivers/firmware/efi/capsule.c | 78 ++++++++++++++++++++++++++++++++++++++++++
> drivers/firmware/efi/capsule.h | 2 ++
> drivers/firmware/efi/efi.c | 8 +++++
> 4 files changed, 89 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/efi/capsule.c
> create mode 100644 drivers/firmware/efi/capsule.h
>
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index d8be608..698846e 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -1,7 +1,7 @@
> #
> # Makefile for linux kernel
> #
> -obj-$(CONFIG_EFI) += efi.o vars.o reboot.o
> +obj-$(CONFIG_EFI) += efi.o vars.o reboot.o capsule.o
> obj-$(CONFIG_EFI_VARS) += efivars.o
> obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
> obj-$(CONFIG_UEFI_CPER) += cper.o
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> new file mode 100644
> index 0000000..1fd78e7
> --- /dev/null
> +++ b/drivers/firmware/efi/capsule.c
> @@ -0,0 +1,78 @@
> +#include <linux/efi.h>
> +#include <linux/slab.h>
> +#include <linux/transaction_helper.h>
> +
> +#include "capsule.h"
> +
> +static struct kset *capsule_kset;
> +static struct transaction_buf *capsule_buf;
> +
> +static int capsule_data_write(struct file *file, struct kobject *kobj,
> + struct bin_attribute *attr,
> + char *buffer, loff_t offset, size_t count)
> +{
> + if (!capsule_buf) {
> + capsule_buf = kmalloc(sizeof(*capsule_buf), GFP_KERNEL);
> + if (!capsule_buf)
> + return -ENOMEM;
> + transaction_init(capsule_buf);
> + }
> +
> + return transaction_write(capsule_buf, buffer, offset, count);
> +}
This seems unlikely to have good effects if two struct files are active at once.
Also, I think you crash if you open and close without calling write,
and I don't know what whether there can be spurious flushes (fsync?).
--Andy
On Wed, 2015-04-29 at 16:25 -0700, Andy Lutomirski wrote:
> On Wed, Apr 29, 2015 at 4:12 PM, James Bottomley
> <[email protected]> wrote:
> > From: James Bottomley <[email protected]>
> >
> > The firmware update should be applied simply by doing
> >
> > cat fw_file > /sys/firmware/capsule/update
> >
> > With a properly formatted fw_file. Any error will be returned on close of
> > stdout. util-linux returns errors correctly from closing stdout, but firmware
> > shippers should check whatever utilities package they use correctly captures
> > the error return on close.
>
> s/util-linux/coreutils/
>
> This still makes my API sense itch. It's kind of an abuse of
> open/write/close.
It works ... and according to Alan, NFS is already doing it. I suppose
we can have a do over of the whole debate again ...
> >
> > Signed-off-by: James Bottomley <[email protected]>
> > ---
> > drivers/firmware/efi/Makefile | 2 +-
> > drivers/firmware/efi/capsule.c | 78 ++++++++++++++++++++++++++++++++++++++++++
> > drivers/firmware/efi/capsule.h | 2 ++
> > drivers/firmware/efi/efi.c | 8 +++++
> > 4 files changed, 89 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/firmware/efi/capsule.c
> > create mode 100644 drivers/firmware/efi/capsule.h
> >
> > diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> > index d8be608..698846e 100644
> > --- a/drivers/firmware/efi/Makefile
> > +++ b/drivers/firmware/efi/Makefile
> > @@ -1,7 +1,7 @@
> > #
> > # Makefile for linux kernel
> > #
> > -obj-$(CONFIG_EFI) += efi.o vars.o reboot.o
> > +obj-$(CONFIG_EFI) += efi.o vars.o reboot.o capsule.o
> > obj-$(CONFIG_EFI_VARS) += efivars.o
> > obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o
> > obj-$(CONFIG_UEFI_CPER) += cper.o
> > diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> > new file mode 100644
> > index 0000000..1fd78e7
> > --- /dev/null
> > +++ b/drivers/firmware/efi/capsule.c
> > @@ -0,0 +1,78 @@
> > +#include <linux/efi.h>
> > +#include <linux/slab.h>
> > +#include <linux/transaction_helper.h>
> > +
> > +#include "capsule.h"
> > +
> > +static struct kset *capsule_kset;
> > +static struct transaction_buf *capsule_buf;
> > +
> > +static int capsule_data_write(struct file *file, struct kobject *kobj,
> > + struct bin_attribute *attr,
> > + char *buffer, loff_t offset, size_t count)
> > +{
> > + if (!capsule_buf) {
> > + capsule_buf = kmalloc(sizeof(*capsule_buf), GFP_KERNEL);
> > + if (!capsule_buf)
> > + return -ENOMEM;
> > + transaction_init(capsule_buf);
> > + }
> > +
> > + return transaction_write(capsule_buf, buffer, offset, count);
> > +}
>
> This seems unlikely to have good effects if two struct files are
> active at once.
I thought of threading ->open and using that to make it exclusive. But
then I thought caveat emptor.
I think for multiple files, I need a mutex in the transaction code just
to ensure orderly access.
> Also, I think you crash if you open and close without calling write,
yes there should be an if (!capsule_buf) return -EINVAL in flush
> and I don't know what whether there can be spurious flushes (fsync?).
It turns out that the bdi flusher and the fop->flush() operation are
totally different things. ->flush() is used mostly just to do stuff on
close (NFS uses it to tidy up for instance).
James
On Wed, Apr 29, 2015 at 4:36 PM, James Bottomley
<[email protected]> wrote:
> On Wed, 2015-04-29 at 16:25 -0700, Andy Lutomirski wrote:
>> On Wed, Apr 29, 2015 at 4:12 PM, James Bottomley
>> <[email protected]> wrote:
>> > From: James Bottomley <[email protected]>
>> >
>> > The firmware update should be applied simply by doing
>> >
>> > cat fw_file > /sys/firmware/capsule/update
>> >
>> > With a properly formatted fw_file. Any error will be returned on close of
>> > stdout. util-linux returns errors correctly from closing stdout, but firmware
>> > shippers should check whatever utilities package they use correctly captures
>> > the error return on close.
>>
>> s/util-linux/coreutils/
>>
>> This still makes my API sense itch. It's kind of an abuse of
>> open/write/close.
>
> It works ... and according to Alan, NFS is already doing it. I suppose
> we can have a do over of the whole debate again ...
I think that NFS is at least writing to actual files as opposed to
trying to implement some kind of transactions.
Blech, whatever. This approach certainly works, as long as no one
trips over the busybox thing. Maybe there should also be
/sys/something_that_errors_on_close that people can use as a test.
--Andy
> -----Original Message-----
> From: James Bottomley [mailto:[email protected]]
> Sent: Thursday, April 30, 2015 7:08 AM
> To: [email protected]
> Cc: Kweh, Hock Leong; LKML; Andy Lutomirski; Greg Kroah-Hartman; Peter
> Jones
> Subject: [RFC 0/3] Add capsule update using error on close semantics
>
> This is a straw man implementation. The three patches firstly thread the
> needed ->flush() file op through sysfs and kernfs. The next one extracts
> transaction buffer handling from firmware_class.c and makes it generic in a
> lib helper and the third patch adds a bare bones capsule update (I suspect
> the latter needs more work, since it doesn't implement the scatterlist).
>
> James Bottomley (3):
> sysfs,kernfs: add flush operation
> firmware_class: split out transaction helpers
> efi: add capsule update capability via sysfs
>
> drivers/base/firmware_class.c | 117 ++++---------------------------
> drivers/firmware/efi/Makefile | 2 +-
> drivers/firmware/efi/capsule.c | 78 +++++++++++++++++++++
> drivers/firmware/efi/capsule.h | 2 +
> drivers/firmware/efi/efi.c | 8 +++
> fs/kernfs/file.c | 16 +++++
> fs/sysfs/file.c | 16 +++++
> include/linux/kernfs.h | 2 +
> include/linux/sysfs.h | 2 +
> include/linux/transaction_helper.h | 26 +++++++
> lib/Makefile | 2 +-
> lib/transaction_helper.c | 137
> +++++++++++++++++++++++++++++++++++++
> 12 files changed, 304 insertions(+), 104 deletions(-) create mode 100644
> drivers/firmware/efi/capsule.c create mode 100644
> drivers/firmware/efi/capsule.h create mode 100644
> include/linux/transaction_helper.h
> create mode 100644 lib/transaction_helper.c
>
> James
Hi James,
I like the sysfs enhancement but require Greg to buy in the idea.
For the efi capsule part, Matt has implemented some APIs where
you could get the patch at http://permalink.gmane.org/gmane.linux.kernel.efi/4837.
So, I would think that leveraging the APIs that Matt has created
is a better choice.
Thanks & Regards,
Wilson
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Wed, Apr 29, 2015 at 04:10:52PM -0700, James Bottomley wrote:
> From: James Bottomley <[email protected]>
>
> The firmware class contains code to manage an arbitrary sized buffer for
> discrete read and write operations. We need precisely this ability to update
> firmware capsule files (and likely for other transactions as well), so split
> out the capability into a library helper
>
> Signed-off-by: James Bottomley <[email protected]>
> ---
> drivers/base/firmware_class.c | 117 ++++---------------------------
> include/linux/transaction_helper.h | 26 +++++++
> lib/Makefile | 2 +-
> lib/transaction_helper.c | 137 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 179 insertions(+), 103 deletions(-)
> create mode 100644 include/linux/transaction_helper.h
> create mode 100644 lib/transaction_helper.c
Interesting, do you think there are other places that can use this
"transaction_helper" library? And a bit more documentation would be
nice, but on the whole, no objection from me.
thanks,
greg k-h
On Wed, Apr 29, 2015 at 04:09:45PM -0700, James Bottomley wrote:
> From: James Bottomley <[email protected]>
>
> This is necessary to allow sysfs operations to return error in close (we are
> using close to initiate and return a possible error from a transaction).
>
> Signed-off-by: James Bottomley <[email protected]>
Are there any side-affects now that we have a flush() call for binary
files that the vfs will act differently from not defining one at all?
if not, no objection to me for this change.
thanks,
greg k-h
On Thu, 2015-04-30 at 15:11 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 29, 2015 at 04:10:52PM -0700, James Bottomley wrote:
> > From: James Bottomley <[email protected]>
> >
> > The firmware class contains code to manage an arbitrary sized buffer for
> > discrete read and write operations. We need precisely this ability to update
> > firmware capsule files (and likely for other transactions as well), so split
> > out the capability into a library helper
> >
> > Signed-off-by: James Bottomley <[email protected]>
> > ---
> > drivers/base/firmware_class.c | 117 ++++---------------------------
> > include/linux/transaction_helper.h | 26 +++++++
> > lib/Makefile | 2 +-
> > lib/transaction_helper.c | 137 +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 179 insertions(+), 103 deletions(-)
> > create mode 100644 include/linux/transaction_helper.h
> > create mode 100644 lib/transaction_helper.c
>
> Interesting, do you think there are other places that can use this
> "transaction_helper" library?
Certainly, everywhere binary attributes are used for more than a page of
data, I think. Certainly things like the PPC nvram dump read stuff and
probably a few other architecture specific bin files for nvram. I
imagine there'll be a long series of janatorial patches if people are
interested.
> And a bit more documentation would be
> nice, but on the whole, no objection from me.
Yes, that's why it's an RFC.I wanted to show the plan in code, get all
the modification suggestions and then do the proper implementation.
Docs come after the API is agreed.
James
On Thu, 2015-04-30 at 15:11 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 29, 2015 at 04:09:45PM -0700, James Bottomley wrote:
> > From: James Bottomley <[email protected]>
> >
> > This is necessary to allow sysfs operations to return error in close (we are
> > using close to initiate and return a possible error from a transaction).
> >
> > Signed-off-by: James Bottomley <[email protected]>
>
> Are there any side-affects now that we have a flush() call for binary
> files that the vfs will act differently from not defining one at all?
See answer to Andy, but basically, no, VFS flush is only used to tidy up
before close.
To be honest, it might be nice if fsync *would* issue a transaction
because I think having a multi-transaction API with write/fsync/write is
not such a bad thing. It turns out that fsync and fdatasync do have a
VFS op, but it's ->fsync() not ->flush(), so we have the option to wire
this up if anyone finds a use case.
> if not, no objection to me for this change.
Thanks,
James
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Wed, 29 Apr, at 04:10:52PM, James Bottomley wrote:
> From: James Bottomley <[email protected]>
>
> The firmware class contains code to manage an arbitrary sized buffer for
> discrete read and write operations. We need precisely this ability to update
> firmware capsule files (and likely for other transactions as well), so split
> out the capability into a library helper
>
> Signed-off-by: James Bottomley <[email protected]>
> ---
> drivers/base/firmware_class.c | 117 ++++---------------------------
> include/linux/transaction_helper.h | 26 +++++++
> lib/Makefile | 2 +-
> lib/transaction_helper.c | 137 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 179 insertions(+), 103 deletions(-)
> create mode 100644 include/linux/transaction_helper.h
> create mode 100644 lib/transaction_helper.c
(Sorry, I'm coming to this incredibly late)
This patch is pretty neat and I wish something like this had existed
when I originally wrote the EFI capsule patches.
James, do you have any plans to resubmit this as a non-RFC? If not, do
you mind if I pick this up and rebase my capsule patches ontop of it?
--
Matt Fleming, Intel Open Source Technology Center
On Thu, 2015-08-27 at 15:47 +0100, Matt Fleming wrote:
> On Wed, 29 Apr, at 04:10:52PM, James Bottomley wrote:
> > From: James Bottomley <[email protected]>
> >
> > The firmware class contains code to manage an arbitrary sized buffer for
> > discrete read and write operations. We need precisely this ability to update
> > firmware capsule files (and likely for other transactions as well), so split
> > out the capability into a library helper
> >
> > Signed-off-by: James Bottomley <[email protected]>
> > ---
> > drivers/base/firmware_class.c | 117 ++++---------------------------
> > include/linux/transaction_helper.h | 26 +++++++
> > lib/Makefile | 2 +-
> > lib/transaction_helper.c | 137 +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 179 insertions(+), 103 deletions(-)
> > create mode 100644 include/linux/transaction_helper.h
> > create mode 100644 lib/transaction_helper.c
>
> (Sorry, I'm coming to this incredibly late)
>
> This patch is pretty neat and I wish something like this had existed
> when I originally wrote the EFI capsule patches.
>
> James, do you have any plans to resubmit this as a non-RFC? If not, do
> you mind if I pick this up and rebase my capsule patches ontop of it?
Sort of, but I was stalled trying to work out how to combine with your
capsule patches. If you're going to do that work, I can resubmit my
stuff as a patch with all the changes based on the review comments and
then you can do the real work ...
James
On Thu, 27 Aug, at 09:25:17AM, James Bottomley wrote:
> On Thu, 2015-08-27 at 15:47 +0100, Matt Fleming wrote:
> >
> > (Sorry, I'm coming to this incredibly late)
> >
> > This patch is pretty neat and I wish something like this had existed
> > when I originally wrote the EFI capsule patches.
> >
> > James, do you have any plans to resubmit this as a non-RFC? If not, do
> > you mind if I pick this up and rebase my capsule patches ontop of it?
>
> Sort of, but I was stalled trying to work out how to combine with your
> capsule patches. If you're going to do that work, I can resubmit my
> stuff as a patch with all the changes based on the review comments and
> then you can do the real work ...
Yeah, that sounds great. I'm happy to respin my patches on top.
--
Matt Fleming, Intel Open Source Technology Center