| Message ID | 20190721142308.30306-1-yamada.masahiro@socionext.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 62ec3d13601bd626ca7a0edef6d45dbb753d94e8 |
| Headers | show |
| Series | ASoC: SOF: use __u32 instead of uint32_t in uapi headers | expand |
On 7/21/19 9:23 AM, Masahiro Yamada wrote: > When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to > make sure they can be included from user-space. > > Currently, header.h and fw.h are excluded from the test coverage. > To make them join the compile-test, we need to fix the build errors > attached below. > > For a case like this, we decided to use __u{8,16,32,64} variable types > in this discussion: > > https://lkml.org/lkml/2019/6/5/18 these files are shared with the SOF project and used as is (with minor formatting) for the firmware compilation. I am not sure I understand the ask here, are you really asking SOF to use linux-specific type definitions? > > Build log: > > CC usr/include/sound/sof/header.h.s > CC usr/include/sound/sof/fw.h.s > In file included from <command-line>:32:0: > ./usr/include/sound/sof/header.h:19:2: error: unknown type name ‘uint32_t’ > uint32_t magic; /**< 'S', 'O', 'F', '\0' */ > ^~~~~~~~ > ./usr/include/sound/sof/header.h:20:2: error: unknown type name ‘uint32_t’ > uint32_t type; /**< component specific type */ > ^~~~~~~~ > ./usr/include/sound/sof/header.h:21:2: error: unknown type name ‘uint32_t’ > uint32_t size; /**< size in bytes of data excl. this struct */ > ^~~~~~~~ > ./usr/include/sound/sof/header.h:22:2: error: unknown type name ‘uint32_t’ > uint32_t abi; /**< SOF ABI version */ > ^~~~~~~~ > ./usr/include/sound/sof/header.h:23:2: error: unknown type name ‘uint32_t’ > uint32_t reserved[4]; /**< reserved for future use */ > ^~~~~~~~ > ./usr/include/sound/sof/header.h:24:2: error: unknown type name ‘uint32_t’ > uint32_t data[0]; /**< Component data - opaque to core */ > ^~~~~~~~ > In file included from <command-line>:32:0: > ./usr/include/sound/sof/fw.h:49:2: error: unknown type name ‘uint32_t’ > uint32_t size; /* bytes minus this header */ > ^~~~~~~~ > ./usr/include/sound/sof/fw.h:50:2: error: unknown type name ‘uint32_t’ > uint32_t offset; /* offset from base */ > ^~~~~~~~ > ./usr/include/sound/sof/fw.h:64:2: error: unknown type name ‘uint32_t’ > uint32_t size; /* bytes minus this header */ > ^~~~~~~~ > ./usr/include/sound/sof/fw.h:65:2: error: unknown type name ‘uint32_t’ > uint32_t num_blocks; /* number of blocks */ > ^~~~~~~~ > ./usr/include/sound/sof/fw.h:73:2: error: unknown type name ‘uint32_t’ > uint32_t file_size; /* size of file minus this header */ > ^~~~~~~~ > ./usr/include/sound/sof/fw.h:74:2: error: unknown type name ‘uint32_t’ > uint32_t num_modules; /* number of modules */ > ^~~~~~~~ > ./usr/include/sound/sof/fw.h:75:2: error: unknown type name ‘uint32_t’ > uint32_t abi; /* version of header format */ > ^~~~~~~~ > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > include/uapi/sound/sof/fw.h | 16 +++++++++------- > include/uapi/sound/sof/header.h | 14 ++++++++------ > 2 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/include/uapi/sound/sof/fw.h b/include/uapi/sound/sof/fw.h > index 1afca973eb09..e9f697467a86 100644 > --- a/include/uapi/sound/sof/fw.h > +++ b/include/uapi/sound/sof/fw.h > @@ -13,6 +13,8 @@ > #ifndef __INCLUDE_UAPI_SOF_FW_H__ > #define __INCLUDE_UAPI_SOF_FW_H__ > > +#include <linux/types.h> > + > #define SND_SOF_FW_SIG_SIZE 4 > #define SND_SOF_FW_ABI 1 > #define SND_SOF_FW_SIG "Reef" > @@ -46,8 +48,8 @@ enum snd_sof_fw_blk_type { > > struct snd_sof_blk_hdr { > enum snd_sof_fw_blk_type type; > - uint32_t size; /* bytes minus this header */ > - uint32_t offset; /* offset from base */ > + __u32 size; /* bytes minus this header */ > + __u32 offset; /* offset from base */ > } __packed; > > /* > @@ -61,8 +63,8 @@ enum snd_sof_fw_mod_type { > > struct snd_sof_mod_hdr { > enum snd_sof_fw_mod_type type; > - uint32_t size; /* bytes minus this header */ > - uint32_t num_blocks; /* number of blocks */ > + __u32 size; /* bytes minus this header */ > + __u32 num_blocks; /* number of blocks */ > } __packed; > > /* > @@ -70,9 +72,9 @@ struct snd_sof_mod_hdr { > */ > struct snd_sof_fw_header { > unsigned char sig[SND_SOF_FW_SIG_SIZE]; /* "Reef" */ > - uint32_t file_size; /* size of file minus this header */ > - uint32_t num_modules; /* number of modules */ > - uint32_t abi; /* version of header format */ > + __u32 file_size; /* size of file minus this header */ > + __u32 num_modules; /* number of modules */ > + __u32 abi; /* version of header format */ > } __packed; > > #endif > diff --git a/include/uapi/sound/sof/header.h b/include/uapi/sound/sof/header.h > index 7868990b0d6f..5f4518e7a972 100644 > --- a/include/uapi/sound/sof/header.h > +++ b/include/uapi/sound/sof/header.h > @@ -9,6 +9,8 @@ > #ifndef __INCLUDE_UAPI_SOUND_SOF_USER_HEADER_H__ > #define __INCLUDE_UAPI_SOUND_SOF_USER_HEADER_H__ > > +#include <linux/types.h> > + > /* > * Header for all non IPC ABI data. > * > @@ -16,12 +18,12 @@ > * Used by any bespoke component data structures or binary blobs. > */ > struct sof_abi_hdr { > - uint32_t magic; /**< 'S', 'O', 'F', '\0' */ > - uint32_t type; /**< component specific type */ > - uint32_t size; /**< size in bytes of data excl. this struct */ > - uint32_t abi; /**< SOF ABI version */ > - uint32_t reserved[4]; /**< reserved for future use */ > - uint32_t data[0]; /**< Component data - opaque to core */ > + __u32 magic; /**< 'S', 'O', 'F', '\0' */ > + __u32 type; /**< component specific type */ > + __u32 size; /**< size in bytes of data excl. this struct */ > + __u32 abi; /**< SOF ABI version */ > + __u32 reserved[4]; /**< reserved for future use */ > + __u32 data[0]; /**< Component data - opaque to core */ > } __packed; > > #endif >
On Mon, 22 Jul 2019 14:49:34 +0200, Pierre-Louis Bossart wrote: > > > > On 7/21/19 9:23 AM, Masahiro Yamada wrote: > > When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to > > make sure they can be included from user-space. > > > > Currently, header.h and fw.h are excluded from the test coverage. > > To make them join the compile-test, we need to fix the build errors > > attached below. > > > > For a case like this, we decided to use __u{8,16,32,64} variable types > > in this discussion: > > > > https://lkml.org/lkml/2019/6/5/18 > > these files are shared with the SOF project and used as is (with minor > formatting) for the firmware compilation. I am not sure I understand > the ask here, are you really asking SOF to use linux-specific type > definitions? Actually this is linux-kernel UAPI header files, so yes, we should follow the convention there as much as possible. So far we haven't been strict about these types. But now we have a unit test for checking it, so it's a good opportunity to address the issues. thanks, Takashi > > > > > Build log: > > > > CC usr/include/sound/sof/header.h.s > > CC usr/include/sound/sof/fw.h.s > > In file included from <command-line>:32:0: > > ./usr/include/sound/sof/header.h:19:2: error: unknown type name ‘uint32_t’ > > uint32_t magic; /**< 'S', 'O', 'F', '\0' */ > > ^~~~~~~~ > > ./usr/include/sound/sof/header.h:20:2: error: unknown type name ‘uint32_t’ > > uint32_t type; /**< component specific type */ > > ^~~~~~~~ > > ./usr/include/sound/sof/header.h:21:2: error: unknown type name ‘uint32_t’ > > uint32_t size; /**< size in bytes of data excl. this struct */ > > ^~~~~~~~ > > ./usr/include/sound/sof/header.h:22:2: error: unknown type name ‘uint32_t’ > > uint32_t abi; /**< SOF ABI version */ > > ^~~~~~~~ > > ./usr/include/sound/sof/header.h:23:2: error: unknown type name ‘uint32_t’ > > uint32_t reserved[4]; /**< reserved for future use */ > > ^~~~~~~~ > > ./usr/include/sound/sof/header.h:24:2: error: unknown type name ‘uint32_t’ > > uint32_t data[0]; /**< Component data - opaque to core */ > > ^~~~~~~~ > > In file included from <command-line>:32:0: > > ./usr/include/sound/sof/fw.h:49:2: error: unknown type name ‘uint32_t’ > > uint32_t size; /* bytes minus this header */ > > ^~~~~~~~ > > ./usr/include/sound/sof/fw.h:50:2: error: unknown type name ‘uint32_t’ > > uint32_t offset; /* offset from base */ > > ^~~~~~~~ > > ./usr/include/sound/sof/fw.h:64:2: error: unknown type name ‘uint32_t’ > > uint32_t size; /* bytes minus this header */ > > ^~~~~~~~ > > ./usr/include/sound/sof/fw.h:65:2: error: unknown type name ‘uint32_t’ > > uint32_t num_blocks; /* number of blocks */ > > ^~~~~~~~ > > ./usr/include/sound/sof/fw.h:73:2: error: unknown type name ‘uint32_t’ > > uint32_t file_size; /* size of file minus this header */ > > ^~~~~~~~ > > ./usr/include/sound/sof/fw.h:74:2: error: unknown type name ‘uint32_t’ > > uint32_t num_modules; /* number of modules */ > > ^~~~~~~~ > > ./usr/include/sound/sof/fw.h:75:2: error: unknown type name ‘uint32_t’ > > uint32_t abi; /* version of header format */ > > ^~~~~~~~ > > > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > --- > > > > include/uapi/sound/sof/fw.h | 16 +++++++++------- > > include/uapi/sound/sof/header.h | 14 ++++++++------ > > 2 files changed, 17 insertions(+), 13 deletions(-) > > > > diff --git a/include/uapi/sound/sof/fw.h b/include/uapi/sound/sof/fw.h > > index 1afca973eb09..e9f697467a86 100644 > > --- a/include/uapi/sound/sof/fw.h > > +++ b/include/uapi/sound/sof/fw.h > > @@ -13,6 +13,8 @@ > > #ifndef __INCLUDE_UAPI_SOF_FW_H__ > > #define __INCLUDE_UAPI_SOF_FW_H__ > > +#include <linux/types.h> > > + > > #define SND_SOF_FW_SIG_SIZE 4 > > #define SND_SOF_FW_ABI 1 > > #define SND_SOF_FW_SIG "Reef" > > @@ -46,8 +48,8 @@ enum snd_sof_fw_blk_type { > > struct snd_sof_blk_hdr { > > enum snd_sof_fw_blk_type type; > > - uint32_t size; /* bytes minus this header */ > > - uint32_t offset; /* offset from base */ > > + __u32 size; /* bytes minus this header */ > > + __u32 offset; /* offset from base */ > > } __packed; > > /* > > @@ -61,8 +63,8 @@ enum snd_sof_fw_mod_type { > > struct snd_sof_mod_hdr { > > enum snd_sof_fw_mod_type type; > > - uint32_t size; /* bytes minus this header */ > > - uint32_t num_blocks; /* number of blocks */ > > + __u32 size; /* bytes minus this header */ > > + __u32 num_blocks; /* number of blocks */ > > } __packed; > > /* > > @@ -70,9 +72,9 @@ struct snd_sof_mod_hdr { > > */ > > struct snd_sof_fw_header { > > unsigned char sig[SND_SOF_FW_SIG_SIZE]; /* "Reef" */ > > - uint32_t file_size; /* size of file minus this header */ > > - uint32_t num_modules; /* number of modules */ > > - uint32_t abi; /* version of header format */ > > + __u32 file_size; /* size of file minus this header */ > > + __u32 num_modules; /* number of modules */ > > + __u32 abi; /* version of header format */ > > } __packed; > > #endif > > diff --git a/include/uapi/sound/sof/header.h b/include/uapi/sound/sof/header.h > > index 7868990b0d6f..5f4518e7a972 100644 > > --- a/include/uapi/sound/sof/header.h > > +++ b/include/uapi/sound/sof/header.h > > @@ -9,6 +9,8 @@ > > #ifndef __INCLUDE_UAPI_SOUND_SOF_USER_HEADER_H__ > > #define __INCLUDE_UAPI_SOUND_SOF_USER_HEADER_H__ > > +#include <linux/types.h> > > + > > /* > > * Header for all non IPC ABI data. > > * > > @@ -16,12 +18,12 @@ > > * Used by any bespoke component data structures or binary blobs. > > */ > > struct sof_abi_hdr { > > - uint32_t magic; /**< 'S', 'O', 'F', '\0' */ > > - uint32_t type; /**< component specific type */ > > - uint32_t size; /**< size in bytes of data excl. this struct */ > > - uint32_t abi; /**< SOF ABI version */ > > - uint32_t reserved[4]; /**< reserved for future use */ > > - uint32_t data[0]; /**< Component data - opaque to core */ > > + __u32 magic; /**< 'S', 'O', 'F', '\0' */ > > + __u32 type; /**< component specific type */ > > + __u32 size; /**< size in bytes of data excl. this struct */ > > + __u32 abi; /**< SOF ABI version */ > > + __u32 reserved[4]; /**< reserved for future use */ > > + __u32 data[0]; /**< Component data - opaque to core */ > > } __packed; > > #endif > > >
On 7/22/19 7:56 AM, Takashi Iwai wrote: > On Mon, 22 Jul 2019 14:49:34 +0200, > Pierre-Louis Bossart wrote: >> >> >> >> On 7/21/19 9:23 AM, Masahiro Yamada wrote: >>> When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to >>> make sure they can be included from user-space. >>> >>> Currently, header.h and fw.h are excluded from the test coverage. >>> To make them join the compile-test, we need to fix the build errors >>> attached below. >>> >>> For a case like this, we decided to use __u{8,16,32,64} variable types >>> in this discussion: >>> >>> https://lkml.org/lkml/2019/6/5/18 >> >> these files are shared with the SOF project and used as is (with minor >> formatting) for the firmware compilation. I am not sure I understand >> the ask here, are you really asking SOF to use linux-specific type >> definitions? > > Actually this is linux-kernel UAPI header files, so yes, we should > follow the convention there as much as possible. > > So far we haven't been strict about these types. But now we have a > unit test for checking it, so it's a good opportunity to address the > issues. Maybe a bit of background. For SOF we split the includes in 4 directories https://github.com/thesofproject/sof/tree/master/src/include - sof: internal includes for firmware only - ipc: definitions of the structures for information exchanged over the IPC channel. This directory is used as is by the Linux kernel and mirrored in include/sound/sof - user: definitions needed for firmware tools, e.g. to generate the image or parse the trace. this directory is not used by the Linux kernel. - kernel: definitions for the firmware format, needed for the loader to parse the firmware files. This is not directly used by applications running on the target, it really defines the content passed to the kernel with request_firmware. This directory is mirrored in the Linux include/uapi/sound/sof directory. Our goal is to minimize the differences and allow deltas e.g. for license or comments. We could add a definition for __u32 when linux is not used, I am just not sure if these two files really fall in the UAPI category and if the checks make sense.
On Mon, Jul 22, 2019 at 3:16 PM Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > On 7/22/19 7:56 AM, Takashi Iwai wrote: > > On Mon, 22 Jul 2019 14:49:34 +0200, > > Pierre-Louis Bossart wrote: > >> > >> > >> > >> On 7/21/19 9:23 AM, Masahiro Yamada wrote: > >>> When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to > >>> make sure they can be included from user-space. > >>> > >>> Currently, header.h and fw.h are excluded from the test coverage. > >>> To make them join the compile-test, we need to fix the build errors > >>> attached below. > >>> > >>> For a case like this, we decided to use __u{8,16,32,64} variable types > >>> in this discussion: > >>> > >>> https://lkml.org/lkml/2019/6/5/18 > >> > >> these files are shared with the SOF project and used as is (with minor > >> formatting) for the firmware compilation. I am not sure I understand > >> the ask here, are you really asking SOF to use linux-specific type > >> definitions? > > > > Actually this is linux-kernel UAPI header files, so yes, we should > > follow the convention there as much as possible. > > > > So far we haven't been strict about these types. But now we have a > > unit test for checking it, so it's a good opportunity to address the > > issues. > > Maybe a bit of background. For SOF we split the includes in 4 directories > > https://github.com/thesofproject/sof/tree/master/src/include > > - sof: internal includes for firmware only > - ipc: definitions of the structures for information exchanged over the > IPC channel. This directory is used as is by the Linux kernel and > mirrored in include/sound/sof > - user: definitions needed for firmware tools, e.g. to generate the > image or parse the trace. this directory is not used by the Linux kernel. > - kernel: definitions for the firmware format, needed for the loader to > parse the firmware files. This is not directly used by applications > running on the target, it really defines the content passed to the > kernel with request_firmware. This directory is mirrored in the Linux > include/uapi/sound/sof directory. > > Our goal is to minimize the differences and allow deltas e.g. for > license or comments. We could add a definition for __u32 when linux is > not used, I am just not sure if these two files really fall in the UAPI > category and if the checks make sense. If you can build all the SOF user space without these headers being installed to /usr/include/sound/sof/, you can move them from include/uapi/sound/sof to include/sounds/sof and leave the types unchanged. Arnd
On Sun, Jul 21, 2019 at 4:25 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > struct snd_sof_blk_hdr { > enum snd_sof_fw_blk_type type; > - uint32_t size; /* bytes minus this header */ > - uint32_t offset; /* offset from base */ > + __u32 size; /* bytes minus this header */ > + __u32 offset; /* offset from base */ > } __packed; > On a related note: Using an 'enum' in an ABI structure is not portable across architectures. This is probably fine in a UAPI as long as user and kernel space agree on the size of an enum, but if the same structure is used to talk to the firmware, it won't work on architectures that have a different size for the first field. Arnd
On Mon, Jul 22, 2019 at 10:40 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sun, Jul 21, 2019 at 4:25 PM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > > > struct snd_sof_blk_hdr { > > enum snd_sof_fw_blk_type type; > > - uint32_t size; /* bytes minus this header */ > > - uint32_t offset; /* offset from base */ > > + __u32 size; /* bytes minus this header */ > > + __u32 offset; /* offset from base */ > > } __packed; > > > > On a related note: Using an 'enum' in an ABI structure is not portable > across architectures. This is probably fine in a UAPI as long as user > and kernel space agree on the size of an enum, but if the same > structure is used to talk to the firmware, it won't work on architectures > that have a different size for the first field. Both comments from Arnd make sense. If this header does not need to be in uapi/, moving it out is fine. But, looks like Mark has already picked up this. (His review is so quick) -- Best Regards Masahiro Yamada
On 7/22/19 8:34 AM, Arnd Bergmann wrote: > On Mon, Jul 22, 2019 at 3:16 PM Pierre-Louis Bossart > <pierre-louis.bossart@linux.intel.com> wrote: >> On 7/22/19 7:56 AM, Takashi Iwai wrote: >>> On Mon, 22 Jul 2019 14:49:34 +0200, >>> Pierre-Louis Bossart wrote: >>>> >>>> >>>> >>>> On 7/21/19 9:23 AM, Masahiro Yamada wrote: >>>>> When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to >>>>> make sure they can be included from user-space. >>>>> >>>>> Currently, header.h and fw.h are excluded from the test coverage. >>>>> To make them join the compile-test, we need to fix the build errors >>>>> attached below. >>>>> >>>>> For a case like this, we decided to use __u{8,16,32,64} variable types >>>>> in this discussion: >>>>> >>>>> https://lkml.org/lkml/2019/6/5/18 >>>> >>>> these files are shared with the SOF project and used as is (with minor >>>> formatting) for the firmware compilation. I am not sure I understand >>>> the ask here, are you really asking SOF to use linux-specific type >>>> definitions? >>> >>> Actually this is linux-kernel UAPI header files, so yes, we should >>> follow the convention there as much as possible. >>> >>> So far we haven't been strict about these types. But now we have a >>> unit test for checking it, so it's a good opportunity to address the >>> issues. >> >> Maybe a bit of background. For SOF we split the includes in 4 directories >> >> https://github.com/thesofproject/sof/tree/master/src/include >> >> - sof: internal includes for firmware only >> - ipc: definitions of the structures for information exchanged over the >> IPC channel. This directory is used as is by the Linux kernel and >> mirrored in include/sound/sof >> - user: definitions needed for firmware tools, e.g. to generate the >> image or parse the trace. this directory is not used by the Linux kernel. >> - kernel: definitions for the firmware format, needed for the loader to >> parse the firmware files. This is not directly used by applications >> running on the target, it really defines the content passed to the >> kernel with request_firmware. This directory is mirrored in the Linux >> include/uapi/sound/sof directory. >> >> Our goal is to minimize the differences and allow deltas e.g. for >> license or comments. We could add a definition for __u32 when linux is >> not used, I am just not sure if these two files really fall in the UAPI >> category and if the checks make sense. > > If you can build all the SOF user space without these headers being > installed to /usr/include/sound/sof/, you can move them from > include/uapi/sound/sof to include/sounds/sof and leave the types > unchanged. yes we don't need those files to build userspace stuff. The idea was that these format definitions establish a contract between userspace (more specifically the files stored in /lib/firmware) and the kernel. IIRC we wanted to make sure that any changes would be tracked as breaking userspace. If the consensus is that the uapi directory is strictly used for builds then we should indeed move those files
On 7/22/19 8:39 AM, Arnd Bergmann wrote: > On Sun, Jul 21, 2019 at 4:25 PM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: >> >> struct snd_sof_blk_hdr { >> enum snd_sof_fw_blk_type type; >> - uint32_t size; /* bytes minus this header */ >> - uint32_t offset; /* offset from base */ >> + __u32 size; /* bytes minus this header */ >> + __u32 offset; /* offset from base */ >> } __packed; >> > > On a related note: Using an 'enum' in an ABI structure is not portable > across architectures. This is probably fine in a UAPI as long as user > and kernel space agree on the size of an enum, but if the same > structure is used to talk to the firmware, it won't work on architectures > that have a different size for the first field. yes, we've removed all enums in SOF and missed this one. This should be changed, thanks for the note.
On Mon, Jul 22, 2019 at 5:18 PM Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > On 7/22/19 8:34 AM, Arnd Bergmann wrote: > > On Mon, Jul 22, 2019 at 3:16 PM Pierre-Louis Bossart > > <pierre-louis.bossart@linux.intel.com> wrote: > >> On 7/22/19 7:56 AM, Takashi Iwai wrote: > >>> On Mon, 22 Jul 2019 14:49:34 +0200, > >> Our goal is to minimize the differences and allow deltas e.g. for > >> license or comments. We could add a definition for __u32 when linux is > >> not used, I am just not sure if these two files really fall in the UAPI > >> category and if the checks make sense. > > > > If you can build all the SOF user space without these headers being > > installed to /usr/include/sound/sof/, you can move them from > > include/uapi/sound/sof to include/sounds/sof and leave the types > > unchanged. > > yes we don't need those files to build userspace stuff. The idea was > that these format definitions establish a contract between userspace > (more specifically the files stored in /lib/firmware) and the kernel. > IIRC we wanted to make sure that any changes would be tracked as > breaking userspace. If the consensus is that the uapi directory is > strictly used for builds then we should indeed move those files I don't see a problem with keeping the files in uapi for practical purposes, but then I think it makes sense to apply the same rules as for other uapi headers and use user-space clean type names. Arnd
diff --git a/include/uapi/sound/sof/fw.h b/include/uapi/sound/sof/fw.h index 1afca973eb09..e9f697467a86 100644 --- a/include/uapi/sound/sof/fw.h +++ b/include/uapi/sound/sof/fw.h @@ -13,6 +13,8 @@ #ifndef __INCLUDE_UAPI_SOF_FW_H__ #define __INCLUDE_UAPI_SOF_FW_H__ +#include <linux/types.h> + #define SND_SOF_FW_SIG_SIZE 4 #define SND_SOF_FW_ABI 1 #define SND_SOF_FW_SIG "Reef" @@ -46,8 +48,8 @@ enum snd_sof_fw_blk_type { struct snd_sof_blk_hdr { enum snd_sof_fw_blk_type type; - uint32_t size; /* bytes minus this header */ - uint32_t offset; /* offset from base */ + __u32 size; /* bytes minus this header */ + __u32 offset; /* offset from base */ } __packed; /* @@ -61,8 +63,8 @@ enum snd_sof_fw_mod_type { struct snd_sof_mod_hdr { enum snd_sof_fw_mod_type type; - uint32_t size; /* bytes minus this header */ - uint32_t num_blocks; /* number of blocks */ + __u32 size; /* bytes minus this header */ + __u32 num_blocks; /* number of blocks */ } __packed; /* @@ -70,9 +72,9 @@ struct snd_sof_mod_hdr { */ struct snd_sof_fw_header { unsigned char sig[SND_SOF_FW_SIG_SIZE]; /* "Reef" */ - uint32_t file_size; /* size of file minus this header */ - uint32_t num_modules; /* number of modules */ - uint32_t abi; /* version of header format */ + __u32 file_size; /* size of file minus this header */ + __u32 num_modules; /* number of modules */ + __u32 abi; /* version of header format */ } __packed; #endif diff --git a/include/uapi/sound/sof/header.h b/include/uapi/sound/sof/header.h index 7868990b0d6f..5f4518e7a972 100644 --- a/include/uapi/sound/sof/header.h +++ b/include/uapi/sound/sof/header.h @@ -9,6 +9,8 @@ #ifndef __INCLUDE_UAPI_SOUND_SOF_USER_HEADER_H__ #define __INCLUDE_UAPI_SOUND_SOF_USER_HEADER_H__ +#include <linux/types.h> + /* * Header for all non IPC ABI data. * @@ -16,12 +18,12 @@ * Used by any bespoke component data structures or binary blobs. */ struct sof_abi_hdr { - uint32_t magic; /**< 'S', 'O', 'F', '\0' */ - uint32_t type; /**< component specific type */ - uint32_t size; /**< size in bytes of data excl. this struct */ - uint32_t abi; /**< SOF ABI version */ - uint32_t reserved[4]; /**< reserved for future use */ - uint32_t data[0]; /**< Component data - opaque to core */ + __u32 magic; /**< 'S', 'O', 'F', '\0' */ + __u32 type; /**< component specific type */ + __u32 size; /**< size in bytes of data excl. this struct */ + __u32 abi; /**< SOF ABI version */ + __u32 reserved[4]; /**< reserved for future use */ + __u32 data[0]; /**< Component data - opaque to core */ } __packed; #endif
When CONFIG_UAPI_HEADER_TEST=y, exported headers are compile-tested to make sure they can be included from user-space. Currently, header.h and fw.h are excluded from the test coverage. To make them join the compile-test, we need to fix the build errors attached below. For a case like this, we decided to use __u{8,16,32,64} variable types in this discussion: https://lkml.org/lkml/2019/6/5/18 Build log: CC usr/include/sound/sof/header.h.s CC usr/include/sound/sof/fw.h.s In file included from <command-line>:32:0: ./usr/include/sound/sof/header.h:19:2: error: unknown type name ‘uint32_t’ uint32_t magic; /**< 'S', 'O', 'F', '\0' */ ^~~~~~~~ ./usr/include/sound/sof/header.h:20:2: error: unknown type name ‘uint32_t’ uint32_t type; /**< component specific type */ ^~~~~~~~ ./usr/include/sound/sof/header.h:21:2: error: unknown type name ‘uint32_t’ uint32_t size; /**< size in bytes of data excl. this struct */ ^~~~~~~~ ./usr/include/sound/sof/header.h:22:2: error: unknown type name ‘uint32_t’ uint32_t abi; /**< SOF ABI version */ ^~~~~~~~ ./usr/include/sound/sof/header.h:23:2: error: unknown type name ‘uint32_t’ uint32_t reserved[4]; /**< reserved for future use */ ^~~~~~~~ ./usr/include/sound/sof/header.h:24:2: error: unknown type name ‘uint32_t’ uint32_t data[0]; /**< Component data - opaque to core */ ^~~~~~~~ In file included from <command-line>:32:0: ./usr/include/sound/sof/fw.h:49:2: error: unknown type name ‘uint32_t’ uint32_t size; /* bytes minus this header */ ^~~~~~~~ ./usr/include/sound/sof/fw.h:50:2: error: unknown type name ‘uint32_t’ uint32_t offset; /* offset from base */ ^~~~~~~~ ./usr/include/sound/sof/fw.h:64:2: error: unknown type name ‘uint32_t’ uint32_t size; /* bytes minus this header */ ^~~~~~~~ ./usr/include/sound/sof/fw.h:65:2: error: unknown type name ‘uint32_t’ uint32_t num_blocks; /* number of blocks */ ^~~~~~~~ ./usr/include/sound/sof/fw.h:73:2: error: unknown type name ‘uint32_t’ uint32_t file_size; /* size of file minus this header */ ^~~~~~~~ ./usr/include/sound/sof/fw.h:74:2: error: unknown type name ‘uint32_t’ uint32_t num_modules; /* number of modules */ ^~~~~~~~ ./usr/include/sound/sof/fw.h:75:2: error: unknown type name ‘uint32_t’ uint32_t abi; /* version of header format */ ^~~~~~~~ Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- include/uapi/sound/sof/fw.h | 16 +++++++++------- include/uapi/sound/sof/header.h | 14 ++++++++------ 2 files changed, 17 insertions(+), 13 deletions(-)