[I810_AUDIO] 1/x: Fix wait queue race in drain_dac

[I810_AUDIO] 1/x: Fix wait queue race in drain_dac

[Herbert Xu]

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

Hi:

This is the first of a number of patches to fix DMA bugs in the
OSS i810_audio driver.

This particular one fixes a textbook race condition in drain_dac
that causes it to timeout when it shouldn't.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p1 --]
[-- Type: text/plain, Size: 1771 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.8
diff -u -r1.8 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	3 Sep 2003 10:27:11 -0000	1.8
+++ kernel-source-2.4/drivers/sound/i810_audio.c	21 Nov 2003 11:20:48 -0000
@@ -1231,6 +1231,17 @@
 		spin_lock_irqsave(&state->card->lock, flags);
 		i810_update_ptr(state);
 		count = dmabuf->count;
+
+		/* It seems that we have to set the current state to
+		 * TASK_INTERRUPTIBLE every time to make the process
+		 * really go to sleep.  This also has to be *after* the
+		 * update_ptr() call because update_ptr is likely to
+		 * do a wake_up() which will unset this before we ever
+		 * try to sleep, resuling in a tight loop in this code
+		 * instead of actually sleeping and waiting for an
+		 * interrupt to wake us up!
+		 */
+		__set_current_state(TASK_INTERRUPTIBLE);
 		spin_unlock_irqrestore(&state->card->lock, flags);
 
 		if (count <= 0)
@@ -1250,16 +1261,6 @@
                         break;
                 }
 
