UUID kernel implementation

[keyonjie]

This is the series for UUID support -- Kernel part, it should be used together with the tplg part(thesofproject/sof#2919) and Firmware part(thesofproject/sof#2920).

This is a replacement for #2090

[kv2019i]

Wow, while I was focusing on the docs thesofproject/sof-docs#245 review, it seems there was a massive discussions on the kernel patch as well. The extended_data trick is somewhat convoluted, but I don't really see a cleaner way to get the same effect -- it seems this is the (minimal) price we pay for keeping the ABI MAJOR intact. Together with the sof-docs update, this is good to go from my part, so approving. Thanks @keyonjie !

[marc-hb]

Somewhere in #2090 I found by chance your explanation that the sizeof the extended data is not constant and not part of the ABI because it's not actually the constant sizeof(extended_data) but the variable comp.hdr.size-sizeof(comp_foo). This was your answer to a "reserved" field suggestion. It also cleared some of my confusion about what's variable and what's constant.

I think it would reduce confusion and there would be fewer memory questions (like for instance @ranj063 ) and risk of bugs if you:

• get rid of that error prone substraction. Only additions. comp.hdr.size is not the total size, it's the sizeof(comp_foo) only. Allocate ipc_size = comp.hdr.size + sizeof(extended_data).

• add near the struct declarations that precious explanation from UUID Kernel support #2090 about backward and forward compatibility and why and how it's safe for sizeof(extended data) to be different on both sides.

This would probably simplify the sanity checks in https://github.com/thesofproject/sof/pull/2920/files as well.

[keyonjie]

Somewhere in #2090 I found by chance your explanation that the sizeof the extended data is not constant and not part of the ABI because it's not actually the constant sizeof(extended_data) but the variable comp.hdr.size-sizeof(comp_foo). This was your answer to a "reserved" field suggestion. It also cleared some of my confusion about what's variable and what's constant.

I think it would reduce confusion and there would be fewer memory questions (like for instance @ranj063 ) and risk of bugs if you:

• get rid of that error prone substraction. Only additions. comp.hdr.size is not the total size, it's the sizeof(comp_foo) only. Allocate ipc_size = comp.hdr.size + sizeof(extended_data).
• add near the struct declarations that precious explanation from UUID Kernel support #2090 about backward and forward compatibility and why and how it's safe for sizeof(extended data) to be different on both sides.

This would probably simplify the sanity checks in https://github.com/thesofproject/sof/pull/2920/files as well.

comp.hdr.size is the total size, this is the alignment b/w FW and driver, see in the FW source mailbox_validate().

[marc-hb]

comp.hdr.size is the total size, this is the alignment b/w FW and driver, see in the FW source mailbox_validate().

Yes I'm suggesting not to include the extended data in comp.hdr.size, is it not possible and why not?

[plbossart]

comp.hdr.size is the total size, this is the alignment b/w FW and driver, see in the FW source mailbox_validate().

Yes I'm suggesting not to include the extended data in comp.hdr.size, is it not possible and why not?

I think it's a requirement that the firmware knows the total payload upfront.
That said, this extended data looks like a flexible array, not sure why we keep hiding it.

[marc-hb]

I think it's a requirement that the firmware knows the total payload upfront.

Based on that negative answer to @kv2019i in old #2090 about adding some reserved field and reading the current code at https://github.com/thesofproject/sof/pull/2920/files, from the firmware perspective the total size is: comp->ext_data_offset + sizeof(extended_data)

How's that not available upfront?

In fact comp->ext_data_offset is sizeof(comp_foo). If comp.hdr.size is sizeof(comp_foo) then comp->ext_data_offset could become just a flag. No duplication and simpler sanity checks.

That said, this extended data looks like a flexible array, not sure why we keep hiding it.

As far as I understand extended_data is a variable-size structure concatenated at the end of another variable-size structure (comp_foo). So I don't think you can make the former a flexible array member of the latter because the latter has a variable size too. I'm also not sure it would really make this code clearer either, for a start you'd have to cast it to your actual structure.

I see fairly large and recurring number of questions about

• reserved fields
• flexible array member
• memory allocation

All the above and how much time it took me to understand these size questions demonstrate that the design of the main feature is neither clear nor widely understood yet. This would IMHO be better discussed in one github issue or one documentation PR rather than scattered across multiple code PRs.

[plbossart]

That said, this extended data looks like a flexible array, not sure why we keep hiding it.

As far as I understand extended_data is a variable-size structure concatenated at the end of another variable-size structure (comp_foo). So I don't think you can make the former a flexible array member of the latter because the latter has a variable size too. I'm also not sure it would really make this code clearer either, for a start you'd have to cast it to your actual structure.

I don't think that's the case, we've removed all flexible array parts previously? It'd be really bad if we indeed have two parts of varying size, that is not by design and not the intention.

[marc-hb]

As far as I understand extended_data is a variable-size structure concatenated at the end of another variable-size structure (comp_foo).

I don't think that's the case, we've removed all flexible array parts previously? It'd be really bad if we indeed have two parts of varying size, that is not by design and not the intention.

It depends what you mean by "variable", which is exactly the part of the design that is not documented well enough and the source of multiple questions and confusion. You're right, nothing is variable from a narrow C compiler perspective, however:

sizeof(comp_foo) is "variable" because sizeof(comp_foo1) != sizeof(comp_foo2)

sizeof(extended_data) is variable because the whole purpose is to have backward and forward compatibility across non-matching firmware and kernel with different sizeof(extended_data), see one of @keyonjie's answers about that lost in one of the too many PRs.

[keyonjie]

@marc-hb @plbossart what we are talking here is about IPC commands, we share the exactly same header (struct sof_ipc_cmd_hdr) for all these IPC commands. Among all the different IPC commands, e.g. component new, set/get volume, trigger start/stop, the payload could be very different. What we care most here is the component new IPC commands (where they share the same header struct sof_ipc_comp). I don't think we have data[0]-liked flexible array defined here (though we have it in the TLV set/get data commands), if you ask what is variable here, back to @kv2019i 's summary, I would say they are actually like this:

COMP_COMMON_DATA (fixed, 8 Bytes struct sof_ipc_cmd_hdr for all IPC commands, 28 Bytes struct sof_ipc_comp for all component new IPCs)
COMP_SPECIFIC_DATA (variable, it is not flexible, it is still fixed size for each specific component type)
COMP_COMMON_DATA2 (this is basically fixed in specific ABI version, it vary between different version, as @marc-hb commented, the allow for backward/forward compatible will make this kind of variable to us, @lgirdwood may have different opinion to request using reserved bytes about this, the PROS for my current solution is here #2090 (comment)).

[keyonjie]

Wow, while I was focusing on the docs thesofproject/sof-docs#245 review, it seems there was a massive discussions on the kernel patch as well. The extended_data trick is somewhat convoluted, but I don't really see a cleaner way to get the same effect -- it seems this is the (minimal) price we pay for keeping the ABI MAJOR intact. Together with the sof-docs update, this is good to go from my part, so approving. Thanks @keyonjie !

Thanks @kv2019i your remained comments addressed and updated now.

[ranj063]

@keyonjie Is this change new? I dont remember seeing this before? I remember @juimonen added this specifically for the widget private data thats not the tokens. Please remove this patch from this series and submit a separate patch if you think this is really needed.

[keyonjie]

@keyonjie Is this change new? I dont remember seeing this before? I remember @juimonen added this specifically for the widget private data thats not the tokens. Please remove this patch from this series and submit a separate patch if you think this is really needed.

The github make me confuse about what change you are referring to, I think it's this?
34a79a6

Yes, that question was asked several reviewers and the commit was split out, but yes, I can move it out and submit another PR with it.

[keyonjie]

@ranj063 the separated PR is here and it is the prerequisite of this UUID PR:
#2360

no time to review this, will let others review and merge if relevant

[keyonjie]

@ranj063 can you help to merge this as Kai and Bard approved it also.

[ranj063]

@ranj063 can you help to merge this as Kai and Bard approved it also.

@keyonjie I need one of them to approve again so we have the required tags to send this upstream

[kv2019i]

Thanks @keyonjie and all reviewers, this has been a very, very long one! The one checkpatch issue relates to stdint types and is a known thing (we use C99 style types to keep definitions aligned with FW). So with that, this is ready to go, merging.