[RFC] ASLA design, depth of code review and lack thereof

[RFC] ASLA design, depth of code review and lack thereof

[viro]

Ladies and gentlemen, may I politely ask what description would fit somebody
who have made the following

        case SNDRV_PCM_FORMAT_FLOAT_BE:
        {
                union {
                        float f;
                        u_int32_t i;
                } u;
                u.f = 0.0;
#ifdef SNDRV_LITTLE_ENDIAN
                return bswap_32(u.i);
#else
                return u.i;
#endif
        }
and quite a few similar, er, wonders an ioctl?

That's right.  This code just has to be in the kernel.  It can't be in
a library, oh no.  It can't be a trivial macro that would result in
compiler generating the constant, no sir - it just had to be proudly
dumped into the great barfbag in the tree.

And that leads to a really interesting question: how many people had ever
read that code?  Or documentation covering that ioctl, while we are at it.

Unless I'm mistaken, ALSA used revision control for a long, long time.
Jaroslav, could you please find the origin of that little wonder and
share with the class - who had done that, why it had been committed into
ALSA tree and how did it manage to survive until the merge into the main
tree?

Re: [RFC] ASLA design, depth of code review and lack thereof

[viro]

On Sat, Jun 05, 2004 at 12:08:19AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> Ladies and gentlemen, may I politely ask what description would fit somebody
> who have made the following
> 
>         case SNDRV_PCM_FORMAT_FLOAT_BE:
>         {
>                 union {
>                         float f;
>                         u_int32_t i;
>                 } u;
>                 u.f = 0.0;
> #ifdef SNDRV_LITTLE_ENDIAN
>                 return bswap_32(u.i);
> #else
>                 return u.i;
> #endif
>         }
> and quite a few similar, er, wonders an ioctl?

... immediately followed by a self-LART - it's still an ugly code, all
right, but that's not an ioctl.

My apologies to everyone.

Re: [RFC] ASLA design, depth of code review and lack thereof

[Linus Torvalds]