-		/* It seems that we have to set the current state to
-		 * TASK_INTERRUPTIBLE every time to make the process
-		 * really go to sleep.  This also has to be *after* the
-		 * update_ptr() call because update_ptr is likely to
-		 * do a wake_up() which will unset this before we ever
-		 * try to sleep, resuling in a tight loop in this code
-		 * instead of actually sleeping and waiting for an
-		 * interrupt to wake us up!
-		 */
-		set_current_state(TASK_INTERRUPTIBLE);
 		/*
 		 * set the timeout to significantly longer than it *should*
 		 * take for the DAC to drain the DMA buffer

[I810_AUDIO] 2/x: Fix wait queue race in drain_dac

[Herbert Xu]

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

Hi:

This patch fixes the value of swptr in case of an underrun/overrun.

Overruns/underruns probably won't occur at all when the driver is
fixed properly, but this doesn't hurt.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p2 --]
[-- Type: text/plain, Size: 1101 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.9
diff -u -r1.9 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 07:05:27 -0000	1.9
+++ kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 07:11:25 -0000
@@ -1445,8 +1445,8 @@
                         }
                         continue;
                 }
-		swptr = dmabuf->swptr;
 		cnt = i810_get_available_read_data(state);
+		swptr = dmabuf->swptr;
 		// this is to make the copy_to_user simpler below
 		if(cnt > (dmabuf->dmasize - swptr))
 			cnt = dmabuf->dmasize - swptr;
@@ -1589,8 +1589,8 @@
                         continue;
                 }
 
-		swptr = dmabuf->swptr;
 		cnt = i810_get_free_write_space(state);
+		swptr = dmabuf->swptr;
 		/* Bound the maximum size to how much we can copy to the
 		 * dma buffer before we hit the end.  If we have more to
 		 * copy then it will get done in a second pass of this

[I810_AUDIO] 3/x: Remove bogus CIV_TO_LVI

[Herbert Xu]

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

This patch removes a pair of bogus LVI assignments.  The explanation in
the comment is wrong because the value of PCIB tells the hardware that
the DMA buffer can be processed even if LVI == CIV.

Setting LVI to CIV + 1 causes overruns when with short writes
(something that vmware is very fond of).
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p3 --]
[-- Type: text/plain, Size: 1428 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.10
diff -u -r1.10 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 07:14:08 -0000	1.10
+++ kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 07:16:08 -0000
@@ -1069,25 +1069,15 @@
 	else
 		port += dmabuf->write_channel->port;
 
-	/* if we are currently stopped, then our CIV is actually set to our
-	 * *last* sg segment and we are ready to wrap to the next.  However,
-	 * if we set our LVI to the last sg segment, then it won't wrap to
-	 * the next sg segment, it won't even get a start.  So, instead, when
-	 * we are stopped, we set both the LVI value and also we increment
-	 * the CIV value to the next sg segment to be played so that when
-	 * we call start_{dac,adc}, things will operate properly
-	 */
 	if (!dmabuf->enable && dmabuf->ready) {
 		if(rec && dmabuf->count < dmabuf->dmasize &&
 		   (dmabuf->trigger & PCM_ENABLE_INPUT))
 		{
-			CIV_TO_LVI(port, 1);
 			__start_adc(state);
 			while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
 		} else if (!rec && dmabuf->count &&
 			   (dmabuf->trigger & PCM_ENABLE_OUTPUT))
 		{
-			CIV_TO_LVI(port, 1);
 			__start_dac(state);
 			while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
 		}

[I810_AUDIO] 4/x: Clean up with macros

[Herbert Xu]

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

This patch adds a number macros to clean up the code.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p4 --]
[-- Type: text/plain, Size: 3521 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.11
diff -u -r1.11 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 07:20:03 -0000	1.11
+++ kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 08:17:09 -0000
@@ -143,6 +143,9 @@
 #define PCI_DEVICE_ID_AMD_8111_AC97	0x746d
 #endif
 
+#define MODULOP2(a, b) ((a) & ((b) - 1))
+#define MASKP2(a, b) ((a) & ~((b) - 1))
+
 static int ftsodell=0;
 static int strict_clocking=0;
 static unsigned int clocking=0;
@@ -204,6 +207,7 @@
 
 #define ENUM_ENGINE(PRE,DIG) 									\
 enum {												\
+	PRE##_BASE =	0x##DIG##0,		/* Base Address */				\
 	PRE##_BDBAR =	0x##DIG##0,		/* Buffer Descriptor list Base Address */	\
 	PRE##_CIV =	0x##DIG##4,		/* Current Index Value */			\
 	PRE##_LVI =	0x##DIG##5,		/* Last Valid Index */				\
@@ -481,8 +485,12 @@
 /* extract register offset from codec struct */
 #define IO_REG_OFF(codec) (((struct i810_card *) codec->private_data)->ac97_id_map[codec->id])
 
+#define GET_CIV(port) MODULOP2(inb((port) + OFF_CIV), SG_LEN)
+#define GET_LVI(port) MODULOP2(inb((port) + OFF_LVI), SG_LEN)
+
 /* set LVI from CIV */
-#define CIV_TO_LVI(port, off) outb((inb(port+OFF_CIV)+off) & 31, port+OFF_LVI)
+#define CIV_TO_LVI(port, off) \
+	outb(MODULOP2(GET_CIV((port)) + (off), SG_LEN), (port) + OFF_LVI)
 
 static struct i810_card *devs = NULL;
 
@@ -752,7 +760,7 @@
 		port_picb = port + OFF_PICB;
 
 	do {
-		civ = inb(port+OFF_CIV) & 31;
+		civ = GET_CIV(port);
 		offset = inw(port_picb);
 		/* Must have a delay here! */ 
 		if(offset == 0)
@@ -772,7 +780,7 @@
 		 * that we won't have to worry about the chip still being
 		 * out of sync with reality ;-)
 		 */
-	} while (civ != (inb(port+OFF_CIV) & 31) || offset != inw(port_picb));
+	} while (civ != GET_CIV(port) || offset != inw(port_picb));
 		 
 	return (((civ + 1) * dmabuf->fragsize - (bytes * offset))
 		% dmabuf->dmasize);
@@ -1124,8 +1132,8 @@
 			/* this is normal for the end of a read */
 			/* only give an error if we went past the */
 			/* last valid sg entry */
