include/linux/soundcard.h endian fix

PATCH In review Stuart Brady 3 participants 5 messages Feb 22, 2004 – Mar 7, 2004

lore

Share & tools

lore

Stuart Brady

author

2004-02-22 07:01 UTC #0 lore raw

PATCH 1/1 include/linux/soundcard.h endian fix

Hi,

It seems that <linux/soundcard.h> defines AFMT_S16_NE as AFMT_S16_LE on hppa. Both GCC 3.0 and 3.3 predefine __hppa__, but not HPPA. IMO, the bug is in soundcard.h, not GCC.

Index: soundcard.h
===================================================================
RCS file: /var/cvs/linux-2.4/include/linux/soundcard.h,v retrieving revision 1.6

soundcard.h +1 -1

diff -u -r1.6 soundcard.h
@@ -179,7 +179,7 @@
  * Some big endian/little endian handling macros
  */
 
-#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(PPC) || defined(__mc68000__)
+#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(__hppa__) || defined(PPC) || defined(__powerpc__) || defined(__mc68000__)
 /* Big endian machines */
 #  define _PATCHKEY(id) (0xfd00|id)
 #  define AFMT_S16_NE AFMT_S16_BE

I've added __powerpc__ too, because PPC won't be defined if you compiled with -ansi.

I noticed this problem when looking at Timidity, which was writing big endian data with AFMT_S16_NE. The OSS 1.1 API Spec[1] (page 38) states:

"The AFMT_S16_NE format can be used when a program wants to encode or decode 16-bit samples locally. It automatically selects the right format for the CPU architecture being compiled for. In this way it's usually possible to simply use signed short format to store the samples."

This doesn't really seem to make _solid_ promises that the format matches the architecture's endianness - "Right format?" "Usually?" I wonder whether Timidity is making an incorrect assumption?

Would it be better to do the following? :

#ifdef(__BYTE_ORDER)
# if defined(__BIG_ENDIAN)
# if __BYTE_ORDER == __BIG_ENDIAN
# define AFMT_S16_NE AFMT_S16_BE
# endif
# elif defined(__LITTLE_ENDIAN)
# if __BYTE_ORDER == __LITTLE_ENDIAN
# define AFMT_S16_NE AFMT_S16_LE
# endif
# end if
#endif

After this, if AFMT_S16_NE is not defined, it might be worth checking the appropriate system-specific predefined macros[2]. If it's still not defined, I think is that it's better _not_ to assume little endian. I've left out __PDP_ENDIAN - I doubt anything uses it these days.

[1] http://www.opensound.com/pguide/oss.pdf

[2] Big endian: _AIX, AIX, sparc, __sparc__, __mc68000__, __m68k__, MIPSEB, __MIPSEB__, __ARMEB, HPPA, __hppa__, PPC, __ppc_, __PPC__, __powerpc__. Little endian: i386, __i386__, __x86_64__, __amd64__, __sh__, __vax__ (yes, really!), MIPSEL, __MIPSEL__, and __ARMEL__, __alpha, __alpha__, __ia64__. It seems that ppc, hppa, ia64, alpha, sparc, mips, arm and superh have both big and little endian modes.

Signature · 1 line

Stuart Brady

Randolph Chung

2004-02-23 10:34 UTC #1 lore raw

Quoted 13 lines

Would it be better to do the following? :
#ifdef(__BYTE_ORDER)
# if defined(__BIG_ENDIAN)
# if __BYTE_ORDER == __BIG_ENDIAN
# define AFMT_S16_NE AFMT_S16_BE
# endif
# elif defined(__LITTLE_ENDIAN)
# if __BYTE_ORDER == __LITTLE_ENDIAN
# define AFMT_S16_NE AFMT_S16_LE
# endif
# end if
#endif

yes, this is much better. If you include asm/byteorder.h then this should always be defined. maybe add a #error to catch the (unlikely) case where neither is defined?

randolph

Signature · 3 lines

Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/

Stuart Brady

author

2004-02-23 19:42 UTC #2 lore raw

Quoted 14 lines · Randolph Chung

Quoted 13 lines
Would it be better to do the following? :
#ifdef(__BYTE_ORDER)
# if defined(__BIG_ENDIAN)
# if __BYTE_ORDER == __BIG_ENDIAN
# define AFMT_S16_NE AFMT_S16_BE
# endif
# elif defined(__LITTLE_ENDIAN)
# if __BYTE_ORDER == __LITTLE_ENDIAN
# define AFMT_S16_NE AFMT_S16_LE
# endif
# end if
#endif

Quoted 3 lines

yes, this is much better. If you include asm/byteorder.h then this should always be defined. maybe add a #error to catch the (unlikely) case where neither is defined?

