[PATCH v2 0/3] Enable user helper interface for efi capsule update

[PATCH v2 0/3] Enable user helper interface for efi capsule update

[Kweh Hock Leong]

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

Hi Guys,

This patchset is created on top of "efi: Capsule update support" patch:
http://permalink.gmane.org/gmane.linux.kernel.efi/4837

It leverages the request_firmware_nowait() to expose the user helper interface for user to upload the capsule binary and calling the
efi_capsule_update() API to pass the binary to EFI firmware.

Besides build in kernel, the design also cater for build as kernel driver module. This patchset introduce a new API (request_firmware_abort()) at firmware_class so that the driver module could be unloaded by calling the API to properly stop user helper interface and release the device.

Thanks.

---
changelog v2:
[PATCH 1/3]
* use fw_lookup_buf() instead of __fw_lookup_buf() function call
* move the fw_lookup_buf() function out of the CONFIG_PM_SLEEP block

[PATCH 2/3]
* no change

[PATCH 3/3]
* no change


Kweh, Hock Leong (3):
  firmware loader: Introduce new API - request_firmware_abort()
  firmware loader: fix hung task warning dump
  efi: Capsule update with user helper interface

 drivers/base/firmware_class.c                  |   56 ++++--
 drivers/firmware/efi/Kconfig                   |   13 ++
 drivers/firmware/efi/Makefile                  |    1 +
 drivers/firmware/efi/efi-capsule-user-helper.c |  246 ++++++++++++++++++++++++
 include/linux/firmware.h                       |    4 +
 5 files changed, 306 insertions(+), 14 deletions(-)
 create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c

-- 
1.7.9.5

[PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()

[Kweh Hock Leong]

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

Besides aborting through user helper interface, a new API
request_firmware_abort() allows kernel driver module to abort the
request_firmware() / request_firmware_nowait() when they are no
longer needed. It is useful for cancelling an outstanding firmware
load if initiated from inside a module where that module is in the
process of being unloaded, since the unload function may not have
a handle to the struct firmware_buf.

Note for people who use request_firmware_nowait():
You are required to do your own synchronization after you call
request_firmware_abort() in order to continue unloading the
module. The example synchronization code shows below:

while (THIS_MODULE && module_refcount(THIS_MODULE))
	msleep(20);

Cc: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/base/firmware_class.c |   52 +++++++++++++++++++++++++++++++----------
 include/linux/firmware.h      |    4 ++++
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d276e33..a238a46 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1291,6 +1291,46 @@ request_firmware_nowait(
 }
 EXPORT_SYMBOL(request_firmware_nowait);
 
+static struct firmware_buf *fw_lookup_buf(const char *fw_name)
+{
+	struct firmware_buf *tmp;
+	struct firmware_cache *fwc = &fw_cache;
+
+	spin_lock(&fwc->lock);
+	tmp = __fw_lookup_buf(fw_name);
+	spin_unlock(&fwc->lock);
+
+	return tmp;
+}
+
+/**
+ * request_firmware_abort - abort the waiting of firmware request
+ * @fw_name: name of firmware file
+ *
+ *	@fw_name is the same name string while calling to request_firmware()
+ *		or request_firmware_nowait().
+ **/
+int request_firmware_abort(const char *fw_name)
+{
+	struct firmware_buf *buf;
+	struct firmware_buf *entry;
+
+	buf = fw_lookup_buf(fw_name);
+	if (!buf)
+		return -ENOENT;
+
+	mutex_lock(&fw_lock);
+	list_for_each_entry(entry, &pending_fw_head, pending_list) {
+		if (entry == buf) {
+			__fw_load_abort(buf);
+			break;
+		}
+	}
+	mutex_unlock(&fw_lock);
+	return 0;
+}
+EXPORT_SYMBOL(request_firmware_abort);
+
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
@@ -1324,18 +1364,6 @@ static int cache_firmware(const char *fw_name)
 	return ret;
 }
 