On Sat, 5 Jun 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
>
> 
>         case SNDRV_PCM_FORMAT_FLOAT_BE:
>         {
>                 union {
>                         float f;
>                         u_int32_t i;
>                 } u;
>                 u.f = 0.0;
> #ifdef SNDRV_LITTLE_ENDIAN
>                 return bswap_32(u.i);
> #else
>                 return u.i;
> #endif

So what I wonder about is why anybody does something like this in the 
first place?

Any IEEE format architecture will make 0.0 be all-zeroes, last I saw. In
fact, any architecture (IEEE or not) where that isn't true will have
serious problems with floating point values in bss (hint: the bss isn't
initialzed to 0.0, it's initialized to the bit pattern 0).

So what the above boils dow to is a very very strange way of writing

	return 0;

and it has absolutely _zero_ to do with "little-endian" or anything else 
for that matter.

		Linus

Re: [RFC] ASLA design, depth of code review and lack thereof

[viro]

On Fri, Jun 04, 2004 at 04:29:20PM -0700, Linus Torvalds wrote:
> 
> 
> On Sat, 5 Jun 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
> >
> > 
> >         case SNDRV_PCM_FORMAT_FLOAT_BE:
> >         {
> >                 union {
> >                         float f;
> >                         u_int32_t i;
> >                 } u;
> >                 u.f = 0.0;
> > #ifdef SNDRV_LITTLE_ENDIAN
> >                 return bswap_32(u.i);
> > #else
> >                 return u.i;
> > #endif
> 
> So what I wonder about is why anybody does something like this in the 
> first place?
> 
> Any IEEE format architecture will make 0.0 be all-zeroes, last I saw. In
> fact, any architecture (IEEE or not) where that isn't true will have
> serious problems with floating point values in bss (hint: the bss isn't
> initialzed to 0.0, it's initialized to the bit pattern 0).
> 
> So what the above boils dow to is a very very strange way of writing
> 
> 	return 0;
> 
> and it has absolutely _zero_ to do with "little-endian" or anything else 
> for that matter.

	While we are at it, the function in question
(sound/core/pcm_misc.::snd_pcm_format_silence_64())
is a plain and simple array in disguise - it should've been

u_int64_t snd_pcm_format_silence_64(snd_pcm_format_t format)
{
	unsigned index = snd_enum_to_int(format); /* just a cast, hopefully */
	return index < SNDRV_PCM_FORMAT_LAST ? filler[index] : -EINVAL;
}

And -EINVAL here, BTW, is a broken interface - none of its callers are
checking for -EINVAL and that's a hell of an odd error value when normal
values can be all over the place in u64 anyway.

Looking at those callers, I'd say that this guy should be

unsigned char * find_filler(snd_pcm_format_t format)

and return a pointer to a (8-element) row in array of patterns.  Because
callers end up truncating the result and then filling a large area with
repeated copies.  All we get from use of u_int64_t is extra PITA with
endianness - memcpy from 1/2/4/8 element array is no less efficient than
assignment from u8/u16/u32/u64.

Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai]

At Sat, 5 Jun 2004 01:04:10 +0100,
viro@parcelfarce.linux.theplanet.co.uk wrote:
> 
> On Fri, Jun 04, 2004 at 04:29:20PM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Sat, 5 Jun 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
> > >
> > > 
> > >         case SNDRV_PCM_FORMAT_FLOAT_BE:
> > >         {
> > >                 union {
> > >                         float f;
> > >                         u_int32_t i;
> > >                 } u;
> > >                 u.f = 0.0;
> > > #ifdef SNDRV_LITTLE_ENDIAN
> > >                 return bswap_32(u.i);
> > > #else
> > >                 return u.i;
> > > #endif
> > 
> > So what I wonder about is why anybody does something like this in the 
> > first place?
> > 
> > Any IEEE format architecture will make 0.0 be all-zeroes, last I saw. In
> > fact, any architecture (IEEE or not) where that isn't true will have
> > serious problems with floating point values in bss (hint: the bss isn't
> > initialzed to 0.0, it's initialized to the bit pattern 0).
> > 
> > So what the above boils dow to is a very very strange way of writing
> > 
> > 	return 0;
> > 
> > and it has absolutely _zero_ to do with "little-endian" or anything else 
> > for that matter.
> 
> 	While we are at it, the function in question
> (sound/core/pcm_misc.::snd_pcm_format_silence_64())
> is a plain and simple array in disguise - it should've been
> 
> u_int64_t snd_pcm_format_silence_64(snd_pcm_format_t format)
> {
> 	unsigned index = snd_enum_to_int(format); /* just a cast, hopefully */
> 	return index < SNDRV_PCM_FORMAT_LAST ? filler[index] : -EINVAL;
> }
> 
> And -EINVAL here, BTW, is a broken interface - none of its callers are
> checking for -EINVAL and that's a hell of an odd error value when normal
> values can be all over the place in u64 anyway.

Agreed.
(Long time ago, we had supported not so many formats like now, so
 switch() was ok at that time... :)

> Looking at those callers, I'd say that this guy should be
> 
> unsigned char * find_filler(snd_pcm_format_t format)
> 
> and return a pointer to a (8-element) row in array of patterns.  Because
> callers end up truncating the result and then filling a large area with
> repeated copies.  All we get from use of u_int64_t is extra PITA with
> endianness - memcpy from 1/2/4/8 element array is no less efficient than
> assignment from u8/u16/u32/u64.

Is it true?  If gcc really optmizes well like this, yes, surely we can
use memcpy for simplicity.


--
Takashi Iwai <tiwai@suse.de>		ALSA Developer - www.alsa-project.org

Re: [RFC] ASLA design, depth of code review and lack thereof

[viro]

On Mon, Jun 07, 2004 at 03:14:06PM +0200, Takashi Iwai wrote:
> > and return a pointer to a (8-element) row in array of patterns.  Because
> > callers end up truncating the result and then filling a large area with
> > repeated copies.  All we get from use of u_int64_t is extra PITA with
> > endianness - memcpy from 1/2/4/8 element array is no less efficient than
> > assignment from u8/u16/u32/u64.
> 
> Is it true?  If gcc really optmizes well like this, yes, surely we can
> use memcpy for simplicity.

__builtin_memcpy() is definitely smart enough for that: e.g. on x86 (and you
don't get much more register-starved than that)
void b(char *);
void a(char *x, int count)
{
        char buf[8];
        int i;
        b(buf);
        for (i = 0; i < count; i++) {
                __builtin_memcpy(x, buf, 8);
                x += 8;
        }
}
will result (with -O2, which is normal for kernel) in
.L6:
        movl    %ecx, (%ebx)
        movl    %edx, 4(%ebx)
        addl    $8, %ebx
        decl    %eax
        jne     .L6
as the main loop, which gives you what you would get from use of u64.
And yes, constant-sized memcpy() in the kernel will be expanded to
__builtin_memcpy().

Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai]

At Mon, 7 Jun 2004 23:40:45 +0100,
viro@parcelfarce.linux.theplanet.co.uk wrote:
> 
> On Mon, Jun 07, 2004 at 03:14:06PM +0200, Takashi Iwai wrote:
> > > and return a pointer to a (8-element) row in array of patterns.  Because
> > > callers end up truncating the result and then filling a large area with
> > > repeated copies.  All we get from use of u_int64_t is extra PITA with
> > > endianness - memcpy from 1/2/4/8 element array is no less efficient than
> > > assignment from u8/u16/u32/u64.
> > 
> > Is it true?  If gcc really optmizes well like this, yes, surely we can
> > use memcpy for simplicity.
> 
> __builtin_memcpy() is definitely smart enough for that: e.g. on x86 (and you
> don't get much more register-starved than that)
> void b(char *);
> void a(char *x, int count)
> {
>         char buf[8];
>         int i;
>         b(buf);
>         for (i = 0; i < count; i++) {
>                 __builtin_memcpy(x, buf, 8);
>                 x += 8;
>         }
> }
> will result (with -O2, which is normal for kernel) in
> .L6:
>         movl    %ecx, (%ebx)
>         movl    %edx, 4(%ebx)
>         addl    $8, %ebx
>         decl    %eax
>         jne     .L6
> as the main loop, which gives you what you would get from use of u64.
> And yes, constant-sized memcpy() in the kernel will be expanded to
> __builtin_memcpy().

Thanks for the confirmation!

The patch below is a draft version for clean-up and optmization of
that ugly stuffs.  The pattern width is checked before filling loop to
pass the constant to memcpy().  If compiler can optimize such a case
by itself, this becomes superfluous.


Takashi


--- linux/sound/core/pcm_misc.c	4 May 2004 15:01:38 -0000	1.11
+++ linux/sound/core/pcm_misc.c	8 Jun 2004 12:40:20 -0000
@@ -23,12 +23,169 @@
 #include <linux/time.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
-#define bswap_16 swab16
-#define bswap_32 swab32
-#define bswap_64 swab64
 #define SND_PCM_FORMAT_UNKNOWN (-1)
-#define snd_enum_to_int(v) (v)
-#define snd_int_to_enum(v) (v)
+
+struct pcm_format_data {
+	char width;	/* bit width */
+	char phys;	/* physical bit width */
+	char le;	/* 0 = big-endian, 1 = little-endian, -1 = others */
+	char signd;	/* 0 = unsigned, 1 = signed, -1 = others */
+	unsigned char silence[8];	/* silence data to fill */
+};
+
+static struct pcm_format_data pcm_formats[SNDRV_PCM_FORMAT_LAST+1] = {
+	[SNDRV_PCM_FORMAT_S8] = {
+		.width = 8, .phys = 8, .le = -1, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_U8] = {
+		.width = 8, .phys = 8, .le = -1, .signd = 0,
+		.silence = { 0x80 },
+	},
+	[SNDRV_PCM_FORMAT_S16_LE] = {
+		.width = 16, .phys = 16, .le = 1, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_S16_BE] = {
+		.width = 16, .phys = 16, .le = 0, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_U16_LE] = {
+		.width = 16, .phys = 16, .le = 1, .signd = 0,
+		.silence = { 0x00, 0x80 },
+	},
+	[SNDRV_PCM_FORMAT_U16_BE] = {
+		.width = 16, .phys = 16, .le = 0, .signd = 0,
+		.silence = { 0x80, 0x00 },
+	},
+	[SNDRV_PCM_FORMAT_S24_LE] = {
+		.width = 24, .phys = 32, .le = 1, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_S24_BE] = {
+		.width = 24, .phys = 32, .le = 0, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_U24_LE] = {
+		.width = 24, .phys = 32, .le = 1, .signd = 0,
+		.silence = { 0x00, 0x00, 0x80 },
+	},
+	[SNDRV_PCM_FORMAT_U24_BE] = {
+		.width = 24, .phys = 32, .le = 0, .signd = 0,
+		.silence = { 0x80, 0x00, 0x00 },
+	},
+	[SNDRV_PCM_FORMAT_S32_LE] = {
+		.width = 32, .phys = 32, .le = 1, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_S32_BE] = {
+		.width = 32, .phys = 32, .le = 0, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_U32_LE] = {
+		.width = 32, .phys = 32, .le = 1, .signd = 0,
+		.silence = { 0x00, 0x00, 0x00, 0x80 },
+	},
+	[SNDRV_PCM_FORMAT_U32_BE] = {
+		.width = 32, .phys = 32, .le = 0, .signd = 0,
+		.silence = { 0x80, 0x00, 0x00, 0x00 },
+	},
+	[SNDRV_PCM_FORMAT_FLOAT_LE] = {
+		.width = 32, .phys = 32, .le = 1, .signd = -1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_FLOAT_BE] = {
+		.width = 32, .phys = 32, .le = 0, .signd = -1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_FLOAT64_LE] = {
+		.width = 64, .phys = 64, .le = 1, .signd = -1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_FLOAT64_BE] = {
+		.width = 64, .phys = 64, .le = 0, .signd = -1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE] = {
+		.width = 32, .phys = 32, .le = 1, .signd = -1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE] = {
+		.width = 32, .phys = 32, .le = 0, .signd = -1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_MU_LAW] = {
+		.width = 8, .phys = 8, .le = -1, .signd = -1,
+		.silence = { 0x7f },
+	},
+	[SNDRV_PCM_FORMAT_A_LAW] = {
+		.width = 8, .phys = 8, .le = -1, .signd = -1,
+		.silence = { 0x55 },
+	},
+	[SNDRV_PCM_FORMAT_IMA_ADPCM] = {
+		.width = 4, .phys = 4, .le = -1, .signd = -1,
+		.silence = {},
+	},
+	/* FIXME: the following three formats are not defined properly yet */
+	[SNDRV_PCM_FORMAT_MPEG] = {
+		.le = -1, .signd = -1,
+	},
+	[SNDRV_PCM_FORMAT_GSM] = {
+		.le = -1, .signd = -1,
+	},
+	[SNDRV_PCM_FORMAT_SPECIAL] = {
+		.le = -1, .signd = -1,
+	},
+	[SNDRV_PCM_FORMAT_S24_3LE] = {
+		.width = 24, .phys = 24, .le = 1, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_S24_3BE] = {
+		.width = 24, .phys = 24, .le = 0, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_U24_3LE] = {
+		.width = 24, .phys = 24, .le = 1, .signd = 0,
+		.silence = { 0x00, 0x00, 0x80 },
+	},
+	[SNDRV_PCM_FORMAT_U24_3BE] = {
+		.width = 24, .phys = 24, .le = 0, .signd = 0,
+		.silence = { 0x80, 0x00, 0x00 },
+	},
+	[SNDRV_PCM_FORMAT_S20_3LE] = {
+		.width = 20, .phys = 24, .le = 1, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_S20_3BE] = {
+		.width = 20, .phys = 24, .le = 0, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_U20_3LE] = {
+		.width = 20, .phys = 24, .le = 1, .signd = 0,
+		.silence = { 0x00, 0x00, 0x08 },
+	},
+	[SNDRV_PCM_FORMAT_U20_3BE] = {
+		.width = 20, .phys = 24, .le = 0, .signd = 0,
+		.silence = { 0x08, 0x00, 0x00 },
+	},
+	[SNDRV_PCM_FORMAT_S18_3LE] = {
+		.width = 18, .phys = 24, .le = 1, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_S18_3BE] = {
+		.width = 18, .phys = 24, .le = 0, .signd = 1,
+		.silence = {},
+	},
+	[SNDRV_PCM_FORMAT_U18_3LE] = {
+		.width = 18, .phys = 24, .le = 1, .signd = 0,
+		.silence = { 0x00, 0x00, 0x02 },
+	},
+	[SNDRV_PCM_FORMAT_U18_3BE] = {
+		.width = 18, .phys = 24, .le = 0, .signd = 0,
+		.silence = { 0x02, 0x00, 0x00 },
+	},
+};
+
 
 /**
  * snd_pcm_format_signed - Check the PCM format is signed linear
@@ -39,38 +196,12 @@
  */
 int snd_pcm_format_signed(snd_pcm_format_t format)
 {
-	switch (snd_enum_to_int(format)) {
-	case SNDRV_PCM_FORMAT_S8:
-	case SNDRV_PCM_FORMAT_S16_LE:
-	case SNDRV_PCM_FORMAT_S16_BE:
-	case SNDRV_PCM_FORMAT_S24_LE:
-	case SNDRV_PCM_FORMAT_S24_BE:
-	case SNDRV_PCM_FORMAT_S32_LE:
-	case SNDRV_PCM_FORMAT_S32_BE:
-	case SNDRV_PCM_FORMAT_S24_3LE:
-	case SNDRV_PCM_FORMAT_S24_3BE:
-	case SNDRV_PCM_FORMAT_S20_3LE:
-	case SNDRV_PCM_FORMAT_S20_3BE:
-	case SNDRV_PCM_FORMAT_S18_3LE:
-	case SNDRV_PCM_FORMAT_S18_3BE:
-		return 1;
-	case SNDRV_PCM_FORMAT_U8:
-	case SNDRV_PCM_FORMAT_U16_LE:
-	case SNDRV_PCM_FORMAT_U16_BE:
-	case SNDRV_PCM_FORMAT_U24_LE:
-	case SNDRV_PCM_FORMAT_U24_BE:
-	case SNDRV_PCM_FORMAT_U32_LE:
-	case SNDRV_PCM_FORMAT_U32_BE:
-	case SNDRV_PCM_FORMAT_U24_3LE:
-	case SNDRV_PCM_FORMAT_U24_3BE:
-	case SNDRV_PCM_FORMAT_U20_3LE:
-	case SNDRV_PCM_FORMAT_U20_3BE:
-	case SNDRV_PCM_FORMAT_U18_3LE:
-	case SNDRV_PCM_FORMAT_U18_3BE:
-		return 0;
-	default:
+	int val;
+	if (format < 0 || format > SNDRV_PCM_FORMAT_LAST)
 		return -EINVAL;
-	}
+	if ((val = pcm_formats[format].signd) < 0)
+		return -EINVAL;
+	return val;
 }
 
 /**
@@ -110,42 +241,12 @@
  */
 int snd_pcm_format_little_endian(snd_pcm_format_t format)
 {
-	switch (snd_enum_to_int(format)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-	case SNDRV_PCM_FORMAT_U16_LE:
-	case SNDRV_PCM_FORMAT_S24_LE:
-	case SNDRV_PCM_FORMAT_U24_LE:
-	case SNDRV_PCM_FORMAT_S32_LE:
-	case SNDRV_PCM_FORMAT_U32_LE:
-	case SNDRV_PCM_FORMAT_FLOAT_LE:
-	case SNDRV_PCM_FORMAT_FLOAT64_LE:
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
-	case SNDRV_PCM_FORMAT_S24_3LE:
-	case SNDRV_PCM_FORMAT_S20_3LE:
-	case SNDRV_PCM_FORMAT_S18_3LE:
-	case SNDRV_PCM_FORMAT_U24_3LE:
-	case SNDRV_PCM_FORMAT_U20_3LE:
-	case SNDRV_PCM_FORMAT_U18_3LE:
-		return 1;
-	case SNDRV_PCM_FORMAT_S16_BE:
-	case SNDRV_PCM_FORMAT_U16_BE:
-	case SNDRV_PCM_FORMAT_S24_BE:
-	case SNDRV_PCM_FORMAT_U24_BE:
-	case SNDRV_PCM_FORMAT_S32_BE:
-	case SNDRV_PCM_FORMAT_U32_BE:
-	case SNDRV_PCM_FORMAT_FLOAT_BE:
-	case SNDRV_PCM_FORMAT_FLOAT64_BE:
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE:
-	case SNDRV_PCM_FORMAT_S24_3BE:
-	case SNDRV_PCM_FORMAT_S20_3BE:
-	case SNDRV_PCM_FORMAT_S18_3BE:
-	case SNDRV_PCM_FORMAT_U24_3BE:
-	case SNDRV_PCM_FORMAT_U20_3BE:
-	case SNDRV_PCM_FORMAT_U18_3BE:
-		return 0;
-	default:
+	int val;
+	if (format < 0 || format > SNDRV_PCM_FORMAT_LAST)
 		return -EINVAL;
-	}
+	if ((val = pcm_formats[format].le) < 0)
+		return -EINVAL;
+	return val;
 }
 
 /**
@@ -190,55 +291,12 @@
  */
 int snd_pcm_format_width(snd_pcm_format_t format)
 {
-	switch (snd_enum_to_int(format)) {
-	case SNDRV_PCM_FORMAT_S8:
-	case SNDRV_PCM_FORMAT_U8:
-		return 8;
-	case SNDRV_PCM_FORMAT_S16_LE:
-	case SNDRV_PCM_FORMAT_S16_BE:
-	case SNDRV_PCM_FORMAT_U16_LE:
-	case SNDRV_PCM_FORMAT_U16_BE:
-		return 16;
-	case SNDRV_PCM_FORMAT_S18_3LE:
-	case SNDRV_PCM_FORMAT_S18_3BE:
-	case SNDRV_PCM_FORMAT_U18_3LE:
-	case SNDRV_PCM_FORMAT_U18_3BE:
-		return 18;
-	case SNDRV_PCM_FORMAT_S20_3LE:
-	case SNDRV_PCM_FORMAT_S20_3BE:
-	case SNDRV_PCM_FORMAT_U20_3LE:
-	case SNDRV_PCM_FORMAT_U20_3BE:
-		return 20;
-	case SNDRV_PCM_FORMAT_S24_LE:
-	case SNDRV_PCM_FORMAT_S24_BE:
-	case SNDRV_PCM_FORMAT_U24_LE:
-	case SNDRV_PCM_FORMAT_U24_BE:
-	case SNDRV_PCM_FORMAT_S24_3LE:
-	case SNDRV_PCM_FORMAT_S24_3BE:
-	case SNDRV_PCM_FORMAT_U24_3LE:
-	case SNDRV_PCM_FORMAT_U24_3BE:
-		return 24;
-	case SNDRV_PCM_FORMAT_S32_LE:
-	case SNDRV_PCM_FORMAT_S32_BE:
-	case SNDRV_PCM_FORMAT_U32_LE:
-	case SNDRV_PCM_FORMAT_U32_BE:
-	case SNDRV_PCM_FORMAT_FLOAT_LE:
-	case SNDRV_PCM_FORMAT_FLOAT_BE:
-		return 32;
-	case SNDRV_PCM_FORMAT_FLOAT64_LE:
-	case SNDRV_PCM_FORMAT_FLOAT64_BE:
-		return 64;
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE:
-		return 32;
-	case SNDRV_PCM_FORMAT_MU_LAW:
-	case SNDRV_PCM_FORMAT_A_LAW:
-		return 8;
-	case SNDRV_PCM_FORMAT_IMA_ADPCM:
-		return 4;
-	default:
+	int val;
+	if (format < 0 || format > SNDRV_PCM_FORMAT_LAST)
 		return -EINVAL;
-	}
+	if ((val = pcm_formats[format].width) == 0)
+		return -EINVAL;
+	return val;
 }
 
 /**
@@ -250,52 +308,12 @@
  */
 int snd_pcm_format_physical_width(snd_pcm_format_t format)
 {
-	switch (snd_enum_to_int(format)) {
-	case SNDRV_PCM_FORMAT_S8:
-	case SNDRV_PCM_FORMAT_U8:
-		return 8;
-	case SNDRV_PCM_FORMAT_S16_LE:
-	case SNDRV_PCM_FORMAT_S16_BE:
-	case SNDRV_PCM_FORMAT_U16_LE:
-	case SNDRV_PCM_FORMAT_U16_BE:
-		return 16;
-	case SNDRV_PCM_FORMAT_S18_3LE:
-	case SNDRV_PCM_FORMAT_S18_3BE:
-	case SNDRV_PCM_FORMAT_U18_3LE:
-	case SNDRV_PCM_FORMAT_U18_3BE:
-	case SNDRV_PCM_FORMAT_S20_3LE:
-	case SNDRV_PCM_FORMAT_S20_3BE:
-	case SNDRV_PCM_FORMAT_U20_3LE:
-	case SNDRV_PCM_FORMAT_U20_3BE:
-	case SNDRV_PCM_FORMAT_S24_3LE:
-	case SNDRV_PCM_FORMAT_S24_3BE:
-	case SNDRV_PCM_FORMAT_U24_3LE:
-	case SNDRV_PCM_FORMAT_U24_3BE:
-		return 24;
-	case SNDRV_PCM_FORMAT_S24_LE:
-	case SNDRV_PCM_FORMAT_S24_BE:
-	case SNDRV_PCM_FORMAT_U24_LE:
-	case SNDRV_PCM_FORMAT_U24_BE:
-	case SNDRV_PCM_FORMAT_S32_LE:
-	case SNDRV_PCM_FORMAT_S32_BE:
-	case SNDRV_PCM_FORMAT_U32_LE:
-	case SNDRV_PCM_FORMAT_U32_BE:
-	case SNDRV_PCM_FORMAT_FLOAT_LE:
-	case SNDRV_PCM_FORMAT_FLOAT_BE:
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE:
-		return 32;
-	case SNDRV_PCM_FORMAT_FLOAT64_LE:
-	case SNDRV_PCM_FORMAT_FLOAT64_BE:
-		return 64;
-	case SNDRV_PCM_FORMAT_MU_LAW:
-	case SNDRV_PCM_FORMAT_A_LAW:
-		return 8;
-	case SNDRV_PCM_FORMAT_IMA_ADPCM:
-		return 4;
-	default:
+	int val;
+	if (format < 0 || format > SNDRV_PCM_FORMAT_LAST)
 		return -EINVAL;
-	}
+	if ((val = pcm_formats[format].phys) == 0)
+		return -EINVAL;
+	return val;
 }
 
 /**
@@ -307,216 +325,25 @@
  */
 ssize_t snd_pcm_format_size(snd_pcm_format_t format, size_t samples)
 {
-	switch (snd_enum_to_int(format)) {
-	case SNDRV_PCM_FORMAT_S8:
-	case SNDRV_PCM_FORMAT_U8:
-		return samples;
-	case SNDRV_PCM_FORMAT_S16_LE:
-	case SNDRV_PCM_FORMAT_S16_BE:
-	case SNDRV_PCM_FORMAT_U16_LE:
-	case SNDRV_PCM_FORMAT_U16_BE:
-		return samples * 2;
-	case SNDRV_PCM_FORMAT_S18_3LE:
-	case SNDRV_PCM_FORMAT_S18_3BE:
-	case SNDRV_PCM_FORMAT_U18_3LE:
-	case SNDRV_PCM_FORMAT_U18_3BE:
-	case SNDRV_PCM_FORMAT_S20_3LE:
-	case SNDRV_PCM_FORMAT_S20_3BE:
-	case SNDRV_PCM_FORMAT_U20_3LE:
-	case SNDRV_PCM_FORMAT_U20_3BE:
-	case SNDRV_PCM_FORMAT_S24_3LE:
-	case SNDRV_PCM_FORMAT_S24_3BE:
-	case SNDRV_PCM_FORMAT_U24_3LE:
-	case SNDRV_PCM_FORMAT_U24_3BE:
-		return samples * 3;
-	case SNDRV_PCM_FORMAT_S24_LE:
-	case SNDRV_PCM_FORMAT_S24_BE:
-	case SNDRV_PCM_FORMAT_U24_LE:
-	case SNDRV_PCM_FORMAT_U24_BE:
-	case SNDRV_PCM_FORMAT_S32_LE:
-	case SNDRV_PCM_FORMAT_S32_BE:
-	case SNDRV_PCM_FORMAT_U32_LE:
-	case SNDRV_PCM_FORMAT_U32_BE:
-	case SNDRV_PCM_FORMAT_FLOAT_LE:
-	case SNDRV_PCM_FORMAT_FLOAT_BE:
-		return samples * 4;
-	case SNDRV_PCM_FORMAT_FLOAT64_LE:
-	case SNDRV_PCM_FORMAT_FLOAT64_BE:
-		return samples * 8;
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE:
-		return samples * 4;
-	case SNDRV_PCM_FORMAT_MU_LAW:
-	case SNDRV_PCM_FORMAT_A_LAW:
-		return samples;
-	case SNDRV_PCM_FORMAT_IMA_ADPCM:
-		if (samples & 1)
-			return -EINVAL;
-		return samples / 2;
-	default:
+	int phys_width = snd_pcm_format_physical_width(format);
+	if (phys_width < 0)
 		return -EINVAL;
-	}
+	return samples * phys_width / 8;
 }
 
 /**
- * snd_pcm_format_silence_64 - return the silent data in 64bit integer
+ * snd_pcm_format_silence_64 - return the silent data in 8 bytes array
  * @format: the format to check
  *
- * Returns the silent data in 64bit integer for the given format.
+ * Returns the format pattern to fill or NULL if error.
  */
-u_int64_t snd_pcm_format_silence_64(snd_pcm_format_t format)
+const unsigned char *snd_pcm_format_silence_64(snd_pcm_format_t format)
 {
-	switch (snd_enum_to_int(format)) {
-	case SNDRV_PCM_FORMAT_S8:
-	case SNDRV_PCM_FORMAT_S16_LE:
-	case SNDRV_PCM_FORMAT_S16_BE:
-	case SNDRV_PCM_FORMAT_S24_LE:
-	case SNDRV_PCM_FORMAT_S24_BE:
-	case SNDRV_PCM_FORMAT_S32_LE:
-	case SNDRV_PCM_FORMAT_S32_BE:
-	case SNDRV_PCM_FORMAT_S24_3LE:
-	case SNDRV_PCM_FORMAT_S24_3BE:
-	case SNDRV_PCM_FORMAT_S20_3LE:
-	case SNDRV_PCM_FORMAT_S20_3BE:
-	case SNDRV_PCM_FORMAT_S18_3LE:
-	case SNDRV_PCM_FORMAT_S18_3BE:
-		return 0;
-	case SNDRV_PCM_FORMAT_U8:
-		return 0x8080808080808080ULL;
-#ifdef SNDRV_LITTLE_ENDIAN
-	case SNDRV_PCM_FORMAT_U16_LE:
-		return 0x8000800080008000ULL;
-	case SNDRV_PCM_FORMAT_U24_LE:
-		return 0x0080000000800000ULL;
-	case SNDRV_PCM_FORMAT_U32_LE:
-		return 0x8000000080000000ULL;
-	case SNDRV_PCM_FORMAT_U16_BE:
-		return 0x0080008000800080ULL;
-	case SNDRV_PCM_FORMAT_U24_BE:
-		return 0x0000800000008000ULL;
-	case SNDRV_PCM_FORMAT_U32_BE:
-		return 0x0000008000000080ULL;
-	case SNDRV_PCM_FORMAT_U24_3LE:
-		return 0x0000800000800000ULL;
-	case SNDRV_PCM_FORMAT_U24_3BE:
-		return 0x0080000080000080ULL;
-	case SNDRV_PCM_FORMAT_U20_3LE:
-		return 0x0000080000080000ULL;
-	case SNDRV_PCM_FORMAT_U20_3BE:
-		return 0x0008000008000008ULL;
-	case SNDRV_PCM_FORMAT_U18_3LE:
-		return 0x0000020000020000ULL;
-	case SNDRV_PCM_FORMAT_U18_3BE:
-		return 0x0002000002000002ULL;
-#else
-	case SNDRV_PCM_FORMAT_U16_LE:
-		return 0x0080008000800080ULL;
-	case SNDRV_PCM_FORMAT_U24_LE:
-		return 0x0000800000008000ULL;
-	case SNDRV_PCM_FORMAT_U32_LE:
-		return 0x0000008000000080ULL;
-	case SNDRV_PCM_FORMAT_U16_BE:
-		return 0x8000800080008000ULL;
-	case SNDRV_PCM_FORMAT_U24_BE:
-		return 0x0080000000800000ULL;
-	case SNDRV_PCM_FORMAT_U32_BE:
-		return 0x8000000080000000ULL;
-	case SNDRV_PCM_FORMAT_U24_3LE:
-		return 0x0080000080000080ULL;
-	case SNDRV_PCM_FORMAT_U24_3BE:
-		return 0x0000800000800000ULL;
-	case SNDRV_PCM_FORMAT_U20_3LE:
-		return 0x0008000008000008ULL;
-	case SNDRV_PCM_FORMAT_U20_3BE:
-		return 0x0000080000080000ULL;
-	case SNDRV_PCM_FORMAT_U18_3LE:
-		return 0x0002000002000002ULL;
-	case SNDRV_PCM_FORMAT_U18_3BE:
-		return 0x0000020000020000ULL;
-#endif
-	case SNDRV_PCM_FORMAT_FLOAT_LE:
-	{
-		union {
-			float f;
-			u_int32_t i;
-		} u;
-		u.f = 0.0;
-#ifdef SNDRV_LITTLE_ENDIAN
-		return u.i;
-#else
-		return bswap_32(u.i);
-#endif
-	}
-	case SNDRV_PCM_FORMAT_FLOAT64_LE:
-	{
-		union {
-			double f;
-			u_int64_t i;
-		} u;
-		u.f = 0.0;
-#ifdef SNDRV_LITTLE_ENDIAN
-		return u.i;
-#else
-		return bswap_64(u.i);
-#endif
-	}
-	case SNDRV_PCM_FORMAT_FLOAT_BE:		
-	{
-		union {
-			float f;
-			u_int32_t i;
-		} u;
-		u.f = 0.0;
-#ifdef SNDRV_LITTLE_ENDIAN
-		return bswap_32(u.i);
-#else
-		return u.i;
-#endif
-	}
-	case SNDRV_PCM_FORMAT_FLOAT64_BE:
-	{
-		union {
-			double f;
-			u_int64_t i;
-		} u;
-		u.f = 0.0;
-#ifdef SNDRV_LITTLE_ENDIAN
-		return bswap_64(u.i);
-#else
-		return u.i;
-#endif
-	}
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
-	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE:
-		return 0;	
-	case SNDRV_PCM_FORMAT_MU_LAW:
-		return 0x7f7f7f7f7f7f7f7fULL;
-	case SNDRV_PCM_FORMAT_A_LAW:
-		return 0x5555555555555555ULL;
-	case SNDRV_PCM_FORMAT_IMA_ADPCM:	/* special case */
-	case SNDRV_PCM_FORMAT_MPEG:
-	case SNDRV_PCM_FORMAT_GSM:
-	case SNDRV_PCM_FORMAT_SPECIAL:
-		return 0;
-	default:
-		return -EINVAL;
-	}
-	return 0;
-}
-
-u_int32_t snd_pcm_format_silence_32(snd_pcm_format_t format)
-{
-	return (u_int32_t)snd_pcm_format_silence_64(format);
-}
-
-u_int16_t snd_pcm_format_silence_16(snd_pcm_format_t format)
-{
-	return (u_int16_t)snd_pcm_format_silence_64(format);
-}
-
-u_int8_t snd_pcm_format_silence(snd_pcm_format_t format)
-{
-	return (u_int8_t)snd_pcm_format_silence_64(format);
+	if (format < 0 || format > SNDRV_PCM_FORMAT_LAST)
+		return NULL;
+	if (! pcm_formats[format].phys)
+		return NULL;
+	return pcm_formats[format].silence;
 }
 
 /**
@@ -531,99 +358,73 @@
  */
 int snd_pcm_format_set_silence(snd_pcm_format_t format, void *data, unsigned int samples)
 {
+	int width;
+	unsigned char *dst, *pat;
+
+	if (format < 0 || format > SNDRV_PCM_FORMAT_LAST)
+		return -EINVAL;
 	if (samples == 0)
 		return 0;
-	switch (snd_pcm_format_width(format)) {
-	case 4: {
-		u_int8_t silence = snd_pcm_format_silence_64(format);
-		unsigned int samples1;
-		if (samples % 2 != 0)
-			return -EINVAL;
-		samples1 = samples / 2;
-		memset(data, silence, samples1);
-		break;
+	width = pcm_formats[format].phys; /* physical width */
+	pat = pcm_formats[format].silence;
+	if (! width)
+		return -EINVAL;
+	/* signed or 1 byte data */
+	if (pcm_formats[format].signd == 1 || width <= 8) {
+		unsigned int bytes = samples * width / 8;
+		memset(data, *pat, bytes);
+		return 0;
 	}
-	case 8: {
-		u_int8_t silence = snd_pcm_format_silence_64(format);
-		memset(data, silence, samples);
-		break;
+	/* non-zero samples, fill using a loop */
+	width /= 8;
+	dst = data;
+#if 0
+	while (samples--) {
+		memcpy(dst, pat, width);
+		dst += width;
 	}
-	case 16: {
-		u_int16_t silence = snd_pcm_format_silence_64(format);
-		if (! silence)
-			memset(data, 0, samples * 2);
-		else {
-			u_int16_t *data16 = data;
-			while (samples-- > 0)
-				*data16++ = silence;
+#else
+	/* a bit optimization for constant width */
+	switch (width) {
+	case 2:
+		while (samples--) {
+			memcpy(dst, pat, 2);
+			dst += 2;
 		}
 		break;
-	}
-	case 24: {
-		u_int32_t silence = snd_pcm_format_silence_64(format);
-		if (! silence)
-			memset(data, 0, samples * 3);
-		else {
-			while (samples-- > 0) {
-				u_int8_t *data8 = data;
-#ifdef SNDRV_LITTLE_ENDIAN
-				*data8++ = silence >> 0;
-				*data8++ = silence >> 8;
-				*data8++ = silence >> 16;
-#else
-				*data8++ = silence >> 16;
-				*data8++ = silence >> 8;
-				*data8++ = silence >> 0;
-#endif
-			}
+	case 3:
+		while (samples--) {
+			memcpy(dst, pat, 3);
+			dst += 3;
 		}
 		break;
-	}
-	case 32: {
-		u_int32_t silence = snd_pcm_format_silence_64(format);
-		if (! silence)
-			memset(data, 0, samples * 4);
-		else {
-			u_int32_t *data32 = data;
-			while (samples-- > 0)
-				*data32++ = silence;
+	case 4:
+		while (samples--) {
+			memcpy(dst, pat, 4);
+			dst += 4;
 		}
 		break;
-	}
-	case 64: {
-		u_int64_t silence = snd_pcm_format_silence_64(format);
-		if (! silence)
-			memset(data, 0, samples * 8);
-		else {
-			u_int64_t *data64 = data;
-			while (samples-- > 0)
-				*data64++ = silence;
+	case 8:
+		while (samples--) {
+			memcpy(dst, pat, 8);
+			dst += 8;
 		}
 		break;
 	}
-	default:
-		return -EINVAL;
-	}
+#endif
 	return 0;
 }
 
-static int linear_formats[4*2*2] = {
-	SNDRV_PCM_FORMAT_S8,
-	SNDRV_PCM_FORMAT_S8,
-	SNDRV_PCM_FORMAT_U8,
-	SNDRV_PCM_FORMAT_U8,
-	SNDRV_PCM_FORMAT_S16_LE,
-	SNDRV_PCM_FORMAT_S16_BE,
-	SNDRV_PCM_FORMAT_U16_LE,
-	SNDRV_PCM_FORMAT_U16_BE,
-	SNDRV_PCM_FORMAT_S24_LE,
-	SNDRV_PCM_FORMAT_S24_BE,
-	SNDRV_PCM_FORMAT_U24_LE,
-	SNDRV_PCM_FORMAT_U24_BE,
-	SNDRV_PCM_FORMAT_S32_LE,
-	SNDRV_PCM_FORMAT_S32_BE,
-	SNDRV_PCM_FORMAT_U32_LE,
-	SNDRV_PCM_FORMAT_U32_BE
+/* [width][unsigned][bigendian] */
+static int linear_formats[4][2][2] = {
+	{{ SNDRV_PCM_FORMAT_S8, SNDRV_PCM_FORMAT_S8},
+	 { SNDRV_PCM_FORMAT_U8, SNDRV_PCM_FORMAT_U8}},
+	{{SNDRV_PCM_FORMAT_S16_LE, SNDRV_PCM_FORMAT_S16_BE},
+	 {SNDRV_PCM_FORMAT_U16_LE, SNDRV_PCM_FORMAT_U16_BE}},
+	{{SNDRV_PCM_FORMAT_S24_LE, SNDRV_PCM_FORMAT_S24_BE},
+	 {SNDRV_PCM_FORMAT_U24_LE, SNDRV_PCM_FORMAT_U24_BE}},
+	{{SNDRV_PCM_FORMAT_S32_LE, SNDRV_PCM_FORMAT_S32_BE},
+	 {SNDRV_PCM_FORMAT_U32_LE, SNDRV_PCM_FORMAT_U32_BE}}
 };
 
 /**
@@ -636,23 +437,12 @@
  */
 snd_pcm_format_t snd_pcm_build_linear_format(int width, int unsignd, int big_endian)
 {
-	switch (width) {
-	case 8:
-		width = 0;
-		break;
-	case 16:
-		width = 1;
-		break;
-	case 24:
-		width = 2;
-		break;
-	case 32:
-		width = 3;
-		break;
-	default:
+	if (width & 7)
 		return SND_PCM_FORMAT_UNKNOWN;
-	}
-	return snd_int_to_enum(((int(*)[2][2])linear_formats)[width][!!unsignd][!!big_endian]);
+	width = (width / 8) - 1;
+	if (width < 0 || width >= 4)
+		return SND_PCM_FORMAT_UNKNOWN;
+	return linear_formats[width][!!unsignd][!!big_endian];
 }
 
 /**
--- linux/include/sound/pcm.h	19 Mar 2004 20:13:58 -0000	1.41
+++ linux/include/sound/pcm.h	8 Jun 2004 10:42:00 -0000
@@ -851,7 +851,7 @@
 int snd_pcm_format_big_endian(snd_pcm_format_t format);
 int snd_pcm_format_width(snd_pcm_format_t format);			/* in bits */
 int snd_pcm_format_physical_width(snd_pcm_format_t format);		/* in bits */
-u_int64_t snd_pcm_format_silence_64(snd_pcm_format_t format);
+const unsigned char *snd_pcm_format_silence_64(snd_pcm_format_t format);
 int snd_pcm_format_set_silence(snd_pcm_format_t format, void *buf, unsigned int frames);
 snd_pcm_format_t snd_pcm_build_linear_format(int width, int unsignd, int big_endian);
 ssize_t snd_pcm_format_size(snd_pcm_format_t format, size_t samples);
--- linux/sound/core/oss/pcm_plugin.c	6 Feb 2004 17:46:56 -0000	1.17
+++ linux/sound/core/oss/pcm_plugin.c	8 Jun 2004 12:25:36 -0000
@@ -846,41 +846,31 @@
 			 size_t samples, int format)
 {
 	/* FIXME: sub byte resolution and odd dst_offset */
-	char *dst;
+	unsigned char *dst;
 	unsigned int dst_step;
 	int width;
-	u_int64_t silence;
+	const unsigned char *silence;
 	if (!dst_area->addr)
 		return 0;
 	dst = dst_area->addr + (dst_area->first + dst_area->step * dst_offset) / 8;
 	width = snd_pcm_format_physical_width(format);
+	if (width <= 0)
+		return -EINVAL;
+	if (dst_area->step == (unsigned int) width && width >= 8)
+		return snd_pcm_format_set_silence(format, dst, samples);
 	silence = snd_pcm_format_silence_64(format);
-	if (dst_area->step == (unsigned int) width) {
-		size_t dwords = samples * width / 64;
-		u_int64_t *dst64 = (u_int64_t *)dst;
-
-		samples -= dwords * 64 / width;
-		while (dwords-- > 0)
-			*dst64++ = silence;
-		if (samples == 0)
-			return 0;
-		dst = (char *)dst64;
-	}
+	if (! silence)
+		return -EINVAL;
 	dst_step = dst_area->step / 8;
-	switch (width) {
-	case 4: {
-		u_int8_t s0 = silence & 0xf0;
-		u_int8_t s1 = silence & 0x0f;
+	if (width == 4) {
+		/* Ima ADPCM */
 		int dstbit = dst_area->first % 8;
 		int dstbit_step = dst_area->step % 8;
 		while (samples-- > 0) {
-			if (dstbit) {
+			if (dstbit)
 				*dst &= 0xf0;
-				*dst |= s1;
-			} else {
+			else
 				*dst &= 0x0f;
-				*dst |= s0;
-			}
 			dst += dst_step;
 			dstbit += dstbit_step;
 			if (dstbit == 8) {
@@ -888,41 +878,12 @@
 				dstbit = 0;
 			}
 		}
-		break;
-	}
-	case 8: {
-		u_int8_t sil = silence;
-		while (samples-- > 0) {
-			*dst = sil;
-			dst += dst_step;
-		}
-		break;
-	}
-	case 16: {
-		u_int16_t sil = silence;
-		while (samples-- > 0) {
-			*(u_int16_t*)dst = sil;
-			dst += dst_step;
-		}
-		break;
-	}
-	case 32: {
-		u_int32_t sil = silence;
-		while (samples-- > 0) {
-			*(u_int32_t*)dst = sil;
-			dst += dst_step;
-		}
-		break;
-	}
-	case 64: {
+	} else {
+		width /= 8;
 		while (samples-- > 0) {
-			*(u_int64_t*)dst = silence;
+			memcpy(dst, silence, width);
 			dst += dst_step;
 		}
-		break;
-	}
-	default:
-		snd_BUG();
 	}
 	return 0;
 }
@@ -942,18 +903,18 @@
 	if (!dst_area->addr)
 		return 0;
 	width = snd_pcm_format_physical_width(format);
+	if (width <= 0)
+		return -EINVAL;
 	if (src_area->step == (unsigned int) width &&
-	    dst_area->step == (unsigned int) width) {
+	    dst_area->step == (unsigned int) width && width >= 8) {
 		size_t bytes = samples * width / 8;
-		samples -= bytes * 8 / width;
 		memcpy(dst, src, bytes);
-		if (samples == 0)
-			return 0;
+		return 0;
 	}
 	src_step = src_area->step / 8;
 	dst_step = dst_area->step / 8;
-	switch (width) {
-	case 4: {
+	if (width == 4) {
+		/* Ima ADPCM */
 		int srcbit = src_area->first % 8;
 		int srcbit_step = src_area->step % 8;
 		int dstbit = dst_area->first % 8;
@@ -963,12 +924,11 @@
 			if (srcbit)
 				srcval = *src & 0x0f;
 			else
-				srcval = *src & 0xf0;
+				srcval = (*src & 0xf0) >> 4;
 			if (dstbit)
-				*dst &= 0xf0;
+				*dst = (*dst & 0xf0) | srcval;
 			else
-				*dst &= 0x0f;
-			*dst |= srcval;
+				*dst = (*dst & 0x0f) | (srcval << 4);
 			src += src_step;
 			srcbit += srcbit_step;
 			if (srcbit == 8) {
@@ -982,42 +942,13 @@
 				dstbit = 0;
 			}
 		}
-		break;
-	}
-	case 8: {
-		while (samples-- > 0) {
-			*dst = *src;
-			src += src_step;
-			dst += dst_step;
-		}
-		break;
-	}
-	case 16: {
+	} else {
+		width /= 8;
 		while (samples-- > 0) {
-			*(u_int16_t*)dst = *(u_int16_t*)src;
+			memcpy(dst, src, width);
 			src += src_step;
 			dst += dst_step;
 		}
-		break;
-	}
-	case 32: {
-		while (samples-- > 0) {
-			*(u_int32_t*)dst = *(u_int32_t*)src;
-			src += src_step;
-			dst += dst_step;
-		}
-		break;
-	}
-	case 64: {
-		while (samples-- > 0) {
-			*(u_int64_t*)dst = *(u_int64_t*)src;
-			src += src_step;
-			dst += dst_step;
-		}
-		break;
-	}
-	default:
-		snd_BUG();
 	}
 	return 0;
 }

Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai]

At Fri, 4 Jun 2004 16:29:20 -0700 (PDT),
Linus Torvalds wrote:
> 
> 
> 
> On Sat, 5 Jun 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
> >
> > 
> >         case SNDRV_PCM_FORMAT_FLOAT_BE:
> >         {
> >                 union {
> >                         float f;
> >                         u_int32_t i;
> >                 } u;
> >                 u.f = 0.0;
> > #ifdef SNDRV_LITTLE_ENDIAN
> >                 return bswap_32(u.i);
> > #else
> >                 return u.i;
> > #endif
> 
> So what I wonder about is why anybody does something like this in the 
> first place?
> 
> Any IEEE format architecture will make 0.0 be all-zeroes, last I saw. In
> fact, any architecture (IEEE or not) where that isn't true will have
> serious problems with floating point values in bss (hint: the bss isn't
> initialzed to 0.0, it's initialized to the bit pattern 0).
> 
> So what the above boils dow to is a very very strange way of writing
> 
> 	return 0;
> 
> and it has absolutely _zero_ to do with "little-endian" or anything else 
> for that matter.

Yes, fully agreed.
We'll fix the code around this.


--
Takashi Iwai <tiwai@suse.de>		ALSA Developer - www.alsa-project.org

Re: [RFC] ASLA design, depth of code review and lack thereof

[Jeff Garzik]

While we're bitching about ALSA, can we please kill the 
subsystem-specific malloc and "magic cast" wrappers?

This debug machinery is better done elsewhere...

Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai]

At Fri, 04 Jun 2004 19:37:54 -0400,
Jeff Garzik wrote:
> 
> 
> While we're bitching about ALSA, can we please kill the 
> subsystem-specific malloc and "magic cast" wrappers?
> This debug machinery is better done elsewhere...

Of course we can remove kmalloc/vmalloc wrappers for cast checking
once when the kernel core provides a similar debugging mechanism.
Do you know any alternative?


Takashi

Re: [RFC] ASLA design, depth of code review and lack thereof

[Jeff Garzik]

Takashi Iwai wrote:
> At Fri, 04 Jun 2004 19:37:54 -0400,
> Jeff Garzik wrote:
> 
>>
>>While we're bitching about ALSA, can we please kill the 
>>subsystem-specific malloc and "magic cast" wrappers?
>>This debug machinery is better done elsewhere...
> 
> 
> Of course we can remove kmalloc/vmalloc wrappers for cast checking
> once when the kernel core provides a similar debugging mechanism.
> Do you know any alternative?


CONFIG_DEBUG_SLAB and CONFIG_DEBUG_PAGEALLOC.  Other stuff should go 
into ALSA developer trees or use of a debugger, not be in mainline 
kernel code.

	Jeff

Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai]

At Mon, 07 Jun 2004 09:47:40 -0400,
Jeff Garzik wrote:
> 
> Takashi Iwai wrote:
> > At Fri, 04 Jun 2004 19:37:54 -0400,
> > Jeff Garzik wrote:
> > 
> >>
> >>While we're bitching about ALSA, can we please kill the 
> >>subsystem-specific malloc and "magic cast" wrappers?
> >>This debug machinery is better done elsewhere...
> > 
> > 
> > Of course we can remove kmalloc/vmalloc wrappers for cast checking
> > once when the kernel core provides a similar debugging mechanism.
> > Do you know any alternative?
> 
> 
> CONFIG_DEBUG_SLAB and CONFIG_DEBUG_PAGEALLOC.  Other stuff should go 
> into ALSA developer trees or use of a debugger, not be in mainline 
> kernel code.

They're nice but they don't provide "cast checking", no?
The main purpose of the magic_* stuffs in ALSA is to check the cast of
the void pointer back to the original data type, which the compiler
can't check.

Maybe we can implement only this "magic" check separetly and get rid
of allocation checks, but I'm not sure whether it's worthy to do
that.


Takashi

Re: [RFC] ASLA design, depth of code review and lack thereof

[Jeff Garzik]

Takashi Iwai wrote:
> They're nice but they don't provide "cast checking", no?
> The main purpose of the magic_* stuffs in ALSA is to check the cast of
> the void pointer back to the original data type, which the compiler
> can't check.

Sure -- and that magic cast stuff is horribly bloated, and not needed in 
good code.

No other code in Linux does this -- therefore it should be removed.

	Jeff

Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai]

At Mon, 07 Jun 2004 10:05:41 -0400,
Jeff Garzik wrote:
> 
> Takashi Iwai wrote:
> > They're nice but they don't provide "cast checking", no?
> > The main purpose of the magic_* stuffs in ALSA is to check the cast of
> > the void pointer back to the original data type, which the compiler
> > can't check.
> 
> Sure -- and that magic cast stuff is horribly bloated, and not needed in 
> good code.

A good code needs never debugging stuffs :)

I agree that the current ALSA's magic stuff is much bigger than
needed, though.

> No other code in Linux does this -- therefore it should be removed.

Hmm?  The magic check itself is found in many linux driver codes...


Takashi

Re: [RFC] ASLA design, depth of code review and lack thereof

[Russell King]

On Mon, Jun 07, 2004 at 03:57:51PM +0200, Takashi Iwai wrote:
> They're nice but they don't provide "cast checking", no?
> The main purpose of the magic_* stuffs in ALSA is to check the cast of
> the void pointer back to the original data type, which the compiler
> can't check.
> 
> Maybe we can implement only this "magic" check separetly and get rid
> of allocation checks, but I'm not sure whether it's worthy to do
> that.

I must ask the question: why is ALSA such a special case that it
needs this level of "magic" checking, when the rest of the kernel
has happily used void pointers to carry driver specific data around
for the last 10 years without serious incident or any major debugging
problems?

As long as objects are cleanly defined, and it is obvious what these
private driver specific pointers are for, then this "magic" become
unnecessary.  Take a look at the driver model for instance.

For instance, dev_set_drvdata() and dev_get_drvdata() provide access
to a clearly defined void pointer for drivers to use.  It is clear
that a device driver uses it to place its private data structure
there, and it is the only code which should be accessing that.

I guess though that the problem area for ALSA is the way the snd_pcm_t
private_data member magically appears in the snd_pcm_substream_t
private_data member behind the drivers back, so it's unclear who
actually owns the data in the private_data members.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai]

At Mon, 7 Jun 2004 15:18:06 +0100,
Russell King wrote:
> 
> On Mon, Jun 07, 2004 at 03:57:51PM +0200, Takashi Iwai wrote:
> > They're nice but they don't provide "cast checking", no?
> > The main purpose of the magic_* stuffs in ALSA is to check the cast of
> > the void pointer back to the original data type, which the compiler
> > can't check.
> > 
> > Maybe we can implement only this "magic" check separetly and get rid
> > of allocation checks, but I'm not sure whether it's worthy to do
> > that.
> 
> I must ask the question: why is ALSA such a special case that it
> needs this level of "magic" checking, when the rest of the kernel
> has happily used void pointers to carry driver specific data around
> for the last 10 years without serious incident or any major debugging
> problems?

Sure, it isn't always needed for the matured drivers.

The magic check is a (kind of) debugging stuff, as you can see even
in linux/spinlock.h.  The ugliness in ALSA's implementation is that it
isn't transparent but forces to use some unusual macros.  From the
viewpoint of code readability, I'm willing to get rid of them, too.
But this indeed can help sometimes to catch silly bugs, especially in
the initial development phase.  That's the reason I hesitate to remove
it completely.


> As long as objects are cleanly defined, and it is obvious what these
> private driver specific pointers are for, then this "magic" become
> unnecessary.  Take a look at the driver model for instance.
> 
> For instance, dev_set_drvdata() and dev_get_drvdata() provide access
> to a clearly defined void pointer for drivers to use.  It is clear
> that a device driver uses it to place its private data structure
> there, and it is the only code which should be accessing that.

That's a good point.  Having the interface for accessing such a
private data is better.

However, the obtained data is still void pointer.  Suppose that you
have a driver spreaded over several source files and you change the
data type of this private data.  A careless programmer like me might
forget to change a part in another file.  The driver still compiles
and runs fine unless it hits casually the unchanged part and results
in memory corruption.

That's a silly scenario, but I believe the dynamic cast check is
usefull for catching such a bug easily.  That's why we see similar
magic checks here and there.  The question is how clean it's
represented.

I'm thinking of a simpler way, i.e. introducing macros something like:

#ifdef DEBUG
#define MAGIC_NUMBER	unsigned int __magic_number;
#define SET_MAGIC(rec,magic)	((rec)->__magic_number = (magic))
#define CHECK_MAGIC(rec,magic,action)	do { \
	if ((rec) == NULL ||  (rec)->__magic_number != (magic)) {\
		SOME_ERROR(); action; \
	} } while (0)
#else
#define MAGIC_NUMBER
#define SET_MAGIC(rec,magic)
#define CHEC_MAGIC(rec,magic,action)
#endif

struct foo {
	...
	MAGIC_NUMBER;
};

#define MY_MAGIC	0xabcd1234

static void allocate()
{
	struct foo *data = kmalloc(sizeof(*data));
	SET_MAGIC(data, MY_MAGIC);
	...
}

static int handler(void *private_data)
{
	struct foo *data = private_data;
	CHECK_MAGIC(data, MY_MAGIC, return -EINVAL);
	...
}

In this case, we can simply remove *_MAGIC from the final version (if
wanted), while the current ALSA magic implementation can't be removed
simply.


> I guess though that the problem area for ALSA is the way the snd_pcm_t
> private_data member magically appears in the snd_pcm_substream_t
> private_data member behind the drivers back, so it's unclear who
> actually owns the data in the private_data members.

Well, this is not the case.  Usually a driver uses only one of them.


Takashi