Re: [PATCH v1 0/8] Extend FW framework for user FW uploads

Re: [PATCH v1 0/8] Extend FW framework for user FW uploads

[Russ Weight]

Hi Yilun,

Since this discussion is specific to FPGA, I have reduced the distribution list and
have added the linux-fpga mailing list. Please see inline comments below.

On 3/16/22 18:55, Xu Yilun wrote:
> On Wed, Mar 16, 2022 at 10:39:39AM -0700, Russ Weight wrote:
>>
>> 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?
> The granularity is specific to the HW. If we could independently update
> firmware for one sub device and not impact the other component on the card,
> an interface could be registered by the sub device driver. If there is a
> firmware updating that impacts the whole card, an interface could be
> registered by the parent card driver. Alternatively, we could have a single
> interface for parent card which serves all different types of updates
> inside the card, the parent card driver handles each update request
> internally.
>
> My main concern is we should be clear about the scope of the update,
> then find the right device driver to create the interface. To avoid that
> the device is impacted by an update but its driver is unaware of it.
>
> I think registering the firmware update interface in a data transfer engine
> driver is general not a good idea. The update is not to change the data
> transfer engine itself.

Today I was looking at how to register the firmware update interface at
the PCI device level. As you know, the callbacks that perform the actual
update are part of the BMC secure-update sub-driver. I have been looking
at how to make the connection between to the two drivers.

The card interface will not always be PCI, so we can't assume that in
placing the firmware node. The only workable solution I have come up with
is to pass a register-function pointer and device handle from the card/bus
interface driver through multiple driver layers to the secure update driver.
The secure update driver would then call the function and pass back the
device handle and the secure update call-back functions.

This seems pretty messy to me. Is there another approach that is more
standard and/or less messy?

Do you think it is critical to place the firmware node at the card interface?
I understand your argument about scope, but it is the BMC driver that manages
the card. It seems acceptable to me that the firmware interface be attached
to the BMC driver.

- Russ

> Thanks, Yilun
>> - 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@intel.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