-static struct firmware_buf *fw_lookup_buf(const char *fw_name)
-{
-	struct firmware_buf *tmp;
-	struct firmware_cache *fwc = &fw_cache;
-
-	spin_lock(&fwc->lock);
-	tmp = __fw_lookup_buf(fw_name);
-	spin_unlock(&fwc->lock);
-
-	return tmp;
-}
-
 /**
  * uncache_firmware - remove one cached firmware image
  * @fw_name: the firmware image name
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5952933..ed90c8b 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,6 +47,7 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
+int request_firmware_abort(const char *fw_name);
 #else
 static inline int request_firmware(const struct firmware **fw,
 				   const char *name,
@@ -66,6 +67,9 @@ static inline void release_firmware(const struct firmware *fw)
 {
 }
 
+static inline int request_firmware_abort(const char *fw_name)
+{
+}
 #endif
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-- 
1.7.9.5

Re: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()

[Henrique de Moraes Holschuh]

On Mon, 03 Nov 2014, Kweh Hock Leong wrote:
> Note for people who use request_firmware_nowait():
> You are required to do your own synchronization after you call
> request_firmware_abort() in order to continue unloading the
> module. The example synchronization code shows below:
> 
> while (THIS_MODULE && module_refcount(THIS_MODULE))
> 	msleep(20);

This is _so_ asking for people to get it wrong, it is not funny :-(

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()

[Matt Fleming]

On Mon, 2014-11-03 at 08:15 -0200, Henrique de Moraes Holschuh wrote:
> On Mon, 03 Nov 2014, Kweh Hock Leong wrote:
> > Note for people who use request_firmware_nowait():
> > You are required to do your own synchronization after you call
> > request_firmware_abort() in order to continue unloading the
> > module. The example synchronization code shows below:
> > 
> > while (THIS_MODULE && module_refcount(THIS_MODULE))
> > 	msleep(20);
> 
> This is _so_ asking for people to get it wrong, it is not funny :-(

Yeah. How about we make request_firmware_abort() synchronous so that
drivers don't have to perform these reference counting games?

Re: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()

[Greg Kroah-Hartman]

On Mon, Nov 03, 2014 at 11:07:08AM +0800, Kweh Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> 
> Besides aborting through user helper interface, a new API
> request_firmware_abort() allows kernel driver module to abort the
> request_firmware() / request_firmware_nowait() when they are no
> longer needed. It is useful for cancelling an outstanding firmware
> load if initiated from inside a module where that module is in the
> process of being unloaded, since the unload function may not have
> a handle to the struct firmware_buf.
> 
> Note for people who use request_firmware_nowait():
> You are required to do your own synchronization after you call
> request_firmware_abort() in order to continue unloading the
> module. The example synchronization code shows below:
> 
> while (THIS_MODULE && module_refcount(THIS_MODULE))
> 	msleep(20);

As others have pointed out, this isn't ok, and is totally racy and
should never end up in a driver.  Never mess with THIS_MODULE from
within the same module, otherwise it's racy and broken code.

So can you please rework this to not require this?

thanks,

greg k-h

RE: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()

[Kweh, Hock Leong]

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Sunday, November 09, 2014 3:07 AM
>
> >
> > Besides aborting through user helper interface, a new API
> > request_firmware_abort() allows kernel driver module to abort the
> > request_firmware() / request_firmware_nowait() when they are no longer
> > needed. It is useful for cancelling an outstanding firmware load if
> > initiated from inside a module where that module is in the process of
> > being unloaded, since the unload function may not have a handle to the
> > struct firmware_buf.
> >
> > Note for people who use request_firmware_nowait():
> > You are required to do your own synchronization after you call
> > request_firmware_abort() in order to continue unloading the module.
> > The example synchronization code shows below:
> >
> > while (THIS_MODULE && module_refcount(THIS_MODULE))
> > 	msleep(20);
> 
> As others have pointed out, this isn't ok, and is totally racy and should never
> end up in a driver.  Never mess with THIS_MODULE from within the same
> module, otherwise it's racy and broken code.
> 
> So can you please rework this to not require this?
> 
> thanks,
> 
> greg k-h

Hi everyone,

First of all, I would like to apologize if my commit message gives you guys an impression
that to use request_firmware_abort(), you guys MUST do the synchronization on your own. 
But the fact is, it is not a MUST. Below will provide more detail.

Regarding this synchronization topic, I would like to open a discussion to get a
better approach to handle this problem. Before jumping onto the design, I would
like to give a background of why I am doing in this way.

- Only doing module unload is required to be aware of this synchronization
	-> Ensuring the call back does not fall into unloaded code which may cause
	     undefined behavior.
	-> Ensuring the put_device() & module_put() code have finished in firmware_class.c
	     function request_firmware_work_func() before the device is unregistered
	     and module unloaded happen.

- Not all the situations are required to do this synchronization:
	-> Implementation that use request_firmware() do not need this synchronization
	     due to it will get synced by returning at the same place the caller call
	     request_firmware().
	-> Implementation that will not unload the call back code and without relying
	     device refcount / module refcount do not need this synchronization (for e.g.
	     calling the request_firmware_nowait() without passing the MODULE and DEVICE
	     parameters as showed below:
	     request_firmware_nowait(NULL, FW_ACTION_NOHOTPLUG, "xxx", NULL,
				             GFP_KERNEL, NULL, callbackfn);).

- Following the original request_firmware_nowait() design thought
	-> Strongly believe the original design of request_firmware_nowait() that using the get_device(),
	     put_device(), try_get_module() & module_put() also expect the caller side to do the
	     synchronization themselves if they have relying on these counter to continue their works.

	     Let's take out this newly introduce API (request_firmware_abort()) and focus only on the
	     original design (request_firmware_nowait()). If there is a caller design that after the callback
	     happen and it need to do platform_device_unregister(), the original design also expect the
	    caller do its own synchronization before it can do the device unregister.

- Use cases that do not need the synchronization:
	-> Depend on a volatile condition in order to expose firmware upload interface: Like a design
	      only in a particular window period then open the interface, when outside of the window
	      then it need to disable the interface. This design also does not need the synchronization
	      as it do not unload the callback module code and not relying on the device refcount or
	      module refcount to do it stuff.
	-> System reboot: When system reboot, the system would not unload the module code and
	      also would not unregister the device. So it does not meet the conditions (unload call back
	      code and have dependency on device refcount / module refcount). This does not need
	      the synchronization.

Due to the above reasons, in summary, I think the caller has the responsibility to take care of this
synchronization whenever needed on the caller side. What do you guys think?


Come back to the design if really need to implement it in firmware_class. Below shows some design
thought:

- Complexity of implementing the synchronization inside firmware_class
	-> Cannot use module refcount or device refcount to be the synchronization method
	     due to:
	     (1) firmware_work data struct will get freed  at request_firmware_work_func()
	            after calling the __fw_load_abort() just before you could do the synchronization
	            at the end of request_firmware_abort().
	     (2) each different caller may have different refcount number in their device/module
	            data struct where it is impossible for firmware_class to take a number and wait
	            for it. Only caller know better what is the number they should wait for.
	      (3) As mentioned above, caller who use request_firmware_nowait() may not passing
	             the module or device parameters.
	-> firmware_buf data struct also will get freed after calling the __fw_load_abort() just
                     before you could do the synchronization at the end of request_firmware_abort().
	-> If implement wait completion for the synchronization, below are something we need
	     to watch out/take care:
	     (1) completion struct cannot tight to firmware_buf & firmware_work data structs as they
	            are freed before you can use it.
	     (2) one global completion struct may not work due to request_firmware_nowait() can
	            be called multiple time.
	     (3) it may have chances to race with userland issuing "echo 0 > loading" which eventually
	            happened in such scenario that complete_all() being called before wait_for_completion()
	            being called.

I am thinking that I will stick back with wait completion method for the synchronization instead of using
semaphore. Trying to overcome the above complexity, I need to introduce another struct that won't be
freed and must able to link with firmware_buf data struct. And I also need to implement some kind of
lock to prevent race condition. 

Do you guys think this is feasible? Or the most easiest way to do the synchronization is at the
platform_device_unregister() or module unload code which before they unregister/unload, they need
to wait until refcount = 0 then they can do their job.

Looking forward to hear from you guys. Thanks. :-)


Regards,
Wilson

Re: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()

[Matt Fleming]

On Thu, 13 Nov, at 02:51:28AM, Kweh, Hock Leong wrote:
> 
> Hi everyone,
> 
> First of all, I would like to apologize if my commit message gives you guys an impression
> that to use request_firmware_abort(), you guys MUST do the synchronization on your own. 
> But the fact is, it is not a MUST. Below will provide more detail.
> 
> Regarding this synchronization topic, I would like to open a discussion to get a
> better approach to handle this problem. Before jumping onto the design, I would
> like to give a background of why I am doing in this way.
> 
> - Only doing module unload is required to be aware of this synchronization
> 	-> Ensuring the call back does not fall into unloaded code which may cause
> 	     undefined behavior.
> 	-> Ensuring the put_device() & module_put() code have finished in firmware_class.c
> 	     function request_firmware_work_func() before the device is unregistered
> 	     and module unloaded happen.

Shouldn't the existing module_{put,get}() and {put,get}_device() calls
provide all the necessary synchronisation?

Module unload should not be possible while other code is using the
module (and the module refcnt has been incremented accordindly).

Right?

-- 
Matt Fleming, Intel Open Source Technology Center

RE: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()

[Kweh, Hock Leong]

> -----Original Message-----
> From: Matt Fleming [mailto:matt@console-pimps.org]
> Sent: Monday, November 17, 2014 11:12 PM
> >
> > - Only doing module unload is required to be aware of this synchronization
> > 	-> Ensuring the call back does not fall into unloaded code which may
> cause
> > 	     undefined behavior.
> > 	-> Ensuring the put_device() & module_put() code have finished in
> firmware_class.c
> > 	     function request_firmware_work_func() before the device is
> unregistered
> > 	     and module unloaded happen.
> 
> Shouldn't the existing module_{put,get}() and {put,get}_device() calls
> provide all the necessary synchronisation?
> 
> Module unload should not be possible while other code is using the
> module (and the module refcnt has been incremented accordindly).
> 
> Right?
> 
> --
> Matt Fleming, Intel Open Source Technology Center

Hi Matt,

Yes, you are right. If the module refcount is not zero, you will get error
message and returned while you do "rmmod". But I strongly believe if we
have the capability in our code to take care of it by doing synchronization,
we should take care of it in case people are doing "rmmod -f". Don't
you think so?


Regards,
Wilson

Re: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()

[Greg Kroah-Hartman]

On Tue, Nov 18, 2014 at 06:31:38AM +0000, Kweh, Hock Leong wrote:
> > -----Original Message-----
> > From: Matt Fleming [mailto:matt@console-pimps.org]
> > Sent: Monday, November 17, 2014 11:12 PM
> > >
> > > - Only doing module unload is required to be aware of this synchronization
> > > 	-> Ensuring the call back does not fall into unloaded code which may
> > cause
> > > 	     undefined behavior.
> > > 	-> Ensuring the put_device() & module_put() code have finished in
> > firmware_class.c
> > > 	     function request_firmware_work_func() before the device is
> > unregistered
> > > 	     and module unloaded happen.
> > 
> > Shouldn't the existing module_{put,get}() and {put,get}_device() calls
> > provide all the necessary synchronisation?
> > 
> > Module unload should not be possible while other code is using the
> > module (and the module refcnt has been incremented accordindly).
> > 
> > Right?
> > 
> > --
> > Matt Fleming, Intel Open Source Technology Center
> 
> Hi Matt,
> 
> Yes, you are right. If the module refcount is not zero, you will get error
> message and returned while you do "rmmod". But I strongly believe if we
> have the capability in our code to take care of it by doing synchronization,
> we should take care of it in case people are doing "rmmod -f". Don't
> you think so?

If you do 'rmmod -f' you get to keep all of the broken pieces of your
kernel, no need to try to help out with crazy things like that.

greg k-h

[PATCH v2 2/3] firmware loader: fix hung task warning dump

[Kweh Hock Leong]

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

When using request_firmware_nowait() with FW_ACTION_NOHOTPLUG param to
expose user helper interface, if the user do not react immediately, after
120 seconds there will be a hung task warning message dumped as below:

[ 3000.784235] INFO: task kworker/0:0:8259 blocked for more than 120 seconds.
[ 3000.791281]       Tainted: G            E 3.16.0-rc1-yocto-standard #41
[ 3000.798082] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 3000.806072] kworker/0:0     D cd0075c8     0  8259      2 0x00000000
[ 3000.812765] Workqueue: events request_firmware_work_func
[ 3000.818253]  cd375e18 00000046 0000000e cd0075c8 000000f0 cd40ea00 cd375fec 1b883e89
[ 3000.826374]  0000026b cd40ea00 80000000 00000001 cd0075c8 00000000 cd375de4 c119917f
[ 3000.834492]  cd563360 cd375df4 c119a0ab cd563360 00000000 cd375e24 c119a1d6 00000000
[ 3000.842616] Call Trace:
[ 3000.845252]  [<c119917f>] ? kernfs_next_descendant_post+0x3f/0x50
[ 3000.851543]  [<c119a0ab>] ? kernfs_activate+0x6b/0xc0
[ 3000.856790]  [<c119a1d6>] ? kernfs_add_one+0xd6/0x130
[ 3000.862047]  [<c15fdb02>] schedule+0x22/0x60
[ 3000.866548]  [<c15fd195>] schedule_timeout+0x175/0x1d0
[ 3000.871887]  [<c119b391>] ? __kernfs_create_file+0x71/0xa0
[ 3000.877574]  [<c119bb9a>] ? sysfs_add_file_mode_ns+0xaa/0x180
[ 3000.883533]  [<c15fe84f>] wait_for_completion+0x6f/0xb0
[ 3000.888961]  [<c1065200>] ? wake_up_process+0x40/0x40
[ 3000.894219]  [<c13cb600>] _request_firmware+0x750/0x9f0
[ 3000.899666]  [<c1382a7f>] ? n_tty_receive_buf2+0x1f/0x30
[ 3000.905200]  [<c13cba02>] request_firmware_work_func+0x22/0x50
[ 3000.911235]  [<c10550d2>] process_one_work+0x122/0x380
[ 3000.916571]  [<c1055859>] worker_thread+0xf9/0x470
[ 3000.921555]  [<c1055760>] ? create_and_start_worker+0x50/0x50
[ 3000.927497]  [<c1055760>] ? create_and_start_worker+0x50/0x50
[ 3000.933448]  [<c105a5ff>] kthread+0x9f/0xc0
[ 3000.937850]  [<c15ffd40>] ret_from_kernel_thread+0x20/0x30
[ 3000.943548]  [<c105a560>] ? kthread_worker_fn+0x100/0x100

This patch change the wait_for_completion() function call to
wait_for_completion_interruptible() function call for solving the issue.

Cc: Matt Fleming <matt.fleming@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
---
 drivers/base/firmware_class.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index a238a46..639eeff 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -908,7 +908,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD);
 	}
 
-	wait_for_completion(&buf->completion);
+	retval = wait_for_completion_interruptible(&buf->completion);
 
 	cancel_delayed_work_sync(&fw_priv->timeout_work);
 	if (!buf->data)
@@ -985,7 +985,7 @@ static int sync_cached_firmware_buf(struct firmware_buf *buf)
 			break;
 		}
 		mutex_unlock(&fw_lock);
-		wait_for_completion(&buf->completion);
+		ret = wait_for_completion_interruptible(&buf->completion);
 		mutex_lock(&fw_lock);
 	}
 	mutex_unlock(&fw_lock);
-- 
1.7.9.5

Re: [PATCH v2 2/3] firmware loader: fix hung task warning dump

[Greg Kroah-Hartman]

On Mon, Nov 03, 2014 at 11:07:09AM +0800, Kweh Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> 
> When using request_firmware_nowait() with FW_ACTION_NOHOTPLUG param to
> expose user helper interface, if the user do not react immediately, after
> 120 seconds there will be a hung task warning message dumped as below:
> 
> [ 3000.784235] INFO: task kworker/0:0:8259 blocked for more than 120 seconds.
> [ 3000.791281]       Tainted: G            E 3.16.0-rc1-yocto-standard #41
> [ 3000.798082] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [ 3000.806072] kworker/0:0     D cd0075c8     0  8259      2 0x00000000
> [ 3000.812765] Workqueue: events request_firmware_work_func
> [ 3000.818253]  cd375e18 00000046 0000000e cd0075c8 000000f0 cd40ea00 cd375fec 1b883e89
> [ 3000.826374]  0000026b cd40ea00 80000000 00000001 cd0075c8 00000000 cd375de4 c119917f
> [ 3000.834492]  cd563360 cd375df4 c119a0ab cd563360 00000000 cd375e24 c119a1d6 00000000
> [ 3000.842616] Call Trace:
> [ 3000.845252]  [<c119917f>] ? kernfs_next_descendant_post+0x3f/0x50
> [ 3000.851543]  [<c119a0ab>] ? kernfs_activate+0x6b/0xc0
> [ 3000.856790]  [<c119a1d6>] ? kernfs_add_one+0xd6/0x130
> [ 3000.862047]  [<c15fdb02>] schedule+0x22/0x60
> [ 3000.866548]  [<c15fd195>] schedule_timeout+0x175/0x1d0
> [ 3000.871887]  [<c119b391>] ? __kernfs_create_file+0x71/0xa0
> [ 3000.877574]  [<c119bb9a>] ? sysfs_add_file_mode_ns+0xaa/0x180
> [ 3000.883533]  [<c15fe84f>] wait_for_completion+0x6f/0xb0
> [ 3000.888961]  [<c1065200>] ? wake_up_process+0x40/0x40
> [ 3000.894219]  [<c13cb600>] _request_firmware+0x750/0x9f0
> [ 3000.899666]  [<c1382a7f>] ? n_tty_receive_buf2+0x1f/0x30
> [ 3000.905200]  [<c13cba02>] request_firmware_work_func+0x22/0x50
> [ 3000.911235]  [<c10550d2>] process_one_work+0x122/0x380
> [ 3000.916571]  [<c1055859>] worker_thread+0xf9/0x470
> [ 3000.921555]  [<c1055760>] ? create_and_start_worker+0x50/0x50
> [ 3000.927497]  [<c1055760>] ? create_and_start_worker+0x50/0x50
> [ 3000.933448]  [<c105a5ff>] kthread+0x9f/0xc0
> [ 3000.937850]  [<c15ffd40>] ret_from_kernel_thread+0x20/0x30
> [ 3000.943548]  [<c105a560>] ? kthread_worker_fn+0x100/0x100
> 
> This patch change the wait_for_completion() function call to
> wait_for_completion_interruptible() function call for solving the issue.
> 
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh Hock Leong]

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

Introducing a kernel module to expose user helper interface for
user to upload capsule binaries. This module leverage the
request_firmware_nowait() to expose an interface to user.

Example steps to load the capsule binary:
1.) echo 1 > /sys/class/firmware/efi-capsule-file/loading
2.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data
3.) echo 0 > /sys/class/firmware/efi-capsule-file/loading

Whereas, this module does not allow the capsule binaries to be
obtained from the request_firmware library search path. If any
capsule binary loaded from firmware seach path, the module will
stop functioning.

Besides the request_firmware user helper interface, this module
also expose a 'capsule_loaded' file note for user to verify
the number of successfully uploaded capsule binaries. This
file note has the read only attribute.

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

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..7dc814e 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
 config EFI_ARMSTUB
 	bool
 
+config EFI_CAPSULE_USER_HELPER
+	tristate "EFI capsule user mode helper"
+	depends on EFI
+	select FW_LOADER
+	select FW_LOADER_USER_HELPER
+	help
+	  This option exposes the user mode helper interface for user to load
+	  EFI capsule binary and update the EFI firmware after 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..63f6910 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_USER_HELPER)	+= efi-capsule-user-helper.o
diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c b/drivers/firmware/efi/efi-capsule-user-helper.c
new file mode 100644
index 0000000..84a1628
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-user-helper.c
@@ -0,0 +1,246 @@
+/*
+ * EFI capsule user mode helper interface driver.
+ *
+ * Copyright 2014 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/delay.h>
+#include <linux/highmem.h>
+#include <linux/slab.h>
+#include <linux/mutex.h>
+#include <linux/reboot.h>
+#include <linux/efi.h>
+#include <linux/firmware.h>
+
+#define CAPSULE_NAME "efi-capsule-file"
+#define DEV_NAME "efi_capsule_user_helper"
+#define STRING_INTEGER_MAX_LENGTH 13
+
+static DEFINE_MUTEX(user_helper_lock);
+static int capsule_count;
+static int stop_request;
+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;
+
+	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;
+	}
+	++capsule_count;
+	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;
+}
+
+/*
+ * This callback function will be called by request_firmware_nowait() when
+ * user has loaded the capsule binary or aborted user helper interface
+ */
+static void callbackfn_efi_capsule(const struct firmware *fw, void *context)
+{
+	int ret = 0;
+
+	if (fw) {
+		/*
+		 * Binary not getting from user helper interface, fw->pages
+		 * is equal to NULL
+		 */
+		if (!fw->pages) {
+			pr_err("%s: ERROR: Capsule binary '%s' at %s\n",
+			       __func__, CAPSULE_NAME,
+			       "firmware lib search path are not supported");
+			pr_err("user helper interface disabled\n");
+			stop_request = 1;
+		} else {
+			efi_capsule_store(fw);
+		}
+		release_firmware(fw);
+	}
+
+	mutex_lock(&user_helper_lock);
+	if (!stop_request) {
+		ret = request_firmware_nowait(THIS_MODULE,
+					      FW_ACTION_NOHOTPLUG,
+					      CAPSULE_NAME,
+					      &efi_capsule_pdev->dev,
+					      GFP_KERNEL, NULL,
+					      callbackfn_efi_capsule);
+		if (ret) {
+			pr_err("%s: request_firmware_nowait() failed\n",
+			       __func__);
+		}
+	}
+	mutex_unlock(&user_helper_lock);
+}
+
+static ssize_t capsule_loaded_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	return snprintf(buf, STRING_INTEGER_MAX_LENGTH, "%d\n", capsule_count);
+}
+
+static DEVICE_ATTR_RO(capsule_loaded);
+
+/* stop user helper interface for reboot or module unload */
+static void capsule_stop_user_helper(void)
+{
+	mutex_lock(&user_helper_lock);
+	stop_request = 1;
+	mutex_unlock(&user_helper_lock);
+	request_firmware_abort(CAPSULE_NAME);
+}
+
+/* reboot notifier for avoid deadlock with usermode_lock */
+static int capsule_shutdown_notify(struct notifier_block *nb,
+				   unsigned long sys_state,
+				   void *reboot_cmd)
+{
+	capsule_stop_user_helper();
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block capsule_shutdown_nb = {
+	.notifier_call = capsule_shutdown_notify,
+	/*
+	 * In order to reboot properly, it is required to ensure the priority
+	 * here is higher than firmware_class fw_shutdown_nb priority
+	 */
+	.priority = 1,
+};
+
+/*
+ * Use request_firmware_nowait() exposing an user helper interface to obtain
+ * capsule binary from user space
+ */
+static int __init efi_capsule_user_helper_init(void)
+{
+	int ret;
+
+	register_reboot_notifier(&capsule_shutdown_nb);
+
+	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 check the number of binaries that
+	 * are successfully loaded
+	 */
+	ret = device_create_file(&efi_capsule_pdev->dev,
+				 &dev_attr_capsule_loaded);
+	if (ret) {
+		pr_err("%s: device_create_file() failed\n", __func__);
+		return ret;
+	}
+
+	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
+				      CAPSULE_NAME, &efi_capsule_pdev->dev,
+				      GFP_KERNEL, NULL, callbackfn_efi_capsule);
+	if (ret) {
+		pr_err("%s: request_firmware_nowait() failed\n", __func__);
+		goto out;
+	}
+
+	return 0;
+
+out:
+	device_remove_file(&efi_capsule_pdev->dev, &dev_attr_capsule_loaded);
+	unregister_reboot_notifier(&capsule_shutdown_nb);
+	return ret;
+}
+module_init(efi_capsule_user_helper_init);
+
+/*
+ * rmmod app itself will stop you to remove the module if the user
+ * helper interface is still exposing. In order to remove this driver,
+ * you are require to do the command as below:
+ * rmmod -f efi_capsule_user_helper.ko
+ */
+static void __exit efi_capsule_user_helper_exit(void)
+{
+	unregister_reboot_notifier(&capsule_shutdown_nb);
+
+	capsule_stop_user_helper();
+	/*
+	 * synchronization is needed to make sure request_firmware is fully
+	 * aborted
+	 */
+	while (efi_capsule_pdev->dev.kobj.kref.refcount.counter > 3)
+		msleep(20); /* avoid busy waiting for cooperative kernel */
+
+	device_remove_file(&efi_capsule_pdev->dev, &dev_attr_capsule_loaded);
+	platform_device_unregister(efi_capsule_pdev);
+}
+module_exit(efi_capsule_user_helper_exit);
+
+MODULE_DESCRIPTION("EFI Capsule user helper binary load utility");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Greg Kroah-Hartman]

