Date:   Thu, 10 Feb 2022 18:29:11 -0800
From:   Russ Weight <russell.h.weight@...el.com>
To:     Luis Chamberlain <mcgrof@...nel.org>
CC:     <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
        <linux-kernel@...r.kernel.org>, <trix@...hat.com>,
        <lgoncalv@...hat.com>, <yilun.xu@...el.com>, <hao.wu@...el.com>,
        <matthew.gerlach@...el.com>, <basheer.ahmed.muddebihal@...el.com>,
        <tianfei.zhang@...el.com>
Subject: Re: [RFC PATCH 4/5] firmware_loader: Add firmware-upload support



On 2/3/22 2:59 PM, Luis Chamberlain wrote:
> On Thu, Feb 03, 2022 at 02:30:51PM -0700, Russ Weight wrote:
>> Extend the firmware subsystem to support a persistent sysfs interface that
>> userspace may use to initiate a firmware update. For example, FPGA based
>> PCIe cards load firmware and FPGA images from local FLASH when the card
>> boots. The images in FLASH may be updated with new images provided by the
>> user at his/her convenience.
>>
>> A device driver may call fw_upload_register() to expose persistent
>> "loading" and "data" sysfs files. These files are used in the same way as
>> the fallback sysfs "loading" and "data" files. When 0 is written to
>> "loading" to complete the write of firmware data, the data is transferred
>> to the lower-level driver using pre-registered call-back functions. The
>> data transfer is done in the context of a kernel worker thread.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
>> ---
>>  .../ABI/testing/sysfs-class-firmware          |  32 +++
>>  .../driver-api/firmware/fw_upload.rst         |  86 +++++++
>>  Documentation/driver-api/firmware/index.rst   |   1 +
>>  drivers/base/firmware_loader/Kconfig          |  14 ++
>>  drivers/base/firmware_loader/Makefile         |   1 +
>>  drivers/base/firmware_loader/firmware.h       |   6 +
>>  drivers/base/firmware_loader/fw_sysfs.c       |  50 +++-
>>  drivers/base/firmware_loader/fw_sysfs.h       |   4 +
>>  drivers/base/firmware_loader/fw_upload.c      | 229 ++++++++++++++++++
>>  drivers/base/firmware_loader/fw_upload.h      |  24 ++
>>  drivers/base/firmware_loader/main.c           |  16 +-
>>  include/linux/firmware.h                      |  72 ++++++
>>  12 files changed, 523 insertions(+), 12 deletions(-)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-firmware
>>  create mode 100644 Documentation/driver-api/firmware/fw_upload.rst
>>  create mode 100644 drivers/base/firmware_loader/fw_upload.c
>>  create mode 100644 drivers/base/firmware_loader/fw_upload.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-firmware b/Documentation/ABI/testing/sysfs-class-firmware
>> new file mode 100644
>> index 000000000000..a2e518f0bf8a
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-firmware
>> @@ -0,0 +1,32 @@
>> +What: 		/sys/class/firmware/.../data
>> +Date:		Mar 2022
>> +KernelVersion:	5.18
>> +Contact:	Russ Weight <russell.h.weight@...el.com>
>> +Description:	The data sysfs file is used for firmware-fallback and for
>> +		firmware uploads. Cat a firmware image to this sysfs file
>> +		after you echo 1 to the loading sysfs file. When the firmware
>> +		image write is complete, echo 0 to the loading sysfs file. This
>> +		sequence will signal the completion of the firmware write and
>> +		signal the lower-level driver that the firmware data is
>> +		available.
>> +
>> +What: 		/sys/class/firmware/.../loading
>> +Date:		Mar 2022
>> +KernelVersion:	5.18
>> +Contact:	Russ Weight <russell.h.weight@...el.com>
>> +Description:	The loading sysfs file is used for both firmware-fallback and
>> +		for firmware uploads. Echo 1 onto the loading file to indicate
>> +		you are writing a firmware file to the data sysfs node. Echo
>> +		-1 onto this file to abort the data write or echo 0 onto this
>> +		file to indicate that the write is complete. For firmware
>> +		uploads, the zero value also triggers the transfer of the
>> +		firmware data to the lower-level device driver.
>> +
>> +What: 		/sys/class/firmware/.../timeout
>> +Date:		Mar 2022
>> +KernelVersion:	5.18
>> +Contact:	Russ Weight <russell.h.weight@...el.com>
>> +Description:	This file supports the timeout mechanism for firmware
>> +		fallback.  This file has no affect on firmware uploads. For
>> +		more information on timeouts please see the documentation
>> +		for firmware fallback.
>> diff --git a/Documentation/driver-api/firmware/fw_upload.rst b/Documentation/driver-api/firmware/fw_upload.rst
>> new file mode 100644
>> index 000000000000..bf272f627a1f
>> --- /dev/null
>> +++ b/Documentation/driver-api/firmware/fw_upload.rst
>> @@ -0,0 +1,86 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=============
>> +fw_upload API
>> +=============
>> +
>> +A device driver that registers with the firmware loader will expose
>> +persistent sysfs nodes to enable users to initiate firmware updates for
>> +that device.  It is the responsibility of the device driver and/or the
>> +device itself to perform any validation on the data received. Firmware
>> +upload uses the same *loading* and *data* sysfs files described in the
>> +documentation for firmware fallback.
>> +
>> +Register for firmware upload
>> +============================
>> +
>> +A device driver registers for firmware upload by calling fw_upload_register().
>> +Among the parameter list is a name to identify the device under
>> +/sys/class/firmware. A user may initiate a firmware upload by echoing
>> +a 1 to the *loading* sysfs file for the target device. Next, the user writes
>> +the firmware image to the *data* sysfs file. After writing the firmware
>> +data, the user echos 0 to the *loading* sysfs file to signal completion.
>> +Echoing 0 to *loading* also triggers the transfer of the firmware to the
>> +lower-lever device driver in the context of a kernel worker thread.
>> +
>> +To use the fw_upload API, write a driver that implements a set of ops. The
>> +probe function calls fw_upload_register() and the remove function calls
>> +fw_upload_unregister() such as::
>> +
>> +	static const struct fw_upload_ops m10bmc_ops = {
>> +		.prepare = m10bmc_sec_prepare,
>> +		.write = m10bmc_sec_write,
>> +		.poll_complete = m10bmc_sec_poll_complete,
>> +		.cancel = m10bmc_sec_cancel,
>> +		.cleanup = m10bmc_sec_cleanup,
>> +	};
>> +
>> +	static int m10bmc_sec_probe(struct platform_device *pdev)
>> +	{
>> +		const char *fw_name, *truncate;
>> +		struct m10bmc_sec *sec;
>> +		struct fw_upload *fwl;
>> +		unsigned int len;
>> +
>> +		sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
>> +		if (!sec)
>> +			return -ENOMEM;
>> +
>> +		sec->dev = &pdev->dev;
>> +		sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
>> +		dev_set_drvdata(&pdev->dev, sec);
>> +
>> +		fw_name = dev_name(sec->dev);
>> +		truncate = strstr(fw_name, ".auto");
>> +		len = (truncate) ? truncate - fw_name : strlen(fw_name);
>> +		sec->fw_name = kmemdup_nul(fw_name, len, GFP_KERNEL);
>> +
>> +		fwl = fw_upload_register(sec->dev, sec->fw_name, &m10bmc_ops, sec);
>> +		if (IS_ERR(fwl)) {
>> +			dev_err(sec->dev, "Firmware Upload driver failed to start\n");
>> +			kfree(sec->fw_name);
>> +			return PTR_ERR(fwl);
>> +		}
>> +
>> +		sec->fwl = fwl;
>> +		return 0;
>> +	}
>> +
>> +	static int m10bmc_sec_remove(struct platform_device *pdev)
>> +	{
>> +		struct m10bmc_sec *sec = dev_get_drvdata(&pdev->dev);
>> +
>> +		fw_upload_unregister(sec->fwl);
>> +		kfree(sec->fw_name);
>> +		return 0;
>> +	}
>> +
>> +fw_upload_register
>> +------------------
>> +.. kernel-doc:: drivers/base/firmware_loader/fw_upload.c
>> +   :functions: fw_upload_register
>> +
>> +fw_upload_unregister
>> +--------------------
>> +.. kernel-doc:: drivers/base/firmware_loader/fw_upload.c
>> +   :functions: fw_upload_unregister
>> diff --git a/Documentation/driver-api/firmware/index.rst b/Documentation/driver-api/firmware/index.rst
>> index 57415d657173..9d2c19dc8e36 100644
>> --- a/Documentation/driver-api/firmware/index.rst
>> +++ b/Documentation/driver-api/firmware/index.rst
>> @@ -8,6 +8,7 @@ Linux Firmware API
>>     core
>>     efi/index
>>     request_firmware
>> +   fw_upload
>>     other_interfaces
>>  
>>  .. only::  subproject and html
>> diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig
>> index 1bfe18900ed5..cee662f3277b 100644
>> --- a/drivers/base/firmware_loader/Kconfig
>> +++ b/drivers/base/firmware_loader/Kconfig
>> @@ -185,5 +185,19 @@ config FW_CACHE
>>  
>>  	  If unsure, say Y.
>>  
>> +config FW_UPLOAD
>> +	bool "Enable users to initiate firmware updates using sysfs"
>> +	select FW_LOADER_SYSFS
>> +	select FW_LOADER_PAGED_BUF
>> +	help
>> +	  Enabling this option will allow device drivers to expose a persistent
>> +	  sysfs interface that allows firmware updates to be initiated from
>> +	  userspace. For example, FPGA based PCIe cards load firmware and FPGA
>> +	  images from local FLASH when the card boots. The images in FLASH may
>> +	  be updated with new images provided by the user. Enable this device
>> +	  to support cards that rely on user-initiated updates for firmware files.
>> +
>> +	  If unsure, say N.
>> +
>>  endif # FW_LOADER
>>  endmenu
>> diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
>> index 787c833d0c6e..52ef64bd9357 100644
>> --- a/drivers/base/firmware_loader/Makefile
>> +++ b/drivers/base/firmware_loader/Makefile
>> @@ -7,5 +7,6 @@ firmware_class-objs := main.o
>>  firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
>>  firmware_class-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += fallback_platform.o
>>  firmware_class-$(CONFIG_FW_LOADER_SYSFS) += fw_sysfs.o
>> +firmware_class-$(CONFIG_FW_UPLOAD) += fw_upload.o
>>  
>>  obj-y += builtin/
>> diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
>> index 58768d16f8df..4019f9423de8 100644
>> --- a/drivers/base/firmware_loader/firmware.h
>> +++ b/drivers/base/firmware_loader/firmware.h
>> @@ -87,6 +87,7 @@ struct fw_priv {
>>  };
>>  
>>  extern struct mutex fw_lock;
>> +extern struct firmware_cache fw_cache;
>>  
>>  static inline bool __fw_state_check(struct fw_priv *fw_priv,
>>  				    enum fw_status status)
>> @@ -154,7 +155,12 @@ static inline bool fw_state_is_done(struct fw_priv *fw_priv)
>>  	return __fw_state_check(fw_priv, FW_STATUS_DONE);
>>  }
>>  
>> +int alloc_lookup_fw_priv(const char *fw_name, struct firmware_cache *fwc,
>> +			 struct fw_priv **fw_priv, void *dbuf, size_t size,
>> +			 size_t offset, u32 opt_flags);
>>  int assign_fw(struct firmware *fw, struct device *device);
>> +void free_fw_priv(struct fw_priv *fw_priv);
>> +void fw_state_init(struct fw_priv *fw_priv);
>>  
>>  #ifdef CONFIG_FW_LOADER
>>  bool firmware_is_builtin(const struct firmware *fw);
>> diff --git a/drivers/base/firmware_loader/fw_sysfs.c b/drivers/base/firmware_loader/fw_sysfs.c
>> index 70cb1d67ffb2..9b0cd37c81df 100644
>> --- a/drivers/base/firmware_loader/fw_sysfs.c
>> +++ b/drivers/base/firmware_loader/fw_sysfs.c
>> @@ -6,8 +6,8 @@
>>  #include <linux/slab.h>
>>  #include <linux/types.h>
>>  
>> -#include "firmware.h"
>>  #include "fw_sysfs.h"
>> +#include "fw_upload.h"
>>  
>>  /*
>>   * sysfs support for firmware loader
>> @@ -80,6 +80,10 @@ static void fw_dev_release(struct device *dev)
>>  {
>>  	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
>>  
>> +	if (fw_sysfs->fw_upload_priv) {
>> +		free_fw_priv(fw_sysfs->fw_priv);
>> +		kfree(fw_sysfs->fw_upload_priv);
>> +	}
>>  	kfree(fw_sysfs);
>>  }
>>  
>> @@ -165,6 +169,9 @@ static ssize_t firmware_loading_store(struct device *dev,
>>  				      const char *buf, size_t count)
>>  {
>>  	struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
>> +#ifdef CONFIG_FW_UPLOAD
>> +	struct fw_upload_priv *fwlp;
>> +#endif
>>  	struct fw_priv *fw_priv;
>>  	ssize_t written = count;
>>  	int loading = simple_strtol(buf, NULL, 10);
>> @@ -211,6 +218,42 @@ static ssize_t firmware_loading_store(struct device *dev,
>>  				written = rc;
>>  			} else {
>>  				fw_state_done(fw_priv);
>> +
>> +#ifdef CONFIG_FW_UPLOAD
>> +				/*
>> +				 * For fw_uploads, start a worker thread to upload
>> +				 * data to the parent driver.
>> +				 */
>> +				if (!fw_sysfs->fw_upload_priv)
>> +					break;
>> +
>> +				if (!fw_priv->size) {
>> +					fw_free_paged_buf(fw_priv);
>> +					fw_state_init(fw_sysfs->fw_priv);
>> +					break;
>> +				}
>> +
>> +				fwlp = fw_sysfs->fw_upload_priv;
>> +				mutex_lock(&fwlp->lock);
>> +
>> +				/* Do not interfere an on-going fw_upload */
>> +				if (fwlp->progress != FW_UPLOAD_PROG_IDLE) {
>> +					mutex_unlock(&fwlp->lock);
>> +					written = -EBUSY;
>> +					goto out;
>> +				}
>> +
>> +				fwlp->progress = FW_UPLOAD_PROG_RECEIVING;
>> +				fwlp->err_code = 0;
>> +				fwlp->remaining_size = fw_priv->size;
>> +				fwlp->data = fw_priv->data;
>> +				pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
>> +					 __func__, fw_priv->fw_name,
>> +					 fw_priv, fw_priv->data,
>> +					 (unsigned int)fw_priv->size);
>> +				queue_work(system_long_wq, &fwlp->work);
>> +				mutex_unlock(&fwlp->lock);
>> +#endif
> If you're going to split this work up it'd be nice to avoid to have
> #ifdef stuff here.
Yes - I'll encapsulate the code into another function and move it
to the sysfs-upload.c file.

