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

["Gustavo A. R. Silva" <gustavoars@kernel.org>]

On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:

Adding Alex, Christian and DRM lists to the thread.

> Hi KSPP community,
> 
> I've been working on replacing 1-element arrays with flex array members
> on the drm/amdgpu files. I came across one insteresting case which I
> may need to pick your brains to find a solution for it.
> 
> The structure below has two fake flexible arrays but I would get an
> error if I try make them both FAM. How should/could I deal with the
> asRegIndexBuf in this case? In theory, DECLARE_FLEX_ARRAY would "work"
> but that doesn't seem to be its intended usage as far I've searched.
> (unless I got it wrong, if that's the case, feel free to set me straight)
> 
> Any ideas? 
> 
> 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].

> 
> 	/* size: 15, cachelines: 1, members: 4 */
> 	/* last cacheline: 15 bytes */
> } __attribute__((__packed__));

Alex, Christian,

It looks like this structure is only being used as a template to populate
instances of struct atom_mc_reg_table[2] and that these[3] are the only
places where asRegIndexBuf[] is being used. Apparently, this array is only
being used to retrieve it's address so that a pointer can jump[4] in chucks
of size sizeof(ATOM_INIT_REG_INDEX_FORMAT):

drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1461:
1461:	format = (ATOM_INIT_REG_INDEX_FORMAT *)
1462:		((u8 *)format + sizeof(ATOM_INIT_REG_INDEX_FORMAT));

up to (VBIOS_MC_REGISTER_ARRAY_SIZE * sizeof(ATOM_INIT_REG_INDEX_FORMAT))[5],

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.

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.

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

Thanks!
--
Gustavo

[1] https://elixir.bootlin.com/linux/latest/C/ident/asRegDataBuf
[2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c#L1441
[3] https://elixir.bootlin.com/linux/latest/C/ident/asRegIndexBuf