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

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

[Michal Simek]

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) {
+		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);
+			print_hex_dump_bytes("CUSTOM ", DUMP_PREFIX_OFFSET,
+					     end, diff);
+		}
+	}
 }
 
 int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
-- 
2.43.0

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

[Michal Simek]

On 3/21/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) {
> +		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);
> +			print_hex_dump_bytes("CUSTOM ", DUMP_PREFIX_OFFSET,
> +					     end, diff);
> +		}
> +	}
>   }
>   
>   int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,

Can someone pick it up? Or should I take it?

Thanks,
Michal

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

On Tue, Apr 08, 2025 at 10:18:58AM +0200, Michal Simek wrote:
> 
> 
> On 3/21/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) {
> > +		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);
> > +			print_hex_dump_bytes("CUSTOM ", DUMP_PREFIX_OFFSET,
> > +					     end, diff);
> > +		}
> > +	}
> >   }
> >   int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,
> 
> Can someone pick it up? Or should I take it?

It's got Sughosh's tags so I'm fine with you picking it up.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Heinrich Schuchardt]

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"

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

Best regards

Heinrich

> +			print_hex_dump_bytes("CUSTOM ", DUMP_PREFIX_OFFSET,
> +					     end, diff);
> +		}
> +	}
>   }
>
>   int do_fwu_mdata_read(struct cmd_tbl *cmdtp, int flag,

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

[Michal Simek]

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