linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* via82cxxx_audio locking problems
@ 2001-09-20  8:39 Thomas Sailer
  2001-09-20 11:33 ` Nicholas Knight
  2001-09-20 16:33 ` via82cxxx_audio locking problems Jeff Garzik
  0 siblings, 2 replies; 13+ messages in thread
From: Thomas Sailer @ 2001-09-20  8:39 UTC (permalink / raw)
  To: jgarzik, linux-kernel

This applies to version 1.1.5 as well as the version in
linux-2.4.10-pre12 and linux-2.4.9-ac12.

1) There is one semaphore (syscall_sem) that is held during
   calls from userspace. It is even kept while going to sleep
   during read and write syscalls. This locks out other users,
   eg. mixers, for a potentially very long time, seconds are
   common but it may almost be arbitrarily long. Try changing
   the volume with eg. gmix while playing something with eg. xmms.

   Dropping and reacquiring syscall_sem around interruptible_sleep_on
   in via_dsp_do_read, via_dsp_do_write and via_dsp_drain_playback
   should solve the problem. Does anyone see a problem with this?

2) When some kind of error happens during read or write after
   some samples have already been dequeued and copied to the user
   buffer, the number of copied bytes should be returned instead
   of the error code, to avoid loosing samples.

3) The use of interruptible_sleep_on results in a small race where
   wake_ups may be lost. Unlikely to hit though.

4) The down_trylock and returning -EAGAIN in via_down_syscall looks
   questionable, EAGAIN with O_NONBLOCK normally means I/O has to
   be completed first, not that there is contention on some internal
   synchronisation primitive.

Jeff, do you object any of this? Would you accept a patch to ameliorate
the situation? Or would you like to fix this yourself?

Tom

PS: Is there any better publicly available chip documentation than
the 120 page PDF file?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-20  8:39 via82cxxx_audio locking problems Thomas Sailer
@ 2001-09-20 11:33 ` Nicholas Knight
  2001-09-20 12:07   ` Adrian Cox
  2001-09-20 16:33 ` via82cxxx_audio locking problems Jeff Garzik
  1 sibling, 1 reply; 13+ messages in thread
From: Nicholas Knight @ 2001-09-20 11:33 UTC (permalink / raw)
  To: t.sailer, Thomas Sailer, jgarzik, linux-kernel; +Cc: adrian

On Thursday 20 September 2001 01:39 am, Thomas Sailer wrote:
> This applies to version 1.1.5 as well as the version in
> linux-2.4.10-pre12 and linux-2.4.9-ac12.
>
> 1) There is one semaphore (syscall_sem) that is held during
>    calls from userspace. It is even kept while going to sleep
>    during read and write syscalls. This locks out other users,
>    eg. mixers, for a potentially very long time, seconds are
>    common but it may almost be arbitrarily long. Try changing
>    the volume with eg. gmix while playing something with eg. xmms.
>

thankyouthankyouthankyouthankyouthankyou
Adrian Cox was working on this after I raised the issue on the list, but 
nobody got anywhere. All we knew was that there were temporary lockups 
appearing when anything was using the mixer.

>    Dropping and reacquiring syscall_sem around interruptible_sleep_on
>    in via_dsp_do_read, via_dsp_do_write and via_dsp_drain_playback
>    should solve the problem. Does anyone see a problem with this?
>
> 2) When some kind of error happens during read or write after
>    some samples have already been dequeued and copied to the user
>    buffer, the number of copied bytes should be returned instead
>    of the error code, to avoid loosing samples.
>
> 3) The use of interruptible_sleep_on results in a small race where
>    wake_ups may be lost. Unlikely to hit though.
>
> 4) The down_trylock and returning -EAGAIN in via_down_syscall looks
>    questionable, EAGAIN with O_NONBLOCK normally means I/O has to
>    be completed first, not that there is contention on some internal
>    synchronisation primitive.
>
> Jeff, do you object any of this? Would you accept a patch to ameliorate
> the situation? Or would you like to fix this yourself?

I've emailed Jeff 2 or 3 times about the lock/freezes related to 
volume/mixer control, and never got a response, I'm not sure he's 
actually maintaining the driver. The last release of it from him was all 
fixes from someone else.

Adrian Cox was helping to track this down before, and is probably the 
best bet, I've CC'd Adrian to this. Unfortunitely I haven't seen any 
messages from him on lk in a while, so I'm not sure if he's on vacation 
or what.

My advice is, give us a patch! :) Since Jeff hasn't been heard from for a 
while, I can't see anyone, including Jeff, having a problem with this.

Is there anyone out there that could start maintaining the driver? 
Inactive drivers are kind of a pain :(

>
> Tom
>
> PS: Is there any better publicly available chip documentation than
> the 120 page PDF file?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-20 11:33 ` Nicholas Knight
@ 2001-09-20 12:07   ` Adrian Cox
  2001-09-20 12:24     ` Nicholas Knight
  0 siblings, 1 reply; 13+ messages in thread
