Re: [PATCH v6 15/25] spl: Convert ext to use spl_load

[Sean Anderson <seanga2@gmail.com>]

On 11/7/23 23:24, Simon Glass wrote:
> Hi Sean,
> 
> On Sun, 5 Nov 2023 at 19:26, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> This converts the ext load method to use spl_load. As a consequence, it
>> also adds support for FIT and IMX images.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> Changes in v6:
>> - Explicitly initialize load_info members
>>
>> Changes in v5:
>> - Rework to load header in spl_load
>>
>>   common/spl/spl_ext.c     | 36 ++++++++++++++++++------------------
>>   include/spl_load.h       |  1 +
>>   test/image/spl_load_fs.c |  9 ++++++---
>>   3 files changed, 25 insertions(+), 21 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> but nit below
> 
>>
>> diff --git a/common/spl/spl_ext.c b/common/spl/spl_ext.c
>> index af836ca15b8..d280b69c387 100644
>> --- a/common/spl/spl_ext.c
>> +++ b/common/spl/spl_ext.c
>> @@ -2,25 +2,35 @@
>>
>>   #include <common.h>
>>   #include <env.h>
>> -#include <mapmem.h>
>>   #include <part.h>
>>   #include <spl.h>
>> +#include <spl_load.h>
>>   #include <asm/u-boot.h>
>>   #include <ext4fs.h>
>>   #include <errno.h>
>>   #include <image.h>
>>
>> +static ulong spl_fit_read(struct spl_load_info *load, ulong file_offset,
>> +                         ulong size, void *buf)
>> +{
>> +       int ret;
>> +       loff_t actlen;
>> +
>> +       ret = ext4fs_read(buf, file_offset, size, &actlen);
>> +       if (ret)
>> +               return ret;
>> +       return actlen;
>> +}
>> +
>>   int spl_load_image_ext(struct spl_image_info *spl_image,
>>                         struct spl_boot_device *bootdev,
>>                         struct blk_desc *block_dev, int partition,
>>                         const char *filename)
>>   {
>>          s32 err;
>> -       struct legacy_img_hdr *header;
>> -       loff_t filelen, actlen;
>> +       loff_t filelen;
>>          struct disk_partition part_info = {};
>> -
>> -       header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
>> +       struct spl_load_info load;
>>
>>          if (part_get_info(block_dev, partition, &part_info)) {
>>                  printf("spl: no partition table found\n");
>> @@ -42,20 +52,10 @@ int spl_load_image_ext(struct spl_image_info *spl_image,
>>                  puts("spl: ext4fs_open failed\n");
>>                  goto end;
>>          }
>> -       err = ext4fs_read((char *)header, 0, sizeof(struct legacy_img_hdr), &actlen);
>> -       if (err < 0) {
>> -               puts("spl: ext4fs_read failed\n");
>> -               goto end;
>> -       }
>>
>> -       err = spl_parse_image_header(spl_image, bootdev, header);
>> -       if (err < 0) {
>> -               puts("spl: ext: failed to parse image header\n");
>> -               goto end;
>> -       }
>> -
>> -       err = ext4fs_read(map_sysmem(spl_image->load_addr, filelen), 0, filelen,
>> -                         &actlen);
>> +       spl_set_bl_len(&load, 1);
>> +       load.read = spl_fit_read;
>> +       err = spl_load(spl_image, bootdev, &load, filelen, 0);
>>
>>   end:
>>   #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>> diff --git a/include/spl_load.h b/include/spl_load.h
>> index 406f8b577b2..65aa6bb4493 100644
>> --- a/include/spl_load.h
>> +++ b/include/spl_load.h
>> @@ -95,6 +95,7 @@ static inline int _spl_load(struct spl_image_info *spl_image,
>>    * inline if there is one caller, and extern otherwise.
>>    */
>>   #define SPL_LOAD_USERS \
>> +       IS_ENABLED(CONFIG_SPL_FS_EXT4) + \
>>          0
>>
>>   #if SPL_LOAD_USERS > 1
>> diff --git a/test/image/spl_load_fs.c b/test/image/spl_load_fs.c
>> index 59d0244d44b..01559e98c4f 100644
>> --- a/test/image/spl_load_fs.c
>> +++ b/test/image/spl_load_fs.c
>> @@ -422,20 +422,23 @@ static int spl_test_mmc(struct unit_test_state *uts, const char *test_name,
>>          spl_mmc_clear_cache();
>>          spl_fat_force_reregister();
>>
>> -       if (type == LEGACY &&
>> -           spl_test_mmc_fs(uts, test_name, type, create_ext2, false))
>> +       if (spl_test_mmc_fs(uts, test_name, type, create_ext2, false))
>>                  return CMD_RET_FAILURE;
>>
>> -       if (type != IMX8 &&
>> +       if (type != IMX8 && type != LEGACY_LZMA &&
>>              spl_test_mmc_fs(uts, test_name, type, create_fat, false))
>>                  return CMD_RET_FAILURE;
> 
> This is not a command.
> 
> How about:
> 
> if (type != IMX8 && type != LEGACY_LZMA &&
>     ut_assertok(spl_test_mmc_fs(...

There are already assertions in spl_test_mmc_fs. So if we fail here, we
already have something printed on the console.

--Sean

>>
>> +       if (type == LEGACY_LZMA)
>> +               return 0;
>> +
>>          return do_spl_test_load(uts, test_name, type,
>>                                  SPL_LOAD_IMAGE_GET(0, BOOT_DEVICE_MMC1,
>>                                                     spl_mmc_load_image),
>>                                  spl_test_mmc_write_image);
>>   }
>>   SPL_IMG_TEST(spl_test_mmc, LEGACY, DM_FLAGS);
>> +SPL_IMG_TEST(spl_test_mmc, LEGACY_LZMA, DM_FLAGS);
>>   SPL_IMG_TEST(spl_test_mmc, IMX8, DM_FLAGS);
>>   SPL_IMG_TEST(spl_test_mmc, FIT_EXTERNAL, DM_FLAGS);
>>   SPL_IMG_TEST(spl_test_mmc, FIT_INTERNAL, DM_FLAGS);
>> --
>> 2.37.1
>>
> 
> Regards,
> Simon