SoundScape driver... FIXMEs

SoundScape driver... FIXMEs

[Chris Rankin]

Hi,

I saw the SoundScape PnP driver appear in CVS today,
along with a number of FIXMEs at the top of the file.
I presume that these FIXMEs are a barrier to fully
integrating the driver, and that's what you mean by
"deadlock for alsa-kernel"?

Anyway:
- the CONFIG_SND_OSSEMUL dependency was a holdover
from my development cycle and can be removed. It was
placed there to guarantee that I was correctly
including my current ALSA configuration. (Previously,
I had been loading a non-OSS-enabled snd-sscape.o
module into an OSS-enabled ALSA setup. And since
struct _snd_card is different depending on whether OSS
emulation has been enabled or not, this was causing
strange memory errors.) My snd-sscape.o works just
fine with native alsa-utils, and so OSS emulation is
definitely an optional extra.

- a non-modular build wasn't a priority because I was
under the impression that one of the goals for Linux
2.5+ was to *remove* all compiled-in drivers? However,
I could probably provide something, if absolutely
necessary. 

- I was not aware that "verify_area()" had been
deprecated. There were no comments to this effect in
the <asm/uaccess.h> header, and a grep through the
2.4.19 tree shows that it is HEAVILY used. In fact,
verify_area() appears to be the public interface for
the same access-checking function invoked by
copy_from_user() and copy_to_user(). The point about
calling verify_area() at all is to validate the entire
DMA upload block at once, so that I can safely copy it
piecemeal later. It seems to be more efficient to DMA
directly from userspace rather than copying the block
into *more* temporary storage and then DMA-ing from
there. Is there a race condition between the
verify_area() and the underlying __copy_from_user()?
If so, then how is this same race resolved by
copy_from_user()?

Anyway, thanks for merging the driver,
Chris


__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: SoundScape driver... FIXMEs

[Takashi Iwai]

At Sun, 22 Sep 2002 20:09:52 +0100 (BST),
Chris Rankin wrote:
> 
> Hi,
> 
> I saw the SoundScape PnP driver appear in CVS today,
> along with a number of FIXMEs at the top of the file.
> I presume that these FIXMEs are a barrier to fully
> integrating the driver, and that's what you mean by
> "deadlock for alsa-kernel"?

i guess it means "the condition for the driver to be raised to
alsa-kernel (i.e. 2.5 kernel) tree"

> - a non-modular build wasn't a priority because I was
> under the impression that one of the goals for Linux
> 2.5+ was to *remove* all compiled-in drivers?

i don't think so.  more modulalization doesn't mean to remove the
ability of built-in drivers.  (and i heard that Linus still prefers
built-in drivers to modules :)

> However, I could probably provide something, if absolutely
> necessary. 

i don't see any particular part that restricts to module (except that
the driver needs firmware to be loaded via ioctl).
am i missing something?


> - I was not aware that "verify_area()" had been
> deprecated. There were no comments to this effect in
> the <asm/uaccess.h> header, and a grep through the
> 2.4.19 tree shows that it is HEAVILY used. In fact,
> verify_area() appears to be the public interface for
> the same access-checking function invoked by
> copy_from_user() and copy_to_user(). The point about
> calling verify_area() at all is to validate the entire
> DMA upload block at once, so that I can safely copy it
> piecemeal later. It seems to be more efficient to DMA
> directly from userspace rather than copying the block
> into *more* temporary storage and then DMA-ing from
> there. Is there a race condition between the
> verify_area() and the underlying __copy_from_user()?
> If so, then how is this same race resolved by
> copy_from_user()?

no race condition between them.  verify_area() is simply not necessary
if you use copy_from/to_user().
generally it's recommended to check the return value from
copy_from/to_user() at each call and to return -EFAULT, because it can
be faster.


another point:  doesn't the busy-loop in host_read/write_ctrl_unsafe
need udelay() or something to produce a certain delay length?
otherwise the timeout is very dependent on a machine.


ciao,

Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: SoundScape driver... FIXMEs

[Chris Rankin]

 --- Takashi Iwai <tiwai@suse.de> wrote: > At Sun, 22
Sep 2002 20:09:52 +0100 (BST),
> i don't think so.  more modulalization doesn't mean
> to remove the
> ability of built-in drivers.  (and i heard that
> Linus still prefers
> built-in drivers to modules :)