On Mon, Nov 03, 2014 at 11:07:10AM +0800, Kweh Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> 
> Introducing a kernel module to expose user helper interface for
> user to upload capsule binaries. This module leverage the
> request_firmware_nowait() to expose an interface to user.
> 
> Example steps to load the capsule binary:
> 1.) echo 1 > /sys/class/firmware/efi-capsule-file/loading
> 2.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data
> 3.) echo 0 > /sys/class/firmware/efi-capsule-file/loading
> 
> Whereas, this module does not allow the capsule binaries to be
> obtained from the request_firmware library search path. If any
> capsule binary loaded from firmware seach path, the module will
> stop functioning.
> 
> Besides the request_firmware user helper interface, this module
> also expose a 'capsule_loaded' file note for user to verify
> the number of successfully uploaded capsule binaries. This
> file note has the read only attribute.

Andy, here's the steps to load a capsule.  I don't have a problem with
this, it's userspace driven, no "autoloading" of files in /lib/, and
works with sysfs.

Have any objection to it, I don't.

Full patch left below...

> 
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/Kconfig                   |   13 ++
>  drivers/firmware/efi/Makefile                  |    1 +
>  drivers/firmware/efi/efi-capsule-user-helper.c |  246 ++++++++++++++++++++++++
>  3 files changed, 260 insertions(+)
>  create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index f712d47..7dc814e 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
>  config EFI_ARMSTUB
>  	bool
>  
> +config EFI_CAPSULE_USER_HELPER
> +	tristate "EFI capsule user mode helper"
> +	depends on EFI
> +	select FW_LOADER
> +	select FW_LOADER_USER_HELPER
> +	help
> +	  This option exposes the user mode helper interface for user to load
> +	  EFI capsule binary and update the EFI firmware after 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..63f6910 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_USER_HELPER)	+= efi-capsule-user-helper.o
> diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c b/drivers/firmware/efi/efi-capsule-user-helper.c
> new file mode 100644
> index 0000000..84a1628
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-user-helper.c
> @@ -0,0 +1,246 @@
> +/*
> + * EFI capsule user mode helper interface driver.
> + *
> + * Copyright 2014 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/delay.h>
> +#include <linux/highmem.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/reboot.h>
> +#include <linux/efi.h>
> +#include <linux/firmware.h>
> +
> +#define CAPSULE_NAME "efi-capsule-file"
> +#define DEV_NAME "efi_capsule_user_helper"
> +#define STRING_INTEGER_MAX_LENGTH 13
> +
> +static DEFINE_MUTEX(user_helper_lock);
> +static int capsule_count;
> +static int stop_request;
> +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;
> +
> +	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;
> +	}
> +	++capsule_count;
> +	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;
> +}
> +
> +/*
> + * This callback function will be called by request_firmware_nowait() when
> + * user has loaded the capsule binary or aborted user helper interface
> + */
> +static void callbackfn_efi_capsule(const struct firmware *fw, void *context)
> +{
> +	int ret = 0;
> +
> +	if (fw) {
> +		/*
> +		 * Binary not getting from user helper interface, fw->pages
> +		 * is equal to NULL
> +		 */
> +		if (!fw->pages) {
> +			pr_err("%s: ERROR: Capsule binary '%s' at %s\n",
> +			       __func__, CAPSULE_NAME,
> +			       "firmware lib search path are not supported");
> +			pr_err("user helper interface disabled\n");
> +			stop_request = 1;
> +		} else {
> +			efi_capsule_store(fw);
> +		}
> +		release_firmware(fw);
> +	}
> +
> +	mutex_lock(&user_helper_lock);
> +	if (!stop_request) {
> +		ret = request_firmware_nowait(THIS_MODULE,
> +					      FW_ACTION_NOHOTPLUG,
> +					      CAPSULE_NAME,
> +					      &efi_capsule_pdev->dev,
> +					      GFP_KERNEL, NULL,
> +					      callbackfn_efi_capsule);
> +		if (ret) {
> +			pr_err("%s: request_firmware_nowait() failed\n",
> +			       __func__);
> +		}
> +	}
> +	mutex_unlock(&user_helper_lock);
> +}
> +
> +static ssize_t capsule_loaded_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	return snprintf(buf, STRING_INTEGER_MAX_LENGTH, "%d\n", capsule_count);
> +}
> +
> +static DEVICE_ATTR_RO(capsule_loaded);
> +
> +/* stop user helper interface for reboot or module unload */
> +static void capsule_stop_user_helper(void)
> +{
> +	mutex_lock(&user_helper_lock);
> +	stop_request = 1;
> +	mutex_unlock(&user_helper_lock);
> +	request_firmware_abort(CAPSULE_NAME);
> +}
> +
> +/* reboot notifier for avoid deadlock with usermode_lock */
> +static int capsule_shutdown_notify(struct notifier_block *nb,
> +				   unsigned long sys_state,
> +				   void *reboot_cmd)
> +{
> +	capsule_stop_user_helper();
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block capsule_shutdown_nb = {
> +	.notifier_call = capsule_shutdown_notify,
> +	/*
> +	 * In order to reboot properly, it is required to ensure the priority
> +	 * here is higher than firmware_class fw_shutdown_nb priority
> +	 */
> +	.priority = 1,
> +};
> +
> +/*
> + * Use request_firmware_nowait() exposing an user helper interface to obtain
> + * capsule binary from user space
> + */
> +static int __init efi_capsule_user_helper_init(void)
> +{
> +	int ret;
> +
> +	register_reboot_notifier(&capsule_shutdown_nb);
> +
> +	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 check the number of binaries that
> +	 * are successfully loaded
> +	 */
> +	ret = device_create_file(&efi_capsule_pdev->dev,
> +				 &dev_attr_capsule_loaded);
> +	if (ret) {
> +		pr_err("%s: device_create_file() failed\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
> +				      CAPSULE_NAME, &efi_capsule_pdev->dev,
> +				      GFP_KERNEL, NULL, callbackfn_efi_capsule);
> +	if (ret) {
> +		pr_err("%s: request_firmware_nowait() failed\n", __func__);
> +		goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	device_remove_file(&efi_capsule_pdev->dev, &dev_attr_capsule_loaded);
> +	unregister_reboot_notifier(&capsule_shutdown_nb);
> +	return ret;
> +}
> +module_init(efi_capsule_user_helper_init);
> +
> +/*
> + * rmmod app itself will stop you to remove the module if the user
> + * helper interface is still exposing. In order to remove this driver,
> + * you are require to do the command as below:
> + * rmmod -f efi_capsule_user_helper.ko
> + */
> +static void __exit efi_capsule_user_helper_exit(void)
> +{
> +	unregister_reboot_notifier(&capsule_shutdown_nb);
> +
> +	capsule_stop_user_helper();
> +	/*
> +	 * synchronization is needed to make sure request_firmware is fully
> +	 * aborted
> +	 */
> +	while (efi_capsule_pdev->dev.kobj.kref.refcount.counter > 3)
> +		msleep(20); /* avoid busy waiting for cooperative kernel */
> +
> +	device_remove_file(&efi_capsule_pdev->dev, &dev_attr_capsule_loaded);
> +	platform_device_unregister(efi_capsule_pdev);
> +}
> +module_exit(efi_capsule_user_helper_exit);
> +
> +MODULE_DESCRIPTION("EFI Capsule user helper binary load utility");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.9.5

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Mon, Nov 3, 2014 at 8:32 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Nov 03, 2014 at 11:07:10AM +0800, Kweh Hock Leong wrote:
>> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
>>
>> Introducing a kernel module to expose user helper interface for
>> user to upload capsule binaries. This module leverage the
>> request_firmware_nowait() to expose an interface to user.
>>
>> Example steps to load the capsule binary:
>> 1.) echo 1 > /sys/class/firmware/efi-capsule-file/loading
>> 2.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data
>> 3.) echo 0 > /sys/class/firmware/efi-capsule-file/loading
>>
>> Whereas, this module does not allow the capsule binaries to be
>> obtained from the request_firmware library search path. If any
>> capsule binary loaded from firmware seach path, the module will
>> stop functioning.
>>
>> Besides the request_firmware user helper interface, this module
>> also expose a 'capsule_loaded' file note for user to verify
>> the number of successfully uploaded capsule binaries. This
>> file note has the read only attribute.
>
> Andy, here's the steps to load a capsule.  I don't have a problem with
> this, it's userspace driven, no "autoloading" of files in /lib/, and
> works with sysfs.
>
> Have any objection to it, I don't.

After reading the code, I still have objections.

The full workflow seems to be, from the user's POV:

1. load the module

2. hope that there isn't a file called /lib/firmware/efi-capsule-file

3. echo 1 >.../loading

4. cat firmware >.../data

5. echo 0 >.../loading

6. efi_update_capsule gets called.  The return value ends up in the
kernel logs somewhere but doesn't seem to make it to userspace.

7. reboot(), but only if the capsule you loaded requires a reboot,
which may or may not be detectable without the kernel's help (I'm not
sure about this point).

If you want to load a second capsule (which seems like a reasonable
thing to do if the first capsule was the kind that is processed
immediately), then rmmod -f the module and start over?

This seems like an unpleasant interface.  I think that ioctl or a
dedicated custom sysfs file would be considerably nicer.  It would
allow you to upload a capsule and get an indication of success or
failure all at once, and it lets you load more than one capsule.
Also, it gets rid of some of the really weird module refcounting stuff
that's going on -- the only unusual thing the module would do would be
to pin itself once a reboot-requiring capsule is loaded.

--Andy

