Hi Masahisa, On Wed, 19 Apr 2023 at 23:17, Masahisa Kojima <masahisa.koj...@linaro.org> wrote: > > Hi Simon, > > On Wed, 19 Apr 2023 at 10:47, Simon Glass <s...@chromium.org> wrote: > > > > Hi Masahisa, > > > > On Mon, 10 Apr 2023 at 03:07, Masahisa Kojima > > <masahisa.koj...@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.koj...@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.stgit@localhost/ > > > > > 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. > ===
Let's agree for the moment that the spec actually requires a reboot to initial the update. For a test, we can split it into three parts: - setting up the update - triggering the update - processing the update We can write separate tests for each of these: - test that the update is set up correctly - test that the update is triggered on a reboot, if one is present - test that the update is processed correctly There is no need to bring them all together. > > > > > 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. OK great. > > > > > 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. Actually almost all the test code can go in C. See here for the trade-offs [1]. Regards, Simon [1] https://u-boot.readthedocs.io/en/latest/develop/tests_writing.html