ext_manifest: define EMPTY with explicit 0

[fredoh9]

Explicitly EMPTY is added as 0 for ext manifest config data.

Signed-off-by: Fred Oh fred.oh@linux.intel.com

NO ABI change is required!
related issue: #3507
I will update linux pr thesofproject/linux#2297 shortly.

[lgirdwood]

Still not following why we need a "last" ? Surely if one side does not know about an enum then we just ignore.

[ranj063]

@lgirdwood it is used to defien the fixed length array of the elems in the config data. It isnt really needed in the kernel as the kernel can determine the number of elements based on the hdr->size and the elem size.

/** EXT_MAN_ELEM_CONFIG_DATA elements (ABI3.17) */
struct ext_man_config_data {
struct ext_man_elem_header hdr;

    struct config_elem elems[EXT_MAN_CONFIG_LAST_ELEM];

} __packed;

[fredoh9]

@lgirdwood SOF firmware header need to define the array with LAST_ELEM. This is definitely required here.
We must keep same struct/enum for kernel header also, no? For practically kernel header don't need LAST_ELEM. I agree we(kernel driver) can calculate it by hdr->size and the elem size. I kept that in kernel header to match data structure with SOF fw side.

[lgirdwood]

This will constantly need updating unless it's set at MAX_INT or we use hdr->size * elem->size.

[fredoh9]

@lgirdwood I defined MAX macro and removed LAST_ELEM. @ktrzcinx I chose 8 as initial MAX value for each extended manifest. Do you think 8 is good total number to start with?

[ktrzcinx]

I don't like the idea of handling this as constant size array from kernel side - there's no reason for that, it can be easily handled as flex array so I don't see any pros of constant size array here, only a drawback of synchronising this value.
@fredoh9 how would you like to handle older FW with lower number of elements (older one) after introducing explicit dictionary capacity in kernel?

From FW side, explicit defining capacity instead using last enum value has some pros (explicit solution) and drawbacks (manual updating size) so I think it can be done here, but as long as we use each key once or none at all, I don't see need for it.

Explicit defining EXT_MAN_CONFIG_EMPTY is change in good direction.

[fredoh9]

@ktrzcinx the only reason I'm moving to fixed size, there was concern about ABI change at every update. Kernel driver parses this with hdr information.

[fredoh9]

I need only explicit EMPTY(0) to avoid waring at the moment. I can only add EMPTY=0 for this PR. I did diverse review unnecessarily.

[lgirdwood]

@ktrzcinx @fredoh9 just to confirm any trailing space after this table is 0 (meaning legacy works). @kv2019i how has the kernel dealt with this in the past, is this solution backwards compatible for you ?

[ktrzcinx]

@ktrzcinx @fredoh9 just to confirm any trailing space after this table is 0 (meaning legacy works)

yes, it's defined in linker script and may be change to any other value if there will be such a need in the future.
Example:

xt-objdump -sj ".fw_metadata" ./src/arch/xtensa/sof-cnl
...
 22000190 00000000 00000000 dec0dec0 ffeedec0  ................
 220001a0 06000000 30000000 02000000 00100000  ....0...........   <- Extended manifest found module, type: 0x0006 size: 0x0030 (  48) 
 220001b0 03000000 00200000 00000000 00000000  ..... ..........
 220001c0 00000000 00000000 dec0dec0 ffeedec0  ................
 220001d0 00000000 50000000 3c000000 01000500  ....P...<.......   <- Extended manifest found module, type: 0x0000 size: 0x0050 (  80)
...

[paulstelian97]

Is this still needed?

[fredoh9]

No we don't need this any more.