>
> Full patch left below...
>
>>
>> Cc: Matt Fleming <matt.fleming@intel.com>
>> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
>> ---
>>  drivers/firmware/efi/Kconfig                   |   13 ++
>>  drivers/firmware/efi/Makefile                  |    1 +
>>  drivers/firmware/efi/efi-capsule-user-helper.c |  246 ++++++++++++++++++++++++
>>  3 files changed, 260 insertions(+)
>>  create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
>>
>> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> index f712d47..7dc814e 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
>>  config EFI_ARMSTUB
>>       bool
>>
>> +config EFI_CAPSULE_USER_HELPER
>> +     tristate "EFI capsule user mode helper"
>> +     depends on EFI
>> +     select FW_LOADER
>> +     select FW_LOADER_USER_HELPER
>> +     help
>> +       This option exposes the user mode helper interface for user to load
>> +       EFI capsule binary and update the EFI firmware after 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..63f6910 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_USER_HELPER)        += efi-capsule-user-helper.o
>> diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c b/drivers/firmware/efi/efi-capsule-user-helper.c
>> new file mode 100644
>> index 0000000..84a1628
>> --- /dev/null
>> +++ b/drivers/firmware/efi/efi-capsule-user-helper.c
>> @@ -0,0 +1,246 @@
>> +/*
>> + * EFI capsule user mode helper interface driver.
>> + *
>> + * Copyright 2014 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/delay.h>
>> +#include <linux/highmem.h>
>> +#include <linux/slab.h>
>> +#include <linux/mutex.h>
>> +#include <linux/reboot.h>
>> +#include <linux/efi.h>
>> +#include <linux/firmware.h>
>> +
>> +#define CAPSULE_NAME "efi-capsule-file"
>> +#define DEV_NAME "efi_capsule_user_helper"
>> +#define STRING_INTEGER_MAX_LENGTH 13
>> +
>> +static DEFINE_MUTEX(user_helper_lock);
>> +static int capsule_count;
>> +static int stop_request;
>> +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;
>> +
>> +     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;
>> +     }
>> +     ++capsule_count;
>> +     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;
>> +}
>> +
>> +/*
>> + * This callback function will be called by request_firmware_nowait() when
>> + * user has loaded the capsule binary or aborted user helper interface
>> + */
>> +static void callbackfn_efi_capsule(const struct firmware *fw, void *context)
>> +{
>> +     int ret = 0;
>> +
>> +     if (fw) {
>> +             /*
>> +              * Binary not getting from user helper interface, fw->pages
>> +              * is equal to NULL
>> +              */
>> +             if (!fw->pages) {
>> +                     pr_err("%s: ERROR: Capsule binary '%s' at %s\n",
>> +                            __func__, CAPSULE_NAME,
>> +                            "firmware lib search path are not supported");
>> +                     pr_err("user helper interface disabled\n");
>> +                     stop_request = 1;
>> +             } else {
>> +                     efi_capsule_store(fw);
>> +             }
>> +             release_firmware(fw);
>> +     }
>> +
>> +     mutex_lock(&user_helper_lock);
>> +     if (!stop_request) {
>> +             ret = request_firmware_nowait(THIS_MODULE,
>> +                                           FW_ACTION_NOHOTPLUG,
>> +                                           CAPSULE_NAME,
>> +                                           &efi_capsule_pdev->dev,
>> +                                           GFP_KERNEL, NULL,
>> +                                           callbackfn_efi_capsule);
>> +             if (ret) {
>> +                     pr_err("%s: request_firmware_nowait() failed\n",
>> +                            __func__);
>> +             }
>> +     }
>> +     mutex_unlock(&user_helper_lock);
>> +}
>> +
>> +static ssize_t capsule_loaded_show(struct device *dev,
>> +                                struct device_attribute *attr, char *buf)
>> +{
>> +     return snprintf(buf, STRING_INTEGER_MAX_LENGTH, "%d\n", capsule_count);
>> +}
>> +
>> +static DEVICE_ATTR_RO(capsule_loaded);
>> +
>> +/* stop user helper interface for reboot or module unload */
>> +static void capsule_stop_user_helper(void)
>> +{
>> +     mutex_lock(&user_helper_lock);
>> +     stop_request = 1;
>> +     mutex_unlock(&user_helper_lock);
>> +     request_firmware_abort(CAPSULE_NAME);
>> +}
>> +
>> +/* reboot notifier for avoid deadlock with usermode_lock */
>> +static int capsule_shutdown_notify(struct notifier_block *nb,
>> +                                unsigned long sys_state,
>> +                                void *reboot_cmd)
>> +{
>> +     capsule_stop_user_helper();
>> +     return NOTIFY_DONE;
>> +}
>> +
>> +static struct notifier_block capsule_shutdown_nb = {
>> +     .notifier_call = capsule_shutdown_notify,
>> +     /*
>> +      * In order to reboot properly, it is required to ensure the priority
>> +      * here is higher than firmware_class fw_shutdown_nb priority
>> +      */
>> +     .priority = 1,
>> +};
>> +
>> +/*
>> + * Use request_firmware_nowait() exposing an user helper interface to obtain
>> + * capsule binary from user space
>> + */
>> +static int __init efi_capsule_user_helper_init(void)
>> +{
>> +     int ret;
>> +
>> +     register_reboot_notifier(&capsule_shutdown_nb);
>> +
>> +     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 check the number of binaries that
>> +      * are successfully loaded
>> +      */
>> +     ret = device_create_file(&efi_capsule_pdev->dev,
>> +                              &dev_attr_capsule_loaded);
>> +     if (ret) {
>> +             pr_err("%s: device_create_file() failed\n", __func__);
>> +             return ret;
>> +     }
>> +
>> +     ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
>> +                                   CAPSULE_NAME, &efi_capsule_pdev->dev,
>> +                                   GFP_KERNEL, NULL, callbackfn_efi_capsule);
>> +     if (ret) {
>> +             pr_err("%s: request_firmware_nowait() failed\n", __func__);
>> +             goto out;
>> +     }
>> +
>> +     return 0;
>> +
>> +out:
>> +     device_remove_file(&efi_capsule_pdev->dev, &dev_attr_capsule_loaded);
>> +     unregister_reboot_notifier(&capsule_shutdown_nb);
>> +     return ret;
>> +}
>> +module_init(efi_capsule_user_helper_init);
>> +
>> +/*
>> + * rmmod app itself will stop you to remove the module if the user
>> + * helper interface is still exposing. In order to remove this driver,
>> + * you are require to do the command as below:
>> + * rmmod -f efi_capsule_user_helper.ko
>> + */
>> +static void __exit efi_capsule_user_helper_exit(void)
>> +{
>> +     unregister_reboot_notifier(&capsule_shutdown_nb);
>> +
>> +     capsule_stop_user_helper();
>> +     /*
>> +      * synchronization is needed to make sure request_firmware is fully
>> +      * aborted
>> +      */
>> +     while (efi_capsule_pdev->dev.kobj.kref.refcount.counter > 3)
>> +             msleep(20); /* avoid busy waiting for cooperative kernel */
>> +
>> +     device_remove_file(&efi_capsule_pdev->dev, &dev_attr_capsule_loaded);
>> +     platform_device_unregister(efi_capsule_pdev);
>> +}
>> +module_exit(efi_capsule_user_helper_exit);
>> +
>> +MODULE_DESCRIPTION("EFI Capsule user helper binary load utility");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.7.9.5



-- 
Andy Lutomirski
AMA Capital Management, LLC

RE: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2409 bytes --]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> >
> > Andy, here's the steps to load a capsule.  I don't have a problem with
> > this, it's userspace driven, no "autoloading" of files in /lib/, and
> > works with sysfs.
> >
> > Have any objection to it, I don't.

Thanks Greg for helping me on the explanation. I would like to apologize if
my cover letter/commit messages did misleading.

> 
> After reading the code, I still have objections.
> 
> The full workflow seems to be, from the user's POV:
> 
> 1. load the module
> 
> 2. hope that there isn't a file called /lib/firmware/efi-capsule-file
> 
> 3. echo 1 >.../loading
> 
> 4. cat firmware >.../data
> 
> 5. echo 0 >.../loading
> 
> 6. efi_update_capsule gets called.  The return value ends up in the kernel
> logs somewhere but doesn't seem to make it to userspace.
> 
> 7. reboot(), but only if the capsule you loaded requires a reboot, which may
> or may not be detectable without the kernel's help (I'm not sure about this
> point).
> 
> If you want to load a second capsule (which seems like a reasonable thing to
> do if the first capsule was the kind that is processed immediately), then
> rmmod -f the module and start over?

You no need to do rmmod in order to upload the 2nd capsule binary. You just need to
do the 3 steps as you mentioned in your 3, 4 and 5 for your 2nd or 3rd capsule binaries.
Then the last, you do the reboot.

> 
> This seems like an unpleasant interface.  I think that ioctl or a dedicated
> custom sysfs file would be considerably nicer.  It would allow you to upload a
> capsule and get an indication of success or failure all at once, and it lets you
> load more than one capsule.
> Also, it gets rid of some of the really weird module refcounting stuff that's
> going on -- the only unusual thing the module would do would be to pin itself
> once a reboot-requiring capsule is loaded.
> 
> --Andy
> 

Regarding the synchronization, it is only required for module unload. The code is designed
in such a way that allow to be built as a kernel module or built into the kernel. If you choose
the built in kernel method, you won't come into the module unload situation which require
the synchronization.


Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Mon, Nov 3, 2014 at 10:04 PM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto@amacapital.net]
>> >
>> > Andy, here's the steps to load a capsule.  I don't have a problem with
>> > this, it's userspace driven, no "autoloading" of files in /lib/, and
>> > works with sysfs.
>> >
>> > Have any objection to it, I don't.
>
> Thanks Greg for helping me on the explanation. I would like to apologize if
> my cover letter/commit messages did misleading.
>
>>
>> After reading the code, I still have objections.
>>
>> The full workflow seems to be, from the user's POV:
>>
>> 1. load the module
>>
>> 2. hope that there isn't a file called /lib/firmware/efi-capsule-file
>>
>> 3. echo 1 >.../loading
>>
>> 4. cat firmware >.../data
>>
>> 5. echo 0 >.../loading
>>
>> 6. efi_update_capsule gets called.  The return value ends up in the kernel
>> logs somewhere but doesn't seem to make it to userspace.
>>
>> 7. reboot(), but only if the capsule you loaded requires a reboot, which may
>> or may not be detectable without the kernel's help (I'm not sure about this
>> point).
>>
>> If you want to load a second capsule (which seems like a reasonable thing to
>> do if the first capsule was the kind that is processed immediately), then
>> rmmod -f the module and start over?
>
> You no need to do rmmod in order to upload the 2nd capsule binary. You just need to
> do the 3 steps as you mentioned in your 3, 4 and 5 for your 2nd or 3rd capsule binaries.
> Then the last, you do the reboot.

OK.  I missed that you request firmware again after each request finishes.

>
>>
>> This seems like an unpleasant interface.  I think that ioctl or a dedicated
>> custom sysfs file would be considerably nicer.  It would allow you to upload a
>> capsule and get an indication of success or failure all at once, and it lets you
>> load more than one capsule.
>> Also, it gets rid of some of the really weird module refcounting stuff that's
>> going on -- the only unusual thing the module would do would be to pin itself
>> once a reboot-requiring capsule is loaded.
>>
>> --Andy
>>
>
> Regarding the synchronization, it is only required for module unload. The code is designed
> in such a way that allow to be built as a kernel module or built into the kernel. If you choose
> the built in kernel method, you won't come into the module unload situation which require
> the synchronization.
>

It seems like a large fraction of the code in this module exists just
to work around the fact that request_firmware doesn't do what you want
it to do.  You have code to:

 - Prevent the /lib/firmware mechanism from working.
 - Avoid a deadlock by doing strange things in the unload code.
 - Allow more than one capsule per module load.  (Isn't this hard to
use?  User code will have to wait for the next firmware request before
sending a second capsule.)

All of this is for dubious gain.  You have to do three separate opens
in sysfs to upload a capsule, and there's no way to report back to
userspace whether the EFI call worked and whether a reboot is needed.

What's the benefit of using the firmware interface here?

--Andy

RE: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2063 bytes --]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Tuesday, November 04, 2014 2:32 PM
> 
> It seems like a large fraction of the code in this module exists just to work
> around the fact that request_firmware doesn't do what you want it to do.
> You have code to:
> 
>  - Prevent the /lib/firmware mechanism from working.
>  - Avoid a deadlock by doing strange things in the unload code.
>  - Allow more than one capsule per module load.  (Isn't this hard to use?
> User code will have to wait for the next firmware request before sending a
> second capsule.)

I did try to upload more than one capsule binaries and I don't observe a long wait
is required. In fact, it seem to me the interface is instantly re-created.

> 
> All of this is for dubious gain.  You have to do three separate opens in sysfs to
> upload a capsule, and there's no way to report back to userspace whether
> the EFI call worked and whether a reboot is needed.

I have not encounter any capsule update that does not require reboot.
Base on my understanding, the EFI firmware only do the binary decoding
during the next reboot. If the binary is broken/corrupted, you would only
know during the next reboot. On kernel driver point of view, it just registers
the binary by calling the EFI API UpdateCapsule(). May be Matt you could
help out with this?

Regarding the EFI call failed message, besides obtains from the dmesg log,
you also can verify that through this sysfs:
/sys/devices/platform/efi_capsule_user_helper/capsule_loaded

I did mention this in the commit message as showed below:
Besides the request_firmware user helper interface, this module also exposes
a 'capsule_loaded' file note for user to verify the number of successfully uploaded
capsule binaries. This file note has the read only attribute.

> 
> What's the benefit of using the firmware interface here?
> 
> --Andy

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Matt Fleming]

On Mon, 2014-11-03 at 22:32 -0800, Andy Lutomirski wrote:
> 
> It seems like a large fraction of the code in this module exists just
> to work around the fact that request_firmware doesn't do what you want
> it to do.  You have code to:
> 
>  - Prevent the /lib/firmware mechanism from working.
>  - Avoid a deadlock by doing strange things in the unload code.
>  - Allow more than one capsule per module load.  (Isn't this hard to
> use?  User code will have to wait for the next firmware request before
> sending a second capsule.)
> 
> All of this is for dubious gain.  You have to do three separate opens
> in sysfs to upload a capsule, and there's no way to report back to
> userspace whether the EFI call worked and whether a reboot is needed.

Whether or not a reboot is required is indicated in the capsule image
itself, i.e. the capsule tells the firmware whether an immediate reboot
is required not the other way around.

The firmware does tell the kernel what *kind* of a reboot is required,
but that doesn't need reporting to userspace.

> What's the benefit of using the firmware interface here?