>
>> diff --git a/drivers/base/firmware_loader/fw_upload.c b/drivers/base/firmware_loader/fw_upload.c
> No need for fw prefix, sysfs-upload.[ch] would make it clearer.
OK - I'll switch to sysfs-upload.[ch]
>
>> +EXPORT_SYMBOL_GPL(fw_upload_register);
> firmware_upload_register()
>
>> +EXPORT_SYMBOL_GPL(fw_upload_unregister);
> firmware_upload_unregister()
Sure - I'll change the register/unregister function names.
>
>> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
>> index 3b057dfc8284..9b109f8ff627 100644
>> --- a/include/linux/firmware.h
>> +++ b/include/linux/firmware.h
>> @@ -17,6 +17,56 @@ struct firmware {
>>  	void *priv;
>>  };
>>  
>
>> +/* Update progress codes */
>> +#define FW_UPLOAD_PROG_IDLE		0
>> +#define FW_UPLOAD_PROG_RECEIVING	1
>> +#define FW_UPLOAD_PROG_PREPARING	2
>> +#define FW_UPLOAD_PROG_TRANSFERRING	3
>> +#define FW_UPLOAD_PROG_PROGRAMMING	4
>> +#define FW_UPLOAD_PROG_MAX		5
> enums with kdoc allows you to nicely document these as well.
OK. I'll switch to enums. I also realised that the FW_UPLOAD_PROG*
constants can be defined in a smaller scope. I'll move these to
sysfs-upload.h.

>> +
>> +/* Update error progress codes */
>> +#define FW_UPLOAD_ERR_HW_ERROR		1
>> +#define FW_UPLOAD_ERR_TIMEOUT		2
>> +#define FW_UPLOAD_ERR_CANCELED		3
>> +#define FW_UPLOAD_ERR_BUSY		4
>> +#define FW_UPLOAD_ERR_INVALID_SIZE	5
>> +#define FW_UPLOAD_ERR_RW_ERROR		6
>> +#define FW_UPLOAD_ERR_WEAROUT		7
>> +#define FW_UPLOAD_ERR_MAX		8
> enums with kdoc allows you to nicely document these as well.
Thanks,
- Russ
>
>   Luis

Powered by blists - more mailing lists