Re: via82cxxx_audio locking problems

[Thomas Sailer <sailer@scs.ch>]

[-- 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;
 }