Hi,
Attached a patch for the emu10k1 kernel driver against 2.4.11pre6.
Changes:
Mixer improvements.
Fixed a dead lock in emu10k1_volxxx_irqhandler.
Small code cleanup.
The patch against 2.4.10-ac10 can be found at:
ftp://opensource.creative.com/pub/dist/emu10k1-2.4.10-ac10-2.patch
(it's basically the same except for the Configure.help file)
Please apply,
Rui Sousa
On Tue, 9 Oct 2001, Rui Sousa wrote:
>
> Attached a patch for the emu10k1 kernel driver against 2.4.11pre6.
Applied. However..
> Changes:
> Mixer improvements.
> Fixed a dead lock in emu10k1_volxxx_irqhandler.
The locking still looks _very_ suspicious. The code seems to lock at all
the wrong places, leaving a lot of accesses completely outside any
locking. In particular, look at how the code in emu10k1_set_oss_vol()
(which was apparently the one involved in the irqhandler lockup) now does
not protect the mixer_state thing in _any_ way.
In general it is a bug to leave locking to the low-level routines, in this
case to "emu10k1_set_volume_gpr()". I realize that somebody went "Ugh, we
should try to make the locking regions as small as possible", but the fact
is, locks that are _too_ small are no better than no locks at all.
It may be that there is some good reason for why the mixer_state thing
doesn't need a lock - I only gave the patch a quick see-though, but if so
maybe a single line comment migth be appropriate.
Linus
On Tue, 9 Oct 2001, Linus Torvalds wrote:
Actually there is no locking implemented for the ac97 codec mixer.
It never seemed worth it, even if there are potential races in the code.
To have two applications accessing the mixer at the same time is a _very_
rare condition and the worse that can happen is to set a wrong volume
value. Anyway, I will give it another look and try to come up with a fix.
The locking that we care about (mgr->lock) is for the dsp microcode, where
we don't want to have it changing under us. Writing to the wrong
dsp register can have some interesting effects.
Rui Sousa
>
> On Tue, 9 Oct 2001, Rui Sousa wrote:
> >
> > Attached a patch for the emu10k1 kernel driver against 2.4.11pre6.
>
> Applied. However..
>
> > Changes:
> > Mixer improvements.
> > Fixed a dead lock in emu10k1_volxxx_irqhandler.
>
> The locking still looks _very_ suspicious. The code seems to lock at all
> the wrong places, leaving a lot of accesses completely outside any
> locking. In particular, look at how the code in emu10k1_set_oss_vol()
> (which was apparently the one involved in the irqhandler lockup) now does
> not protect the mixer_state thing in _any_ way.
>
> In general it is a bug to leave locking to the low-level routines, in this
> case to "emu10k1_set_volume_gpr()". I realize that somebody went "Ugh, we
> should try to make the locking regions as small as possible", but the fact
> is, locks that are _too_ small are no better than no locks at all.
>
> It may be that there is some good reason for why the mixer_state thing
> doesn't need a lock - I only gave the patch a quick see-though, but if so
> maybe a single line comment migth be appropriate.
>
> Linus
>
>
> _______________________________________________
> Emu10k1-devel mailing list
> [email protected]
> http://opensource.creative.com/mailman/listinfo/emu10k1-devel
>
On Tue, 9 Oct 2001, Rui Sousa wrote:
> Actually there is no locking implemented for the ac97 codec mixer.
> It never seemed worth it, even if there are potential races in the code.
> To have two applications accessing the mixer at the same time is a _very_
> rare condition and the worse that can happen is to set a wrong volume
> value. Anyway, I will give it another look and try to come up with a fix.
I have a patch in the wings which adds locking to mixer accesses, for
via82cxxx_audio, because the lack of locking was causing problems. So,
some people with some apps do indeed notice...
Jeff
On Tue, 9 Oct 2001, Jeff Garzik wrote:
Point taken.
>From what I see doing locking with a spinlock is quite tricky.
codec->read_mixer = ac97_read_mixer; //can be called holding spinlock
codec->write_mixer = ac97_write_mixer; //can be called holding spinlock
codec->recmask_io = ac97_recmask_io;
codec->mixer_ioctl = ac97_mixer_ioctl; //in general can't be called
holding spinlock
and ac97_mixer_ioctl() itself calls ac97_read/write_mixer().
A semaphore on the mixer device open function would do just fine If I
didn't had an interrupt handler also touching the ac97_codec...
Rui
> On Tue, 9 Oct 2001, Rui Sousa wrote:
> > Actually there is no locking implemented for the ac97 codec mixer.
> > It never seemed worth it, even if there are potential races in the code.
> > To have two applications accessing the mixer at the same time is a _very_
> > rare condition and the worse that can happen is to set a wrong volume
> > value. Anyway, I will give it another look and try to come up with a fix.
>
> I have a patch in the wings which adds locking to mixer accesses, for
> via82cxxx_audio, because the lack of locking was causing problems. So,
> some people with some apps do indeed notice...
>
> Jeff
>
>
>
>
> _______________________________________________
> Emu10k1-devel mailing list
> [email protected]
> http://opensource.creative.com/mailman/listinfo/emu10k1-devel
>
On Tue, 9 Oct 2001, Rui Sousa wrote:
> On Tue, 9 Oct 2001, Jeff Garzik wrote:
> From what I see doing locking with a spinlock is quite tricky.
>
> codec->read_mixer = ac97_read_mixer; //can be called holding spinlock
> codec->write_mixer = ac97_write_mixer; //can be called holding spinlock
> codec->recmask_io = ac97_recmask_io;
> codec->mixer_ioctl = ac97_mixer_ioctl; //in general can't be called
> holding spinlock
>
> and ac97_mixer_ioctl() itself calls ac97_read/write_mixer().
>
> A semaphore on the mixer device open function would do just fine If I
> didn't had an interrupt handler also touching the ac97_codec...
Yep, that's how the via audio problem was solved, with a mixer
semaphore. Having your interrupt handler touch ac97_codec definitely
complicates things beyond that simple solution, though. If your only
concern is the intr handler you could create a dont-touch-ac97-in-intr
flag, and set that flag (only) via spin_lock_irq. Then you don't
have to stay inside a spinlock the entire time.
Jeff