Re: sound/pcm/* bugs (was: Re: page fault panic tracked down (selwakeuppri()) - really sound/pcm/*)

From: Don Lewis <truckman_at_FreeBSD.org>
Date: Sun, 11 Jan 2004 14:26:16 -0800 (PST)
On 11 Jan, Mathew Kanner wrote:
> On Jan 11, Cameron Grant wrote:
>> >	Perhaps a heavy handed approach but until someone can untangle
>> >and own this problem...
>> 
>> if and when real life stops getting in my way, i'll resume working on it.
>> 
>> >	Whimsically, I wish some super-hacker could wrestle, unify,
>> >unifdef and de-kobj the sound code so I could eventually comprehend
>> >it.
>> 
>> there really are very few ifdefs in it.  in 5.x most of them are 
>> unnecessary.
>> 
>> de-kobjification would be absolutely stupid.  completely the wrong 
>> direction.
>> 
>> i don't know what you mean by 'unify'.
> 
> 	I'd hoped the "whimsical" beginning of my sentence conveyed
> its desperate and non-literal tone.  I don't want anybody to wrestle
> code.  I've seen it before and the bruises are ugly.
> 
> 	But since you seem to be interested in my opinion I'll explain
> a little more.
> 
> 	I'm not suggested dropping kobj's from the hardware/pcm
> relationship.
> 
> 	I would like to see the vchans and format conversion either
> dropped or integrated completely.
> 
> 	I would like to unifdef anything possible.  Drop the use of
> snd_mutex_*, and general just assume that the code base is at least
> 5.x (the code is already moving in that direction, though not
> deliberately).

One of my favorites is code that locks a channel using CHN_LOCK() and
then unlocks it with pcm_chnrelease().

> 	The feeder/mixer/vchan concepts are difficult for me and I
> believe that de-kobj'ing would benefit the next generation of sound
> hacker to come.  Or maybe a few lines of comments would clear
> everything up, it's hard to tell.

I spent several days digging through this code and I still don't
understand it very well.  It is easy to get confused between
sndbuf_setblksz() is is a trivial accessor function, and the similar
sounding chn_setblocksize(), which does all sorts of stuff.  It is hard
to tell at a glance the differences between sndbuf_resize() and
sndbuf_remalloc().

It is interesting that chn_reset() calls chn_setblocksize() with the
blkcnt and blksz parameters set to 0, and chn_setblocksize() passes
these arguments to sndbuf_remalloc() which returns EINVAL in this case.

I spent a lot of time flipping between files to try to follow the flow
of the code.  This was made more difficult by parameters that change
name and order.  There appear to be some unnecessary conversions to and
from void *.  For instance the last parameter to FEEDER_FEED() is always
a snd_dbuf *, but the underlying routines, such a feed_vchan_s16()
prototype this to a void * and convert it back to a snd_dbuf *.

It took quite a while to track down the buffer overflow.  I still don't
understand the trigger mechanism, and my two attempts to fix the problem
both resulted in a non-functional driver.
Received on Sun Jan 11 2004 - 13:27:08 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:37:37 UTC