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

2014-11-03 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-03 Thread 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?

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-03 Thread Andy Lutomirski 
On 11/02/2014 07:07 PM, Kweh Hock Leong wrote:
 From: Kweh, Hock Leong 
 hock.leong.kweh-ral2jqcrhueavxtiumw...@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
 

--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-03 Thread 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-ral2jqcrhueavxtiumw...@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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-03 Thread Andy Lutomirski 
On Mon, Nov 3, 2014 at 1:27 PM, Greg Kroah-Hartman
gre...@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-ral2jqcrhueavxtiumw...@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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-03 Thread 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
 gre...@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-ral2jqcrhueavxtiumw...@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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-03 Thread 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
 gre...@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
  gre...@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-ral2jqcrhueavxtiumw...@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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-03 Thread Andy Lutomirski 
On Mon, Nov 3, 2014 at 4:38 PM, Greg Kroah-Hartman
gre...@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
 gre...@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
  gre...@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-ral2jqcrhueavxtiumw...@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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-11-03 Thread Greg Kroah-Hartman 
On Mon, Nov 03, 2014 at 11:07:10AM +0800, Kweh Hock Leong wrote:
 From: Kweh, Hock Leong hock.leong.k...@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.flem...@intel.com
 Signed-off-by: Kweh, Hock Leong hock.leong.k...@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 000..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++) {

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

2014-11-03 Thread Andy Lutomirski 
On Mon, Nov 3, 2014 at 8:32 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:
 On Mon, Nov 03, 2014 at 11:07:10AM +0800, Kweh Hock Leong wrote:
 From: Kweh, Hock Leong hock.leong.k...@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.flem...@intel.com
 Signed-off-by: Kweh, Hock Leong hock.leong.k...@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 000..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

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

2014-11-03 Thread Kweh, Hock Leong 
 -Original Message-
 From: Andy Lutomirski [mailto:l...@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

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

2014-11-03 Thread Andy Lutomirski 
On Mon, Nov 3, 2014 at 10:04 PM, Kweh, Hock Leong
hock.leong.k...@intel.com wrote:
 -Original Message-
 From: Andy Lutomirski [mailto:l...@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
--
To unsubscribe from this list: send the line unsubscribe linux-efi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html