This header is used in userland - is using <asm/byteorder.h> still okay? <endian.h> defines both __LITTLE_ENDIAN and __BIG_ENDIAN - the fact that one of these exists means nothing - you have to look at __BYTE_ORDER. <asm/byteorder.h> will define one but not the other, and __BYTE_ORDER is not defined. If a user includes <endian.h> followed by <sys/soundcard.h>, and we use <asm/byteorder.h>, then both __LITTLE_ENDIAN and __BIG_ENDIAN will be defined at the same time. How about this? :

Index: soundcard.h
===================================================================
RCS file: /var/cvs/linux-2.4/include/linux/soundcard.h,v retrieving revision 1.6

soundcard.h +29 -7

diff -u -r1.6 soundcard.h
@@ -39,6 +39,13 @@
 /* In Linux we need to be prepared for cross compiling */
 #include <linux/ioctl.h>
 
+/* Endian macros. Note that they have a different meaning in the kernel.
+#ifdef __KERNEL__
+#  include <asm/byteorder.h>
+#else
+#  include <endian.h>
+#endif
+
 /*
  *	Supported card ID numbers (Should be somewhere else?)
  */

@@ -179,13 +186,28 @@
  * Some big endian/little endian handling macros
  */
 
-#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(PPC) || defined(__mc68000__)
-/* Big endian machines */
-#  define _PATCHKEY(id) (0xfd00|id)
-#  define AFMT_S16_NE AFMT_S16_BE
-#else
-#  define _PATCHKEY(id) ((id<<8)|0xfd)
-#  define AFMT_S16_NE AFMT_S16_LE
+#if defined(__BIG_ENDIAN)
+#  if defined(__KERNEL__) || (defined(__BYTE_ORDER) && __BYTE_ORDER == __BIG_ENDIAN)
+#    if defined(AFMT_S16_NE) || defined(_PATCHKEY)
+#      error AFMT_S16_NE (or _PATCHKEY) is already defined
+#    endif
+#    define AFMT_S16_NE AFMT_S16_BE
+#    define _PATCHKEY(id) (0xfd00|id)
+#  endif
+#endif
+
+#if defined(__LITTLE_ENDIAN)
+#  if defined(__KERNEL__) || (defined(__BYTE_ORDER) && __BYTE_ORDER == __LITTLE_ENDIAN)
+#    if defined(AFMT_S16_NE) || defined(_PATCHKEY)
+#      error AFMT_S16_NE (or _PATCHKEY) is already defined
+#    endif
+#    define AFMT_S16_NE AFMT_S16_LE
+#    define _PATCHKEY(id) ((id<<8)|0xfd)
+#  endif
+#endif
+
+#if !defined(AFMT_S16_NE)
+#  error Failed to define AFMT_S16_NE
 #endif
 
 /*

I suppose the system-specific tests aren't necessary - if you're using our soundcard.h, then you have our <endian.h>, too. I think the #error is a good idea for the same reason - if the #error happens, something really is wrong, and apps might otherwise silently test for AFMT_S16_NE, possibly even defining it themselves.

The test for AIX || _AIX strongly implies that this header was once intended to be used on other systems. Should we just ignore that?

BTW, AFMT_S32_NE isn't defined, but it's in the specification. It might be worth adding that. AFMT_U16_NE isn't even in the spec.

Signature · 1 line

Stuart Brady

Stuart Brady

author

2004-03-07 17:32 UTC #3 lore raw

Does anyone have any thoughts regarding this patch?

Index: soundcard.h
===================================================================
RCS file: /var/cvs/linux-2.4/include/linux/soundcard.h,v retrieving revision 1.6

soundcard.h +1 -1

diff -u -r1.6 soundcard.h
@@ -179,7 +179,7 @@
  * Some big endian/little endian handling macros
  */
 
-#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(PPC) || defined(__mc68000__)
+#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(__hppa__) || defined(PPC) || defined(__powerpc__) || defined(__mc68000__)
 /* Big endian machines */
 #  define _PATCHKEY(id) (0xfd00|id)
 #  define AFMT_S16_NE AFMT_S16_BE

Or alternatively:

Index: soundcard.h
===================================================================
RCS file: /var/cvs/linux-2.4/include/linux/soundcard.h,v retrieving revision 1.6

soundcard.h +29 -7

diff -u -r1.6 soundcard.h
@@ -39,6 +39,13 @@
 /* In Linux we need to be prepared for cross compiling */
 #include <linux/ioctl.h>
 
+/* Endian macros. Note that they have a different meaning in the kernel. */
+#ifdef __KERNEL__
+#  include <asm/byteorder.h>
+#else
+#  include <endian.h>
+#endif
+
 /*
  *	Supported card ID numbers (Should be somewhere else?)
  */

@@ -179,13 +186,28 @@
  * Some big endian/little endian handling macros
  */
 
