On Monday, August 22, 2016 5:32:58 PM CEST Thierry Reding wrote:

On Mon, Aug 22, 2016 at 04:42:32PM +0200, Arnd Bergmann wrote:

quoted

On Monday, August 22, 2016 4:02:11 PM CEST Thierry Reding wrote:

quoted

quoted

quoted

+struct mrq_request {
+	/** @brief MRQ number of the request */
+	uint32_t mrq;
+	/** @brief flags for the request */
+	uint32_t flags;
+} __ABI_PACKED;

Marking the structure as packed may result in byte-wise access, depending
on compiler flags. Is that what you intended? The structure is fully
packed already, so you won't avoid any padding here.

Agreed, the packing seems unnecessary in many places. However this is
defining an ABI that's used across multiple operating systems, so the
packing may still be required on some systems or toolchains to ensure
the exact same format in the transport.

However, if __ABI_PACKED is defined to an empty string, it is different
in some cases.

Also, setting 'NO_GCC_EXTENSIONS' changes the structure layout of
some of the structures, by adding an extra member. If the firmware
has a compiler that is less than 10 years old, I'd suggest using C99
syntax instead, which should avoid those differences and eliminate
all gcc extensions.

I think this isn't only about the firmware (which, as far as I can tell,
is always built with a non-ancient version of GCC). The same header file
is used in other operating systems and I have no idea about the
toolchain situation there.

As for the NO_GCC_EXTENSIONS I think that's only used to avoid empty
structures and zero-sized arrays, which I assume not all supported
toolchains can deal with.

Sivaram, Timo: can you shed any light on the scope of operating systems
and toolchains that we need to support? Any ideas, short of manual
editing, that we can try to eliminate some of Arnd's concerns?

Ok. To clarify, C99 supports this syntax:

struct variable_length_struct {
	int length;
	char data[];
};

struct empty_struct {
	char nothing[];
};

which could be used in place of the gcc specific syntax in
portable code.

	Arnd