i810_audio.c v0.11 is still broken (deadlocks)

i810_audio.c v0.11 is still broken (deadlocks)

[Andris Pavenis]

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

Downloaded i810_audio.c v0.11 this morning. 

After loading it I'm getting system deadlock immediatelly after first attempt 
to use sound support as i810_write() calls i810_update_lvi() without setting
PCM_ENABLE_INPUT bit. Added setting this bit in i810_write() and
also some protection against deadlocks in __i810_update_lvi() with 
corresponding error message (see attached diffs).

Results: sound seems to work as far as I have tested for not a very long time.

I'm still getting messages that __i810_update_lvi() is called without setting
PCM_ENABLE_INPUT (which would cause deadlock without protection
I added there).

Andris

[-- Attachment #2: i810_audio.c.diff --]
[-- Type: text/x-diff, Size: 856 bytes --]

--- i810_audio.c-0.11	Fri Dec  7 08:29:51 2001
+++ i810_audio.c	Fri Dec  7 11:28:32 2001
@@ -959,7 +959,11 @@
 		} else {
 			__start_dac(state);
 		}
-		while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
+		if (dmabuf->trigger & PCM_ENABLE_INPUT) {
+    			while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
+		} else {
+			printk (KERN_ERR "i810_audio: __i810_update_lvi called without setting PCM_ENABLE_INPUT\n");
+		}
 	}
 
 	/* swptr - 1 is the tail of our transfer */
@@ -1509,6 +1513,12 @@
 		x = dmabuf->fragsize - (swptr % dmabuf->fragsize);
 		memset(dmabuf->rawbuf + swptr, '\0', x);
 	}
+	// There is data waiting to be played
+	/*
+	 * Force the trigger setting since we would
+	 * deadlock with it set any other way
+	 */
+	dmabuf->trigger = PCM_ENABLE_OUTPUT;
 	i810_update_lvi(state,0);
 ret:
         set_current_state(TASK_RUNNING);

Re: i810_audio.c v0.11 is still broken (deadlocks)

[Andris Pavenis]

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

On Friday 07 December 2001 12:21, Andris Pavenis wrote:
> Downloaded i810_audio.c v0.11 this morning.
>
> After loading it I'm getting system deadlock immediatelly after first
> attempt to use sound support as i810_write() calls i810_update_lvi()
> without setting PCM_ENABLE_INPUT bit. Added setting this bit in
> i810_write() and
> also some protection against deadlocks in __i810_update_lvi() with
> corresponding error message (see attached diffs).
>
> Results: sound seems to work as far as I have tested for not a very long
> time.
>
> I'm still getting messages that __i810_update_lvi() is called without
> setting PCM_ENABLE_INPUT (which would cause deadlock without protection
> I added there).

My previous patch was incorrect

Here is updated version of the patch against i810_audio.c v 0.11 (from
http://people.redhat.com/dledford/i810_audio.c.gz), which seems to work for 
me. I still left in debugging output.

Andris

[-- Attachment #2: i810_audio.c.diff2 --]
[-- Type: text/x-diff, Size: 3185 bytes --]

--- i810_audio.c-0.11	Fri Dec  7 08:29:51 2001
+++ i810_audio.c	Fri Dec  7 14:37:30 2001
@@ -690,25 +690,35 @@
 	spin_unlock_irqrestore(&card->lock, flags);
 }
 
-static inline void __start_adc(struct i810_state *state)
+static inline int __start_adc(struct i810_state *state)
 {
+	int ret = 0;
 	struct dmabuf *dmabuf = &state->dmabuf;
 
+	if ( ! (dmabuf->trigger & PCM_ENABLE_INPUT) ) {
+		printk (KERN_ERR "i810_audio: __start_adc called without PCM_ENABLE_INPUT set\n");
+		dmabuf->trigger |= PCM_ENABLE_INPUT;
+	}
+
 	if (dmabuf->count < dmabuf->dmasize && dmabuf->ready && !dmabuf->enable &&
 	    (dmabuf->trigger & PCM_ENABLE_INPUT)) {
+		ret = 1;
 		dmabuf->enable |= ADC_RUNNING;
 		outb((1<<4) | (1<<2) | 1, state->card->iobase + PI_CR);
 	}
+	return ret;
 }
 
-static void start_adc(struct i810_state *state)
+static int start_adc(struct i810_state *state)
 {
+	int ret;
 	struct i810_card *card = state->card;
 	unsigned long flags;
 
 	spin_lock_irqsave(&card->lock, flags);
-	__start_adc(state);
+	ret = __start_adc(state);
 	spin_unlock_irqrestore(&card->lock, flags);
+	return ret;
 }
 
 /* stop playback (lock held) */
@@ -736,24 +746,34 @@
 	spin_unlock_irqrestore(&card->lock, flags);
 }	
 
-static inline void __start_dac(struct i810_state *state)
+static inline int __start_dac(struct i810_state *state)
 {
+	int ret = 0;
 	struct dmabuf *dmabuf = &state->dmabuf;
 
+	if ( ! (dmabuf->trigger & PCM_ENABLE_OUTPUT) ) {
+		printk (KERN_ERR "i810_audio: __start_adc called without PCM_ENABLE_INPUT set\n");
+		dmabuf->trigger |= PCM_ENABLE_OUTPUT;
+	}
+
 	if (dmabuf->count > 0 && dmabuf->ready && !dmabuf->enable &&
 	    (dmabuf->trigger & PCM_ENABLE_OUTPUT)) {
+		ret = 1;
 		dmabuf->enable |= DAC_RUNNING;
 		outb((1<<4) | (1<<2) | 1, state->card->iobase + PO_CR);
 	}
+	return ret;
 }
-static void start_dac(struct i810_state *state)
+static int start_dac(struct i810_state *state)
 {
+	int ret;
 	struct i810_card *card = state->card;
 	unsigned long flags;
 
 	spin_lock_irqsave(&card->lock, flags);
-	__start_dac(state);
+	ret = __start_dac(state);
 	spin_unlock_irqrestore(&card->lock, flags);
+	return ret;
 }
 
 #define DMABUF_DEFAULTORDER (16-PAGE_SHIFT)
@@ -936,7 +956,7 @@
 static void __i810_update_lvi(struct i810_state *state, int rec)
 {
 	struct dmabuf *dmabuf = &state->dmabuf;
-	int x, port;
+	int x, port, ok;
 	
 	port = state->card->iobase;
 	if(rec)
@@ -955,11 +975,13 @@
 	if (!dmabuf->enable) {
 		outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
 		if(rec) {
-			__start_adc(state);
+			ok = __start_adc(state);
 		} else {
-			__start_dac(state);
+			ok = __start_dac(state);
+		}
+		if (ok) {
+			while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
 		}
-		while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
 	}
 
 	/* swptr - 1 is the tail of our transfer */
@@ -1509,6 +1531,12 @@
 		x = dmabuf->fragsize - (swptr % dmabuf->fragsize);
 		memset(dmabuf->rawbuf + swptr, '\0', x);
 	}
+	// There is data waiting to be played
+	/*
+	 * Force the trigger setting since we would
+	 * deadlock with it set any other way
+	 */
+	dmabuf->trigger = PCM_ENABLE_OUTPUT;
 	i810_update_lvi(state,0);
 ret:
         set_current_state(TASK_RUNNING);