I originally implemented something to send capsules to the firmware via
sysfs files back in 2013 and I basically ended up duplicating 25% of the
code that's already in drivers/base/firmware_class.c.

If you're objecting to the lack of modularity in firmware_class.c, then
we could probably carve up the functionality we require a little more
neatly (like not having to do the /lib/firmware avoidance hacks), but
firmware_class.c should definitely be used as the foundation.

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Nov 4, 2014 6:18 AM, "Matt Fleming" <matt.fleming@intel.com> wrote:
>
> On Mon, 2014-11-03 at 22:32 -0800, Andy Lutomirski wrote:
> >
> > It seems like a large fraction of the code in this module exists just
> > to work around the fact that request_firmware doesn't do what you want
> > it to do.  You have code to:
> >
> >  - Prevent the /lib/firmware mechanism from working.
> >  - Avoid a deadlock by doing strange things in the unload code.
> >  - Allow more than one capsule per module load.  (Isn't this hard to
> > use?  User code will have to wait for the next firmware request before
> > sending a second capsule.)

I think that the firmware directory goes away and comes back.  Try cd
/sys/firmware/efi-capsule-file and upload one capsule.  I bet that ls
will show an empty directory.

> >
> > All of this is for dubious gain.  You have to do three separate opens
> > in sysfs to upload a capsule, and there's no way to report back to
> > userspace whether the EFI call worked and whether a reboot is needed.
>
> Whether or not a reboot is required is indicated in the capsule image
> itself, i.e. the capsule tells the firmware whether an immediate reboot
> is required not the other way around.
>
> The firmware does tell the kernel what *kind* of a reboot is required,
> but that doesn't need reporting to userspace.
>
> > What's the benefit of using the firmware interface here?
>
> I originally implemented something to send capsules to the firmware via
> sysfs files back in 2013 and I basically ended up duplicating 25% of the
> code that's already in drivers/base/firmware_class.c.
>
> If you're objecting to the lack of modularity in firmware_class.c, then
> we could probably carve up the functionality we require a little more
> neatly (like not having to do the /lib/firmware avoidance hacks), but
> firmware_class.c should definitely be used as the foundation.
>

I don't particularly care what the foundation is, but given that a
misc char device currently looks like it would be considerably less
code for a nicer interface, using the firmware class in its current
state doesn't look so great.

If I were to write user code for this, I'd really want a single
transaction that uploads a capsule and gets a success/failure
indication.  Using ioctl or a single big write sound fine.

Reading the count of successful loaded capsules, writing to 'loading',
writing the data, writing to loading, reading the count again, and
checking that the value is right *works*, but it's quite baroque.  And
it will never scale well, because AFAICT the "count" file is not even
in the same sysfs directory as the rest of the control files.

Basically, I agree that EFI capsules are firmware, and using the
"firmware" mechanism makes logical sense.  But the firmware class is
designed for kernel drivers that request firmware, user code that just
blindly supplies an existing file, user code that doesn't care at all
whether the firmware loaded correctly (because, for many drivers,
there is probably no synchronous way to figure out whether the upload
worked anyway), and firmware loading being idempotent.

For EFI capsules and other flash-my-device mechanisms, we want user
code to send firmware independently of any driver request, user code
to actively think about what it wants to send, user code to find out
what happened as a result of the request, and user code to actively
think about whether it should send another update.

If someone wants to make firmware_class work very nicely for this
interface, that sounds great.  I'd recommend using a non-overlapping
set of filenames in the sysfs directory to avoid confusing existing
tools, especially since it's not obvious to be that the kernel has any
way at all to detect that what it thinks is an intentional capsule
load request is actually an old version of udev mindlessly responding
to a firmware loading request (via udevadm trigger if nothing else).
I kind of suspect that it will end up sharing very little code with
the normal firmware mechanism, though.

This thing really does sound like it'll be 20-30 lines of code *total*
using a misc device, and the earlier patches in the series could
possibly be dropped entirely.

--Andy

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Greg KH]

On Tue, Nov 04, 2014 at 06:57:21AM -0800, Andy Lutomirski wrote:
> On Nov 4, 2014 6:18 AM, "Matt Fleming" <matt.fleming@intel.com> wrote:
> >
> > On Mon, 2014-11-03 at 22:32 -0800, Andy Lutomirski wrote:
> > >
> > > It seems like a large fraction of the code in this module exists just
> > > to work around the fact that request_firmware doesn't do what you want
> > > it to do.  You have code to:
> > >
> > >  - Prevent the /lib/firmware mechanism from working.
> > >  - Avoid a deadlock by doing strange things in the unload code.
> > >  - Allow more than one capsule per module load.  (Isn't this hard to
> > > use?  User code will have to wait for the next firmware request before
> > > sending a second capsule.)
> 
> I think that the firmware directory goes away and comes back.  Try cd
> /sys/firmware/efi-capsule-file and upload one capsule.  I bet that ls
> will show an empty directory.
> 
> > >
> > > All of this is for dubious gain.  You have to do three separate opens
> > > in sysfs to upload a capsule, and there's no way to report back to
> > > userspace whether the EFI call worked and whether a reboot is needed.
> >
> > Whether or not a reboot is required is indicated in the capsule image
> > itself, i.e. the capsule tells the firmware whether an immediate reboot
> > is required not the other way around.
> >
> > The firmware does tell the kernel what *kind* of a reboot is required,
> > but that doesn't need reporting to userspace.
> >
> > > What's the benefit of using the firmware interface here?
> >
> > I originally implemented something to send capsules to the firmware via
> > sysfs files back in 2013 and I basically ended up duplicating 25% of the
> > code that's already in drivers/base/firmware_class.c.
> >
> > If you're objecting to the lack of modularity in firmware_class.c, then
> > we could probably carve up the functionality we require a little more
> > neatly (like not having to do the /lib/firmware avoidance hacks), but
> > firmware_class.c should definitely be used as the foundation.
> >
> 
> I don't particularly care what the foundation is, but given that a
> misc char device currently looks like it would be considerably less
> code for a nicer interface, using the firmware class in its current
> state doesn't look so great.

Then fix the firmware class code.

Please, don't create random /dev nodes for firmware images like this,
use the firmware code, that is what it is there for, and it is correct
to use it for this type of interface.

> If I were to write user code for this, I'd really want a single
> transaction that uploads a capsule and gets a success/failure
> indication.  Using ioctl or a single big write sound fine.

That's what you are using with the firmware interface, so this patch is
currect.

> Basically, I agree that EFI capsules are firmware, and using the
> "firmware" mechanism makes logical sense.  But the firmware class is
> designed for kernel drivers that request firmware, user code that just
> blindly supplies an existing file, user code that doesn't care at all
> whether the firmware loaded correctly (because, for many drivers,
> there is probably no synchronous way to figure out whether the upload
> worked anyway), and firmware loading being idempotent.
> 
> For EFI capsules and other flash-my-device mechanisms, we want user
> code to send firmware independently of any driver request, user code
> to actively think about what it wants to send, user code to find out
> what happened as a result of the request, and user code to actively
> think about whether it should send another update.
> 
> If someone wants to make firmware_class work very nicely for this
> interface, that sounds great.  I'd recommend using a non-overlapping
> set of filenames in the sysfs directory to avoid confusing existing
> tools, especially since it's not obvious to be that the kernel has any
> way at all to detect that what it thinks is an intentional capsule
> load request is actually an old version of udev mindlessly responding
> to a firmware loading request (via udevadm trigger if nothing else).
> I kind of suspect that it will end up sharing very little code with
> the normal firmware mechanism, though.
> 
> This thing really does sound like it'll be 20-30 lines of code *total*
> using a misc device, and the earlier patches in the series could
> possibly be dropped entirely.

I don't want to see the misc interface used for this, sorry you don't
like it, but I feel the firmware interface is correct.

greg k-h

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Tue, Nov 4, 2014 at 7:40 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Nov 04, 2014 at 06:57:21AM -0800, Andy Lutomirski wrote:
>>
>> I don't particularly care what the foundation is, but given that a
>> misc char device currently looks like it would be considerably less
>> code for a nicer interface, using the firmware class in its current
>> state doesn't look so great.
>
> Then fix the firmware class code.
>
> Please, don't create random /dev nodes for firmware images like this,
> use the firmware code, that is what it is there for, and it is correct
> to use it for this type of interface.
>
>> If I were to write user code for this, I'd really want a single
>> transaction that uploads a capsule and gets a success/failure
>> indication.  Using ioctl or a single big write sound fine.
>
> That's what you are using with the firmware interface, so this patch is
> currect.

Am I missing something here?  The current proposal is missing the
success/failure part, unless you count the loaded count (in a
different sysfs directory) as a useful interface for that.

>
>> Basically, I agree that EFI capsules are firmware, and using the
>> "firmware" mechanism makes logical sense.  But the firmware class is
>> designed for kernel drivers that request firmware, user code that just
>> blindly supplies an existing file, user code that doesn't care at all
>> whether the firmware loaded correctly (because, for many drivers,
>> there is probably no synchronous way to figure out whether the upload
>> worked anyway), and firmware loading being idempotent.
>>
>> For EFI capsules and other flash-my-device mechanisms, we want user
>> code to send firmware independently of any driver request, user code
>> to actively think about what it wants to send, user code to find out
>> what happened as a result of the request, and user code to actively
>> think about whether it should send another update.
>>
>> If someone wants to make firmware_class work very nicely for this
>> interface, that sounds great.  I'd recommend using a non-overlapping
>> set of filenames in the sysfs directory to avoid confusing existing
>> tools, especially since it's not obvious to be that the kernel has any
>> way at all to detect that what it thinks is an intentional capsule
>> load request is actually an old version of udev mindlessly responding
>> to a firmware loading request (via udevadm trigger if nothing else).
>> I kind of suspect that it will end up sharing very little code with
>> the normal firmware mechanism, though.
>>
>> This thing really does sound like it'll be 20-30 lines of code *total*
>> using a misc device, and the earlier patches in the series could
>> possibly be dropped entirely.
>
> I don't want to see the misc interface used for this, sorry you don't
> like it, but I feel the firmware interface is correct.
>

As I said, I don't object to the use of firmware class.  I'd just kind
of like the actual result of doing so to not suck.

Other things I noticed on brief review of the code:

The firmware class has a caching mechanism.  I have no idea how it
works, but it needs to be reliably disabled for efi-capsule-file.  Is
it?

There's a timeout mechanism.  That mechanism should not be invoked for
efi-capsule-file.  I haven't spotted code to disable it.

The count of loaded capsules is in a separate platform device.  It is
really okay for this driver to have two separate sysfs directories
with names that user code needs to hard code?  Shouldn't there be just
one?

Using the NULLness of fw->pages as an indication of where the firmware
came from seems extremely unreliable and likely to break in
interesting ways in the future.

It looks like every firmware request either results in a uevent or a
helper invocation.  Neither is appropriate here.  Should they be
turned off (by some new interface in the firmware class)?

This might all be nicely resolved by adding a new interface to
firmware_class that says "I want userspace to be able to send me
firmware zero or more times, and I want to handle each blob
individually."

--Andy

RE: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1413 bytes --]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> Sent: Wednesday, November 05, 2014 12:36 AM
> 
> Am I missing something here?  The current proposal is missing the
> success/failure part, unless you count the loaded count (in a different sysfs
> directory) as a useful interface for that.

Here is my sample shell script which allow me to do multi capsule binaries upload
and obtain error message if error occur:

#!/bin/sh

old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)

for arg in "$@"
do
	if [ -f $arg ]
	then
		echo 1 > /sys/class/firmware/efi-capsule-file/loading
		cat $arg > /sys/class/firmware/efi-capsule-file/data
		echo 0 > /sys/class/firmware/efi-capsule-file/loading

		oldtime=$(date +%S)
		oldtime=$(((time + 2) % 60))
		until [ -f /sys/class/firmware/efi-capsule-file/loading ]
		do
			newtime=$(date +%S)
			if [ $newtime -eq $oldtime ]
			then
				break
			fi
		done

		old=$((old + 1))
		new=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
		if [ ! $new -eq $old ]
		then
			echo "Capsule binary $arg upload failed"
			dmesg | tail | grep -v platform | grep -e efi
			exit 1
		fi
	else
		echo "File $arg not found !!"
	fi
done
exit 0


Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Nov 6, 2014 4:56 AM, "Kweh, Hock Leong" <hock.leong.kweh@intel.com> wrote:
>
> > -----Original Message-----
> > From: Andy Lutomirski [mailto:luto@amacapital.net]
> > Sent: Wednesday, November 05, 2014 12:36 AM
> >
> > Am I missing something here?  The current proposal is missing the
> > success/failure part, unless you count the loaded count (in a different sysfs
> > directory) as a useful interface for that.
>
> Here is my sample shell script which allow me to do multi capsule binaries upload
> and obtain error message if error occur:
>
> #!/bin/sh
>
> old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
>
> for arg in "$@"
> do
>         if [ -f $arg ]
>         then
>                 echo 1 > /sys/class/firmware/efi-capsule-file/loading
>                 cat $arg > /sys/class/firmware/efi-capsule-file/data
>                 echo 0 > /sys/class/firmware/efi-capsule-file/loading

I think you have a race.  Try putting msleep(1000) after the
request_firmware_nowait call, and I bet this will fail on the second
try.