-			if((inb(state->card->iobase + PI_CIV) & 31) !=
-			   (inb(state->card->iobase + PI_LVI) & 31)) {
+			if (GET_CIV(state->card->iobase + PI_BASE) !=
+			    GET_LVI(state->card->iobase + PI_BASE)) {
 				printk(KERN_WARNING "i810_audio: DMA overrun on read\n");
 				dmabuf->error++;
 			}
@@ -1149,13 +1157,13 @@
 			/* this is normal for the end of a write */
 			/* only give an error if we went past the */
 			/* last valid sg entry */
-			if((inb(state->card->iobase + PO_CIV) & 31) !=
-			   (inb(state->card->iobase + PO_LVI) & 31)) {
+			if (GET_CIV(state->card->iobase + PO_BASE) !=
+			    GET_LVI(state->card->iobase + PO_BASE)) {
 				printk(KERN_WARNING "i810_audio: DMA overrun on write\n");
 				printk("i810_audio: CIV %d, LVI %d, hwptr %x, "
 					"count %d\n",
-					inb(state->card->iobase + PO_CIV) & 31,
-					inb(state->card->iobase + PO_LVI) & 31,
+					GET_CIV(state->card->iobase + PO_BASE),
+					GET_LVI(state->card->iobase + PO_BASE),
 					dmabuf->hwptr, dmabuf->count);
 				dmabuf->error++;
 			}
@@ -3042,7 +3050,7 @@
 			goto config_out;
 		}
 		dmabuf->count = dmabuf->dmasize;
-		CIV_TO_LVI(card->iobase+dmabuf->write_channel->port, 31);
+		CIV_TO_LVI(card->iobase+dmabuf->write_channel->port, -1);
 		save_flags(flags);
 		cli();
 		start_dac(state);

[I810_AUDIO] 5/x: Fixed partial DMA transfers

[Herbert Xu]

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

This patch fixes a longstanding bug in this driver where partial fragments
are fed to the hardware.  Worse yet, those fragments are then extended
while the hardware is doing DMA transfers causing all sorts of problems.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p5 --]
[-- Type: text/plain, Size: 6494 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.12
diff -u -r1.12 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 08:22:41 -0000	1.12
+++ kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 08:22:43 -0000
@@ -1070,31 +1070,41 @@
 {
 	struct dmabuf *dmabuf = &state->dmabuf;
 	int x, port;
-	
+	int trigger;
+	int count, fragsize;
+	void (*start)(struct i810_state *);
+
+	count = dmabuf->count;
 	port = state->card->iobase;
-	if(rec)
+	if (rec) {
 		port += dmabuf->read_channel->port;
-	else
+		trigger = PCM_ENABLE_INPUT;
+		start = __start_adc;
+		count = dmabuf->dmasize - count;
+	} else {
 		port += dmabuf->write_channel->port;
+		trigger = PCM_ENABLE_OUTPUT;
+		start = __start_dac;
+	}
+
+	/* Do not process partial fragments. */
+	fragsize = dmabuf->fragsize;
+	if (count < fragsize)
+		return;
 
 	if (!dmabuf->enable && dmabuf->ready) {
-		if(rec && dmabuf->count < dmabuf->dmasize &&
-		   (dmabuf->trigger & PCM_ENABLE_INPUT))
-		{
-			__start_adc(state);
-			while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
-		} else if (!rec && dmabuf->count &&
-			   (dmabuf->trigger & PCM_ENABLE_OUTPUT))
-		{
-			__start_dac(state);
-			while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
-		}
+		if (!(dmabuf->trigger & trigger))
+			return;
+
+		start(state);
+		while (!(inb(port + OFF_CR) & ((1<<4) | (1<<2))))
+			;
 	}
 
-	/* swptr - 1 is the tail of our transfer */
-	x = (dmabuf->dmasize + dmabuf->swptr - 1) % dmabuf->dmasize;
-	x /= dmabuf->fragsize;
-	outb(x, port+OFF_LVI);
+	/* MASKP2(swptr, fragsize) - 1 is the tail of our transfer */
+	x = MODULOP2(MASKP2(dmabuf->swptr, fragsize) - 1, dmabuf->dmasize);
+	x /= fragsize;
+	outb(x, port + OFF_LVI);
 }
 
 static void i810_update_lvi(struct i810_state *state, int rec)
@@ -1114,13 +1124,17 @@
 {
 	struct dmabuf *dmabuf = &state->dmabuf;
 	unsigned hwptr;
+	unsigned fragmask, dmamask;
 	int diff;
 
-	/* error handling and process wake up for DAC */
+	fragmask = MASKP2(~0, dmabuf->fragsize);
+	dmamask = MODULOP2(~0, dmabuf->dmasize);
+
+	/* error handling and process wake up for ADC */
 	if (dmabuf->enable == ADC_RUNNING) {
 		/* update hardware pointer */
-		hwptr = i810_get_dma_addr(state, 1);
-		diff = (dmabuf->dmasize + hwptr - dmabuf->hwptr) % dmabuf->dmasize;
+		hwptr = i810_get_dma_addr(state, 1) & fragmask;
+		diff = (hwptr - dmabuf->hwptr) & dmamask;
 #if defined(DEBUG_INTERRUPTS) || defined(DEBUG_MMAP)
 		printk("ADC HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
 #endif
@@ -1138,14 +1152,14 @@
 				dmabuf->error++;
 			}
 		}
-		if (dmabuf->count > dmabuf->userfragsize)
+		if (diff)
 			wake_up(&dmabuf->wait);
 	}
 	/* error handling and process wake up for DAC */
 	if (dmabuf->enable == DAC_RUNNING) {
 		/* update hardware pointer */
-		hwptr = i810_get_dma_addr(state, 0);
-		diff = (dmabuf->dmasize + hwptr - dmabuf->hwptr) % dmabuf->dmasize;
+		hwptr = i810_get_dma_addr(state, 0) & fragmask;
+		diff = (hwptr - dmabuf->hwptr) & dmamask;
 #if defined(DEBUG_INTERRUPTS) || defined(DEBUG_MMAP)
 		printk("DAC HWP %d,%d,%d\n", hwptr, dmabuf->hwptr, diff);
 #endif
@@ -1168,7 +1182,7 @@
 				dmabuf->error++;
 			}
 		}
