[PATCH v4 3/6] tools: Add mkfwumdata tool for FWU metadata image

[Jassi Brar]

On Mon, 17 Apr 2023 at 09:29, Michal Simek <michal.simek at amd.com> wrote:
>
>
>
> On 4/17/23 15:48, Jassi Brar wrote:
> > On Mon, 17 Apr 2023 at 01:38, Michal Simek <michal.simek at amd.com> wrote:
> >> On 4/14/23 17:02, Jassi Brar wrote:
> >
> >>>>>>> +
> >>>>>>> +/* This will dynamically allocate the fwu_mdata */
> >>>>>>> +#define CONFIG_FWU_NUM_BANKS         0
> >>>>>>> +#define CONFIG_FWU_NUM_IMAGES_PER_BANK       0
> >>>>>>
> >>>>>> These two are completely unused.
> >>>>>>
> >>>>> these are necessary for include/fwu_mdata.h that comes later.
> >>>>
> >>>> If that's come later, add it later.
> >>>>
> >>> Can you please actually look at the code to see the underlying constraint?
> >>
> >> Ok. I misunderstand your comment.
> >>
> >> You have it there because of #include <fwu_mdata.h> and using macros there.
> >> Can you just allocated that structures at run time instead of statically defined
> >> them via array? That would clean that design and also fix checkpatch issue.
> >>
> > I can't see how dynamically allocating an array of packed structures
> > inside an array of packed structures , makes the design any cleaner.
>
> It is not marked as __packed and based on what you are saying it should be
> marked like that.
>
Those structures already existed before my patchset. And I definitely
remember they were suggested to be marked as packed but were not. So
now you have to read to code to understand what they are.

> >
> > I think this is a valid case where we can ignore the checkpatch
> > warning because we know what we are doing.
>
> I will let maintainer, who will merge this code, to decide on this.
> I would at least make that comment much bigger to explain it better
> that you are doing it because you don't want to redefine structures from
> fwu_mdata.h.
>
you mean redeclare, right?  That is what the two define's do effectively.

-j