[BUG] ext manifest has extra element

[fredoh9]

Describe the bug
Working on thesofproject/linux#2297.
Parsing CAVS EXT MANIFEST, it is supposed to parse total 3 elems from ext_man_cavs_config_data but total 5 of elems are parsed. There are unnecessary extra elems. It causes warnings.

Should EXT_MAN_CAVS_CONFIG_LAST_ELEM be up to date with last element? like below?

/* EXT_MAN_ELEM_PLATFORM_CONFIG_DATA elements identificators */
enum cavs_config_elem_type {
	EXT_MAN_CAVS_CONFIG_LPRO	= 1,
	EXT_MAN_CAVS_CONFIG_OUTBOX_SIZE,
	EXT_MAN_CAVS_CONFIG_INBOX_SIZE,
	EXT_MAN_CAVS_CONFIG_LAST_ELEM = EXT_MAN_CAVS_CONFIG_INBOX_SIZE,	/**< keep up to date with last element */
};

/* EXT_MAN_ELEM_PLATFORM_CONFIG_DATA elements */
struct ext_man_cavs_config_data {
	struct ext_man_elem_header hdr;

	struct config_elem elems[EXT_MAN_CAVS_CONFIG_LAST_ELEM];
} __packed;

And even with this fix, first 0 is unavoidable. as we start from 1. (short log is below) one warning is always printed in kernel side.
Kernel folks are very picky about unnecessary warnings. Any suggestion about this also?

How about define uninitialized elem_type,
EXT_MAN_CAVS_CONFIG_UNINIT = 0,

To Reproduce
Need to test with thesofproject/linux#2297.

Reproduction Rate
100%

Expected behavior
Only 3 elements are parsed

Impact
What impact does this issue have on your progress (e.g., annoyance, showstopper)

Environment
SOF main branch + #2957 and linux pr 2297

Screenshots or console output

[    1.582426] sof-audio-pci 0000:00:1f.3: hdr->type: 6, hdr->size = 48, elem_num=5
[    1.582428] sof-audio-pci 0000:00:1f.3: FW clock config: LPRO
[    1.582430] sof-audio-pci 0000:00:1f.3: unsupported token type: 2 value=4096
[    1.582431] sof-audio-pci 0000:00:1f.3: unsupported token type: 3 value=8192
[    1.582433] sof-audio-pci 0000:00:1f.3: unsupported token type: 0 value=0
[    1.582434] sof-audio-pci 0000:00:1f.3: unsupported token type: 0 value=0

[fredoh9]

In second thought, I don't remember why we have to start this from 1. I kind of remember it caused some issues but I'm not sure that concern is still valid. Can we start this enum from 0 also?

EXT_MAN_CAVS_CONFIG_LPRO = 0,

[ranj063]

In second thought, I don't remember why we have to start this from 1. I kind of remember it caused some issues but I'm not sure that concern is still valid. Can we start this enum from 0 also?

EXT_MAN_CAVS_CONFIG_LPRO = 0,

It needs to start from one for the exac reason that we have static sizes array and the elems in the static array will be initialized to 0. Then you wouldnt know if it is a real element or the default element in the array?

[ranj063]

besides if you only parse the exact number of elems based on size, you wont have this problem isnt it?

[fredoh9]

Ah, I now remember the discussion why it starts from 1.

[lyakh]

In second thought, I don't remember why we have to start this from 1. I kind of remember it caused some issues but I'm not sure that concern is still valid. Can we start this enum from 0 also?
EXT_MAN_CAVS_CONFIG_LPRO = 0,

It needs to start from one for the exac reason that we have static sizes array and the elems in the static array will be initialized to 0. Then you wouldnt know if it is a real element or the default element in the array?

Aren't there also maybe some other fields that we can check to verify, that the element is valid? Or you could make the array size array[LAST - 1];? But as discussed in thesofproject/linux#2297 I don't find the idea of the size of an array being exactly equal to the current number of possible elements, good.

[ktrzcinx]

Aren't there also maybe some other fields that we can check to verify, that the element is valid?

What profit it has above just skipping zero filled elements ? Maybe we should just explicit mark EXT_MAN_CONFIG_EMPTY.

I don't find the idea of the size of an array being exactly equal to the current number of possible elements

it's rather array capacity, defined to workaround xcc problems with static flex arrays

[lgirdwood]

Parsing CAVS EXT MANIFEST, it is supposed to parse total 3 elems from ext_man_cavs_config_data but total 5 of elems are parsed. There are unnecessary extra elems. It causes warnings.

This is intentional as it means we can append new manifest elems without breaking the kernel (kernel just ignores the new feature and in this case prints a warning). The last elem ID does NOT have to be ABI aligned between FW and kernel, as long as the array elem count is correct.

[xiulipan]

@fredoh9 I also have a RFC #3527 to add a FIRST enum. This will keep minor when we add new enum.

[lgirdwood]

@fredoh9 just fyi @ktrzcinx and @xiulipan are syncing on fix for this given discussion today on call.

[github-actions]

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.