-		if (dmabuf->count < (dmabuf->dmasize-dmabuf->userfragsize))
+		if (diff)
 			wake_up(&dmabuf->wait);
 	}
 }
@@ -1185,7 +1199,6 @@
 		dmabuf->swptr = dmabuf->hwptr;
 	}
 	free = dmabuf->dmasize - dmabuf->count;
-	free -= (dmabuf->hwptr % dmabuf->fragsize);
 	if(free < 0)
 		return(0);
 	return(free);
@@ -1203,12 +1216,27 @@
 		dmabuf->swptr = dmabuf->hwptr;
 	}
 	avail = dmabuf->count;
-	avail -= (dmabuf->hwptr % dmabuf->fragsize);
 	if(avail < 0)
 		return(0);
 	return(avail);
 }
 
+static inline void fill_partial_frag(struct dmabuf *dmabuf)
+{
+	unsigned fragsize;
+	unsigned swptr, len;
+
+	fragsize = dmabuf->fragsize;
+	swptr = dmabuf->swptr;
+	len = fragsize - MODULOP2(dmabuf->swptr, fragsize);
+	if (len == fragsize)
+		return;
+
+	memset(dmabuf->rawbuf + swptr, '\0', len);
+	dmabuf->swptr = MODULOP2(swptr + len, dmabuf->dmasize);
+	dmabuf->count += len;
+}
+
 static int drain_dac(struct i810_state *state, int signals_allowed)
 {
 	DECLARE_WAITQUEUE(wait, current);
@@ -1223,6 +1251,22 @@
 		stop_dac(state);
 		return 0;
 	}