I definitely hadn't heard that! I thought Linus's
preferred mode of use was to load all necessary
modules up front at boot-time and then leave them
loaded (i.e. not using kmod). IMHO, monolithic kernels
are just a PITA.

> i don't see any particular part that restricts to
> module (except that
> the driver needs firmware to be loaded via ioctl).
> am i missing something?

Only that it's OBSCENELY more difficult to develop a
driver if you compile it into the kernel every time...
Unless you ENJOY rebooting your machine, of course. 

> > - I was not aware that "verify_area()" had been
> > deprecated.

> verify_area() is simply not necessary
> if you use copy_from/to_user().

Aha, but that's not the same as "deprecated" at all
;-).

I only do one big "verify_area()" up front to simplify
the DMA-uploading loop. This way, I know before I even
begin that *entire* address range is valid, and so I
don't need to worry about __copy_from_user() being
passed an invalid address.

There is only one verify_area() call, but several
__copy_from_user() calls because the firmware is 64K
long, but I upload it in contiguous chunks.

> generally it's recommended to check the return value
> from
> copy_from/to_user() at each call and to return
> -EFAULT, because it can be faster.

Note that copy_from_user() effectively calls
verify_area() and so may return -EFAULT. However, I
call __copy_from_user() instead, having already
checked the entire address range. In other words, I
perform 1 call to verify_area() and N calls to
__copy_from_user(), rather than the equivalent of N
calls to verify_area() and N calls to
__copy_from_user(). Also, I don't need to keep on
checking for -EFAULT
 inside the loop.

> another point:  doesn't the busy-loop in
> host_read/write_ctrl_unsafe
> need udelay() or something to produce a certain
> delay length?
> otherwise the timeout is very dependent on a
> machine.

Yes, possibly. Provided udelay() isn't deprecated and
doesn't schedule or anything. I tested this driver on
a P120 - not exactly a speed daemon... ;-)!

Cheers,
Chris


__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: SoundScape driver... FIXMEs

[Takashi Iwai]

At Mon, 23 Sep 2002 13:28:56 +0100 (BST),
Chris Rankin wrote:
> 
> > i don't see any particular part that restricts to
> > module (except that
> > the driver needs firmware to be loaded via ioctl).
> > am i missing something?
> 
> Only that it's OBSCENELY more difficult to develop a
> driver if you compile it into the kernel every time...
> Unless you ENJOY rebooting your machine, of course. 

yes, but it should not be the reason to stop compiling the built-in
driver via ifdef.  why not just put a warning something like "this
driver is not tested as built-in", if you are not sure?

> > > - I was not aware that "verify_area()" had been
> > > deprecated.
> 
> > verify_area() is simply not necessary
> > if you use copy_from/to_user().
> 
> Aha, but that's not the same as "deprecated" at all
> ;-).
 
no.  it's still alive even on 2.5 tree.

> I only do one big "verify_area()" up front to simplify
> the DMA-uploading loop. This way, I know before I even
> begin that *entire* address range is valid, and so I
> don't need to worry about __copy_from_user() being
> passed an invalid address.
> 
> There is only one verify_area() call, but several
> __copy_from_user() calls because the firmware is 64K
> long, but I upload it in contiguous chunks.
> 
> > generally it's recommended to check the return value
> > from
> > copy_from/to_user() at each call and to return
> > -EFAULT, because it can be faster.
> 
> Note that copy_from_user() effectively calls
> verify_area() and so may return -EFAULT. However, I
> call __copy_from_user() instead, having already
> checked the entire address range. In other words, I
> perform 1 call to verify_area() and N calls to
> __copy_from_user(), rather than the equivalent of N
> calls to verify_area() and N calls to
> __copy_from_user(). Also, I don't need to keep on
> checking for -EFAULT
>  inside the loop.

yeah, i know.  that's why i wrote "generally", above. 
i don't think putting these checks into one verify_area() improves the
performance so much.  checking the return value of copy_from/to_user()
is more obvious, i think.  anyway, it's a matter of taste.

> > another point:  doesn't the busy-loop in
> > host_read/write_ctrl_unsafe
> > need udelay() or something to produce a certain
> > delay length?
> > otherwise the timeout is very dependent on a
> > machine.
> 
> Yes, possibly. Provided udelay() isn't deprecated and
> doesn't schedule or anything.

no and no.  it's a simply busy delay for the given time length
with _relatively_ good accuracy (up to 1ms).

> I tested this driver on
> a P120 - not exactly a speed daemon... ;-)!

