[PATCH] i810_audio fix for version 0.11

[PATCH] i810_audio fix for version 0.11

[Nathan Bryant]

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


With this patch, it seems to work fine. Without, it hangs on write.

[-- Attachment #2: 11fixed.diff --]
[-- Type: text/plain, Size: 538 bytes --]

--- i810_audio.c.11	Thu Dec  6 18:07:35 2001
+++ linux/drivers/sound/i810_audio.c	Thu Dec  6 21:27:42 2001
@@ -955,8 +955,13 @@
 	if (!dmabuf->enable) {
 		outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
 		if(rec) {
+			/* must set trigger or we won't really start the
+			   converter, and we'll hang waiting for it to
+			   start. */
+			dmabuf->trigger = PCM_ENABLE_INPUT;
 			__start_adc(state);
 		} else {
+			dmabuf->trigger = PCM_ENABLE_OUTPUT;
 			__start_dac(state);
 		}
 		while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

 > With this patch, it seems to work fine. Without, it hangs on write.

I met case when dmabuf->count==0 when __start_dac() is called. As result
I still got system freezing even if PCM_ENABLE_INPUT or 
PCM_ENABLE_OUTPUT were set accordingly (I used different patch, see 
another patch I sent today).

My latest revision of patch "survives" without problems already some 
hours (normally I'm not listening radio through internet all time, but 
this time I do ...)

Andris

Re: [PATCH] i810_audio fix for version 0.11

[Nathan Bryant]

Andris Pavenis wrote:

>  > With this patch, it seems to work fine. Without, it hangs on write.
> 
> I met case when dmabuf->count==0 when __start_dac() is called. As result
> I still got system freezing even if PCM_ENABLE_INPUT or 
> PCM_ENABLE_OUTPUT were set accordingly (I used different patch, see 
> another patch I sent today).
> 
> My latest revision of patch "survives" without problems already some 
> hours (normally I'm not listening radio through internet all time, but 
> this time I do ...)
> 
> Andris
> 
> 
> 
> 
> 
> 

i knew i shoula been a little less lazy with that one...

haven't looked at your revision yet but we should just clean up and make 
update_lvi self-contained so that it always does *something* appropriate 
regardless of state. maybe that's what you did. ;-)

(fyi, i'm not subscribed to linux-kernel, too much volume for the few 
specific interests i have, i don't see some of this stuff until, and if, 
i go digging thru archives)

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

On Fri, 7 Dec 2001, Nathan Bryant wrote:

> Andris Pavenis wrote:
> 
> >  > With this patch, it seems to work fine. Without, it hangs on write.
> > 
> > I met case when dmabuf->count==0 when __start_dac() is called. As result
> > I still got system freezing even if PCM_ENABLE_INPUT or 
> > PCM_ENABLE_OUTPUT were set accordingly (I used different patch, see 
> > another patch I sent today).
> > 
> > My latest revision of patch "survives" without problems already some 
> > hours (normally I'm not listening radio through internet all time, but 
> > this time I do ...)
> > 
> > Andris
> 
> i knew i shoula been a little less lazy with that one...
> 
> haven't looked at your revision yet but we should just clean up and make 
> update_lvi self-contained so that it always does *something* appropriate 
> regardless of state. maybe that's what you did. ;-)
> 
> (fyi, i'm not subscribed to linux-kernel, too much volume for the few 
> specific interests i have, i don't see some of this stuff until, and if, 
> i go digging thru archives)
> 

It seems that I can remove debug code from my version of update (or put
it inside '#ifdef DEBUG'). I'm torturing it practically without stop
and no problems found yet (initially listening some radio station with
RealPlayer, now put noatun playing one MP3 in a loop without stop)

About my patch: I changed __start_dac, start_dac, __start_adc and
start_adc to return integer (non zero if it is doing something at all).
I used this return code to see whether I should do a loop in 
__i810_update_lvi. 

At least I haven't got any message from debuging output I left in.

Andris

Re: [PATCH] i810_audio fix for version 0.11

[Doug Ledford]

Nathan Bryant wrote:

> Andris Pavenis wrote:
>
>>  > With this patch, it seems to work fine. Without, it hangs on write.
>>
>> I met case when dmabuf->count==0 when __start_dac() is called. As result
>> I still got system freezing even if PCM_ENABLE_INPUT or 
>> PCM_ENABLE_OUTPUT were set accordingly (I used different patch, see 
>> another patch I sent today).
>>
>> My latest revision of patch "survives" without problems already some 
>> hours (normally I'm not listening radio through internet all time, 
>> but this time I do ...)
>>
>> Andris
>>
>>
>>
>>
>>
>>
>
> i knew i shoula been a little less lazy with that one...
>
> haven't looked at your revision yet but we should just clean up and 
> make update_lvi self-contained so that it always does *something* 
> appropriate regardless of state. maybe that's what you did. ;-)
>
> (fyi, i'm not subscribed to linux-kernel, too much volume for the few 
> specific interests i have, i don't see some of this stuff until, and 
> if, i go digging thru archives)
>
Well, unfortunately, neither of the patches you guys sent do what I was 
looking for ;-)  My goal with that code was to enable a specific certain 
behaviour, and because of the deadlock I have to make a few changes 
elsewhere for it to work properly.  The workaround patches are fine for 
now, but later today I'll make a 0.12 that fixes it the way I'm looking 
for.  (Hint: it's legal for a program to call SETTRIGGER to disable PCM 
output, then call the write() routine to fill the buffer, then call 
SETTRIGGER again to start output, otherwise known as pre buffering, and 
I want to support that without forcing the DAC to be started on 
update_lvi())

The real answer is multipart:

1) during i810_open go back to the old behaviour of setting 
dmabuf->trigger to PCM_ENABLE_INPUT and/or OUTPUT based on file mode.

