Global UUID registry, cleanup, simplification #9261
Conversation
andyross
requested review from
akloniex,
marc-hb,
kv2019i,
lyakh,
iuliana-prodan,
dbaluta,
abonislawski,
ranj063,
bkokoszx,
pblaszko,
marcinszkudlinski,
LaurentiuM1234,
bardliao,
singalsu,
yaochunhung,
kuanhsuncheng,
bhiregoudar,
sunilkumardommati,
a team,
tlissows,
fkwasowi,
iganakov,
RanderWang,
lgirdwood,
plbossart,
mmaka1 and
lbetlej
as code owners
|
Heh, look at all the reviewers. Tree-wide changes add basically everyone, I guess. FWIW I don't expect this to progress that quickly, it'll likely take a while to duplicate and resolve all the CI failures I haven't run locally. And as mentioned there's a Zephyr PR that needs to merge first anyway. Please review though. |
src/include/sof/lib/uuid.h
Outdated
| * \param uuid_name UUID symbol name declared with DECLARE_SOF_UUID() or | ||
| * DECLARE_SOF_RT_UUID(). | ||
| */ | ||
| #define SOF_UUID(uuid_name) (&(uuid_name ## _ldc)) |
There was a problem hiding this comment.
Oops, squashme patch wasn't and got left in, will fix
Very happy you're volunteering to fix all the tests and infrastructure. We need more people like you! For a start, there is a lot that can/should be done in https://github.com/thesofproject/sof-test - which you already know since you've been using it regularly? |
Yes you do for simple testing purposes. It's called "Continuous" Integration.
There are multiple levels of reviews. There is (at least):
This giant PR successfully provides the first one. Smaller PRs solve the second one. |
The sof-logger/smex tools and the .ldc formats are all dead code in Zephyr builds. The logging/trace macros don't emit the correct metadata anymore, using Zephry logging instead (and I don't think they ever did?), and the resulting .ldc files are degenerate containing just the header and the records for component UUIDs, which nothing uses. Save a few milliseconds of build time and a few bytes of output, and free up the evolution path by not having to support legacy tools. Signed-off-by: Andy Ross <[email protected]>
Stumbled on this. Too hard to fix with the existing macros and very low priority given existing DSP architectures, but IMHO important to note. Also remove the spurious extra zero byte from the entity_name initializer. String literals are already null-terminated and string initializers are already defined to zero-fill the trailing bytes. This is a noop, except that it will cause the compiler to emit a string length overflow warning one byte earlier than necessary. Signed-off-by: Andy Ross <[email protected]>
andyross
force-pushed
the
uuid-registry
branch
2 times, most recently
from
a9d2117
to
25cd28c
Compare
DECLARE_SOF_UUID() and the newer DECLARE_SOF_RT_UUID() were doing exactly the same thing. They create a sof_uuid_entry struct to link into the ".static_uuids" section, which is treated specially by the linker and not included in the firmware binary. The only difference is the "_RT_" function also create a duplicate of that struct that lives in .rodata to which it assigns the same symbol name as DECLARE_SOF_UUID() uses (and creates an internal "_ldc" symbol for the hidden struct) that is usable at runtime (hence the name). But SOF is linked with -fdata-sections, so the extra struct will be dropped at link time anyway if unused. And the symbol name is identical and visible in both variants even though it's only legally used in one. There's no reason to carry two different APIs around, unify. Signed-off-by: Andy Ross <[email protected]>
Complete the unification of the diverged UUID APIs with a big rename. Call it "DEFINE" instead of "DECLARE" since this is in fact a C struct definition and not just a declaration of a type or extern symbol. Signed-off-by: Andy Ross <[email protected]>
Zephyr has a similar trick to the .static_uuids trick SOF had been playing that works in a (mostly) portable way and has some nice features like runtime FOREACH iteration. Use that for the UUID table, allowing it to be linked in globally in Zephyr builds in a uniform way. Also includes some general cleanup to try to reduce the amount of backslashery required to express the various macros. Signed-off-by: Andy Ross <[email protected]>
UUIDs are defined with both a string name (used mostly just for trace
output on legacy xtos builds) and a symbol name used as a global
variable to tie the struct to e.g. component driver definitions. And
because human beings are allowed to type them in, they have been
somewhat inconsistently defined. Normalize them so the string name
and the symbol name match (the symbol has a "_uuid" suffix).
Some of these rules are fairly regular:
* Some of the component drivers added a "_comp" to the global symbol
name and some didn't. Strip the ones that included it.
* Some naming liked dashes where underscores would be present in a
symbol name (e.g. "dw-dma" for dw_dma_uuid). Unify the conventions
so all name strings are valid C symbols.
* Applying those rules produces a collision between "dai" UUIDs
defined in dai.c, dai_legacy.c and dai_zephyr.c, so the latter two
have been renamed to "dai_legacy" and "dai_zephyr".
And in a handful of spots the code just wasn't consistent. These
UUIDs have been manually renamed, generally trying to pick a name the
corresponds to the original string name, or to the C file that defines
them if that seems impractical:
Orig. String Name Orig. Symbol New Unified Name
================= ============ ================
Maxim DSM smart_amp maxim_dsm
Passthru Amp smart_amp passthru_smart_amp
agent_work agent_work_task agent_work
cadence_codec cadence cadence_codec
channel_map chmap chmap
comp_task idc_comp_task idc_comp
component comp component
dp_schedule dp_sched dp_sched
dts_codec dts dts
edf_schedule edf_sched edf_sched
google_hotword_detect ghd google_hotword
ipcgw ipcgtw ipcgw
irq_818x irq_mt818x irq_mt818x
kd_test keyword keyword
ll_schedule ll_sched ll_sched
memory mem mem
micfil_dai micfil micfil
mix_in mixin mixin
mix_out mixout mixout
modules intel modules
passthrough_codec passthrough passthrough
pga volume volume
posix_ipc_task ipc_task ipc_task
schedule sch schedule
spi_completion spi_compl_task spi_completion
waves_codec waves waves
zll_schedule zll_sched zll_sched
Signed-off-by: Andy Ross <[email protected]>
If UUIDs are by definition unique, then the stored names that refer to them must also be unique (otherwise there's no value in trying to assign unique identifiers to them in the first place). Fix this up by changing names (not IDs) where needed. The common antipatterns here are: 1. Cut-and-paste copies of drivers where the UUID values themselves were changed for the new driver but the code around them was not. These are resolved by including the platform/variant name in the UUID name. 2. Variant UUIDs for IPC3/IPC4 builds. The reason isn't clear to me, but some drivers build with different UUID values depending on the build-time protocol configuration. But these then name themselves identically, producing a collision. Resolved by putting a "4" suffix on the IPC4 symbol name. Signed-off-by: Andy Ross <[email protected]>
In a few cases we have multiple component code trying to reuse the same UUID value for differing purposes and with different names. But we can resolve that by sharing names rather than changing a permanent UUID assignment: * Post-name-normalization, the "dai-legacy" and "dai-zephyr" components share a single UUID with different names (they both used to be "dai"), AND with the trace context of another "dai" that has a different (!) ID value in src/lib. Fix this by using a single UUID for the former two that collide, and renaming the latter "dai_lib". * A single UUID got cut/pasted between the irq drivers of mt8195, imx "irqsteer" and imx "generic". Since these will never be used in a single image, give them all the single name "interrupt" and let them share the UUID value. Signed-off-by: Andy Ross <[email protected]>
The library code edf_sched and ll_sched_lib components were generated with UUIDs that collide with the core EDF scheduler component, which is illegal. This is test code though and doesn't need to match external artifacts, so just renumber. Signed-off-by: Andy Ross <[email protected]>
This DAI driver got labelled with the same UUID as the "hsdai" component, which is illegal. Generate a new UUID. This is relatively safe, as DAIs are fixed with hardware and don't get referenced by ID in external topology. Still frustrating to have to do. Signed-off-by: Andy Ross <[email protected]>
These two drivers both got cloned UUIDs from the acp_hs component, which is illegal. Renumber them. This is relatively safe, as clock and DMA drivers are specific to a platform and not referenced via external files like topology. Signed-off-by: Andy Ross <[email protected]>
Add a very simple uuid-registry.txt file containing all known UUIDs in the tree, use it to generate a C header (the script validates it in the process) that can then be used for a simplified SOF_DEFINE_REG_UUID() mechanism that avoids the risk and temptation temptation of components incorrectly implementing UUIDs. The intent is that in the longer term, this file can be used by other downstream tooling (manifest and topology generation) to more easily reference known IDs by name in a way that avoids duplication and error. Signed-off-by: Andy Ross <[email protected]>
Strip out all the literall UUID management from existing C code (the API itself still works for any out-of-tree or test code users) and exclusively use the new, much simpler, SOF_DEFINE_REG_UUID() macro which sources IDs from the registry by name. Signed-off-by: Andy Ross <[email protected]>
|
Rebased. One more separate-toplevel-cmake spot for sof-logger needed. Cleaned up some of the checkpatch warnings. CI looked good otherwise as of the last update. Removing draft status. |
|
Merging due to large surface area of string changes to reduce conflicts. |
|
Wow, that was fast. Will push a cleanup patch to sof-test that removes the final error (which is by-design now), maybe in combination with some more logging cleanup to more firmly wall off the old dma-trace code from Zephyr builds such that they don't need the (ignored) trace-context UUIDs in core at all. |
@andyross please also add support for LLEXT UUIDs |
Thanks @andyross - yeah, lets deal with the final error as the next step and follow up with llext and more cleanup. |
This is a situation where scratching an itch (trying to get a clean mapping of component "names" I could track with some topology tooling I was experimenting with) turned into a big bleeding wound. But the healing is probably worth it.
Basically SOF's UUID hygiene had broken down in a bunch of places, and the cleanup led to a global registry that should be simpler to maintain in the long term. There's lots of code motion here but no signficant behavior changes, just a new and cleaner API. Please read the commit messages carefully, but the executive summary is:
No more typing UUIDs into C code. All IDs are defined with unique names via
SOF_DEFINE_REG_UUID(name);A handful of spots where we had colliding names and (gulp) IDs have been corrected.
IDs are specified in a minimally complicated text file at the top of the source tree. The build time C header generation automatically validates this file so it will stay clean.
UUID string names (for IDs referenced by the build) are available at runtime in Zephyr builds via the pre-existing sof_uuid_entry struct, and you can enumerate over them via the Zephyr iterable sections API. XTOS builds remain compatible, but don't get the new feature. (This does have an image size cost of ~1-2k in existing firmware builds.)
There remain a few spots where separate components share UUIDs. These have been left in place for compatibility, though obviously they can't (and haven't been) be built into a single image. The names of those IDs have been unified, however.
Longer term the hope is that this can be unified with things like topology and manifest generation, both to prevent errors due to miscopied IDs and to simplify tooling by giving users the ability to use a plaintext name instead.
Note also that this requires a Zephyr PR to build successfully, which corrects some mistakes with "snippets" in the Intel and NXP linker scripts. Builds without them drop the runtime UUID structs into invalid or orphan addresses (which are linker warnings, but not errors) and will cause crashes at runtime: zephyrproject-rtos/zephyr#75055Actually all that's needed is the ROM_SECTIONS PR that already merged upstream, my fixup crossed, and there's no reason not to use the variant that merged. The next Zephyr uprev (e.g. to the 3.7.0 LTS version which is mere hours from shipping) should catch this automatically