ah, then the current driver will fail definitely on 2GHz P4 ;)


ciao,

Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: SoundScape driver... FIXMEs

[Chris Rankin]

 --- Takashi Iwai <tiwai@suse.de> wrote: > At Mon, 23
> yes, but it should not be the reason to stop
> compiling the built-in
> driver via ifdef.

Well, it would be more accurate to say that the
"built-in" code wasn't a priority. I was also hoping
that Linux 2.5+ would make the entire problem go
away... ;-).

> why not just put a warning something like "this
> driver is not tested as built-in", if you are not
> sure?

Yeah, I can do that. The difference with the built-in
case is also that the driver is going to have to
wrestle with the ISA-PnP bus, try and grab resources
and probably activate the hardware. Now THAT could get
ugly. For example, my test box is so short of
resources that there is one only one practical way to
configure it so that every device works. (Some of the
those other devices can reasonably be expected to have
modular drivers, regardless of whether this driver is
compiled into the kernel or not.) This implies that
I'd need a lot of extra kernel options... Ugh.

> > > verify_area() is simply not necessary
> > > if you use copy_from/to_user().
> > 
> > Aha, but that's not the same as "deprecated" at
> all
> > ;-).
>  
> no.  it's still alive even on 2.5 tree.

So that FIXME is incorrect?

> I
> > perform 1 call to verify_area() and N calls to
> > __copy_from_user(), rather than the equivalent of
> N
> > calls to verify_area() and N calls to
> > __copy_from_user(). Also, I don't need to keep on
> > checking for -EFAULT
> >  inside the loop.
> 
> yeah, i know.  that's why i wrote "generally",
> above. 
> i don't think putting these checks into one
> verify_area() improves the
> performance so much.  checking the return value of
> copy_from/to_user()
> is more obvious, i think.

It wasn't really a "performance-thing". The logic was
easier to write with one fewer error to check for. Why
tie myself up in knots making sure I release all
resources along yet *another* error path, when I get
check for that particular error up-front?

> > > another point:  doesn't the busy-loop in
> > > host_read/write_ctrl_unsafe
> > > need udelay() or something to produce a certain
> > > delay length?
> > > otherwise the timeout is very dependent on a
> > > machine.
> > 
> > Yes, possibly. Provided udelay() isn't deprecated
> and
> > doesn't schedule or anything.
> 
> no and no.  it's a simply busy delay for the given
> time length
> with _relatively_ good accuracy (up to 1ms).
> 
> > I tested this driver on
> > a P120 - not exactly a speed daemon... ;-)!
> 
> ah, then the current driver will fail definitely on
> 2GHz P4 ;)

Yeah, if you can find one with an ISA slot free...
;-). Anyway, I'll put that udelay() in then; udelay(1)
will probably be enough. I'll also remove that #error
if there's no OSS emulation, and I don't think any
action is needed for the verify_area() calls.

Chris


__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: SoundScape driver... FIXMEs

[Takashi Iwai]

At Mon, 23 Sep 2002 20:43:19 +0100 (BST),
Chris Rankin wrote:
> 
> > > > another point:  doesn't the busy-loop in
> > > > host_read/write_ctrl_unsafe
> > > > need udelay() or something to produce a certain
> > > > delay length?
> > > > otherwise the timeout is very dependent on a
> > > > machine.
> > > 
> > > Yes, possibly. Provided udelay() isn't deprecated
> > and
> > > doesn't schedule or anything.
> > 
> > no and no.  it's a simply busy delay for the given
> > time length
> > with _relatively_ good accuracy (up to 1ms).
> > 
> > > I tested this driver on
> > > a P120 - not exactly a speed daemon... ;-)!
> > 
> > ah, then the current driver will fail definitely on
> > 2GHz P4 ;)
> 
> Yeah, if you can find one with an ISA slot free...
> ;-). Anyway, I'll put that udelay() in then; udelay(1)
> will probably be enough.

then please don't use HZ for timeout value, since HZ can be different
from 100 on non-i386 architectures (and even on i386 on 2.5).

> I'll also remove that #error
> if there's no OSS emulation, and I don't think any
> action is needed for the verify_area() calls.

i forgot to mention - please add #include <linux/delay.h>
if you use udelay() or mdelay().  

and leave verify_area() as you like.  as said, checking the return
value from copy_from/to_user() is the general way, but of course it's
not the only way and everybody may write his code :)


Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf