* Re: [PATCH v3 7/8] test_firmware: Error injection for firmware upload
@ 2022-04-19 13:01 kernel test robot
0 siblings, 0 replies; 2+ messages in thread
From: kernel test robot @ 2022-04-19 13:01 UTC (permalink / raw)
To: kbuild
[-- Attachment #1: Type: text/plain, Size: 2849 bytes --]
CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220418223753.639058-8-russell.h.weight@intel.com>
References: <20220418223753.639058-8-russell.h.weight@intel.com>
TO: Russ Weight <russell.h.weight@intel.com>
TO: mcgrof(a)kernel.org
TO: rafael(a)kernel.org
TO: linux-kernel(a)vger.kernel.org
CC: trix(a)redhat.com
CC: lgoncalv(a)redhat.com
CC: yilun.xu(a)intel.com
CC: hao.wu(a)intel.com
CC: matthew.gerlach(a)linux.intel.com
CC: basheer.ahmed.muddebihal(a)intel.com
CC: tianfei.zhang(a)intel.com
CC: Russ Weight <russell.h.weight@intel.com>
Hi Russ,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on shuah-kselftest/next mcgrof/sysctl-next linus/master v5.18-rc3 next-20220419]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Russ-Weight/Extend-FW-framework-for-user-FW-uploads/20220419-064126
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git 3123109284176b1532874591f7c81f3837bbdc17
:::::: branch date: 14 hours ago
:::::: commit date: 14 hours ago
compiler: mipsel-linux-gcc (GCC) 11.2.0
reproduce (cppcheck warning):
# apt-get install cppcheck
git checkout 2cf984eba934c1a6b1bcfc317886e89180bcb5e3
cppcheck --quiet --enable=style,performance,portability --template=gcc FILE
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
cppcheck possible warnings: (new ones prefixed by >>, may not real problems)
>> lib/test_firmware.c:1071:18: warning: Uninitialized variables: tst.name, tst.buf, tst.size, tst.cancel_request, tst.inject, tst.fwl [uninitvar]
upload_release(tst);
^
vim +1071 lib/test_firmware.c
cb4d12e8d75964 Russ Weight 2022-04-18 1064
cb4d12e8d75964 Russ Weight 2022-04-18 1065 static void upload_release_all(void)
cb4d12e8d75964 Russ Weight 2022-04-18 1066 {
cb4d12e8d75964 Russ Weight 2022-04-18 1067 struct test_firmware_upload *tst, *tmp;
cb4d12e8d75964 Russ Weight 2022-04-18 1068
cb4d12e8d75964 Russ Weight 2022-04-18 1069 list_for_each_entry_safe(tst, tmp, &test_upload_list, node) {
cb4d12e8d75964 Russ Weight 2022-04-18 1070 list_del(&tst->node);
cb4d12e8d75964 Russ Weight 2022-04-18 @1071 upload_release(tst);
cb4d12e8d75964 Russ Weight 2022-04-18 1072 }
cb4d12e8d75964 Russ Weight 2022-04-18 1073 test_fw_config->upload_name = NULL;
cb4d12e8d75964 Russ Weight 2022-04-18 1074 }
cb4d12e8d75964 Russ Weight 2022-04-18 1075
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH v3 0/8] Extend FW framework for user FW uploads @ 2022-04-18 22:37 Russ Weight 2022-04-18 22:37 ` [PATCH v3 7/8] test_firmware: Error injection for firmware upload Russ Weight 0 siblings, 1 reply; 2+ messages in thread From: Russ Weight @ 2022-04-18 22:37 UTC (permalink / raw) To: mcgrof, rafael, linux-kernel Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, basheer.ahmed.muddebihal, tianfei.zhang, Russ Weight 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/secure-update.1 $ 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 v2 -> v3: - Added Reviewed-by tag - Added kdoc support for enum fw_upload_prog progress codes Changelog v1 -> v2: - Rebased to 5.18-rc2. - It was discovered that the new function in v1, fw_state_is_done(), is equivalent to the existing fw_sysfs_done() function. Renamed fw_sysfs_done() and fw_sysfs_loading() to fw_state_is_done() and fw_state_is_loading() respectively, and placed them along side companion functions in drivers/base/firmware_loader/firmware.h. - Removed the "if !fw_sysfs_done(fw_priv))" condition in switch case 1 of firmware_loading_store(). It is rendered unnecessary by other changes to the function. - Updated documentation Date and KernelVersion fields to July 2022 and 5.19. - Unconditionally set fw_priv->is_paged_buf to true in firmware_upload_register(); 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 | 126 +++++ 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 | 16 + drivers/base/firmware_loader/main.c | 18 +- drivers/base/firmware_loader/sysfs.c | 423 +++++++++++++++++ drivers/base/firmware_loader/sysfs.h | 100 ++++ drivers/base/firmware_loader/sysfs_upload.c | 397 ++++++++++++++++ drivers/base/firmware_loader/sysfs_upload.h | 54 +++ 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, 1910 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
* [PATCH v3 7/8] test_firmware: Error injection for firmware upload 2022-04-18 22:37 [PATCH v3 0/8] Extend FW framework for user FW uploads Russ Weight @ 2022-04-18 22:37 ` Russ Weight 0 siblings, 0 replies; 2+ messages in thread From: Russ Weight @ 2022-04-18 22:37 UTC (permalink / raw) To: mcgrof, rafael, linux-kernel Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, basheer.ahmed.muddebihal, tianfei.zhang, Russ Weight Add error injection capability to the test_firmware module specifically for firmware upload testing. Error injection instructions are transferred as the first part of the firmware payload. The format of an error injection string is similar to the error strings that may be read from the error sysfs node. To inject the error "programming:hw-error", one would use the error injection string "inject:programming:hw-error" as the firmware payload: $ echo 1 > loading $ echo inject:programming:hw-error > data $ echo 0 > loading $ cat status idle $ cat error programming:hw-error The first part of the error string is the progress state of the upload at the time of the error. The progress state would be one of the following: "preparing", "transferring", or "programming". The second part of the error string is one of the following: "hw-error", "timeout", "device-busy", "invalid-file-size", "read-write-error", "flash-wearout", and "user-abort". Note that all of the error strings except "user-abort" will fail without delay. The "user-abort" error will cause the firmware upload to stall at the requested progress state for up to 5 minutes to allow you to echo 1 to the cancel sysfs node. It is this cancellation that causes the 'user-abort" error. If the upload is not cancelled within the 5 minute time period, then the upload will complete without an error. Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> Signed-off-by: Russ Weight <russell.h.weight@intel.com> --- v2: - No changes from v1 v3: - Added Reviewed-by tag --- lib/test_firmware.c | 127 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 122 insertions(+), 5 deletions(-) diff --git a/lib/test_firmware.c b/lib/test_firmware.c index 2b8c56d7bf37..76115c1a2629 100644 --- a/lib/test_firmware.c +++ b/lib/test_firmware.c @@ -117,12 +117,18 @@ struct test_config { struct device *device); }; +struct upload_inject_err { + const char *prog; + enum fw_upload_err err_code; +}; + struct test_firmware_upload { char *name; struct list_head node; char *buf; size_t size; bool cancel_request; + struct upload_inject_err inject; struct fw_upload *fwl; }; @@ -1067,20 +1073,105 @@ static void upload_release_all(void) test_fw_config->upload_name = NULL; } +/* + * This table is replicated from .../firmware_loader/sysfs_upload.c + * and needs to be kept in sync. + */ +static const char * const fw_upload_err_str[] = { + [FW_UPLOAD_ERR_NONE] = "none", + [FW_UPLOAD_ERR_HW_ERROR] = "hw-error", + [FW_UPLOAD_ERR_TIMEOUT] = "timeout", + [FW_UPLOAD_ERR_CANCELED] = "user-abort", + [FW_UPLOAD_ERR_BUSY] = "device-busy", + [FW_UPLOAD_ERR_INVALID_SIZE] = "invalid-file-size", + [FW_UPLOAD_ERR_RW_ERROR] = "read-write-error", + [FW_UPLOAD_ERR_WEAROUT] = "flash-wearout", +}; + +static void upload_err_inject_error(struct test_firmware_upload *tst, + const u8 *p, const char *prog) +{ + enum fw_upload_err err; + + for (err = FW_UPLOAD_ERR_NONE + 1; err < FW_UPLOAD_ERR_MAX; err++) { + if (strncmp(p, fw_upload_err_str[err], + strlen(fw_upload_err_str[err])) == 0) { + tst->inject.prog = prog; + tst->inject.err_code = err; + return; + } + } +} + +static void upload_err_inject_prog(struct test_firmware_upload *tst, + const u8 *p) +{ + static const char * const progs[] = { + "preparing:", "transferring:", "programming:" + }; + int i; + + for (i = 0; i < ARRAY_SIZE(progs); i++) { + if (strncmp(p, progs[i], strlen(progs[i])) == 0) { + upload_err_inject_error(tst, p + strlen(progs[i]), + progs[i]); + return; + } + } +} + +#define FIVE_MINUTES_MS (5 * 60 * 1000) +static enum fw_upload_err +fw_upload_wait_on_cancel(struct test_firmware_upload *tst) +{ + int ms_delay; + + for (ms_delay = 0; ms_delay < FIVE_MINUTES_MS; ms_delay += 100) { + msleep(100); + if (tst->cancel_request) + return FW_UPLOAD_ERR_CANCELED; + } + return FW_UPLOAD_ERR_NONE; +} + static enum fw_upload_err test_fw_upload_prepare(struct fw_upload *fwl, const u8 *data, u32 size) { struct test_firmware_upload *tst = fwl->dd_handle; + enum fw_upload_err ret = FW_UPLOAD_ERR_NONE; + const char *progress = "preparing:"; tst->cancel_request = false; - if (!size || size > TEST_UPLOAD_MAX_SIZE) - return FW_UPLOAD_ERR_INVALID_SIZE; + if (!size || size > TEST_UPLOAD_MAX_SIZE) { + ret = FW_UPLOAD_ERR_INVALID_SIZE; + goto err_out; + } + + if (strncmp(data, "inject:", strlen("inject:")) == 0) + upload_err_inject_prog(tst, data + strlen("inject:")); memset(tst->buf, 0, TEST_UPLOAD_MAX_SIZE); tst->size = size; - return FW_UPLOAD_ERR_NONE; + if (tst->inject.err_code == FW_UPLOAD_ERR_NONE || + strncmp(tst->inject.prog, progress, strlen(progress)) != 0) + return FW_UPLOAD_ERR_NONE; + + if (tst->inject.err_code == FW_UPLOAD_ERR_CANCELED) + ret = fw_upload_wait_on_cancel(tst); + else + ret = tst->inject.err_code; + +err_out: + /* + * The cleanup op only executes if the prepare op succeeds. + * If the prepare op fails, it must do it's own clean-up. + */ + tst->inject.err_code = FW_UPLOAD_ERR_NONE; + tst->inject.prog = NULL; + + return ret; } static enum fw_upload_err test_fw_upload_write(struct fw_upload *fwl, @@ -1088,6 +1179,7 @@ static enum fw_upload_err test_fw_upload_write(struct fw_upload *fwl, u32 size, u32 *written) { struct test_firmware_upload *tst = fwl->dd_handle; + const char *progress = "transferring:"; u32 blk_size; if (tst->cancel_request) @@ -1097,17 +1189,33 @@ static enum fw_upload_err test_fw_upload_write(struct fw_upload *fwl, memcpy(tst->buf + offset, data + offset, blk_size); *written = blk_size; - return FW_UPLOAD_ERR_NONE; + + if (tst->inject.err_code == FW_UPLOAD_ERR_NONE || + strncmp(tst->inject.prog, progress, strlen(progress)) != 0) + return FW_UPLOAD_ERR_NONE; + + if (tst->inject.err_code == FW_UPLOAD_ERR_CANCELED) + return fw_upload_wait_on_cancel(tst); + + return tst->inject.err_code; } static enum fw_upload_err test_fw_upload_complete(struct fw_upload *fwl) { struct test_firmware_upload *tst = fwl->dd_handle; + const char *progress = "programming:"; if (tst->cancel_request) return FW_UPLOAD_ERR_CANCELED; - return FW_UPLOAD_ERR_NONE; + if (tst->inject.err_code == FW_UPLOAD_ERR_NONE || + strncmp(tst->inject.prog, progress, strlen(progress)) != 0) + return FW_UPLOAD_ERR_NONE; + + if (tst->inject.err_code == FW_UPLOAD_ERR_CANCELED) + return fw_upload_wait_on_cancel(tst); + + return tst->inject.err_code; } static void test_fw_upload_cancel(struct fw_upload *fwl) @@ -1117,11 +1225,20 @@ static void test_fw_upload_cancel(struct fw_upload *fwl) tst->cancel_request = true; } +static void test_fw_cleanup(struct fw_upload *fwl) +{ + struct test_firmware_upload *tst = fwl->dd_handle; + + tst->inject.err_code = FW_UPLOAD_ERR_NONE; + tst->inject.prog = NULL; +} + static const struct fw_upload_ops upload_test_ops = { .prepare = test_fw_upload_prepare, .write = test_fw_upload_write, .poll_complete = test_fw_upload_complete, .cancel = test_fw_upload_cancel, + .cleanup = test_fw_cleanup }; static ssize_t upload_register_store(struct device *dev, -- 2.25.1 ^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-04-19 13:01 UTC | newest] Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-19 13:01 [PATCH v3 7/8] test_firmware: Error injection for firmware upload kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2022-04-18 22:37 [PATCH v3 0/8] Extend FW framework for user FW uploads Russ Weight 2022-04-18 22:37 ` [PATCH v3 7/8] test_firmware: Error injection for firmware upload Russ Weight
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.