Hi Simon,
On Wed, 19 Apr 2023 at 10:47, Simon Glass sjg@chromium.org wrote:
Hi Masahisa,
On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima masahisa.kojima@linaro.org wrote:
This test covers FMP versioning for both raw and FIT image, and both signed and non-signed capsule update.
Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org
Changes in v5:
- get aligned to the device tree based versioning
Newly created in v4
test/py/tests/test_efi_capsule/conftest.py | 73 +++++++ .../test_capsule_firmware_fit.py | 187 ++++++++++++++++ .../test_capsule_firmware_raw.py | 201 ++++++++++++++++++ .../test_capsule_firmware_signed_fit.py | 165 ++++++++++++++ .../test_capsule_firmware_signed_raw.py | 169 +++++++++++++++ test/py/tests/test_efi_capsule/version.dts | 27 +++ 6 files changed, 822 insertions(+) create mode 100644 test/py/tests/test_efi_capsule/version.dts
I hate to say this, but we must fix the reboot stuff here before adding more code.
I read the previous discussion. https://lore.kernel.org/u-boot/164388018493.446835.11931101380744085380.stgi...
The test needs to tell U-Boot to do the update (e.g. via a command), then continue. It should not reboot sandbox.I have said this before but the feedback was not understood or taken. If you need help designing this, please let me know. In fact I am happy to create a patch for one of the tests if that is what it takes to convey this.
Current efi capsule update test tries to verify the following behavior stated in the UEFI spec v2.10, so the reboot is required to trigger the capsule update.
=== quote from UEFI v2.10 P.243 8.5.5 Delivery of Capsules via file on Mass Storage Device
As an alternative to the UpdateCapsule() runtime API, capsules of any type supported by platform may also be delivered to firmware via a file within the EFI system partition on the mass storage device targeted for boot. Capsules staged using this method are processed on the next system restart. This method is only available when booting from mass storage devices which are formatted with GPT and contain an EFI System Partition in the device image. System firmware will search for capsule when EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED bit in OsIndications is set as described in Exchanging information between the OS and Firmware. ===
Also, while it is normal to have a fair bit of duplication in tests, just to make them easier to read, it does make them hard to m
aintain,
and the duplication here seems very large.
We have the same code repeated but with different code style or indentation. Sorry, but this is not setting us up for the future, in terms of maintaining this code. Just search for 'env set -e -nv -bs -rt OsIndications =0x0000000000000004' for example.
The common code needs to go in functions in a separate Python file, as we have done for other tests. The goal should be to use the same code for the same functions. Sure there will be some duplication, but it is too much.
These fixes should happen before we accept any more non=trivial patches to this code.
I agree, I will try to create common functions to reduce code duplication.
BTW I do wonder whether the test would be better written mostly in C, since from what I can see, it mostly just runs commands. You can still have a Python wrapper - see for example how test_vbe works. It uses 'ut xxx -f' to execute the C part of a Python test. You can do setup in Python, then run the C test. What do you think?
It is just my opinion, if the test requires some setup in Python, I like Python to write the test cases as far as tests can be implemented in Python, because we can maintain the test code in one place.
Thanks, Masahisa Kojima
Regards, Simon