+
+	spin_lock_irqsave(&state->card->lock, flags);
+
+	fill_partial_frag(dmabuf);
+
+	/* 
+	 * This will make sure that our LVI is correct, that our
+	 * pointer is updated, and that the DAC is running.  We
+	 * have to force the setting of dmabuf->trigger to avoid
+	 * any possible deadlocks.
+	 */
+	dmabuf->trigger = PCM_ENABLE_OUTPUT;
+	i810_update_lvi(state, 0);
+
+	spin_unlock_irqrestore(&state->card->lock, flags);
+
 	add_wait_queue(&dmabuf->wait, &wait);
 	for (;;) {
 
@@ -1245,16 +1289,6 @@
 		if (count <= 0)
 			break;
 
-		/* 
-		 * This will make sure that our LVI is correct, that our
-		 * pointer is updated, and that the DAC is running.  We
-		 * have to force the setting of dmabuf->trigger to avoid
-		 * any possible deadlocks.
-		 */
-		if(!dmabuf->enable) {
-			dmabuf->trigger = PCM_ENABLE_OUTPUT;
-			i810_update_lvi(state,0);
-		}
                 if (signal_pending(current) && signals_allowed) {
                         break;
                 }
@@ -1339,11 +1373,10 @@
 			if(status & DMA_INT_DCH)
 				printk("DCH -");
 #endif
-			if(dmabuf->enable & DAC_RUNNING)
-				count = dmabuf->count;
-			else
-				count = dmabuf->dmasize - dmabuf->count;
-			if(count > 0) {
+			count = dmabuf->count;
+			if(dmabuf->enable & ADC_RUNNING)
+				count = dmabuf->dmasize - count;
+			if (count >= (int)dmabuf->fragsize) {
 				outb(inb(port+OFF_CR) | 1, port+OFF_CR);
 #ifdef DEBUG_INTERRUPTS
 				printk(" CONTINUE ");
@@ -1514,7 +1547,7 @@
 			goto done;
 		}
 
-		swptr = (swptr + cnt) % dmabuf->dmasize;
+		swptr = MODULOP2(swptr + cnt, dmabuf->dmasize);
 
 		spin_lock_irqsave(&card->lock, flags);
 
@@ -1548,7 +1581,7 @@
 	ssize_t ret;
 	unsigned long flags;
 	unsigned int swptr = 0;
-	int cnt, x;
+	int cnt;
         DECLARE_WAITQUEUE(waita, current);
 
 #ifdef DEBUG2
@@ -1672,10 +1705,6 @@
 		ret += cnt;
 		spin_unlock_irqrestore(&state->card->lock, flags);
 	}
-	if (swptr % dmabuf->fragsize) {
-		x = dmabuf->fragsize - (swptr % dmabuf->fragsize);
-		memset(dmabuf->rawbuf + swptr, '\0', x);
-	}
 ret:
 	i810_update_lvi(state,0);
         set_current_state(TASK_RUNNING);

Re: [I810_AUDIO] 5/x: Fixed partial DMA transfers

[Herbert Xu]

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

This patch fixes SETTRIGGER with playback so that the LVI is always
set and the DAC is always started.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p6 --]
[-- Type: text/plain, Size: 933 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.13
diff -u -r1.13 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 08:26:51 -0000	1.13
+++ kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 08:36:07 -0000
@@ -2218,10 +2218,10 @@
 				dmabuf->swptr = dmabuf->hwptr;
 				dmabuf->count = i810_get_free_write_space(state);
 				dmabuf->swptr = (dmabuf->swptr + dmabuf->count) % dmabuf->dmasize;
-				__i810_update_lvi(state, 0);
 				spin_unlock_irqrestore(&state->card->lock, flags);
-			} else
-				start_dac(state);
+			}
+			i810_update_lvi(state, 0);
+			start_dac(state);
 		}
 		if((file->f_mode & FMODE_READ) && (val & PCM_ENABLE_INPUT) && !(dmabuf->enable & ADC_RUNNING)) {
 			if (!dmabuf->read_channel) {

[I810_AUDIO] 7/x: Fix OSS fragments

[Herbert Xu]

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

This patch makes userfragsize do what it's meant to do: do not start
DAC/ADC until a full fragment is available.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p7 --]
[-- Type: text/plain, Size: 2091 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.14
diff -u -r1.14 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 08:39:42 -0000	1.14
+++ kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 10:00:30 -0000
@@ -1438,6 +1438,7 @@
 	unsigned long flags;
 	unsigned int swptr;
 	int cnt;
+	int pending;
         DECLARE_WAITQUEUE(waita, current);
 
 #ifdef DEBUG2
@@ -1463,6 +1464,8 @@
 		return -EFAULT;
 	ret = 0;
 
+	pending = 0;
+
         add_wait_queue(&dmabuf->wait, &waita);
 	while (count > 0) {
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -1556,7 +1559,7 @@
                         continue;
                 }
 		dmabuf->swptr = swptr;
-		dmabuf->count -= cnt;
+		pending = dmabuf->count -= cnt;
 		spin_unlock_irqrestore(&card->lock, flags);
 
 		count -= cnt;
@@ -1564,7 +1567,9 @@
 		ret += cnt;
 	}
  done:
-	i810_update_lvi(state,1);
+	pending = dmabuf->dmasize - pending;
+	if (dmabuf->enable || pending >= dmabuf->userfragsize)
+		i810_update_lvi(state, 1);
         set_current_state(TASK_RUNNING);
         remove_wait_queue(&dmabuf->wait, &waita);
 
@@ -1581,6 +1586,7 @@
 	ssize_t ret;
 	unsigned long flags;
 	unsigned int swptr = 0;
+	int pending;
 	int cnt;
         DECLARE_WAITQUEUE(waita, current);
 
@@ -1606,6 +1612,8 @@
 		return -EFAULT;
 	ret = 0;
 
+	pending = 0;
+
         add_wait_queue(&dmabuf->wait, &waita);
 	while (count > 0) {
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -1698,7 +1706,7 @@
                 }
 
 		dmabuf->swptr = swptr;
-		dmabuf->count += cnt;
+		pending = dmabuf->count += cnt;
 
 		count -= cnt;
 		buffer += cnt;
@@ -1706,7 +1714,8 @@
 		spin_unlock_irqrestore(&state->card->lock, flags);
 	}
 ret:
-	i810_update_lvi(state,0);
+	if (dmabuf->enable || pending >= dmabuf->userfragsize)
+		i810_update_lvi(state, 0);
         set_current_state(TASK_RUNNING);
         remove_wait_queue(&dmabuf->wait, &waita);
 

[I810_AUDIO] 8/x: Remove divides on playback

[Herbert Xu]

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

This patch removes a couple of divides on the playback path.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p8 --]
[-- Type: text/plain, Size: 1739 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.15
diff -u -r1.15 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 23:51:09 -0000	1.15
+++ kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 23:51:29 -0000
@@ -100,6 +100,7 @@
 #include <linux/smp_lock.h>
 #include <linux/ac97_codec.h>
 #include <linux/wrapper.h>
+#include <linux/bitops.h>
 #include <asm/uaccess.h>
 #include <asm/hardirq.h>
 
@@ -990,6 +991,7 @@
 	dmabuf->numfrag = SG_LEN;
 	dmabuf->fragsize = dmabuf->dmasize/dmabuf->numfrag;
 	dmabuf->fragsamples = dmabuf->fragsize >> 1;
+	dmabuf->fragshift = ffs(dmabuf->fragsize) - 1;
 	dmabuf->userfragsize = dmabuf->ossfragsize;
 	dmabuf->userfrags = dmabuf->dmasize/dmabuf->ossfragsize;
 
@@ -997,16 +999,12 @@
 
 	if(dmabuf->ossmaxfrags == 4) {
 		fragint = 8;
-		dmabuf->fragshift = 2;
 	} else if (dmabuf->ossmaxfrags == 8) {
 		fragint = 4;
-		dmabuf->fragshift = 3;
 	} else if (dmabuf->ossmaxfrags == 16) {
 		fragint = 2;
-		dmabuf->fragshift = 4;
 	} else {
 		fragint = 1;
-		dmabuf->fragshift = 5;
 	}
 	/*
 	 *	Now set up the ring 
@@ -1103,7 +1101,7 @@
 
 	/* MASKP2(swptr, fragsize) - 1 is the tail of our transfer */
 	x = MODULOP2(MASKP2(dmabuf->swptr, fragsize) - 1, dmabuf->dmasize);
-	x /= fragsize;
+	x >>= dmabuf->fragshift;
 	outb(x, port + OFF_LVI);
 }
 
@@ -1697,7 +1695,7 @@
 			goto ret;
 		}
 