>
>                 oldtime=$(date +%S)
>                 oldtime=$(((time + 2) % 60))
>                 until [ -f /sys/class/firmware/efi-capsule-file/loading ]
>                 do
>                         newtime=$(date +%S)
>                         if [ $newtime -eq $oldtime ]
>                         then
>                                 break
>                         fi
>                 done
>
>                 old=$((old + 1))
>                 new=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)

I think that firmware_class doesn't call the callback until after
loading is closed for the second time.  If so, then this is racy.  Try
inserting msleep(1000) at the beginning of your callback and uploading
a capsule that should load successfully -- this will report failure,
but a future upload may get very confused.  Also, what does the
firmware class do when simultaneous uploads of the same file with
different contents are in flight?  Is that possible?


--Andy

RE: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2735 bytes --]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@amacapital.net]
> > #!/bin/sh
> >
> > old=$(cat
> > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
> >
> > for arg in "$@"
> > do
> >         if [ -f $arg ]
> >         then
> >                 echo 1 > /sys/class/firmware/efi-capsule-file/loading
> >                 cat $arg > /sys/class/firmware/efi-capsule-file/data
> >                 echo 0 > /sys/class/firmware/efi-capsule-file/loading
> 
> I think you have a race.  Try putting msleep(1000) after the
> request_firmware_nowait call, and I bet this will fail on the second try.

Sorry for the late response. I don't really catch the race condition that
you are referring? Are you trying to tell that the user script could run faster
before the previous callback function actually end? Will such scenario happen?
In the callback function, after the request_firmware_nowait(), I don't have
any codes will delay the callback function to end. Besides, there is a mutex_lock
protecting the request_firmware_nowait() calling. Won't that take care of the
issue?

> 
> >
> >                 oldtime=$(date +%S)
> >                 oldtime=$(((time + 2) % 60))
> >                 until [ -f /sys/class/firmware/efi-capsule-file/loading ]
> >                 do
> >                         newtime=$(date +%S)
> >                         if [ $newtime -eq $oldtime ]
> >                         then
> >                                 break
> >                         fi
> >                 done
> >
> >                 old=$((old + 1))
> >                 new=$(cat
> > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
> 
> I think that firmware_class doesn't call the callback until after loading is closed
> for the second time.  If so, then this is racy.  Try inserting msleep(1000) at the
> beginning of your callback and uploading a capsule that should load
> successfully -- this will report failure, but a future upload may get very
> confused. Also, what does the firmware class do when simultaneous
> uploads of the same file with different contents are in flight?  Is that possible?

Sorry again, I can't really catch you on this race condition statement. Are you
trying to tell if user is doing this:

echo 1 > /sys/class/firmware/efi-capsule-file/loading
cat capsule1 > /sys/class/firmware/efi-capsule-file/data
cat capsule2 > /sys/class/firmware/efi-capsule-file/data
echo 0 > /sys/class/firmware/efi-capsule-file/loading

If so, capsule2 will be the one we will obtain in the callback function.


Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Mon, Nov 10, 2014 at 12:31 AM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto@amacapital.net]
>> > #!/bin/sh
>> >
>> > old=$(cat
>> > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
>> >
>> > for arg in "$@"
>> > do
>> >         if [ -f $arg ]
>> >         then
>> >                 echo 1 > /sys/class/firmware/efi-capsule-file/loading
>> >                 cat $arg > /sys/class/firmware/efi-capsule-file/data
>> >                 echo 0 > /sys/class/firmware/efi-capsule-file/loading
>>
>> I think you have a race.  Try putting msleep(1000) after the
>> request_firmware_nowait call, and I bet this will fail on the second try.
>
> Sorry for the late response. I don't really catch the race condition that
> you are referring? Are you trying to tell that the user script could run faster
> before the previous callback function actually end? Will such scenario happen?
> In the callback function, after the request_firmware_nowait(), I don't have
> any codes will delay the callback function to end. Besides, there is a mutex_lock
> protecting the request_firmware_nowait() calling. Won't that take care of the
> issue?

In callbackfn_efi_capsule, you call request_firmware_nowait.  When
that callback is invoked, I think that the
/sys/class/firmware/efi-capsule-file directory doesn't exist at all.
If the callback takes longer than it takes your script to make it
through a full iteration, then it will try uploading the second
capsule before the firmware class directory is there, so it will fail.

But I just realized that your script has a loop below to handle that.
It's this:

                 oldtime=$(date +%S)
                 oldtime=$(((time + 2) % 60))
                 until [ -f /sys/class/firmware/efi-capsule-file/loading ]
                 do
                         newtime=$(date +%S)
                         if [ $newtime -eq $oldtime ]
                         then
                                 break
                         fi
                 done

Aside from the fact that this loop itself is racy (it may loop forever
if something goes wrong in the kernel, since $newtime -eq $oldtime may
never happen), it should help, if you're lucky.  But there's another
bug.

>>
>> I think that firmware_class doesn't call the callback until after loading is closed
>> for the second time.  If so, then this is racy.  Try inserting msleep(1000) at the
>> beginning of your callback and uploading a capsule that should load
>> successfully -- this will report failure, but a future upload may get very
>> confused. Also, what does the firmware class do when simultaneous
>> uploads of the same file with different contents are in flight?  Is that possible?
>
> Sorry again, I can't really catch you on this race condition statement. Are you
> trying to tell if user is doing this:
>
> echo 1 > /sys/class/firmware/efi-capsule-file/loading
> cat capsule1 > /sys/class/firmware/efi-capsule-file/data
> cat capsule2 > /sys/class/firmware/efi-capsule-file/data
> echo 0 > /sys/class/firmware/efi-capsule-file/loading
>
> If so, capsule2 will be the one we will obtain in the callback function.

Here's the race:

User:
echo 1 > /sys/class/firmware/efi-capsule-file/loading
cat capsule1 > /sys/class/firmware/efi-capsule-file/data
echo 0 > /sys/class/firmware/efi-capsule-file/loading

Kernel: Be a little slow here due to preemption or whatever.

User:
-f /sys/class/firmware/efi-capsule-file/loading returns true
capsules_loaded == 0
Assume failure, incorrectly

Kernel: catch up and increment capsules_loaded.

If these patches get applied, then I think that the protocol needs to
be documented in Documentation/ABI.  It should say something like:

To upload an EFI capsule, do this:

Write 1 to /sys/class/firmware/efi-capsule-file/loading
Write the capsule to /sys/class/firmware/efi-capsule-file/data
Write 0 to /sys/class/firmware/efi-capsule-file/loading

Make sure that /sys/class/firmware/efi-capsule-file disappears and
comes back, perhaps by cd-ing there and waiting for all the files in
the directory to go away.

Then, and only then, read capsules_loaded to detect success.


Once you've written that doc, please seriously consider whether this
interface is justifiable.  I think it sucks.

--Andy

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Matt Fleming]

On Tue, 04 Nov, at 08:35:40AM, Andy Lutomirski wrote:
> 
> Am I missing something here?  The current proposal is missing the
> success/failure part, unless you count the loaded count (in a
> different sysfs directory) as a useful interface for that.
 
As Wilson pointed out, you only get the ability to make meaningful
success/failure declarations once you've performed the reboot.

I know of no firmware that will hot-patch itself when you call
UpdateCapsule(). A reboot is always required. Certainly that's the way
Windows will work from what I've read, which means that for x86 it's
pretty much set in stone.

Which means there's only so much info you can return to userspace once
you've handed the blob to the firmware. I don't see a huge problem with
printing things in kernel buffer, since that's how other
firmware-related things work today.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Andy Lutomirski]

On Nov 8, 2014 5:05 AM, "Matt Fleming" <matt@console-pimps.org> wrote:
>
> On Tue, 04 Nov, at 08:35:40AM, Andy Lutomirski wrote:
> >
> > Am I missing something here?  The current proposal is missing the
> > success/failure part, unless you count the loaded count (in a
> > different sysfs directory) as a useful interface for that.
>
> As Wilson pointed out, you only get the ability to make meaningful
> success/failure declarations once you've performed the reboot.
>
> I know of no firmware that will hot-patch itself when you call
> UpdateCapsule(). A reboot is always required. Certainly that's the way
> Windows will work from what I've read, which means that for x86 it's
> pretty much set in stone.

I dunno.  If nothing else, efi_capsule_update can fail due to ENOMEM.

>
> Which means there's only so much info you can return to userspace once
> you've handed the blob to the firmware. I don't see a huge problem with
> printing things in kernel buffer, since that's how other
> firmware-related things work today.

I think the kernel log is fine.  But if the code is going to report
success / failure to userspace at all, shouldn't that indication be
reliable?

TBH, I find this discussion very strange.  In summary:

me: This API is really awkward.

others: But it's using the subsystem that it should be using.

me: Then fix the subsystem?

others: The subsystem the correct choice.

me: But the API is still really awkward, and, by the way, it probably
has at least two races that user code could hit.  And, by the way, the
sample script written by the author of the patches is subject to
*both* races most likely and therefore won't work reliably.

you: Common use cases (e.g. Windows-style uses, perhaps) don't need
the features that are racy anyway.


My only response is that (a) something else might want the full
functionality and (b) Wilson's actual example script exercises the
racy code.

I think I'm done reviewing these patches.  I'll probably grumble at
the result the first time I actually try to install an EFI capsule,
though.

--Andy

P.S. What happens when a strange UEFI BIOS really wants two capsules,
and the second one will brick the machine if the first one isn't
there, and the first one failed to load but no one noticed because
there's no useful error handling?

Re: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Roy Franz]

