Date: Mon, 07 Jun 2004 15:14:06 +0200
Message-ID: <s5hfz974ijl.wl@alsa2.suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: viro@parcelfarce.linux.theplanet.co.uk
Cc: Linus Torvalds <torvalds@osdl.org>, linux-kernel@vger.kernel.org,
       perex@suse.cz
Subject: Re: [RFC] ASLA design, depth of code review and lack thereof
In-Reply-To: <20040605000410.GT12308@parcelfarce.linux.theplanet.co.uk>
References: <20040604230819.GR12308@parcelfarce.linux.theplanet.co.uk>
	<Pine.LNX.4.58.0406041622480.7010@ppc970.osdl.org>
	<20040605000410.GT12308@parcelfarce.linux.theplanet.co.uk>
User-Agent: Wanderlust/2.10.1 (Watching The Wheels) SEMI/1.14.5 (Awara-Onsen) FLIM/1.14.5 (Demachiyanagi) APEL/10.6 MULE XEmacs/21.4 (patch 13) (Rational FORTRAN) (i386-suse-linux)
MIME-Version: 1.0 (generated by SEMI 1.14.5 - "Awara-Onsen")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2767
Lines: 76

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/