-		swptr = (swptr + cnt) % dmabuf->dmasize;
+		swptr = MODULOP2(swptr + cnt, dmabuf->dmasize);
 
 		spin_lock_irqsave(&state->card->lock, flags);
                 if (PM_SUSPENDED(card)) {

[I810_AUDIO] 9/x: Fix drain_dac loop when signals_allowed == 0

[Herbert Xu]

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

This patch fixes another bug in the drain_dac wait loop when it is
called with signals_allowed == 0.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p9 --]
[-- Type: text/plain, Size: 752 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.16
diff -u -r1.16 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 23:53:34 -0000	1.16
+++ kernel-source-2.4/drivers/sound/i810_audio.c	22 Nov 2003 23:55:57 -0000
@@ -1281,7 +1281,8 @@
 		 * instead of actually sleeping and waiting for an
 		 * interrupt to wake us up!
 		 */
-		__set_current_state(TASK_INTERRUPTIBLE);
+		__set_current_state(signals_allowed ?
+				    TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE);
 		spin_unlock_irqrestore(&state->card->lock, flags);
 
 		if (count <= 0)

[I810_AUDIO] 10/x: Fix reads/writes % 4 != 0

[Herbert Xu]

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

This patch removes another bogus chunk of code that breaks when
the application does a partial write.

In particular, a read/write of x bytes where x % 4 != 0 will loop forever.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p10 --]
[-- Type: text/plain, Size: 1484 bytes --]

Index: kernel-source-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.17
diff -u -r1.17 i810_audio.c
--- kernel-source-2.4/drivers/sound/i810_audio.c	23 Nov 2003 00:34:22 -0000	1.17
+++ kernel-source-2.4/drivers/sound/i810_audio.c	23 Nov 2003 10:59:46 -0000
@@ -1487,15 +1487,6 @@
 
 		if (cnt > count)
 			cnt = count;
-		/* Lop off the last two bits to force the code to always
-		 * write in full samples.  This keeps software that sets
-		 * O_NONBLOCK but doesn't check the return value of the
-		 * write call from getting things out of state where they
-		 * think a full 4 byte sample was written when really only
-		 * a portion was, resulting in odd sound and stereo
-		 * hysteresis.
-		 */
-		cnt &= ~0x3;
 		if (cnt <= 0) {
 			unsigned long tmo;
 			/*
@@ -1643,15 +1634,6 @@
 #endif
 		if (cnt > count)
 			cnt = count;
-		/* Lop off the last two bits to force the code to always
-		 * write in full samples.  This keeps software that sets
-		 * O_NONBLOCK but doesn't check the return value of the
-		 * write call from getting things out of state where they
-		 * think a full 4 byte sample was written when really only
-		 * a portion was, resulting in odd sound and stereo
-		 * hysteresis.
-		 */
-		cnt &= ~0x3;
 		if (cnt <= 0) {
 			unsigned long tmo;
 			// There is data waiting to be played

Re: [I810_AUDIO] 1/x: Fix wait queue race in drain_dac

[Jeff Garzik]

Herbert,

Just wanted to 'ack' your i810_audio patches.  I think Alan stuck me 
with i810_audio ("you last touched it, you're it") so I'll look over and 
merge your patches into 2.4.x and 2.6.x.

i810_audio also needs to be MMIO-ized, which will be a simple but 
staggeringly huge change...

	Jeff

Re: [I810_AUDIO] 1/x: Fix wait queue race in drain_dac

[Jeff Garzik]

Herbert Xu wrote:
> Hi:
> 
> This is the first of a number of patches to fix DMA bugs in the
> OSS i810_audio driver.
> 
> This particular one fixes a textbook race condition in drain_dac
> that causes it to timeout when it shouldn't.

Herbert,

Thanks much for these i810_audio patches.  I've been meaning to review 
them in-depth for some time.

Could you be kind and "spell out" the patch-1 race for me?

Also, it seems to me that you would want to check for signal_pending()
(a) just after the schedule_timeout(), and
(b) -after- testing the 'signals_allowed' variable  ;-)

Comments?

	Jeff

Re: [I810_AUDIO] 2/x: Fix wait queue race in drain_dac

[Jeff Garzik]

thanks, applied patch-2 to 2.4 and 2.6

Re: [I810_AUDIO] 1/x: Fix wait queue race in drain_dac

[Herbert Xu]

On Sun, Jan 11, 2004 at 04:00:41PM -0500, Jeff Garzik wrote:
> 
> Thanks much for these i810_audio patches.  I've been meaning to review 
> them in-depth for some time.

Thanks a lot for reviewing them.

> Could you be kind and "spell out" the patch-1 race for me?

Prior to the patch, if an interrupt occured between the count check
and the setting of the current state the wait will timeout instead
of waking up immediately.

> Also, it seems to me that you would want to check for signal_pending()
> (a) just after the schedule_timeout(), and
> (b) -after- testing the 'signals_allowed' variable  ;-)

