Messages in this thread | | | Subject | Re: [RFC PATCH 4/5] firmware_loader: Add firmware-upload support | From | Russ Weight <> | Date | Thu, 10 Feb 2022 18:29:11 -0800 |
| |
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@intel.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@intel.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@intel.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@intel.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
| |