[PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Masahiro Yamada]

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
-- 
2.17.1

Applied "ASoC: SOF: use __u32 instead of uint32_t in uapi headers" to the asoc tree

[Mark Brown]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6632 bytes --]

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.com
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
-- 
2.20.1

Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Pierre-Louis Bossart]

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
> 

Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Takashi Iwai]

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
> >
> 

Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Pierre-Louis Bossart]

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.

Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Arnd Bergmann]

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

Re: [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Arnd Bergmann]

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

Re: [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Masahiro Yamada]

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

Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Pierre-Louis Bossart]

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

Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Pierre-Louis Bossart]

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.

Re: [alsa-devel] [PATCH] ASoC: SOF: use __u32 instead of uint32_t in uapi headers

[Arnd Bergmann]

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