From: Adrian Cox @ 2001-09-20 12:07 UTC (permalink / raw)
  To: tegeran; +Cc: t.sailer, Thomas Sailer, jgarzik, linux-kernel

Nicholas Knight wrote:

 
> thankyouthankyouthankyouthankyouthankyou
> Adrian Cox was working on this after I raised the issue on the list, but 
> nobody got anywhere. All we knew was that there were temporary lockups 
> appearing when anything was using the mixer.


This is the right answer. The reason some of us didn't see a problem may 
actually be quite simple: we were using very small buffers in xmms. Once 
I increased the xmms buffer size the problem became visible.

-- 
Adrian Cox   http://www.humboldt.co.uk/


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-20 12:07   ` Adrian Cox
@ 2001-09-20 12:24     ` Nicholas Knight
  2001-09-20 13:40       ` André Dahlqvist
  0 siblings, 1 reply; 13+ messages in thread
From: Nicholas Knight @ 2001-09-20 12:24 UTC (permalink / raw)
  To: Adrian Cox, tegeran; +Cc: t.sailer, Thomas Sailer, jgarzik, linux-kernel

On Thursday 20 September 2001 05:07 am, Adrian Cox wrote:
> Nicholas Knight wrote:
> > thankyouthankyouthankyouthankyouthankyou
> > Adrian Cox was working on this after I raised the issue on the list,
> > but nobody got anywhere. All we knew was that there were temporary
> > lockups appearing when anything was using the mixer.
>
> This is the right answer. The reason some of us didn't see a problem
> may actually be quite simple: we were using very small buffers in xmms.
> Once I increased the xmms buffer size the problem became visible.

Interesting, I just experimented with it, bringing down the buffers to 
200ms (low as they'll go) and pre-buffer % to 0, does seem to have an 
effect, but it doesn't "fix" the problem for me... Should I just conclude 
that the tolerance for this is higher on some boards/chips than on 
others, and that once THIS problem is fixed, it'll go away and I can 
happily use my volume control again?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-20 12:24     ` Nicholas Knight
@ 2001-09-20 13:40       ` André Dahlqvist
  2001-09-20 13:41         ` Thomas Sailer
  2001-09-21  9:27         ` Thomas Sailer
  0 siblings, 2 replies; 13+ messages in thread
From: André Dahlqvist @ 2001-09-20 13:40 UTC (permalink / raw)
  To: Nicholas Knight
  Cc: Adrian Cox, t.sailer, Thomas Sailer, jgarzik, linux-kernel

Nicholas Knight <tegeran@home.com> wrote:

> Interesting, I just experimented with it, bringing down the buffers to 
> 200ms (low as they'll go) and pre-buffer % to 0, does seem to have an 
> effect, but it doesn't "fix" the problem for me...

I'm  using the VIA audio driver and I have what appears to be a very similar
problem:

When I try to move my XMMS window while playing a song the window sometimes
"gets stuck" for a second or so during the move. Moving the window without
any song playing works just fine. I also tried setting the buffer to 200ms
but it didn't solve it for me either.

dmesg reports:

Via 686a audio driver 1.1.14b
PCI: Found IRQ 10 for device 00:07.5
via82cxxx: Codec rate locked at 48Khz
ac97_codec: AC97 Audio codec, id: 0x8384:0x7600 (SigmaTel STAC????)
via82cxxx: board #1 at 0xDC00, IRQ 10
-- 

André Dahlqvist <andre.dahlqvist@telia.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-20 13:40       ` André Dahlqvist
@ 2001-09-20 13:41         ` Thomas Sailer
  2001-09-21  9:27         ` Thomas Sailer
  1 sibling, 0 replies; 13+ messages in thread
