[alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers
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(-)
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
The patch
ASoC: SOF: use __u32 instead of uint32_t in uapi headers
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.3
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 62ec3d13601bd626ca7a0edef6d45dbb753d94e8 Mon Sep 17 00:00:00 2001
From: Masahiro Yamada yamada.masahiro@socionext.com Date: Sun, 21 Jul 2019 23:23:08 +0900 Subject: [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
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 Link: https://lore.kernel.org/r/20190721142308.30306-1-yamada.masahiro@socionext.c... Signed-off-by: Mark Brown broonie@kernel.org --- 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/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:
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:
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/18these 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/18these 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 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/18these 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 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
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: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 */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.
participants (5)
-
Arnd Bergmann -
Mark Brown -
Masahiro Yamada -
Pierre-Louis Bossart -
Takashi Iwai