2) make sure that i810_mmap clears dmabuf->trigger

3) make sure that in both i810_write and i810_read, we force the trigger 
setting when we can't output/input any data because count <= 0

4) in update_lvi make the check something like:

if (!dmabuf->enable && dmabuf->trigger) {
    ....
}

That should solve the problem, I just haven't written it up yet.

Re: [PATCH] i810_audio fix for version 0.11

[Doug Ledford]

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

Doug Ledford wrote:

> Well, unfortunately, neither of the patches you guys sent do what I 
> was looking for ;-)  My goal with that code was to enable a specific 
> certain behaviour, and because of the deadlock I have to make a few 
> changes elsewhere for it to work properly.  The workaround patches are 
> fine for now, but later today I'll make a 0.12 that fixes it the way 
> I'm looking for.  (Hint: it's legal for a program to call SETTRIGGER 
> to disable PCM output, then call the write() routine to fill the 
> buffer, then call SETTRIGGER again to start output, otherwise known as 
> pre buffering, and I want to support that without forcing the DAC to 
> be started on update_lvi())
>
> The real answer is multipart:
>
> 1) during i810_open go back to the old behaviour of setting 
> dmabuf->trigger to PCM_ENABLE_INPUT and/or OUTPUT based on file mode.
>
> 2) make sure that i810_mmap clears dmabuf->trigger
>
> 3) make sure that in both i810_write and i810_read, we force the 
> trigger setting when we can't output/input any data because count <= 0
>
> 4) in update_lvi make the check something like:
>
> if (!dmabuf->enable && dmabuf->trigger) {
>    ....
> }
>
> That should solve the problem, I just haven't written it up yet.
>
>
>
OK, the attached patch should do what is mentioned above.

Doug Ledford


