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

[Sughosh Ganu]

On Fri, 21 Mar 2025 at 14:09, Michal Simek <michal.simek at amd.com> wrote:
>
>
>
> On 3/21/25 08:40, Sughosh Ganu wrote:
> > On Wed, 5 Jun 2024 at 20:25, Michal Simek <michal.simek at amd.com> 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.
> >>
> >> Signed-off-by: Michal Simek <michal.simek at amd.com>
> >> ---
> >
> > Looks good. Just a couple of nits.
> >
> >>
> >> I am using this for some time to check various configurations that's why it
> >> can be useful for others too.
> >> Sughosh: Maybe there is better way how to dump it.
> >> ---
> >>   cmd/fwu_mdata.c | 25 +++++++++++++++++++++++++
> >>   1 file changed, 25 insertions(+)
> >>
> >> diff --git a/cmd/fwu_mdata.c b/cmd/fwu_mdata.c
> >> index 9c048d69a131..ff6435505167 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 0x%x\n", diff);
> >
> > It would be better to mention that the value being printed is the
> > length of the vendor data. Or not print the value.
>
>
> printf("Custom fields covered by CRC len: 0x%x\n", diff);
>
> >
> >> +                       print_hex_dump_bytes("CUSTOM ", DUMP_PREFIX_OFFSET,
> >> +                                            end, diff);
> >> +               }
> >
> > Would be better to do a 'select HEXDUMP if FWU_MDATA_V2' under the
> > CMD_FWU_METADATA config.
>
> I think select is too hard here because hexdump is implementing empty function
> if HEXDUMP is not enabled.
>
> imply HEXDUMP if FWU_MDATA_V2

Fine with me.

-sughosh