From: Thomas Sailer @ 2001-09-20 13:41 UTC (permalink / raw)
  To: André Dahlqvist
  Cc: Nicholas Knight, Adrian Cox, t.sailer, jgarzik, linux-kernel

André Dahlqvist schrieb:

> I'm  using the VIA audio driver and I have what appears to be a very similar
> problem:
> 
> When I try to move my XMMS window while playing a song the window sometimes
> "gets stuck" for a second or so during the move. Moving the window without
> any song playing works just fine. I also tried setting the buffer to 200ms
> but it didn't solve it for me either.

I'm seeing this too. XMMS polls the mixer every couple of seconds,
and runs into the semaphore locking issue. As to why moving windows
in that state is blocked I don't know since I do not know the X internals
well enough...

Tom

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-20  8:39 via82cxxx_audio locking problems Thomas Sailer
  2001-09-20 11:33 ` Nicholas Knight
@ 2001-09-20 16:33 ` Jeff Garzik
  2001-09-21  7:50   ` Adrian Cox
  1 sibling, 1 reply; 13+ messages in thread
From: Jeff Garzik @ 2001-09-20 16:33 UTC (permalink / raw)
  To: Thomas Sailer; +Cc: linux-kernel

On Thu, 20 Sep 2001, Thomas Sailer wrote:

> This applies to version 1.1.5 as well as the version in
> linux-2.4.10-pre12 and linux-2.4.9-ac12.
> 
> 1) There is one semaphore (syscall_sem) that is held during
>    calls from userspace. It is even kept while going to sleep
>    during read and write syscalls. This locks out other users,
>    eg. mixers, for a potentially very long time, seconds are
>    common but it may almost be arbitrarily long. Try changing
>    the volume with eg. gmix while playing something with eg. xmms.
> 
>    Dropping and reacquiring syscall_sem around interruptible_sleep_on
>    in via_dsp_do_read, via_dsp_do_write and via_dsp_drain_playback
>    should solve the problem. Does anyone see a problem with this?

Is there a possibility of do_read being re-entered during that window?
I agree its a problem but the solution sounds racy?

> 2) When some kind of error happens during read or write after
>    some samples have already been dequeued and copied to the user
>    buffer, the number of copied bytes should be returned instead
>    of the error code, to avoid loosing samples.

Very true

> 3) The use of interruptible_sleep_on results in a small race where
>    wake_ups may be lost. Unlikely to hit though.
> 
> 4) The down_trylock and returning -EAGAIN in via_down_syscall looks
>    questionable, EAGAIN with O_NONBLOCK normally means I/O has to
>    be completed first, not that there is contention on some internal
>    synchronisation primitive.

I disagree; I think the idea at aleast is correct:  if contention
exists, it implies that I/O needs to be completed.

> Jeff, do you object any of this? Would you accept a patch to ameliorate
> the situation? Or would you like to fix this yourself?

A fix patch would definitely be accepted...

	Jeff




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-20 16:33 ` via82cxxx_audio locking problems Jeff Garzik
@ 2001-09-21  7:50   ` Adrian Cox
  2001-09-21  8:36     ` David Chow
  0 siblings, 1 reply; 13+ messages in thread
From: Adrian Cox @ 2001-09-21  7:50 UTC (permalink / raw)
  To: Jeff Garzik; +Cc: Thomas Sailer, linux-kernel

Jeff Garzik wrote:

> On Thu, 20 Sep 2001, Thomas Sailer wrote:
>>   Dropping and reacquiring syscall_sem around interruptible_sleep_on
>>   in via_dsp_do_read, via_dsp_do_write and via_dsp_drain_playback
>>   should solve the problem. Does anyone see a problem with this?