[-- Attachment #2: patch-12 --]
[-- Type: text/plain, Size: 1577 bytes --]

--- i810_audio.c.11	Thu Dec  6 16:53:41 2001
+++ i810_audio.c.12	Fri Dec  7 13:32:38 2001
@@ -198,7 +198,7 @@
 #define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)
 
 
-#define DRIVER_VERSION "0.11"
+#define DRIVER_VERSION "0.12"
 
 /* magic numbers to protect our data structures */
 #define I810_CARD_MAGIC		0x5072696E /* "Prin" */
@@ -952,7 +952,7 @@
 	 * 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) {
+	if (!dmabuf->enable && dmabuf->trigger) {
 		outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
 		if(rec) {
 			__start_adc(state);
@@ -1111,8 +1111,11 @@
 
 		/* 
 		 * This will make sure that our LVI is correct, that our
-		 * pointer is updated, and that the DAC is running
+		 * 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);
 
 		if (signal_pending(current))
@@ -2280,6 +2283,7 @@
 			card->states[i] = NULL;;
 			return -EBUSY;
 		}
+		dmabuf->trigger |= PCM_ENABLE_INPUT;
 		i810_set_adc_rate(state, 8000);
 	}
 	if(file->f_mode & FMODE_WRITE) {
@@ -2291,6 +2295,7 @@
 		/* Initialize to 8kHz?  What if we don't support 8kHz? */
 		/*  Let's change this to check for S/PDIF stuff */
 	
+		dmabuf->trigger |= PCM_ENABLE_OUTPUT;
 		if ( spdif_locked ) {
 			i810_set_dac_rate(state, spdif_locked);
 			i810_set_spdif_output(state, AC97_EA_SPSA_3_4, spdif_locked);

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

Sorry, but this patch is still not OK. It still causes system
locking up for me.

In some cases I have (I added printk in __start_dac):
	dmabuf->count = 0
	dmabuf->ready = 1
	dmabuf->enable = 1
	PCM_ENABLE_OUTPUT set in dmabuf->triger

in __start_dac(). As result nothing was done in this procedure
and I got system locking in __i810_update_lvi() immediatelly after
that. This was reason why I added return code to __start_dac,
__start_adc to skip the loop if there is nothing to wait for.
Perhaps checking return code is more efficient and less error prone
that repeating all conditions in __i810_update_lvi. 

Maybe really we should use wait_event_interruptible() instead
of plain loop in __i810_update_lvi(). This happens not so often,
so I don't think it could cause too big delays. At least we could
avoid kernel freezing in this way.

Andris

On Fri, 7 Dec 2001, Doug Ledford wrote:

> Doug Ledford wrote:
> 
> > Well, unfortunately, neither of the patches you guys sent do what I 
> > was looking for ;-)  My goal with that code was to enable a specific 
> > certain behaviour, and because of the deadlock I have to make a few 
> > changes elsewhere for it to work properly.  The workaround patches are 
> > fine for now, but later today I'll make a 0.12 that fixes it the way 
> > I'm looking for.  (Hint: it's legal for a program to call SETTRIGGER 
> > to disable PCM output, then call the write() routine to fill the 
> > buffer, then call SETTRIGGER again to start output, otherwise known as 
> > pre buffering, and I want to support that without forcing the DAC to 
> > be started on update_lvi())
> >
> > The real answer is multipart:
> >
> > 1) during i810_open go back to the old behaviour of setting 
> > dmabuf->trigger to PCM_ENABLE_INPUT and/or OUTPUT based on file mode.
> >
> > 2) make sure that i810_mmap clears dmabuf->trigger
> >
> > 3) make sure that in both i810_write and i810_read, we force the 
> > trigger setting when we can't output/input any data because count <= 0
> >
> > 4) in update_lvi make the check something like:
> >
> > if (!dmabuf->enable && dmabuf->trigger) {
> >    ....
> > }
> >
> > That should solve the problem, I just haven't written it up yet.
> >
> >
> >
> OK, the attached patch should do what is mentioned above.
> 
> Doug Ledford
> 
> 

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

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

On Saturday 08 December 2001 10:39, Andris Pavenis wrote:
> Sorry, but this patch is still not OK. It still causes system
> locking up for me.
>
> In some cases I have (I added printk in __start_dac):
> 	dmabuf->count = 0
> 	dmabuf->ready = 1
> 	dmabuf->enable = 1
> 	PCM_ENABLE_OUTPUT set in dmabuf->triger
>
> in __start_dac(). As result nothing was done in this procedure
> and I got system locking in __i810_update_lvi() immediatelly after
> that. This was reason why I added return code to __start_dac,
> __start_adc to skip the loop if there is nothing to wait for.
> Perhaps checking return code is more efficient and less error prone
> that repeating all conditions in __i810_update_lvi.
>
> Maybe really we should use wait_event_interruptible() instead
> of plain loop in __i810_update_lvi(). This happens not so often,
> so I don't think it could cause too big delays. At least we could
> avoid kernel freezing in this way.
>

Here is my updated patch against v0.12 (I changed version to 0.12a to point 
a version against which is the patch)

