ipc: replace enum fields with uint32_t in some IPC structures by LaurentiuM1234 · Pull Request #9379 · thesofproject/sof

LaurentiuM1234 · 2024-08-20T12:12:02Z

Normally, the type of enums is "unsigned int" or "int". GCC has the "-fshort-enums" option, which instructs the compiler to use the smallest data type that can hold all the values in the enum (i.e: char, short, int or their unsigned variants).

According to the GCC documentation, "-fshort-enums" may be default on some targets. This seems to be the case for "arm-zephyr-eabi-gcc", which is used to build Zephyr on ARM platforms.

On Linux, this is not the case (tested with "aarch64-linux-gnu-gcc"), which means enums such as "enum sof_comp_type" will end up having different sizes on Linux and SOF. Since "enum sof_comp_type" is used in IPC-related structures such as "struct sof_ipc_comp", this means the fields of the structures will end up being placed at different offsets. This, in turn, leads to SOF not being able to properly interpret data passed from Linux.

With this in mind, replace "enum sof_comp_type" from "struct sof_ipc_comp" and "enum ipc4_stream_type" from "struct ipc4_pin_props" with "uint32_t".

Fixes #9378.
Kernel-side change: thesofproject/linux#5148

lyakh · 2024-08-21T06:13:57Z

src/include/ipc/topology.h

 	struct sof_ipc_cmd_hdr hdr;
 	uint32_t id;
-	enum sof_comp_type type;
+	uint32_t type;


I'm for this change - IPC structures are one of few locations where fixed-size types must be used, unlike in multiple other locations, where proper C types should be used instead. But just one proposal - would be good to add a comment with enum X to these fields to help prevent future mistakes

ok, this is fine - its good we can get more coverage with different ISAs, the ARM compiler throwing this up means we should make this change.
One thing, can we inline comment each change to say that the new uint32_t type uses enum X

fixed, also added reference to #9378. Should make it easier for ppl to find why the change was done w/o having to look at commit msg.

LaurentiuM1234 · 2024-08-21T09:21:01Z

