Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members

[Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>]

On Wed, Nov 09, 2022 at 06:45:57PM -0600, Gustavo A. R. Silva wrote:
> On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
> 
> Adding Alex, Christian and DRM lists to the thread.

Thanks so much for your reply Gustavo 
Yep, that's a good idea.

> 
> > struct _ATOM_INIT_REG_BLOCK {
> > 	USHORT                     usRegIndexTblSize;    /*     0     2 */
> > 	USHORT                     usRegDataBlkSize;     /*     2     2 */
> > 	ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1];     /*     4     3 */
> > 	ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1];  /*     7     8 */
> 
> I didn't find evidence that asRegDataBuf is used anywhere in the
> codebase[1].
> ...
> <snip> 
> ...
> As I pointed out above, I don't see asRegDataBuf[] being used in the
> codebase (of course it may describe firmware memory layout). Instead,
> there is this jump to a block of data past asRegIndexBuf[]:
> 
> drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
> 1444:	ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
> 1445:		(ATOM_MEMORY_SETTING_DATA_BLOCK *)
> 1446:		((u8 *)reg_block + (2 * sizeof(u16)) +
> 1447:			 le16_to_cpu(reg_block->usRegIndexTblSize));
> 
> So, it seems the one relevant array, from the kernel side, is
> asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
> structure... or if we could try modifying that struct and only have
> asRegIndexBuf[] as last member? and then we can transform it into a
> flex-array member.

I saw that one too. That would be the way it's currently accessing
asRegDataBuf member as the existing struct would make asRegDataBuf[0] point 
to some index within the asRegIndexBuf member (as you probably got it too)

you are right... asRegDataBuff isn't used from a static analysis
point of view but removing it make the code a bit cryptic, right?

That's pickle, ay? :-)

> 
> If for any strong reasong we cannot remove asRegDataBuf[] then I think we
> could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
> in the structure.
> 

Out of curiosity, why both rather than just asRegIndexBuf?

> But first, of course, Alex, Christian, it'd be really great if we can
> have your input and feedback. :)
> 
> Thanks!
> --
> Gustavo
> 

- Paulo A.