> Is there a possibility of do_read being re-entered during that window?
> I agree its a problem but the solution sounds racy?

What's probably needed is one semaphore to lock read/write and ioctls 
that look at the playback engine, and another semaphore to lock accesses 
to the AC97 codec. That may be simpler to implement than dropping and 
releasing the syscall_sem.

-- 
Adrian Cox   http://www.humboldt.co.uk/


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-21  7:50   ` Adrian Cox
@ 2001-09-21  8:36     ` David Chow
  2001-09-21  8:50       ` David Chow
  0 siblings, 1 reply; 13+ messages in thread
From: David Chow @ 2001-09-21  8:36 UTC (permalink / raw)
  To: Adrian Cox; +Cc: Jeff Garzik, Thomas Sailer, linux-kernel

Adrian Cox ¼g¹D¡G
> 
> Jeff Garzik wrote:
> 
> > On Thu, 20 Sep 2001, Thomas Sailer wrote:
> >>   Dropping and reacquiring syscall_sem around interruptible_sleep_on
> >>   in via_dsp_do_read, via_dsp_do_write and via_dsp_drain_playback
> >>   should solve the problem. Does anyone see a problem with this?
> 
> > Is there a possibility of do_read being re-entered during that window?
> > I agree its a problem but the solution sounds racy?
> 
> What's probably needed is one semaphore to lock read/write and ioctls
> that look at the playback engine, and another semaphore to lock accesses
> to the AC97 codec. That may be simpler to implement than dropping and
> releasing the syscall_sem.
> 
> --
> Adrian Cox   http://www.humboldt.co.uk/
> 
> -
> 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/

I receive the same problem when probing the via sound module. Since I am
in a motherboard design project which I also uses the AC97 interface
from VIA. The sample board from VIA didn't have any problem, but have 3
motherboard from 3rd party, each of them uses the VIA694X + VIA686A,
only one of them have no problem using the sound module, I think it is a
hardware problem or the module itself didn't handle some cases. It seems
it should be the same for hardware design, since different codec may
have different effect on the module. All boards are tested with
Windows98 and Linux and then all works fine runnign Windows. The sample
board from VIA is a VIA694T + VIA686B which all of them are pin-2-pin
compatible with the old 694X+686A. I am sure the problem is from the
driver, but it is hardware dependent??? My board design just use exactly
the same chips of the VIA sample board, then we will be save using the
via_82cxxx module properly. I will try to find out which codec is the
trouble boards using. The one I'm surely stable without locking is using
the VIA codec VT1611A . Does you guys require more information about the
codec specs? I can get them from VIA if you want. Thanks.

regards,

David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-21  8:36     ` David Chow
@ 2001-09-21  8:50       ` David Chow
  0 siblings, 0 replies; 13+ messages in thread
From: David Chow @ 2001-09-21  8:50 UTC (permalink / raw)
  To: Adrian Cox, Jeff Garzik, Thomas Sailer, linux-kernel

David Chow ¼g¹D¡G
> 
> Adrian Cox ¼g¹D¡G
> >
> > Jeff Garzik wrote:
> >
> > > On Thu, 20 Sep 2001, Thomas Sailer wrote:
> > >>   Dropping and reacquiring syscall_sem around interruptible_sleep_on
> > >>   in via_dsp_do_read, via_dsp_do_write and via_dsp_drain_playback
> > >>   should solve the problem. Does anyone see a problem with this?
> >
> > > Is there a possibility of do_read being re-entered during that window?
> > > I agree its a problem but the solution sounds racy?
> >
> > What's probably needed is one semaphore to lock read/write and ioctls
> > that look at the playback engine, and another semaphore to lock accesses
> > to the AC97 codec. That may be simpler to implement than dropping and
> > releasing the syscall_sem.
> >
> > --
> > Adrian Cox   http://www.humboldt.co.uk/
> >
> > -
> > 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/
> 
> I receive the same problem when probing the via sound module. Since I am
> in a motherboard design project which I also uses the AC97 interface
> from VIA. The sample board from VIA didn't have any problem, but have 3
> motherboard from 3rd party, each of them uses the VIA694X + VIA686A,
> only one of them have no problem using the sound module, I think it is a
> hardware problem or the module itself didn't handle some cases. It seems
> it should be the same for hardware design, since different codec may
> have different effect on the module. All boards are tested with
> Windows98 and Linux and then all works fine runnign Windows. The sample
> board from VIA is a VIA694T + VIA686B which all of them are pin-2-pin
> compatible with the old 694X+686A. I am sure the problem is from the
> driver, but it is hardware dependent??? My board design just use exactly
> the same chips of the VIA sample board, then we will be save using the
> via_82cxxx module properly. I will try to find out which codec is the
> trouble boards using. The one I'm surely stable without locking is using
> the VIA codec VT1611A . Does you guys require more information about the
> codec specs? I can get them from VIA if you want. Thanks.
> 
> regards,
> 
> David
> -
> 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/

Just find out the board that cause lock up both uses a Yamaha codec.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-20 13:40       ` André Dahlqvist
  2001-09-20 13:41         ` Thomas Sailer
@ 2001-09-21  9:27         ` Thomas Sailer
  2001-09-21 12:06           ` André Dahlqvist
  1 sibling, 1 reply; 13+ messages in thread
From: Thomas Sailer @ 2001-09-21  9:27 UTC (permalink / raw)
  To: André Dahlqvist
  Cc: Nicholas Knight, Adrian Cox, t.sailer, jgarzik, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

Jeff Garzik schrieb:

> Is there a possibility of do_read being re-entered during that window?
> I agree its a problem but the solution sounds racy?

do_read may be reentered. In that case some of the samples land in one read,
the other part in the other read.

This is not a problem, however. UNIX never made any guarantees about
read from or writes to the same fd. Take pipes for example, they don't
guarantee read or write atomicity either. Well, they do guarantee it
if they are all less than PIPEBUF in size. But so do we, the limit
is not called PIPEBUF, but fragment.

> I disagree; I think the idea at aleast is correct:  if contention
> exists, it implies that I/O needs to be completed.

Well, it depends what the semaphore is supposed to do. If you (ab)use
it as a waitqueue, then yes, the idea is correct. If you only use it
to guard short critical sections, then the cost of reissuing the syscall
is greater than just waiting for the semaphore to be released.

This is the patch I've tested yesterday. It worked so far.

There is however an unrelated problem that happens both with and without
patch. Sometimes playback sound is very distorted, until I either press
stop, then the remaining second or so continues undistortedly, or I try
to debug the problem. Debugging is anyway difficult without reasonable
docs. It may be a PCI bridge latency issue, as the VIA northbridge my
board uses is fairly crappy.

Tom

[-- Attachment #2: via82cxxx_audio.diff --]
[-- Type: application/octet-stream, Size: 7248 bytes --]

--- via82cxxx_audio.c.1.1.15	Thu Sep 20 15:41:30 2001
+++ via82cxxx_audio.c	Thu Sep 20 16:15:05 2001
@@ -12,10 +12,17 @@
  * the driver's Website at
  * http://gtf.org/garzik/drivers/via82cxxx/
  *
+ * Thomas Sailer, 2001-09-20:
+ *   - unlock syscall_sem during sleeping in read/write
+ *   - return byte count in case of partial transfer instead of
+ *     error in read/write syscalls
+ *   - avoid loosing wake_up event
+ *   - nonblocking semaphore down disabled
+ *
  */
 
 
-#define VIA_VERSION	"1.1.15"
+#define VIA_VERSION	"1.1.15tsa"
 
 
 #include <linux/config.h>
@@ -459,6 +466,14 @@
 
 static inline int via_syscall_down (struct via_info *card, int nonblock)
 {
+	/* Thomas Sailer:
+	 * EAGAIN is supposed to be used if IO is pending,
+	 * not if there is contention on some internal
+	 * synchronization primitive which should be
+	 * held only for a short time anyway
+	 */
+	nonblock = 0;
+
 	if (nonblock) {
 		if (down_trylock (&card->syscall_sem))
 			return -EAGAIN;
@@ -1973,18 +1988,27 @@
 				char *userbuf, size_t count,
 				int nonblock)
 {
+        DECLARE_WAITQUEUE(wait, current);
 	const char *orig_userbuf = userbuf;
 	struct via_channel *chan = &card->ch_in;
 	size_t size;
 	int n, tmp;
+	ssize_t ret = 0;
 
 	/* if SGD has not yet been started, start it */
 	via_chan_maybe_start (chan);
 
 handle_one_block:
 	/* just to be a nice neighbor */
-	if (current->need_resched)
+	/* Thomas Sailer:
+	 * But also to ourselves, release semaphore if we do so */
+	if (current->need_resched) {
+		up(&card->syscall_sem);
 		schedule ();
+		ret = via_syscall_down (card, nonblock);
+		if (ret)
+			goto out;
+	}
 
 	/* grab current channel software pointer.  In the case of
 	 * recording, this is pointing to the next buffer that
@@ -1996,21 +2020,37 @@
 	 * to be copied to userland.  sleep until at least
 	 * one buffer has been read from the audio hardware.
 	 */
-	tmp = atomic_read (&chan->n_frags);
-	assert (tmp >= 0);
-	assert (tmp <= chan->frag_number);
-	while (tmp == 0) {
-		if (nonblock || !chan->is_active)
-			return -EAGAIN;
+	add_wait_queue(&chan->wait, &wait);
+	for (;;) {
+		__set_current_state(TASK_INTERRUPTIBLE);
+		tmp = atomic_read (&chan->n_frags);
+		assert (tmp >= 0);
+		assert (tmp <= chan->frag_number);
+		if (tmp)
+			break;
+		if (nonblock || !chan->is_active) {
+			ret = -EAGAIN;
+			break;
+		}
+
+		up(&card->syscall_sem);
 
 		DPRINTK ("Sleeping on block %d\n", n);
-		interruptible_sleep_on (&chan->wait);
+		schedule();
 
-		if (signal_pending (current))
-			return -ERESTARTSYS;
+		ret = via_syscall_down (card, nonblock);
+		if (ret)
+			break;
 
-		tmp = atomic_read (&chan->n_frags);
+		if (signal_pending (current)) {
+			ret = -ERESTARTSYS;
+			break;
+		}
 	}
+        set_current_state(TASK_RUNNING);
+        remove_wait_queue(&chan->wait, &wait);
+	if (ret)
+		goto out;
 
 	/* Now that we have a buffer we can read from, send
 	 * as much as sample data possible to userspace.
@@ -2021,8 +2061,10 @@
 		size = (count < slop_left) ? count : slop_left;
 		if (copy_to_user (userbuf,
 				  chan->pgtbl[n / (PAGE_SIZE / chan->frag_size)].cpuaddr + n % (PAGE_SIZE / chan->frag_size) + chan->slop_len,
-				  size))
-			return -EFAULT;
+				  size)) {
+			ret = -EFAULT;
+			goto out;
+		}
 
 		count -= size;
 		chan->slop_len += size;
@@ -2071,7 +2113,7 @@
 		goto handle_one_block;
 
 out:
-	return userbuf - orig_userbuf;
+	return (userbuf != orig_userbuf) ? (userbuf - orig_userbuf) : ret;
 }
 
 
@@ -2122,16 +2164,25 @@
 				 const char *userbuf, size_t count,
 				 int nonblock)
 {
+        DECLARE_WAITQUEUE(wait, current);
 	const char *orig_userbuf = userbuf;
 	struct via_channel *chan = &card->ch_out;
 	volatile struct via_sgd_table *sgtable = chan->sgtable;
 	size_t size;
 	int n, tmp;
+	ssize_t ret = 0;
 
 handle_one_block:
 	/* just to be a nice neighbor */
-	if (current->need_resched)
+	/* Thomas Sailer:
+	 * But also to ourselves, release semaphore if we do so */
+	if (current->need_resched) {
+		up(&card->syscall_sem);
 		schedule ();
+		ret = via_syscall_down (card, nonblock);
+		if (ret)
+			goto out;
+	}
 
 	/* grab current channel fragment pointer.  In the case of
 	 * playback, this is pointing to the next fragment that
@@ -2143,21 +2194,37 @@
 	 * to be filled by userspace.  Sleep until
 	 * at least one fragment is available for our use.
 	 */
-	tmp = atomic_read (&chan->n_frags);
-	assert (tmp >= 0);
-	assert (tmp <= chan->frag_number);
-	while (tmp == 0) {
-		if (nonblock || !chan->is_enabled)
-			return -EAGAIN;
+	add_wait_queue(&chan->wait, &wait);
+	for (;;) {
+		__set_current_state(TASK_INTERRUPTIBLE);
+		tmp = atomic_read (&chan->n_frags);
+		assert (tmp >= 0);
+		assert (tmp <= chan->frag_number);
+		if (tmp)
+			break;
+		if (nonblock || !chan->is_active) {
+			ret = -EAGAIN;
+			break;
+		}
+
+		up(&card->syscall_sem);
 
 		DPRINTK ("Sleeping on page %d, tmp==%d, ir==%d\n", n, tmp, chan->is_record);
-		interruptible_sleep_on (&chan->wait);
+		schedule();
 
-		if (signal_pending (current))
-			return -ERESTARTSYS;
+		ret = via_syscall_down (card, nonblock);
+		if (ret)
+			break;
 
-		tmp = atomic_read (&chan->n_frags);
+		if (signal_pending (current)) {
+			ret = -ERESTARTSYS;
+			break;
+		}
 	}
+        set_current_state(TASK_RUNNING);
+        remove_wait_queue(&chan->wait, &wait);
+	if (ret)
+		goto out;
 
 	/* Now that we have at least one fragment we can write to, fill the buffer
 	 * as much as possible with data from userspace.
@@ -2167,8 +2234,10 @@
 
 		size = (count < slop_left) ? count : slop_left;
 		if (copy_from_user (chan->pgtbl[n / (PAGE_SIZE / chan->frag_size)].cpuaddr + (n % (PAGE_SIZE / chan->frag_size)) * chan->frag_size + chan->slop_len,
-				    userbuf, size))
-			return -EFAULT;
+				    userbuf, size)) {
+			ret = -EFAULT;
+			goto out;
+		}
 
 		count -= size;
 		chan->slop_len += size;
@@ -2330,6 +2399,9 @@
 static int via_dsp_drain_playback (struct via_info *card,
 				   struct via_channel *chan, int nonblock)
 {
+        DECLARE_WAITQUEUE(wait, current);
+	int ret = 0;
+
 	DPRINTK ("ENTER, nonblock = %d\n", nonblock);
 
 	if (chan->slop_len > 0)
@@ -2340,10 +2412,16 @@
 
 	via_chan_maybe_start (chan);
 
-	while (atomic_read (&chan->n_frags) < chan->frag_number) {
+	add_wait_queue(&chan->wait, &wait);
+	for (;;) {
+		__set_current_state(TASK_INTERRUPTIBLE);
+		if (atomic_read (&chan->n_frags) >= chan->frag_number)
+			break;
+
 		if (nonblock) {
 			DPRINTK ("EXIT, returning -EAGAIN\n");
-			return -EAGAIN;
+			ret = -EAGAIN;
+			break;
 		}
 
 #ifdef VIA_DEBUG
@@ -2372,14 +2450,24 @@
 			printk (KERN_ERR "sleeping but not active\n");
 #endif
 
+		up(&card->syscall_sem);
+
 		DPRINTK ("sleeping, nbufs=%d\n", atomic_read (&chan->n_frags));
-		interruptible_sleep_on (&chan->wait);
+		schedule();
+
+		if ((ret = via_syscall_down (card, nonblock)))
+			break;
 
 		if (signal_pending (current)) {
 			DPRINTK ("EXIT, returning -ERESTARTSYS\n");
-			return -ERESTARTSYS;
+			ret = -ERESTARTSYS;
+			break;
 		}
 	}
+        set_current_state(TASK_RUNNING);
+        remove_wait_queue(&chan->wait, &wait);
+	if (ret)
+		return ret;
 
 #ifdef VIA_DEBUG
 	{
@@ -2408,7 +2496,7 @@
 
 out:
 	DPRINTK ("EXIT, returning 0\n");
-	return 0;
+	return ret;
 }
 
 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: via82cxxx_audio locking problems
  2001-09-21  9:27         ` Thomas Sailer