Andris

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

--- i810_audio.c-0.12	Sat Dec  8 10:24:24 2001
+++ i810_audio.c	Sat Dec  8 11:14:16 2001
@@ -198,7 +198,7 @@
 #define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)
 
 
-#define DRIVER_VERSION "0.12"
+#define DRIVER_VERSION "0.12a"
 
 /* magic numbers to protect our data structures */
 #define I810_CARD_MAGIC		0x5072696E /* "Prin" */
@@ -690,25 +690,30 @@
 	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->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 +741,29 @@
 	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->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 +946,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 +965,12 @@
 	if (!dmabuf->enable && dmabuf->trigger) {
 		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);
 		}
-		while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
+		
+		if (ok) while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
 	}
 
 	/* swptr - 1 is the tail of our transfer */

Re: [PATCH] i810_audio fix for version 0.11

[Doug Ledford]

Andris Pavenis wrote:

>On Saturday 08 December 2001 10:39, Andris Pavenis wrote:
>
>>Sorry, but this patch is still not OK. It still causes system
>>locking up for me.
>>
>>In some cases I have (I added printk in __start_dac):
>>	dmabuf->count = 0
>>	dmabuf->ready = 1
>>	dmabuf->enable = 1
>>	PCM_ENABLE_OUTPUT set in dmabuf->triger
>>
Actually, since the problem is that there are obviously some "just in 
case" type calls if i810_update_lvi(), the best answer is not to even go 
through all those motions when dmabuf->count == 0.  So, I would add a 
line to i810_update_lvi() that makes it return without doing anything 
when dmabuf->count == 0.  That one line should solve your lockups (and 
finalize the 0.12 version).

>>
>>in __start_dac(). As result nothing was done in this procedure
>>and I got system locking in __i810_update_lvi() immediatelly after
>>that. This was reason why I added return code to __start_dac,
>>__start_adc to skip the loop if there is nothing to wait for.
>>Perhaps checking return code is more efficient and less error prone
>>that repeating all conditions in __i810_update_lvi.
>>
>>Maybe really we should use wait_event_interruptible() instead
>>of plain loop in __i810_update_lvi(). This happens not so often,
>>so I don't think it could cause too big delays. At least we could
>>avoid kernel freezing in this way.
>>
>
>Here is my updated patch against v0.12 (I changed version to 0.12a to point 
>a version against which is the patch)
>
>Andris
>
>
>------------------------------------------------------------------------
>
>--- i810_audio.c-0.12	Sat Dec  8 10:24:24 2001
>+++ i810_audio.c	Sat Dec  8 11:14:16 2001
>@@ -198,7 +198,7 @@
> #define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)
> 
> 
>-#define DRIVER_VERSION "0.12"
>+#define DRIVER_VERSION "0.12a"
> 
> /* magic numbers to protect our data structures */
> #define I810_CARD_MAGIC		0x5072696E /* "Prin" */
>@@ -690,25 +690,30 @@
> 	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->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 +741,29 @@
> 	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->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 +946,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 +965,12 @@
> 	if (!dmabuf->enable && dmabuf->trigger) {
> 		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);
> 		}
>-		while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
>+		
>+		if (ok) while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
> 	}
> 
> 	/* swptr - 1 is the tail of our transfer */
>

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

On Saturday 08 December 2001 11:36, Doug Ledford wrote:
> Andris Pavenis wrote:
> >On Saturday 08 December 2001 10:39, Andris Pavenis wrote:
> >>Sorry, but this patch is still not OK. It still causes system
> >>locking up for me.
> >>
> >>In some cases I have (I added printk in __start_dac):
> >>	dmabuf->count = 0
> >>	dmabuf->ready = 1
> >>	dmabuf->enable = 1
> >>	PCM_ENABLE_OUTPUT set in dmabuf->triger
>
> Actually, since the problem is that there are obviously some "just in
> case" type calls if i810_update_lvi(), the best answer is not to even go
> through all those motions when dmabuf->count == 0.  So, I would add a
> line to i810_update_lvi() that makes it return without doing anything
> when dmabuf->count == 0.  That one line should solve your lockups (and
> finalize the 0.12 version).
>