-#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(PPC) || defined(__mc68000__)
-/* Big endian machines */
-#  define _PATCHKEY(id) (0xfd00|id)
-#  define AFMT_S16_NE AFMT_S16_BE
-#else
-#  define _PATCHKEY(id) ((id<<8)|0xfd)
-#  define AFMT_S16_NE AFMT_S16_LE
+#if defined(__BIG_ENDIAN)
+#  if defined(__KERNEL__) || (defined(__BYTE_ORDER) && __BYTE_ORDER == __BIG_ENDIAN)
+#    if defined(AFMT_S16_NE) || defined(_PATCHKEY)
+#      error AFMT_S16_NE (or _PATCHKEY) is already defined
+#    endif
+#    define AFMT_S16_NE AFMT_S16_BE
+#    define _PATCHKEY(id) (0xfd00|id)
+#  endif
+#endif
+
+#if defined(__LITTLE_ENDIAN)
+#  if defined(__KERNEL__) || (defined(__BYTE_ORDER) && __BYTE_ORDER == __LITTLE_ENDIAN)
+#    if defined(AFMT_S16_NE) || defined(_PATCHKEY)
+#      error AFMT_S16_NE (or _PATCHKEY) is already defined
+#    endif
+#    define AFMT_S16_NE AFMT_S16_LE
+#    define _PATCHKEY(id) ((id<<8)|0xfd)
+#  endif
+#endif
+
+#if !defined(AFMT_S16_NE)
+#  error Failed to define AFMT_S16_NE
 #endif
 
 /*

Should I be sending this elsewhere?

Thanks,

Signature · 1 line

Stuart Brady

Helge Deller

2004-03-07 18:02 UTC #4 lore raw

Quoted 10 lines · Stuart Brady

Does anyone have any thoughts regarding this patch?
Index: soundcard.h
===================================================================
RCS file: /var/cvs/linux-2.4/include/linux/soundcard.h,v retrieving revision 1.6
diff -u -r1.6 soundcard.h

soundcard.h line 182

Quoted 5 lines

soundcard.h +1 -1

@@ -179,7 +179,7 @@
  * Some big endian/little endian handling macros
  */
 
-#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(PPC) || defined(__mc68000__)
+#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(__hppa__) || defined(PPC) || defined(__powerpc__) || defined(__mc68000__)

I think you should just use "defined(__hppa__)" and simply drop the "defined(HPPA)" part.

Quoted 49 lines

Or alternatively:

Index: soundcard.h
===================================================================
RCS file: /var/cvs/linux-2.4/include/linux/soundcard.h,v retrieving revision 1.6
diff -u -r1.6 soundcard.h

soundcard.h +22 -7

@@ -39,6 +39,13 @@
 /* In Linux we need to be prepared for cross compiling */
 #include <linux/ioctl.h>
 
+/* Endian macros. Note that they have a different meaning in the kernel. */
+#ifdef __KERNEL__
+#  include <asm/byteorder.h>
+#else
+#  include <endian.h>
+#endif
+
 /*
  *	Supported card ID numbers (Should be somewhere else?)
  */

@@ -179,13 +186,28 @@
  * Some big endian/little endian handling macros
  */
 
-#if defined(_AIX) || defined(AIX) || defined(sparc) || defined(__sparc__) || defined(HPPA) || defined(PPC) || defined(__mc68000__)
-/* Big endian machines */
-#  define _PATCHKEY(id) (0xfd00|id)
-#  define AFMT_S16_NE AFMT_S16_BE
-#else
-#  define _PATCHKEY(id) ((id<<8)|0xfd)
-#  define AFMT_S16_NE AFMT_S16_LE
+#if defined(__BIG_ENDIAN)
+#  if defined(__KERNEL__) || (defined(__BYTE_ORDER) && __BYTE_ORDER == __BIG_ENDIAN)
+#    if defined(AFMT_S16_NE) || defined(_PATCHKEY)
+#      error AFMT_S16_NE (or _PATCHKEY) is already defined
+#    endif
+#    define AFMT_S16_NE AFMT_S16_BE
+#    define _PATCHKEY(id) (0xfd00|id)
+#  endif
+#endif
+
+#if defined(__LITTLE_ENDIAN)
+#  if defined(__KERNEL__) || (defined(__BYTE_ORDER) && __BYTE_ORDER == __LITTLE_ENDIAN)
+#    if defined(AFMT_S16_NE) || defined(_PATCHKEY)
+#      error AFMT_S16_NE (or _PATCHKEY) is already defined
+#    endif

soundcard.h line 213

Quoted 10 lines

soundcard.h +7 -0

+#    define AFMT_S16_NE AFMT_S16_LE
+#    define _PATCHKEY(id) ((id<<8)|0xfd)
+#  endif
+#endif
+
+#if !defined(AFMT_S16_NE)
+#  error Failed to define AFMT_S16_NE
 #endif
 
 /*

Personally I think either is OK.

Quoted 1 line

Should I be sending this elsewhere?

[email protected] ?

Helge