@ 2001-09-21 12:06           ` André Dahlqvist
  2001-09-21 13:01             ` Lockups fixed! (Was: via82cxxx_audio locking problems) André Dahlqvist
  0 siblings, 1 reply; 13+ messages in thread
From: André Dahlqvist @ 2001-09-21 12:06 UTC (permalink / raw)
  To: linux-kernel

Thomas Sailer <sailer@scs.ch> wrote:

> This is the patch I've tested yesterday. It worked so far.

This seams to be against version 1.15 of the VIA driver. -pre13 only seams
to have 1.14 so I got rejects when I tried to apply it:

Hunk #1 FAILED at 12.
Hunk #2 succeeded at 458 (offset -8 lines).
Hunk #3 succeeded at 1973 (offset -15 lines).
Hunk #4 succeeded at 2005 (offset -15 lines).
Hunk #5 succeeded at 2046 (offset -15 lines).
Hunk #6 succeeded at 2098 (offset -15 lines).
Hunk #7 succeeded at 2149 (offset -15 lines).
Hunk #8 succeeded at 2179 (offset -15 lines).
Hunk #9 succeeded at 2219 (offset -15 lines).
Hunk #10 succeeded at 2384 (offset -15 lines).
Hunk #11 succeeded at 2397 (offset -15 lines).
Hunk #12 succeeded at 2435 (offset -15 lines).
Hunk #13 succeeded at 2481 (offset -15 lines).
1 out of 13 hunks FAILED -- saving rejects to file via82cxxx_audio.c.rej

