Re: [PATCH] i810_audio fix for version 0.11

Doug Ledford
2001-12-08T09:36:41Z

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