Date:   Wed, 16 Mar 2022 10:39:39 -0700
From:   Russ Weight <russell.h.weight@...el.com>
To:     Xu Yilun <yilun.xu@...el.com>
CC:     <mcgrof@...nel.org>, <gregkh@...uxfoundation.org>,
        <rafael@...nel.org>, <linux-kernel@...r.kernel.org>,
        <trix@...hat.com>, <lgoncalv@...hat.com>, <hao.wu@...el.com>,
        <matthew.gerlach@...el.com>, <basheer.ahmed.muddebihal@...el.com>,
        <tianfei.zhang@...el.com>
Subject: Re: [PATCH v1 0/8] Extend FW framework for user FW uploads



On 3/15/22 20:32, Xu Yilun wrote:
> On Tue, Mar 08, 2022 at 01:49:24PM -0800, Russ Weight wrote:
>> Extend the firmware loader subsystem to support a persistent sysfs
>> interface that userspace may use to initiate a firmware update. For
>> example, FPGA based PCIe cards automatically load firmware and FPGA images
>> from local FLASH when the card boots. The images in FLASH may be updated
>> with new images that are uploaded by the user.
>>
>> A device driver may call firmware_upload_register() to expose persistent
>> "loading" and "data" sysfs files at /sys/class/firmare/<NAME>/*. These
>> files are used in the same way as the fallback sysfs "loading" and "data"
>> files. However, when 0 is written to "loading" to complete the write of
>> firmware data, the data is also 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.
>>
>> Additional sysfs nodes are added in the same location as "loading" and
>> "data" to monitor the transfer of the image data to the device using
>> callback functions provided by the lower-level device driver and to allow
>> the data transfer to be cancelled.
>>
>> Example usage:
>>
>> $ pwd
>> /sys/class/firmware/n3000bmc-sec-update.8
> I'm good with the firmware update API, but have concern about the
> example.
>
> The n3000 bmc secure update engine is the sub device on N3000 PCIe based
> FPGA card, it could be a data xfer engine which helps to update the
> firmware of the N3000. The N3000 PCI driver knows how the firmware
> uploading affects the card.
>
> So maybe the N3000 PCI driver should register the firmware upload. But
> of course it could interact with the n3000bmc-sec-update driver for
> specific firmware upload ops.
Until now, these interfaces (for the firmware-loader) have been created
(i.e. registered) at a granularity of one interface per firmware file (e.g.
/sys/class/firmware/my-firmware-file.bin/). For secure updates, the files
are self describing, so a single interface is sufficient for various
payloads. It sounds like you are suggesting a coarser granularity that
would allow other firmware files (separate from the secure update driver)
to share a single interface for a PCI card?

- Russ
>
> Thanks,
> Yilun
>
>> $ ls
>> cancel  device  loading  remaining_size  subsystem
>> data    error   power    status          uevent
>> $ echo 1 > loading
>> $ cat /tmp/firmware.bin > data
>> $ echo 0 > loading
>> $ while :; do cat status; cat remaining_size ; sleep 3; done
>> preparing
>> 44590080
>> <--snip-->
>> transferring
>> 44459008
>> transferring
>> 44311552
>> <--snip-->
>> transferring
>> 173056
>> <--snip-->
>> programming
>> 0
>> <--snip-->
>> idle
>> 0
>> ^C
>> $ cat error
>>
>> The first two patches in this set make minor changes to enable the
>> fw_priv data structure and the sysfs interfaces to be used multiple times
>> during the existence of the device driver instance. The third patch is
>> mostly a reorganization of existing code in preparation for sharing common
>> code with the firmware-upload support. The fourth and fifth patches provide
>> the code for user-initiated firmware uploads. The final 3 patches extend
>> selftest support to test firmware-upload functionality.
>>
>>
>> Changelog RFC -> v1:
>>   - Renamed files fw_sysfs.c and fw_sysfs.h to sysfs.c and sysfs.h
>>   - Moved "MODULE_IMPORT_NS(FIRMWARE_LOADER_PRIVATE);" from sysfs.c to
>>     sysfs.h to address an error identified by the kernel test robot
>>     <lkp@...el.com>
>>   - renamed fw_upload_register() and fw_upload_unregister() to
>>     firmware_upload_register() and fw_upload_unregister().
>>   - Moved ifdef'd section of code out of firmware_loading_store() in sysfs.c
>>     into a new function, fw_upload_start(), in sysfs_upload.c.
>>   - Changed #defines to enums for error codes and progress states
>>   - Added additional kernel-doc supported symbols into the documentation.
>>     Some rewording in documentation as well.
>>   - Added module reference counting for the parent module in the
>>     firmware_upload_register() and firmware_upload_unregister() functions
>>     to fix problems found when testing with test_firmware module.
>>   - Removed unnecessary module reference counting for THIS_MODULE.
>>   - Added a new patch to modify the test_firmware module to support
>>     testing of the firmware upload mechanism.
>>   - Added a new patch to modify the test_firmware module to support
>>     error injection for firmware upload.
>>   - Added a new patch to extend the existing firmware selftests to cover
>>     firmware upload.
>>
>> Russ Weight (8):
>>   firmware_loader: Clear data and size in fw_free_paged_buf
>>   firmware_loader: Check fw_state_is_done in loading_store
>>   firmware_loader: Split sysfs support from fallback
>>   firmware_loader: Add firmware-upload support
>>   firmware_loader: Add sysfs nodes to monitor fw_upload
>>   test_firmware: Add test support for firmware upload
>>   test_firmware: Error injection for firmware upload
>>   selftests: firmware: Add firmware upload selftests
>>
>>  .../ABI/testing/sysfs-class-firmware          |  77 ++++
>>  .../driver-api/firmware/fw_upload.rst         | 117 +++++
>>  Documentation/driver-api/firmware/index.rst   |   1 +
>>  drivers/base/firmware_loader/Kconfig          |  18 +
>>  drivers/base/firmware_loader/Makefile         |   2 +
>>  drivers/base/firmware_loader/fallback.c       | 430 -----------------
>>  drivers/base/firmware_loader/fallback.h       |  46 +-
>>  drivers/base/firmware_loader/firmware.h       |  11 +
>>  drivers/base/firmware_loader/main.c           |  18 +-
>>  drivers/base/firmware_loader/sysfs.c          | 435 ++++++++++++++++++
>>  drivers/base/firmware_loader/sysfs.h          | 100 ++++
>>  drivers/base/firmware_loader/sysfs_upload.c   | 396 ++++++++++++++++
>>  drivers/base/firmware_loader/sysfs_upload.h   |  47 ++
>>  include/linux/firmware.h                      |  82 ++++
>>  lib/test_firmware.c                           | 378 +++++++++++++++
>>  tools/testing/selftests/firmware/Makefile     |   2 +-
>>  tools/testing/selftests/firmware/config       |   1 +
>>  tools/testing/selftests/firmware/fw_lib.sh    |   7 +
>>  .../selftests/firmware/fw_run_tests.sh        |   4 +
>>  tools/testing/selftests/firmware/fw_upload.sh | 214 +++++++++
>>  20 files changed, 1900 insertions(+), 486 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/sysfs.c
>>  create mode 100644 drivers/base/firmware_loader/sysfs.h
>>  create mode 100644 drivers/base/firmware_loader/sysfs_upload.c
>>  create mode 100644 drivers/base/firmware_loader/sysfs_upload.h
>>  create mode 100755 tools/testing/selftests/firmware/fw_upload.sh
>>
>> -- 
>> 2.25.1

Powered by blists - more mailing lists