The rejects are these harmless ones:

***************
*** 12,21 ****
   * the driver's Website at
   * http://gtf.org/garzik/drivers/via82cxxx/
   *
   */
  
  
- #define VIA_VERSION	"1.1.15"
  
  
  #include <linux/config.h>
--- 12,28 ----
   * the driver's Website at
   * http://gtf.org/garzik/drivers/via82cxxx/
   *
+  * Thomas Sailer, 2001-09-20:
+  *   - unlock syscall_sem during sleeping in read/write
+  *   - return byte count in case of partial transfer instead of
+  *     error in read/write syscalls
+  *   - avoid loosing wake_up event
+  *   - nonblocking semaphore down disabled
+  *
   */
  
  
+ #define VIA_VERSION	"1.1.15tsa"
  
  
  #include <linux/config.h>
-- 

André Dahlqvist <andre.dahlqvist@telia.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Lockups fixed! (Was: via82cxxx_audio locking problems)
  2001-09-21 12:06           ` André Dahlqvist
@ 2001-09-21 13:01             ` André Dahlqvist
  0 siblings, 0 replies; 13+ messages in thread
From: André Dahlqvist @ 2001-09-21 13:01 UTC (permalink / raw)
  To: linux-kernel

I now tried this patch with 2.4.10-pre13 + VIA audio driver 1.1.15 and fixed
the stalls I used to see! I've only tested it for a few minutes so I can't
comment on any negative sideffects yet though.

Just a WFM.
-- 

André Dahlqvist <andre.dahlqvist@telia.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2001-09-21 12:57 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-09-20  8:39 via82cxxx_audio locking problems Thomas Sailer
2001-09-20 11:33 ` Nicholas Knight
2001-09-20 12:07   ` Adrian Cox
2001-09-20 12:24     ` Nicholas Knight
2001-09-20 13:40       ` André Dahlqvist
2001-09-20 13:41         ` Thomas Sailer
2001-09-21  9:27         ` Thomas Sailer
2001-09-21 12:06           ` André Dahlqvist
2001-09-21 13:01             ` Lockups fixed! (Was: via82cxxx_audio locking problems) André Dahlqvist
2001-09-20 16:33 ` via82cxxx_audio locking problems Jeff Garzik
2001-09-21  7:50   ` Adrian Cox
2001-09-21  8:36     ` David Chow
2001-09-21  8:50       ` David Chow

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).