On Sun, Nov 2, 2014 at 7:07 PM, Kweh Hock Leong
<hock.leong.kweh@intel.com> wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
>
> Introducing a kernel module to expose user helper interface for
> user to upload capsule binaries. This module leverage the
> request_firmware_nowait() to expose an interface to user.
>
> Example steps to load the capsule binary:
> 1.) echo 1 > /sys/class/firmware/efi-capsule-file/loading
> 2.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data
> 3.) echo 0 > /sys/class/firmware/efi-capsule-file/loading
>
> Whereas, this module does not allow the capsule binaries to be
> obtained from the request_firmware library search path. If any
> capsule binary loaded from firmware seach path, the module will
> stop functioning.
>
> Besides the request_firmware user helper interface, this module
> also expose a 'capsule_loaded' file note for user to verify
> the number of successfully uploaded capsule binaries. This
> file note has the read only attribute.
>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> ---
>  drivers/firmware/efi/Kconfig                   |   13 ++
>  drivers/firmware/efi/Makefile                  |    1 +
>  drivers/firmware/efi/efi-capsule-user-helper.c |  246 ++++++++++++++++++++++++
>  3 files changed, 260 insertions(+)
>  create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
>
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index f712d47..7dc814e 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
>  config EFI_ARMSTUB
>         bool
>
> +config EFI_CAPSULE_USER_HELPER
> +       tristate "EFI capsule user mode helper"
> +       depends on EFI
> +       select FW_LOADER
> +       select FW_LOADER_USER_HELPER
> +       help
> +         This option exposes the user mode helper interface for user to load
> +         EFI capsule binary and update the EFI firmware after 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..63f6910 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_USER_HELPER)  += efi-capsule-user-helper.o
> diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c b/drivers/firmware/efi/efi-capsule-user-helper.c
> new file mode 100644
> index 0000000..84a1628
> --- /dev/null
> +++ b/drivers/firmware/efi/efi-capsule-user-helper.c
> @@ -0,0 +1,246 @@
> +/*
> + * EFI capsule user mode helper interface driver.
> + *
> + * Copyright 2014 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/delay.h>
> +#include <linux/highmem.h>
> +#include <linux/slab.h>
> +#include <linux/mutex.h>
> +#include <linux/reboot.h>
> +#include <linux/efi.h>
> +#include <linux/firmware.h>
> +
> +#define CAPSULE_NAME "efi-capsule-file"
> +#define DEV_NAME "efi_capsule_user_helper"
> +#define STRING_INTEGER_MAX_LENGTH 13
> +
> +static DEFINE_MUTEX(user_helper_lock);
> +static int capsule_count;
> +static int stop_request;
> +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;
> +
> +       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;

Hi Hock,

What does the decrement of i do here?  I looked at
efi_capsule_update() and didn't see anything
that would account for this.  It looks like in this failure case one
page won't get freed.

As an aside, when I was looking at efi_update_capsule, I see that Matt
is doing very similar operations
(array of struct page pointers), but does it like this (snipped from his patch):

+ struct page **block_pgs;
...
+       block_pgs = kzalloc(nr_block_pgs * sizeof(*block_pgs), GFP_KERNEL);
+       if (!block_pgs)
+               return -ENOMEM;
+
+       for (i = 0; i < nr_block_pgs; i++) {
+               block_pgs[i] = alloc_page(GFP_KERNEL);
+               if (!block_pgs[i])
+                       goto fail;
+       }

and then can simply free the pages that are not NULL:
+fail:
+       for (i = 0; i < nr_block_pgs; i++) {
+               if (block_pgs[i])
+                       __free_page(block_pgs[i]);
+       }

I think this way is preferable since it doesn't rely on 'i' being
unchanged at the end of the function.  I also
think it would be nice if the capsule code stuck with one idiom for
dealing with arrays of page pointers.

Roy

> +               goto failed;
> +       }
> +       ++capsule_count;
> +       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;
> +}
> +
> +/*
> + * This callback function will be called by request_firmware_nowait() when
> + * user has loaded the capsule binary or aborted user helper interface
> + */
> +static void callbackfn_efi_capsule(const struct firmware *fw, void *context)
> +{
> +       int ret = 0;
> +
> +       if (fw) {
> +               /*
> +                * Binary not getting from user helper interface, fw->pages
> +                * is equal to NULL
> +                */
> +               if (!fw->pages) {
> +                       pr_err("%s: ERROR: Capsule binary '%s' at %s\n",
> +                              __func__, CAPSULE_NAME,
> +                              "firmware lib search path are not supported");
> +                       pr_err("user helper interface disabled\n");
> +                       stop_request = 1;
> +               } else {
> +                       efi_capsule_store(fw);
> +               }
> +               release_firmware(fw);
> +       }
> +
> +       mutex_lock(&user_helper_lock);
> +       if (!stop_request) {
> +               ret = request_firmware_nowait(THIS_MODULE,
> +                                             FW_ACTION_NOHOTPLUG,
> +                                             CAPSULE_NAME,
> +                                             &efi_capsule_pdev->dev,
> +                                             GFP_KERNEL, NULL,
> +                                             callbackfn_efi_capsule);
> +               if (ret) {
> +                       pr_err("%s: request_firmware_nowait() failed\n",
> +                              __func__);
> +               }
> +       }
> +       mutex_unlock(&user_helper_lock);
> +}
> +
> +static ssize_t capsule_loaded_show(struct device *dev,
> +                                  struct device_attribute *attr, char *buf)
> +{
> +       return snprintf(buf, STRING_INTEGER_MAX_LENGTH, "%d\n", capsule_count);
> +}
> +
> +static DEVICE_ATTR_RO(capsule_loaded);
> +
> +/* stop user helper interface for reboot or module unload */
> +static void capsule_stop_user_helper(void)
> +{
> +       mutex_lock(&user_helper_lock);
> +       stop_request = 1;
> +       mutex_unlock(&user_helper_lock);
> +       request_firmware_abort(CAPSULE_NAME);
> +}
> +
> +/* reboot notifier for avoid deadlock with usermode_lock */
> +static int capsule_shutdown_notify(struct notifier_block *nb,
> +                                  unsigned long sys_state,
> +                                  void *reboot_cmd)
> +{
> +       capsule_stop_user_helper();
> +       return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block capsule_shutdown_nb = {
> +       .notifier_call = capsule_shutdown_notify,
> +       /*
> +        * In order to reboot properly, it is required to ensure the priority
> +        * here is higher than firmware_class fw_shutdown_nb priority
> +        */
> +       .priority = 1,
> +};
> +
> +/*
> + * Use request_firmware_nowait() exposing an user helper interface to obtain
> + * capsule binary from user space
> + */
> +static int __init efi_capsule_user_helper_init(void)
> +{
> +       int ret;
> +
> +       register_reboot_notifier(&capsule_shutdown_nb);
> +
> +       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 check the number of binaries that
> +        * are successfully loaded
> +        */
> +       ret = device_create_file(&efi_capsule_pdev->dev,
> +                                &dev_attr_capsule_loaded);
> +       if (ret) {
> +               pr_err("%s: device_create_file() failed\n", __func__);
> +               return ret;
> +       }
> +
> +       ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_NOHOTPLUG,
> +                                     CAPSULE_NAME, &efi_capsule_pdev->dev,
> +                                     GFP_KERNEL, NULL, callbackfn_efi_capsule);
> +       if (ret) {
> +               pr_err("%s: request_firmware_nowait() failed\n", __func__);
> +               goto out;
> +       }
> +
> +       return 0;
> +
> +out:
> +       device_remove_file(&efi_capsule_pdev->dev, &dev_attr_capsule_loaded);
> +       unregister_reboot_notifier(&capsule_shutdown_nb);
> +       return ret;
> +}
> +module_init(efi_capsule_user_helper_init);
> +
> +/*
> + * rmmod app itself will stop you to remove the module if the user
> + * helper interface is still exposing. In order to remove this driver,
> + * you are require to do the command as below:
> + * rmmod -f efi_capsule_user_helper.ko
> + */
> +static void __exit efi_capsule_user_helper_exit(void)
> +{
> +       unregister_reboot_notifier(&capsule_shutdown_nb);
> +
> +       capsule_stop_user_helper();
> +       /*
> +        * synchronization is needed to make sure request_firmware is fully
> +        * aborted
> +        */
> +       while (efi_capsule_pdev->dev.kobj.kref.refcount.counter > 3)
> +               msleep(20); /* avoid busy waiting for cooperative kernel */
> +
> +       device_remove_file(&efi_capsule_pdev->dev, &dev_attr_capsule_loaded);
> +       platform_device_unregister(efi_capsule_pdev);
> +}
> +module_exit(efi_capsule_user_helper_exit);
> +
> +MODULE_DESCRIPTION("EFI Capsule user helper binary load utility");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH v2 3/3] efi: Capsule update with user helper interface

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3803 bytes --]