Why returning non zero from __start_dac() and similar procedures when 
something real has been done there is so bad. Using such return code would
ensure we never try to wait for results of __start_dac() if nothing is done 
by this procedure. I think such way is also more safe against possible future 
modifications as real conditions are only in a single place. Keeping them in 
2 places is possible source of bitrot if driver will be updated in future.

Andris

Re: [PATCH] i810_audio fix for version 0.11

[Doug Ledford]

Andris Pavenis wrote:

> Why returning non zero from __start_dac() and similar procedures when 
> something real has been done there is so bad. 


Personal preference.

> Using such return code would
> ensure we never try to wait for results of __start_dac() if nothing is done 
> by this procedure.


That's part of the point.  In this driver, I try to control when things are 
done and keep track of them in a deterministic way.  Using a return code to 
tell us a function we called did nothing when we shouldn't have called it in 
the first place if it wasn't going to do anything is backwards from the way 
I prefer to handle things.  Namely, find out why the function was called 
when it shouldn't have been and solve the problem.  Note: I don't follow 
that philosophy on all functions, only on very simple ones like this, there 
are a lot of complex functions where you want the function to make those 
decisions.  So, like I said, personal preference on how to handle these things.

> I think such way is also more safe against possible future 
> modifications as real conditions are only in a single place. Keeping them in 
> 2 places is possible source of bitrot if driver will be updated in future.


It's intended to do exactly that.  A lot of what makes this driver work 
properly right now is the LVI handling.  That was severly busted when I 
first got hold of the driver.  I *want* things to break if the LVI handling 
is changed by someone else because that will alert me to the fact that the 
LVI handling is then busted (at least, if they change it incorrectly, if 
they do things right then they will catch problems like this and fix them 
properly and I won't have to do anything).





-- 

  Doug Ledford <dledford@redhat.com>  http://people.redhat.com/dledford
       Please check my web site for aic7xxx updates/answers before
                       e-mailing me about problems

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

On Tuesday 11 December 2001 02:42, Doug Ledford wrote:
> Andris Pavenis wrote:
> > Why returning non zero from __start_dac() and similar procedures when
> > something real has been done there is so bad.
>
> Personal preference.

Thought about slightly different idea but didn't test it (I'm now about 300 km
away from a box where I did the tests and it will be so up to Cristmas):

	move both port operations used before and after call to
	__start_dac() and __start_adc() inside these inline procedures.
	In this case we would have waiting for results of __start_dac
	only when it really needed, so no possibility of deadlock
	there

>
> > Using such return code would
> > ensure we never try to wait for results of __start_dac() if nothing is
> > done by this procedure.
>
> That's part of the point.  In this driver, I try to control when things are
> done and keep track of them in a deterministic way.  Using a return code to
> tell us a function we called did nothing when we shouldn't have called it
> in the first place if it wasn't going to do anything is backwards from the
> way I prefer to handle things.  Namely, find out why the function was
> called when it shouldn't have been and solve the problem.  Note: I don't

If we moved 
                outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
and 
                while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
from inside __i810_update_lvi to __start_adc and __start_dac (inside
if block) we would avoid deadlocks. If You like to have noticed that
__start_dac ot __start_adc is called when they should not, then
add printk with message there (or even disable sound support totally with 
additional error message in log, so user will complain). I think leaving out 
possiblility of deadlocks is too dangerous.

> follow that philosophy on all functions, only on very simple ones like
> this, there are a lot of complex functions where you want the function to
> make those decisions.  So, like I said, personal preference on how to
> handle these things.
>
> > I think such way is also more safe against possible future
> > modifications as real conditions are only in a single place. Keeping them
> > in 2 places is possible source of bitrot if driver will be updated in
> > future.
>
> It's intended to do exactly that.  A lot of what makes this driver work
> properly right now is the LVI handling.  That was severly busted when I
> first got hold of the driver.  I *want* things to break if the LVI handling
> is changed by someone else because that will alert me to the fact that the
> LVI handling is then busted (at least, if they change it incorrectly, if
> they do things right then they will catch problems like this and fix them
> properly and I won't have to do anything).

I would suggest to disable sound support and give reasonable error message
in this case. So user will able to complain if this happens. It's more 
difficult to get usefull report when deadlock happens (and many users may not 
want to debug and provide additional info it in this case any more)

Andris