src/include/module/ipc4/base-config.h


 struct ipc4_pin_props {
 	/* type of the connected stream. */
-	enum ipc4_stream_type stream_type;


About this structure: my initial thought was that this is used in IPC4 communication between Linux and SOF, but after looking a bit more carefully it doesn't seem to be the case.

From this comment:

This file contains structures that are exact copies of an existing ABI used by IOT middleware. They are Intel specific and will be used by one middleware. Some of the structures may contain programming implementations that makes them unsuitable for generic use and general usage. This code is mostly copied "as-is" from existing C++ interface files hence the use of different style in places. The intention is to keep the interface as close as possible to original so it's easier to track changes with IPC host code.

My guess is this is being used by some other entity (?) and it would seem like this is Intel specific.

Since this is also included in files used by IPC3 platforms we get the compilation warning mentioned in #9378 (i.e: warning: 'format' offset 1 in 'struct ipc4_pin_props' isn't aligned to 4) (caused by the size of enum ipc4_stream_type).

With this in mind, I'm not sure the replacement with the uint32_t type is right as I don't fully understand how these IPC4 structures are used. Can someone from Intel comment on this, please?

CC: @RanderWang @ranj063

EDIT: alternatively, since this is not actually used we can file a bug report on this and just go with the "struct sof_ipc_comp" change. The main issue with this is that the many warnings this generates make it hard to spot other, new warnings.

I think this is OK now, we were doing a lot of work at that time to ensure compatibility, but that good now.

alright, I'm still somewhat skeptical of the IPC4-related change but if ppl from Intel are fine with it then that's fine with me

LaurentiuM1234 · 2024-08-21T09:37:46Z

Adding DNM tag and removing from draft to get more comments on this.

lgirdwood · 2024-08-21T12:34:02Z

Adding DNM tag and removing from draft to get more comments on this.

OK, CI results are good so no impact - all ISAs are aligned.

Normally, the type of enums is "unsigned int" or "int". GCC has the "-fshort-enums" option, which instructs the compiler to use the smallest data type that can hold all the values in the enum (i.e: char, short, int or their unsigned variants). According to the GCC documentation, "-fshort-enums" may be default on some targets. This seems to be the case for "arm-zephyr-eabi-gcc", which is used to build Zephyr on ARM platforms. On Linux, this is not the case (tested with "aarch64-linux-gnu-gcc"), which means enums such as "enum sof_comp_type" will end up having different sizes on Linux and SOF. Since "enum sof_comp_type" is used in IPC-related structures such as "struct sof_ipc_comp", this means the fields of the structures will end up being placed at different offsets. This, in turn, leads to SOF not being able to properly interpret data passed from Linux. With this in mind, replace "enum sof_comp_type" from "struct sof_ipc_comp" and "enum ipc4_stream_type" from "struct ipc4_pin_props" with "uint32_t". Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@nxp.com>

LaurentiuM1234 · 2024-08-22T10:29:56Z

Dropping [DNM] tag.

I've also built the following Zephyr sample to emphasize the idea that the size of enums may differ from arch to arch (or toolchain to toolchain):

  1 /*
  2  * Copyright (c) 2012-2014 Wind River Systems, Inc.
  3  *
  4  * SPDX-License-Identifier: Apache-2.0
  5  */
  6 
  7 #include <stdio.h>
  8 #include <zephyr/toolchain.h>
  9 
 10 enum test1 {
 11         A = 0x0,
 12         B = 0x1,
 13         C = 0xFF,
 14 };
 15 
 16 enum test2 {
 17         D = 0x2,
 18         E = 0xFFF,
 19 };
 20 
 21 enum test3 {
 22         F = 0x0,
 23         G = 0x1,
 24         H = 0xFFFF,
 25 };
 26 
 27 BUILD_ASSERT(sizeof(enum test1) == 4, "bad enum test1 size");
 28 BUILD_ASSERT(sizeof(enum test2) == 4, "bad enum test2 size");
 29 BUILD_ASSERT(sizeof(enum test3) == 4, "bad enum test3 size");
 30 
 31 int main(void)
 32 {
 33         printf("Hello World! %s\n", CONFIG_BOARD_TARGET);
 34 
 35         return 0;
 36 }

on 3 different architectures (using 3 dif. toolchains). Leaving the results here in case people might be interested:

XTENSA -> west -v build -p -b imx8mp_evk//adsp samples/hello_world/ -> BUILD OK
ARM64 -> west -v build -p -b imx8mp_evk//a53 samples/hello_world/ -> BUILD OK
ARM -> west -v build -p -b imx8mp_evk//m7 samples/hello_world -> all BUILD_ASSERT() statements fail

CC: @marc-hb you might find this discussion interesting

marc-hb · 2024-08-22T17:09:59Z

Interesting!

I got curious and traced this back to... 2017! When this code was originally added by commit 425aa5e

That commit had a good few other "packed enums" (searched for "packed"). I don't know whether they still exist today.

LaurentiuM1234 mentioned this pull request Aug 20, 2024

ASoC: SOF: ipc: replace "enum sof_comp_type" field with "uint32_t" thesofproject/linux#5148

Merged

lyakh reviewed Aug 21, 2024

View reviewed changes

LaurentiuM1234 commented Aug 21, 2024

View reviewed changes

LaurentiuM1234 changed the title ~~ipc: replace enum fields with uint32_t in some IPC structures~~ [DNM] ipc: replace enum fields with uint32_t in some IPC structures Aug 21, 2024

LaurentiuM1234 marked this pull request as ready for review August 21, 2024 09:37

LaurentiuM1234 requested review from a team, RanderWang, dbaluta, kv2019i, lbetlej, lgirdwood, marcinszkudlinski, mmaka1, pblaszko and plbossart as code owners August 21, 2024 09:37

lgirdwood approved these changes Aug 21, 2024

View reviewed changes

LaurentiuM1234 force-pushed the fix/ipc_struct_offsets branch from 32e81a5 to 9baaee8 Compare August 21, 2024 13:32

cujomalainey approved these changes Aug 21, 2024

View reviewed changes

LaurentiuM1234 changed the title ~~[DNM] ipc: replace enum fields with uint32_t in some IPC structures~~ ipc: replace enum fields with uint32_t in some IPC structures Aug 22, 2024

lgirdwood merged commit 6426967 into thesofproject:main Aug 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipc: replace enum fields with uint32_t in some IPC structures#9379

ipc: replace enum fields with uint32_t in some IPC structures#9379
lgirdwood merged 1 commit intothesofproject:mainfrom
LaurentiuM1234:fix/ipc_struct_offsets

LaurentiuM1234 commented Aug 20, 2024 •

edited

Loading

Uh oh!

lyakh Aug 21, 2024

Uh oh!

lgirdwood Aug 21, 2024

Uh oh!

LaurentiuM1234 Aug 21, 2024

Uh oh!

LaurentiuM1234 Aug 21, 2024 •

edited

Loading

Uh oh!

lgirdwood Aug 21, 2024

Uh oh!

LaurentiuM1234 Aug 21, 2024

Uh oh!

LaurentiuM1234 commented Aug 21, 2024

Uh oh!

lgirdwood commented Aug 21, 2024

Uh oh!

LaurentiuM1234 commented Aug 22, 2024 •

edited

Loading

Uh oh!

marc-hb commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

LaurentiuM1234 commented Aug 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

lgirdwood Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

LaurentiuM1234 Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

LaurentiuM1234 Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

LaurentiuM1234 Aug 21, 2024

Choose a reason for hiding this comment

Uh oh!

LaurentiuM1234 commented Aug 21, 2024

Uh oh!

lgirdwood commented Aug 21, 2024

Uh oh!

LaurentiuM1234 commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LaurentiuM1234 commented Aug 20, 2024 •

edited

Loading

LaurentiuM1234 Aug 21, 2024 •

edited

Loading

LaurentiuM1234 commented Aug 22, 2024 •

edited

Loading