* [parisc-linux] [PATCH] include/linux/soundcard.h endian fix
@ 2004-02-22 7:01 Stuart Brady
2004-02-23 12:24 ` Randolph Chung
0 siblings, 1 reply; 5+ messages in thread
From: Stuart Brady @ 2004-02-22 7:01 UTC (permalink / raw)
To: parisc-linux
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
diff -u -r1.6 soundcard.h
--- soundcard.h 26 Jun 2003 15:08:08 -0000 1.6
+++ soundcard.h 21 Feb 2004 16:04:12 -0000
@@ -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.
--
Stuart Brady
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [parisc-linux] [PATCH] include/linux/soundcard.h endian fix
2004-02-22 7:01 [parisc-linux] [PATCH] include/linux/soundcard.h endian fix Stuart Brady
@ 2004-02-23 12:24 ` Randolph Chung
2004-02-23 19:40 ` Stuart Brady
0 siblings, 1 reply; 5+ messages in thread
From: Randolph Chung @ 2004-02-23 12:24 UTC (permalink / raw)
To: Stuart Brady; +Cc: parisc-linux
> 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
--
Randolph Chung
Debian GNU/Linux Developer, hppa/ia64 ports
http://www.tausq.org/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [parisc-linux] [PATCH] include/linux/soundcard.h endian fix
2004-02-23 12:24 ` Randolph Chung
@ 2004-02-23 19:40 ` Stuart Brady
2004-03-07 17:32 ` Stuart Brady
0 siblings, 1 reply; 5+ messages in thread
From: Stuart Brady @ 2004-02-23 19:40 UTC (permalink / raw)
To: parisc-linux
On Mon, Feb 23, 2004 at 04:24:58AM -0800, Randolph Chung wrote:
> > 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?
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
diff -u -r1.6 soundcard.h
--- soundcard.h 26 Jun 2003 15:08:08 -0000 1.6
+++ soundcard.h 23 Feb 2004 15:15:23 -0000
@@ -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.
--
Stuart Brady
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [parisc-linux] [PATCH] include/linux/soundcard.h endian fix
2004-02-23 19:40 ` Stuart Brady
@ 2004-03-07 17:32 ` Stuart Brady
2004-03-07 18:02 ` Helge Deller
0 siblings, 1 reply; 5+ messages in thread
From: Stuart Brady @ 2004-03-07 17:32 UTC (permalink / raw)
To: parisc-linux
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 26 Jun 2003 15:08:08 -0000 1.6
+++ soundcard.h 21 Feb 2004 16:04:12 -0000
@@ -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
diff -u -r1.6 soundcard.h
--- soundcard.h 26 Jun 2003 15:08:08 -0000 1.6
+++ soundcard.h 07 Mar 2004 17:26:23 -0000
@@ -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,
--
Stuart Brady
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [parisc-linux] [PATCH] include/linux/soundcard.h endian fix
2004-03-07 17:32 ` Stuart Brady
@ 2004-03-07 18:02 ` Helge Deller
0 siblings, 0 replies; 5+ messages in thread
From: Helge Deller @ 2004-03-07 18:02 UTC (permalink / raw)
To: parisc-linux
On Sunday 07 March 2004 18:32, Stuart Brady wrote:
> 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 26 Jun 2003 15:08:08 -0000 1.6
> +++ soundcard.h 21 Feb 2004 16:04:12 -0000
> @@ -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.
> 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 26 Jun 2003 15:08:08 -0000 1.6
> +++ soundcard.h 07 Mar 2004 17:26:23 -0000
> @@ -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
>
> /*
Personally I think either is OK.
> Should I be sending this elsewhere?
linux-kernel@vger.kernel.org ?
Helge
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2004-03-07 18:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-02-22 7:01 [parisc-linux] [PATCH] include/linux/soundcard.h endian fix Stuart Brady
2004-02-23 12:24 ` Randolph Chung
2004-02-23 19:40 ` Stuart Brady
2004-03-07 17:32 ` Stuart Brady
2004-03-07 18:02 ` Helge Deller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.