i810_audio fix for version 0.11

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

Quoted 1 line

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

Quoted 7 lines · Andris Pavenis

Quoted 1 line

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).

Quoted 5 lines

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)

Quoted 14 lines · Andris Pavenis

Quoted 1 line

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.

Quoted 21 lines · Doug Ledford

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

Quoted 10 lines

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

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.

Quoted 9 lines · 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

Quoted 8 lines

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.

Quoted 5 lines

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 renamed from i810_audio.c-0.12 +22 -11

@@ -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 */

Quoted 9 lines · Andris Pavenis

Quoted 8 lines · 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

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.

Quoted 2 lines · Andris Pavenis

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

Personal preference.

Quoted 4 lines

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.

Quoted 4 lines

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).

It still hanged machine after playing KDE startup sound. Didn't tried much more and moved to my modified version of 0.12 which worked

Quoted 2 lines · Andris Pavenis

It still hanged machine after playing KDE startup sound. Didn't tried much more and moved to my modified version of 0.12 which worked

please send me your modified version again. full file would be best, not patch.

i can't reproduce any problems here, so far, so i'm stabbing in the dark. if you can give more detailed information to reproduce, that would be nice: hardware versions, version of KDE, kde settings, (mine doesn't play a startup sound and artsd works fine for everything else i try), artsd settings. if i can reproduce, i can analyze on my own machine, if not, outlook is hazy ;-)

Got also my patched 0.12 freezing system later (kernel 2.4.17). Didn't saw
that earlier with 2.4.17-pre2. Also tried to move port I/O stuff from
__i810_update_lvi() to start_adc() and start_dac() inside if block to avoid any possibility of doing wait loop when noting is done in start_dac() and still get system locking up when loading driver. Perhaps I should
recheck with kernel 2.4.17-pre2 (I didn't have problems with it before
leaving to Finland now more than 2 weeks ago)

KDE-2.2.2 release (compiled here with gcc-2.95.3)

Contents of /proc/pci attached

Now returned in Riga and tested i810_audio driver version 0.13 from

	http://people.redhat.com/dledford/i810_audio.c.gz

with kernel 2.4.17

It still hanged machine after playing KDE startup sound. Didn't tried much more and moved to my modified version of 0.12 which worked

With best regards

Andris
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 2 lines · Andris Pavenis

It still hanged machine after playing KDE startup sound. Didn't tried much more and moved to my modified version of 0.12 which worked

please send me your modified version again. full file would be best, not patch.

i can't reproduce any problems here, so far, so i'm stabbing in the dark. if you can give more detailed information to reproduce, that would be nice: hardware versions, version of KDE, kde settings, (mine doesn't play a startup sound and artsd works fine for everything else i try), artsd settings. if i can reproduce, i can analyze on my own machine, if not, outlook is hazy ;-)