> -----Original Message-----
> From: Roy Franz [mailto:roy.franz@linaro.org]
> Sent: Friday, February 27, 2015 1:07 PM
> 
> On Sun, Nov 2, 2014 at 7:07 PM, Kweh Hock Leong
> > +/*
> > + * 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;
> > +
> > +       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;
> 
> Hi Hock,
> 
> What does the decrement of i do here?  I looked at
> efi_capsule_update() and didn't see anything that would account for this.  It
> looks like in this failure case one page won't get freed.
> 
> As an aside, when I was looking at efi_update_capsule, I see that Matt is
> doing very similar operations (array of struct page pointers), but does it like
> this (snipped from his patch):
> 
> + struct page **block_pgs;
> ...
> +       block_pgs = kzalloc(nr_block_pgs * sizeof(*block_pgs), GFP_KERNEL);
> +       if (!block_pgs)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < nr_block_pgs; i++) {
> +               block_pgs[i] = alloc_page(GFP_KERNEL);
> +               if (!block_pgs[i])
> +                       goto fail;
> +       }
> 
> and then can simply free the pages that are not NULL:
> +fail:
> +       for (i = 0; i < nr_block_pgs; i++) {
> +               if (block_pgs[i])
> +                       __free_page(block_pgs[i]);
> +       }
> 
> I think this way is preferable since it doesn't rely on 'i' being unchanged at the
> end of the function.  I also think it would be nice if the capsule code stuck
> with one idiom for dealing with arrays of page pointers.
> 
> Roy
> 

Hi Roy,

The reason "i" there have to perform a decrement is because a full for loop
will end with one extra incremented value if it does not break out in the middle.

And the efi_capsule_store()'s alloc page is to store the binary content and the
efi_capsule_update() from Matt is to store the efi capsule block descriptor
which will point to the binary blocks content. So, they are different.


Regards,
Wilson
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Andy Lutomirski]

On 11/02/2014 07:07 PM, Kweh Hock Leong wrote:
> From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Hi Guys,
> 
> This patchset is created on top of "efi: Capsule update support" patch:
> http://permalink.gmane.org/gmane.linux.kernel.efi/4837
> 
> It leverages the request_firmware_nowait() to expose the user helper interface for user to upload the capsule binary and calling the
> efi_capsule_update() API to pass the binary to EFI firmware.

I don't get it.  Why is the firmware interface at all reasonable for
uploading capsules?

The firmware interface makes sense for nonvolatile firmware where
hotplugging something or otherwise loading a driver needs a blob.  But
uploading an EFI capsule is an *action*, not something that should
happen transparently.  If there's an EFI firmware update available and
the user wants to install it, then the userspace tool should install it,
and it shouldn't hang around in /lib/firmware.  In fact, you shouldn't
even need /lib to be on writable media to use this.

And you most certainly don't want the EFI capsule hanging around so that
it might be accidentally installed again if the hard disk is moved.

ISTM there should be some file in sysfs to which you can write a
capsule, or perhaps a chardev and an ioctl.

--Andy

> 
> Besides build in kernel, the design also cater for build as kernel driver module. This patchset introduce a new API (request_firmware_abort()) at firmware_class so that the driver module could be unloaded by calling the API to properly stop user helper interface and release the device.
> 
> Thanks.
> 
> ---
> changelog v2:
> [PATCH 1/3]
> * use fw_lookup_buf() instead of __fw_lookup_buf() function call
> * move the fw_lookup_buf() function out of the CONFIG_PM_SLEEP block
> 
> [PATCH 2/3]
> * no change
> 
> [PATCH 3/3]
> * no change
> 
> 
> Kweh, Hock Leong (3):
>   firmware loader: Introduce new API - request_firmware_abort()
>   firmware loader: fix hung task warning dump
>   efi: Capsule update with user helper interface
> 
>  drivers/base/firmware_class.c                  |   56 ++++--
>  drivers/firmware/efi/Kconfig                   |   13 ++
>  drivers/firmware/efi/Makefile                  |    1 +
>  drivers/firmware/efi/efi-capsule-user-helper.c |  246 ++++++++++++++++++++++++
>  include/linux/firmware.h                       |    4 +
>  5 files changed, 306 insertions(+), 14 deletions(-)
>  create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
> 

Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Greg Kroah-Hartman]

On Mon, Nov 03, 2014 at 11:33:23AM -0800, Andy Lutomirski wrote:
> On 11/02/2014 07:07 PM, Kweh Hock Leong wrote:
> > From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > 
> > Hi Guys,
> > 
> > This patchset is created on top of "efi: Capsule update support" patch:
> > http://permalink.gmane.org/gmane.linux.kernel.efi/4837
> > 
> > It leverages the request_firmware_nowait() to expose the user helper interface for user to upload the capsule binary and calling the
> > efi_capsule_update() API to pass the binary to EFI firmware.
> 
> I don't get it.  Why is the firmware interface at all reasonable for
> uploading capsules?

Tradition dictates that BIOS updates go through the firmware interface,
that way you don't have to write a new userspace tool, which is a good
thing.

> The firmware interface makes sense for nonvolatile firmware where
> hotplugging something or otherwise loading a driver needs a blob.

Or BIOS data.  We've been doing it this way for a long time now.

> But uploading an EFI capsule is an *action*, not something that should
> happen transparently.  If there's an EFI firmware update available and
> the user wants to install it, then the userspace tool should install it,
> and it shouldn't hang around in /lib/firmware.  In fact, you shouldn't
> even need /lib to be on writable media to use this.

What does /lib have to do with this?

> And you most certainly don't want the EFI capsule hanging around so that
> it might be accidentally installed again if the hard disk is moved.
> 
> ISTM there should be some file in sysfs to which you can write a
> capsule, or perhaps a chardev and an ioctl.

No, just use the firmware interface please.

thanks,

greg k-h

Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Andy Lutomirski]

On Mon, Nov 3, 2014 at 1:27 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Nov 03, 2014 at 11:33:23AM -0800, Andy Lutomirski wrote:
>> On 11/02/2014 07:07 PM, Kweh Hock Leong wrote:
>> > From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> >
>> > Hi Guys,
>> >
>> > This patchset is created on top of "efi: Capsule update support" patch:
>> > http://permalink.gmane.org/gmane.linux.kernel.efi/4837
>> >
>> > It leverages the request_firmware_nowait() to expose the user helper interface for user to upload the capsule binary and calling the
>> > efi_capsule_update() API to pass the binary to EFI firmware.
>>
>> I don't get it.  Why is the firmware interface at all reasonable for
>> uploading capsules?
>
> Tradition dictates that BIOS updates go through the firmware interface,
> that way you don't have to write a new userspace tool, which is a good
> thing.
>
>> The firmware interface makes sense for nonvolatile firmware where
>> hotplugging something or otherwise loading a driver needs a blob.
>
> Or BIOS data.  We've been doing it this way for a long time now.

On what system?  Dell?

IMO this sucks from a UI point of view.  When I install wifi firmware,
I expect to stick it somewhere and have the driver find it, because
the driver knows exactly when it needs the firmware.  When I update my
BIOS, I want to click a button or type a command and update my bios.

>
>> But uploading an EFI capsule is an *action*, not something that should
>> happen transparently.  If there's an EFI firmware update available and
>> the user wants to install it, then the userspace tool should install it,
>> and it shouldn't hang around in /lib/firmware.  In fact, you shouldn't
>> even need /lib to be on writable media to use this.
>
> What does /lib have to do with this?

Where else does the file come from, given that udev no longer supports
userspace firmware loading?  Is there really some pre-existing tool
that pokes it into the sysfs firmware class thing?

Since EFI capsules are apparently on their way to becoming a
ubiquitous mechanism, I think it might be time to rethink
request_firmware for this.

--Andy

Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Greg Kroah-Hartman]

On Mon, Nov 03, 2014 at 01:32:46PM -0800, Andy Lutomirski wrote:
> On Mon, Nov 3, 2014 at 1:27 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Nov 03, 2014 at 11:33:23AM -0800, Andy Lutomirski wrote:
> >> On 11/02/2014 07:07 PM, Kweh Hock Leong wrote:
> >> > From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> >
> >> > Hi Guys,
> >> >
> >> > This patchset is created on top of "efi: Capsule update support" patch:
> >> > http://permalink.gmane.org/gmane.linux.kernel.efi/4837
> >> >
> >> > It leverages the request_firmware_nowait() to expose the user helper interface for user to upload the capsule binary and calling the
> >> > efi_capsule_update() API to pass the binary to EFI firmware.
> >>
> >> I don't get it.  Why is the firmware interface at all reasonable for
> >> uploading capsules?
> >
> > Tradition dictates that BIOS updates go through the firmware interface,
> > that way you don't have to write a new userspace tool, which is a good
> > thing.
> >
> >> The firmware interface makes sense for nonvolatile firmware where
> >> hotplugging something or otherwise loading a driver needs a blob.
> >
> > Or BIOS data.  We've been doing it this way for a long time now.
> 
> On what system?  Dell?

Yes.

> IMO this sucks from a UI point of view.  When I install wifi firmware,
> I expect to stick it somewhere and have the driver find it, because
> the driver knows exactly when it needs the firmware.  When I update my
> BIOS, I want to click a button or type a command and update my bios.

I agree, it should be "triggered" by something, not just automagically
loaded whenever the kernel randomly looks for it.

> >> But uploading an EFI capsule is an *action*, not something that should
> >> happen transparently.  If there's an EFI firmware update available and
> >> the user wants to install it, then the userspace tool should install it,
> >> and it shouldn't hang around in /lib/firmware.  In fact, you shouldn't
> >> even need /lib to be on writable media to use this.
> >
> > What does /lib have to do with this?
> 
> Where else does the file come from, given that udev no longer supports
> userspace firmware loading?  Is there really some pre-existing tool
> that pokes it into the sysfs firmware class thing?

Well, you can specify other locations than /lib/firmware/ for firmware
updates, but yes, you are right, it should be in /lib somewhere.  But
/lib doesn't need to be writable, it's a read-only file.

> Since EFI capsules are apparently on their way to becoming a
> ubiquitous mechanism, I think it might be time to rethink
> request_firmware for this.

What do you suggest instead?  A "custom" sysfs file?  What is going to
trigger it to be loaded?  A userspace script that someone else has to
write?  :)

thanks,

greg k-h

Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Andy Lutomirski]

On Mon, Nov 3, 2014 at 3:02 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Nov 03, 2014 at 01:32:46PM -0800, Andy Lutomirski wrote:
>> On Mon, Nov 3, 2014 at 1:27 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Nov 03, 2014 at 11:33:23AM -0800, Andy Lutomirski wrote:
>> >> On 11/02/2014 07:07 PM, Kweh Hock Leong wrote:
>> >> > From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> >> >
>> >> > Hi Guys,
>> >> >
>> >> > This patchset is created on top of "efi: Capsule update support" patch:
>> >> > http://permalink.gmane.org/gmane.linux.kernel.efi/4837
>> >> >
>> >> > It leverages the request_firmware_nowait() to expose the user helper interface for user to upload the capsule binary and calling the
>> >> > efi_capsule_update() API to pass the binary to EFI firmware.
>> >>
>> >> I don't get it.  Why is the firmware interface at all reasonable for
>> >> uploading capsules?
>> >
>> > Tradition dictates that BIOS updates go through the firmware interface,
>> > that way you don't have to write a new userspace tool, which is a good
>> > thing.
>> >
>> >> The firmware interface makes sense for nonvolatile firmware where
>> >> hotplugging something or otherwise loading a driver needs a blob.
>> >
>> > Or BIOS data.  We've been doing it this way for a long time now.
>>
>> On what system?  Dell?
>
> Yes.
>
>> IMO this sucks from a UI point of view.  When I install wifi firmware,
>> I expect to stick it somewhere and have the driver find it, because
>> the driver knows exactly when it needs the firmware.  When I update my
>> BIOS, I want to click a button or type a command and update my bios.
>
> I agree, it should be "triggered" by something, not just automagically
> loaded whenever the kernel randomly looks for it.
>
>> >> But uploading an EFI capsule is an *action*, not something that should
>> >> happen transparently.  If there's an EFI firmware update available and
>> >> the user wants to install it, then the userspace tool should install it,
>> >> and it shouldn't hang around in /lib/firmware.  In fact, you shouldn't
>> >> even need /lib to be on writable media to use this.
>> >
>> > What does /lib have to do with this?
>>
>> Where else does the file come from, given that udev no longer supports
>> userspace firmware loading?  Is there really some pre-existing tool
>> that pokes it into the sysfs firmware class thing?
>
> Well, you can specify other locations than /lib/firmware/ for firmware
> updates, but yes, you are right, it should be in /lib somewhere.  But
> /lib doesn't need to be writable, it's a read-only file.
>

I assume that whoever downloaded the firmware update will want to
install it, right?  I don't really expect distros to ship EFI capsules
in packages that install to /lib/firmware.  Won't there be userspace
code that either installs a capsule from some URL or uses some future
magical find-my-firmware service?

>> Since EFI capsules are apparently on their way to becoming a
>> ubiquitous mechanism, I think it might be time to rethink
>> request_firmware for this.
>
> What do you suggest instead?  A "custom" sysfs file?  What is going to
> trigger it to be loaded?  A userspace script that someone else has to
> write?  :)

Some ioctl on /dev/efi_capsule seems reasonable to me, or a new script
that uses a custom sysfs file.  Isn't the Dell thing already a rather
custom script?  You write to a custom sysfs file ("rbu_image_type", I
think) and then the handler for that file calls request_firmware.

I think that we can handle a very small C program or script that
uploads the EFI capsule.

Also, FWIW, I think that there are EFI capsules that aren't firmware
updates.  For example, IIRC there's some mechansim that allows you to
pass data to the next OS that boots via a capsule.  It's probably
buggy on every motherboard in existence, but if it ever worked, using
it through /lib/firmware would make no sense.

--Andy

Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Greg Kroah-Hartman]

On Mon, Nov 03, 2014 at 03:08:08PM -0800, Andy Lutomirski wrote:
> On Mon, Nov 3, 2014 at 3:02 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Nov 03, 2014 at 01:32:46PM -0800, Andy Lutomirski wrote:
> >> On Mon, Nov 3, 2014 at 1:27 PM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Mon, Nov 03, 2014 at 11:33:23AM -0800, Andy Lutomirski wrote:
> >> >> On 11/02/2014 07:07 PM, Kweh Hock Leong wrote:
> >> >> > From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >> >> >
> >> >> > Hi Guys,
> >> >> >
> >> >> > This patchset is created on top of "efi: Capsule update support" patch:
> >> >> > http://permalink.gmane.org/gmane.linux.kernel.efi/4837
> >> >> >
> >> >> > It leverages the request_firmware_nowait() to expose the user helper interface for user to upload the capsule binary and calling the
> >> >> > efi_capsule_update() API to pass the binary to EFI firmware.
> >> >>
> >> >> I don't get it.  Why is the firmware interface at all reasonable for
> >> >> uploading capsules?
> >> >
> >> > Tradition dictates that BIOS updates go through the firmware interface,
> >> > that way you don't have to write a new userspace tool, which is a good
> >> > thing.
> >> >
> >> >> The firmware interface makes sense for nonvolatile firmware where
> >> >> hotplugging something or otherwise loading a driver needs a blob.
> >> >
> >> > Or BIOS data.  We've been doing it this way for a long time now.
> >>
> >> On what system?  Dell?
> >
> > Yes.
> >
> >> IMO this sucks from a UI point of view.  When I install wifi firmware,
> >> I expect to stick it somewhere and have the driver find it, because
> >> the driver knows exactly when it needs the firmware.  When I update my
> >> BIOS, I want to click a button or type a command and update my bios.
> >
> > I agree, it should be "triggered" by something, not just automagically
> > loaded whenever the kernel randomly looks for it.
> >
> >> >> But uploading an EFI capsule is an *action*, not something that should
> >> >> happen transparently.  If there's an EFI firmware update available and
> >> >> the user wants to install it, then the userspace tool should install it,
> >> >> and it shouldn't hang around in /lib/firmware.  In fact, you shouldn't
> >> >> even need /lib to be on writable media to use this.
> >> >
> >> > What does /lib have to do with this?
> >>
> >> Where else does the file come from, given that udev no longer supports
> >> userspace firmware loading?  Is there really some pre-existing tool
> >> that pokes it into the sysfs firmware class thing?
> >
> > Well, you can specify other locations than /lib/firmware/ for firmware
> > updates, but yes, you are right, it should be in /lib somewhere.  But
> > /lib doesn't need to be writable, it's a read-only file.
> >
> 
> I assume that whoever downloaded the firmware update will want to
> install it, right?  I don't really expect distros to ship EFI capsules
> in packages that install to /lib/firmware.  Won't there be userspace
> code that either installs a capsule from some URL or uses some future
> magical find-my-firmware service?

Good point, I don't know.

Who ever wrote this code should know this, can someone please provide a
use-case for how this is all supposed to work?

thanks,

greg k-h

Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Andy Lutomirski]

On Mon, Nov 3, 2014 at 4:38 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Nov 03, 2014 at 03:08:08PM -0800, Andy Lutomirski wrote:
>> On Mon, Nov 3, 2014 at 3:02 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Nov 03, 2014 at 01:32:46PM -0800, Andy Lutomirski wrote:
>> >> On Mon, Nov 3, 2014 at 1:27 PM, Greg Kroah-Hartman
>> >> <gregkh@linuxfoundation.org> wrote:
>> >> > On Mon, Nov 03, 2014 at 11:33:23AM -0800, Andy Lutomirski wrote:
>> >> >> On 11/02/2014 07:07 PM, Kweh Hock Leong wrote:
>> >> >> > From: "Kweh, Hock Leong" <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> >> >> >
>> >> >> > Hi Guys,
>> >> >> >
>> >> >> > This patchset is created on top of "efi: Capsule update support" patch:
>> >> >> > http://permalink.gmane.org/gmane.linux.kernel.efi/4837
>> >> >> >
>> >> >> > It leverages the request_firmware_nowait() to expose the user helper interface for user to upload the capsule binary and calling the
>> >> >> > efi_capsule_update() API to pass the binary to EFI firmware.
>> >> >>
>> >> >> I don't get it.  Why is the firmware interface at all reasonable for
>> >> >> uploading capsules?
>> >> >
>> >> > Tradition dictates that BIOS updates go through the firmware interface,
>> >> > that way you don't have to write a new userspace tool, which is a good
>> >> > thing.
>> >> >
>> >> >> The firmware interface makes sense for nonvolatile firmware where
>> >> >> hotplugging something or otherwise loading a driver needs a blob.
>> >> >
>> >> > Or BIOS data.  We've been doing it this way for a long time now.
>> >>
>> >> On what system?  Dell?
>> >
>> > Yes.
>> >
>> >> IMO this sucks from a UI point of view.  When I install wifi firmware,
>> >> I expect to stick it somewhere and have the driver find it, because
>> >> the driver knows exactly when it needs the firmware.  When I update my
>> >> BIOS, I want to click a button or type a command and update my bios.
>> >
>> > I agree, it should be "triggered" by something, not just automagically
>> > loaded whenever the kernel randomly looks for it.
>> >
>> >> >> But uploading an EFI capsule is an *action*, not something that should
>> >> >> happen transparently.  If there's an EFI firmware update available and
>> >> >> the user wants to install it, then the userspace tool should install it,
>> >> >> and it shouldn't hang around in /lib/firmware.  In fact, you shouldn't
>> >> >> even need /lib to be on writable media to use this.
>> >> >
>> >> > What does /lib have to do with this?
>> >>
>> >> Where else does the file come from, given that udev no longer supports
>> >> userspace firmware loading?  Is there really some pre-existing tool
>> >> that pokes it into the sysfs firmware class thing?
>> >
>> > Well, you can specify other locations than /lib/firmware/ for firmware
>> > updates, but yes, you are right, it should be in /lib somewhere.  But
>> > /lib doesn't need to be writable, it's a read-only file.
>> >
>>
>> I assume that whoever downloaded the firmware update will want to
>> install it, right?  I don't really expect distros to ship EFI capsules
>> in packages that install to /lib/firmware.  Won't there be userspace
>> code that either installs a capsule from some URL or uses some future
>> magical find-my-firmware service?
>
> Good point, I don't know.
>
> Who ever wrote this code should know this, can someone please provide a
> use-case for how this is all supposed to work?

There's a partial description here:

http://download.microsoft.com/download/5/f/5/5f5d16cd-2530-4289-8019-94c6a20bed3c/windows-uefi-firmware-update-platform.docx

It looks like there may need to be a way to atomically load several
capsules in the same UpdateCapsule call so that userspace can do fancy
things like ask for an image to be displayed while the update is
applied.

--Andy

Re: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Matt Fleming]

On Mon, 2014-11-03 at 16:38 -0800, Greg Kroah-Hartman wrote:
> 
> Good point, I don't know.
> 
> Who ever wrote this code should know this, can someone please provide a
> use-case for how this is all supposed to work?

That would be Wilson. He's wanting to use this to send updates to the
firmware on the Intel Quark, I think.

RE: [PATCH v2 0/3] Enable user helper interface for efi capsule update

[Kweh, Hock Leong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 670 bytes --]

> -----Original Message-----
> From: Fleming, Matt
> Sent: Tuesday, November 04, 2014 10:08 PM
> To: Greg Kroah-Hartman
> >
> > Good point, I don't know.
> >
> > Who ever wrote this code should know this, can someone please provide
> > a use-case for how this is all supposed to work?
> 
> That would be Wilson. He's wanting to use this to send updates to the
> firmware on the Intel Quark, I think.

Hi Matt,

I believe Greg already got that from the commit message on my patch 3/3. Thanks.


Regards,
Wilson
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