schedule() already checks for signals.

Cheers,
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [I810_AUDIO] 1/x: Fix wait queue race in drain_dac

[Jeff Garzik]

Herbert Xu wrote:
> On Sun, Jan 11, 2004 at 04:00:41PM -0500, Jeff Garzik wrote:
> 
>>Thanks much for these i810_audio patches.  I've been meaning to review 
>>them in-depth for some time.
> 
> 
> Thanks a lot for reviewing them.
> 
> 
>>Could you be kind and "spell out" the patch-1 race for me?
> 
> 
> Prior to the patch, if an interrupt occured between the count check
> and the setting of the current state the wait will timeout instead
> of waking up immediately.

hmmm, I'll have to think on this one a bit.  You are described observed 
behavior here... can you go a bit deeper, and describe what two code 
paths are racing?  I think I might a _different_ race in the code we're 
looking at, but I do not yet see the race you are describing :(


>>Also, it seems to me that you would want to check for signal_pending()
>>(a) just after the schedule_timeout(), and
>>(b) -after- testing the 'signals_allowed' variable  ;-)
> 
> 
> schedule() already checks for signals.

Well -- A signal won't be pending until after you call 
schedule_timeout() ;-)  A signal, particularly SIGINT, might even occur 
_during_ the schedule_timeout().

	Jeff

Re: [I810_AUDIO] 1/x: Fix wait queue race in drain_dac

[Herbert Xu]

On Mon, Jan 12, 2004 at 04:48:19PM -0500, Jeff Garzik wrote:
> 
> hmmm, I'll have to think on this one a bit.  You are described observed 
> behavior here... can you go a bit deeper, and describe what two code 
> paths are racing?  I think I might a _different_ race in the code we're 
> looking at, but I do not yet see the race you are describing :(

A				B
drain_dac()
	count > 0
				i810_channel_interrupt()
					i810_update_ptr()
						count = 0
						wake_up()
	set_current_state()
	schedule_timeout()
	*TIMEOUT*

> >schedule() already checks for signals.
> 
> Well -- A signal won't be pending until after you call 
> schedule_timeout() ;-)  A signal, particularly SIGINT, might even occur 
> _during_ the schedule_timeout().

Yes, but as long as you check signal_pending at least once in each
loop it does the right thing.  Can you point me to a scenario where
checking it right after schedule_timeout produces different behaviour
than the current code?

Cheers,
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[I810_AUDIO] 11/x: Fix dead lock in drain_dac

[Herbert Xu]

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

This patch fixes a typo in a previous change that causes the driver
to deadlock under SMP.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 617 bytes --]

Index: kernel-2.4/drivers/sound/i810_audio.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.4/drivers/sound/i810_audio.c,v
retrieving revision 1.19
diff -u -r1.19 i810_audio.c
--- kernel-2.4/drivers/sound/i810_audio.c	29 Nov 2003 09:53:43 -0000	1.19
+++ kernel-2.4/drivers/sound/i810_audio.c	16 Jan 2004 22:49:54 -0000
@@ -1260,7 +1260,7 @@
 	 * any possible deadlocks.
 	 */
 	dmabuf->trigger = PCM_ENABLE_OUTPUT;
-	i810_update_lvi(state, 0);
+	__i810_update_lvi(state, 0);
 
 	spin_unlock_irqrestore(&state->card->lock, flags);