Re: [PATCH v2] cmd: fwu: Dump custom fields from mdata structure

[Michal Simek <michal.simek@amd.com>]

On 4/8/25 16:14, Heinrich Schuchardt wrote:
> On 21.03.25 11:25, Michal Simek wrote:
>> The commit cb9ae40a16f0 ("tools: mkfwumdata: add logic to append vendor
>> data to the FWU metadata") added support for adding vendor data to mdata
>> structure but it is not visible anywhere that's why extend fwu command to
>> dump it.
>>
>> Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>> Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> ---
>>
>> Changes in v2:
>> - Extend print message
>> - Cover hexdump dependencies
>>
>> RFC:
>> https://lore.kernel.org/ 
>> r/75c697a4f819bb5e8649ed658c5a559fb8cd1fd9.1717599342.git.michal.simek@amd.com
>>
>> ---
>>   cmd/Kconfig     |  1 +
>>   cmd/fwu_mdata.c | 25 +++++++++++++++++++++++++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 642cc1116e87..1f8aa2521a8e 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -185,6 +185,7 @@ config CMD_UFETCH
>>   config CMD_FWU_METADATA
>>       bool "fwu metadata read"
>>       depends on FWU_MULTI_BANK_UPDATE
>> +    imply HEXDUMP if FWU_MDATA_V2
>>       help
>>         Command to read the metadata and dump it's contents
>>
>> diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
>> index 9c048d69a131..5b5a2e4d1cda 100644
>> --- a/cmd/fwu_mdata.c
>> +++ b/cmd/fwu_mdata.c
>> @@ -7,6 +7,7 @@
>>   #include <dm.h>
>>   #include <fwu.h>
>>   #include <fwu_mdata.h>
>> +#include <hexdump.h>
>>   #include <log.h>
>>   #include <stdio.h>
>>   #include <stdlib.h>
>> @@ -45,6 +46,30 @@ static void print_mdata(struct fwu_data *data)
>>                      img_info->accepted == 0x1 ? "yes" : "no");
>>           }
>>       }
>> +
>> +    if (data->version == 2) {
> 
> Should this be >= 2 ? Or do we intend to drop custom field support in
> future?
> 
>> +        struct fwu_mdata *mdata = data->fwu_mdata;
>> +        struct fwu_fw_store_desc *desc;
>> +        void *end;
>> +        u32 diff;
>> +
>> +        /*
>> +         * fwu_mdata defines only header that's why taking it as array
>> +         * which exactly point to image description location
>> +         */
>> +        desc = (struct fwu_fw_store_desc *)&mdata[1];
>> +
>> +        /* Number of entries is taken from for loop - variable i */
>> +        end = &desc->img_entry[i];
>> +        debug("mdata %p, desc %p, end %p\n", mdata, desc, end);
>> +
>> +        diff = data->metadata_size - ((void *)end - (void *)mdata);
>> +        if (diff) {
>> +            printf("Custom fields covered by CRC len: 0x%x\n", diff);
> 
> The print label is a bit hard to understand. Do you mean:
> 
> "Length of custom fields in bytes: 0x%x\n"

That's long description of len. But we need to know what actually is shown which 
is that custom fields convered by CRC.

> 
> Wouldn't print_hex_dump_bytes() already provide an address column
> conveying that information?

Length information is visible via hexdump and no issue to remove it from print 
above.

Thanks,
Michal