[PATCH] MPU-401 fixes

[PATCH] MPU-401 fixes

[Clemens Ladisch]

- fix deadlock: when snd_mpu401_uart_input_trigger() is interrupted
  immediately after the call to spin_unlock_irqrestore(), the interrupt
  handler won't clear the interrupt condition because the RX_LOOP bit is
  still set
- abort the tx loop when the FIFO is full; otherwise, the inner loop would
  be executed 25600 times
- clear the output trigger bit when output buffer is exhausted


Index: alsa-kernel/drivers/mpu401/mpu401_uart.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/drivers/mpu401/mpu401_uart.c,v
retrieving revision 1.23
diff -u -r1.23 mpu401_uart.c
--- alsa-kernel/drivers/mpu401/mpu401_uart.c	23 Apr 2003 10:01:28 -0000	1.23
+++ alsa-kernel/drivers/mpu401/mpu401_uart.c	5 May 2003 06:28:02 -0000
@@ -93,12 +93,9 @@
 static void _snd_mpu401_uart_interrupt(mpu401_t *mpu)
 {
 	if (test_bit(MPU401_MODE_BIT_INPUT, &mpu->mode)) {
-		if (! test_and_set_bit(MPU401_MODE_BIT_RX_LOOP, &mpu->mode)) {
-			spin_lock(&mpu->input_lock);
-			snd_mpu401_uart_input_read(mpu);
-			spin_unlock(&mpu->input_lock);
-			clear_bit(MPU401_MODE_BIT_RX_LOOP, &mpu->mode);
-		}
+		spin_lock(&mpu->input_lock);
+		snd_mpu401_uart_input_read(mpu);
+		spin_unlock(&mpu->input_lock);
 	} else
 		snd_mpu401_uart_clear_rx(mpu);
  	/* ok. for better Tx performance try do some output when input is done */
@@ -369,9 +366,12 @@
 					break;
 				}
 			}
+			if (timeout == 0)
+				break; /* Tx FIFO full - try again later */
 		} else {
 			snd_mpu401_uart_remove_timer (mpu, 0);
-			max = 1; /* no other data - leave the tx loop */
+			clear_bit(MPU401_MODE_BIT_OUTPUT_TRIGGER, &mpu->mode);
+			break; /* no other data - leave the tx loop */
 		}
 	} while (--max > 0);
 }




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] MPU-401 fixes

[Jaroslav Kysela]

On Mon, 5 May 2003, Clemens Ladisch wrote:

> - fix deadlock: when snd_mpu401_uart_input_trigger() is interrupted
>   immediately after the call to spin_unlock_irqrestore(), the interrupt
>   handler won't clear the interrupt condition because the RX_LOOP bit is
>   still set

Thanks. The RX_LOOP bit is cleared before unlock now.

> - abort the tx loop when the FIFO is full; otherwise, the inner loop would
>   be executed 25600 times

Fixed.

> - clear the output trigger bit when output buffer is exhausted

Not possible. Only the trigger callback can do this thing.

I've added also TX_LOOP protection to the interrupt handler.

Please, could you verify my code? Thanks.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] MPU-401 fixes

[Clemens Ladisch]

Jaroslav Kysela wrote:
> On Mon, 5 May 2003, Clemens Ladisch wrote:
>
> > - fix deadlock: when snd_mpu401_uart_input_trigger() is interrupted
> >   immediately after the call to spin_unlock_irqrestore(), the interrupt
> >   handler won't clear the interrupt condition because the RX_LOOP bit is
> >   still set
>
> Thanks. The RX_LOOP bit is cleared before unlock now.

The same thing will happen when the interrupt occurs immediately before
the call to spin_lock_irqsave().

The real problem is that the interrupt handler _must_ read the data from
the device to clear the MPU401's interrupt condition. Therefore, there
must not be any condition which can prevent the handler from reading data.

> > - abort the tx loop when the FIFO is full; otherwise, the inner loop would
> >   be executed 25600 times
>
> Fixed.

Without the "if (timeout == 0)" test, the loop will be aborted after the
first byte written.


BTW: Where did my patches from Friday go?


Regards,
Clemens




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] MPU-401 fixes

[Takashi Iwai]

At Mon, 05 May 2003 13:19:34 +0200 (METDST),
Clemens Ladisch wrote:
> 
> Jaroslav Kysela wrote:
> > On Mon, 5 May 2003, Clemens Ladisch wrote:
> >
> > > - fix deadlock: when snd_mpu401_uart_input_trigger() is interrupted
> > >   immediately after the call to spin_unlock_irqrestore(), the interrupt
> > >   handler won't clear the interrupt condition because the RX_LOOP bit is
> > >   still set
> >
> > Thanks. The RX_LOOP bit is cleared before unlock now.
> 
> The same thing will happen when the interrupt occurs immediately before
> the call to spin_lock_irqsave().
> 
> The real problem is that the interrupt handler _must_ read the data from
> the device to clear the MPU401's interrupt condition. Therefore, there
> must not be any condition which can prevent the handler from reading data.

hmm, this problem is not easy to solve.

originally, the bit check was introduced to prevent double spinlocks
in the path:

	irq
	-> snd_mpu401_uart_interrupt()
	-> lock
	-> snd_mpu401_uart_input_read()
		-> snd_rawmidi_receive()
			-> rawmidi->event()
				-> rawmidi->trigger() (in seq_midi.c)
					-> snd_mpu401_uart_input_trigger()


> > > - abort the tx loop when the FIFO is full; otherwise, the inner loop would
> > >   be executed 25600 times
> >
> > Fixed.
> 
> Without the "if (timeout == 0)" test, the loop will be aborted after the
> first byte written.

yep.

> 
> BTW: Where did my patches from Friday go?

applied now.  thanks.


ciao,

Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] MPU-401 fixes

[Clemens Ladisch]

Takashi Iwai wrote:
> Clemens Ladisch wrote:
> > Jaroslav Kysela wrote:
> > > On Mon, 5 May 2003, Clemens Ladisch wrote:
> > >
> > > > - fix deadlock: when snd_mpu401_uart_input_trigger() is interrupted
> > > >   immediately after the call to spin_unlock_irqrestore(), the interrupt
> > > >   handler won't clear the interrupt condition because the RX_LOOP bit is
> > > >   still set
> > >
> > > Thanks. The RX_LOOP bit is cleared before unlock now.
> >
> > The same thing will happen when the interrupt occurs immediately before
> > the call to spin_lock_irqsave().
> >
> > The real problem is that the interrupt handler _must_ read the data from
> > the device to clear the MPU401's interrupt condition. Therefore, there
> > must not be any condition which can prevent the handler from reading data.
>
> hmm, this problem is not easy to solve.
>
> originally, the bit check was introduced to prevent double spinlocks
> in the path:
>
> 	irq
> 	-> snd_mpu401_uart_interrupt()
> 	-> lock
> 	-> snd_mpu401_uart_input_read()
> 		-> snd_rawmidi_receive()
> 			-> rawmidi->event()
> 				-> rawmidi->trigger() (in seq_midi.c)
> 					-> snd_mpu401_uart_input_trigger()

AFAICS the only protection needed is against a recursive call of
_input_trigger() but not of _interrupt() because the interrupt routine
cannot be called inside the spin_lock_irqsave(), i.e., the code being
interrupted will not be the _input_read() function.

So a solution would be that _interrupt() sets the RX_LOOP bit, but does
_not_ check it. But then RX_LOOP can be set twice, so we'd need an
atomic_t counter instead.


Regards,
Clemens




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] MPU-401 fixes

[Takashi Iwai]

At Mon, 05 May 2003 17:51:37 +0200 (METDST),
Clemens Ladisch wrote:
> 
> Takashi Iwai wrote:
> > Clemens Ladisch wrote:
> > > Jaroslav Kysela wrote:
> > > > On Mon, 5 May 2003, Clemens Ladisch wrote:
> > > >
> > > > > - fix deadlock: when snd_mpu401_uart_input_trigger() is interrupted
> > > > >   immediately after the call to spin_unlock_irqrestore(), the interrupt
> > > > >   handler won't clear the interrupt condition because the RX_LOOP bit is
> > > > >   still set
> > > >
> > > > Thanks. The RX_LOOP bit is cleared before unlock now.
> > >
> > > The same thing will happen when the interrupt occurs immediately before
> > > the call to spin_lock_irqsave().
> > >
> > > The real problem is that the interrupt handler _must_ read the data from
> > > the device to clear the MPU401's interrupt condition. Therefore, there
> > > must not be any condition which can prevent the handler from reading data.
> >
> > hmm, this problem is not easy to solve.
> >
> > originally, the bit check was introduced to prevent double spinlocks
> > in the path:
> >
> > 	irq
> > 	-> snd_mpu401_uart_interrupt()
> > 	-> lock
> > 	-> snd_mpu401_uart_input_read()
> > 		-> snd_rawmidi_receive()
> > 			-> rawmidi->event()
> > 				-> rawmidi->trigger() (in seq_midi.c)
> > 					-> snd_mpu401_uart_input_trigger()
> 
> AFAICS the only protection needed is against a recursive call of
> _input_trigger() but not of _interrupt() because the interrupt routine
> cannot be called inside the spin_lock_irqsave(), i.e., the code being
> interrupted will not be the _input_read() function.
> 
> So a solution would be that _interrupt() sets the RX_LOOP bit, but does
> _not_ check it. But then RX_LOOP can be set twice, so we'd need an
> atomic_t counter instead.

yes, we need to avoid the double entrance of input_read() from
trigger().  but it's not necessary to be a atomic counter if the bit
flag is set/reset inside the spinlock but is only checked in
trigger() function.

the code is on cvs now.  please check it out.


ciao,

Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [PATCH] MPU-401 fixes

[Jaroslav Kysela]

On Mon, 5 May 2003, Clemens Ladisch wrote:

> Takashi Iwai wrote:
> > Clemens Ladisch wrote:
> > > Jaroslav Kysela wrote:
> > > > On Mon, 5 May 2003, Clemens Ladisch wrote:
> > > >
> > > > > - fix deadlock: when snd_mpu401_uart_input_trigger() is interrupted
> > > > >   immediately after the call to spin_unlock_irqrestore(), the interrupt
> > > > >   handler won't clear the interrupt condition because the RX_LOOP bit is
> > > > >   still set
> > > >
> > > > Thanks. The RX_LOOP bit is cleared before unlock now.
> > >
> > > The same thing will happen when the interrupt occurs immediately before
> > > the call to spin_lock_irqsave().
> > >
> > > The real problem is that the interrupt handler _must_ read the data from
> > > the device to clear the MPU401's interrupt condition. Therefore, there
> > > must not be any condition which can prevent the handler from reading data.
> >
> > hmm, this problem is not easy to solve.
> >
> > originally, the bit check was introduced to prevent double spinlocks
> > in the path:
> >
> > 	irq
> > 	-> snd_mpu401_uart_interrupt()
> > 	-> lock
> > 	-> snd_mpu401_uart_input_read()
> > 		-> snd_rawmidi_receive()
> > 			-> rawmidi->event()
> > 				-> rawmidi->trigger() (in seq_midi.c)
> > 					-> snd_mpu401_uart_input_trigger()
> 
> AFAICS the only protection needed is against a recursive call of
> _input_trigger() but not of _interrupt() because the interrupt routine
> cannot be called inside the spin_lock_irqsave(), i.e., the code being
> interrupted will not be the _input_read() function.

It's not true on MP. The interrupt handler can be run on different CPU 
than trigger.

> So a solution would be that _interrupt() sets the RX_LOOP bit, but does
> _not_ check it. But then RX_LOOP can be set twice, so we'd need an
> atomic_t counter instead.

Ok, I'm now using the spin_trylock check in the trigger function and 
RX_LOOP and TX_LOOP protectors are removed now. Hopefully, it finally 
solves our problem.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs



-------------------------------------------------------
Enterprise Linux Forum Conference & Expo, June 4-6, 2003, Santa Clara
The only event dedicated to issues related to Linux enterprise solutions
www.enterpriselinuxforum.com

Re: [PATCH] MPU-401 fixes

[Takashi Iwai]

At Tue, 6 May 2003 17:09:38 +0200 (CEST),
Jaroslav wrote:
> 
> On Mon, 5 May 2003, Clemens Ladisch wrote:
> 
> > Takashi Iwai wrote:
> > > Clemens Ladisch wrote:
> > > > Jaroslav Kysela wrote:
> > > > > On Mon, 5 May 2003, Clemens Ladisch wrote:
> > > > >
> > > > > > - fix deadlock: when snd_mpu401_uart_input_trigger() is interrupted
> > > > > >   immediately after the call to spin_unlock_irqrestore(), the interrupt
> > > > > >   handler won't clear the interrupt condition because the RX_LOOP bit is
> > > > > >   still set
> > > > >
> > > > > Thanks. The RX_LOOP bit is cleared before unlock now.
> > > >
> > > > The same thing will happen when the interrupt occurs immediately before
> > > > the call to spin_lock_irqsave().
> > > >
> > > > The real problem is that the interrupt handler _must_ read the data from
> > > > the device to clear the MPU401's interrupt condition. Therefore, there
> > > > must not be any condition which can prevent the handler from reading data.
> > >
> > > hmm, this problem is not easy to solve.
> > >
> > > originally, the bit check was introduced to prevent double spinlocks
> > > in the path:
> > >
> > > 	irq
> > > 	-> snd_mpu401_uart_interrupt()
> > > 	-> lock
> > > 	-> snd_mpu401_uart_input_read()
> > > 		-> snd_rawmidi_receive()
> > > 			-> rawmidi->event()
> > > 				-> rawmidi->trigger() (in seq_midi.c)
> > > 					-> snd_mpu401_uart_input_trigger()
> > 
> > AFAICS the only protection needed is against a recursive call of
> > _input_trigger() but not of _interrupt() because the interrupt routine
> > cannot be called inside the spin_lock_irqsave(), i.e., the code being
> > interrupted will not be the _input_read() function.
> 
> It's not true on MP. The interrupt handler can be run on different CPU 
> than trigger.
 
yes, but it's protected by spinlock anyway.

> > So a solution would be that _interrupt() sets the RX_LOOP bit, but does
> > _not_ check it. But then RX_LOOP can be set twice, so we'd need an
> > atomic_t counter instead.
> 
> Ok, I'm now using the spin_trylock check in the trigger function and 
> RX_LOOP and TX_LOOP protectors are removed now. Hopefully, it finally 
> solves our problem.

i'm afraid no.  as mentioned before, we need to consider to avoid two
things:

1. (infinite) recursive call of trigger() (via input_read())
2. double spinlocks around input_read() from irq handler and
   the suceeding call of trigger()

introducing spin_trylock() solves the case #2 but not case #1,
since spin_trylock() always returns 1 on UP.
for #1, an extra (formerly RX_LOOP bit) flag is still necessary.


Takashi


-------------------------------------------------------
Enterprise Linux Forum Conference & Expo, June 4-6, 2003, Santa Clara
The only event dedicated to issues related to Linux enterprise solutions
www.enterpriselinuxforum.com

Re: [PATCH] MPU-401 fixes

[Jaroslav Kysela]

On Tue, 6 May 2003, Takashi Iwai wrote:

> > Ok, I'm now using the spin_trylock check in the trigger function and 
> > RX_LOOP and TX_LOOP protectors are removed now. Hopefully, it finally 
> > solves our problem.
> 
> i'm afraid no.  as mentioned before, we need to consider to avoid two
> things:
> 
> 1. (infinite) recursive call of trigger() (via input_read())
> 2. double spinlocks around input_read() from irq handler and
>    the suceeding call of trigger()
> 
> introducing spin_trylock() solves the case #2 but not case #1,
> since spin_trylock() always returns 1 on UP.
> for #1, an extra (formerly RX_LOOP bit) flag is still necessary.

I see. So the protectors are back in trigger() callbacks.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs



-------------------------------------------------------
Enterprise Linux Forum Conference & Expo, June 4-6, 2003, Santa Clara
The only event dedicated to issues related to Linux enterprise solutions
www.enterpriselinuxforum.com

Re: [PATCH] MPU-401 fixes

[Clemens Ladisch]

Jaroslav Kysela wrote:
> On Tue, 6 May 2003, Takashi Iwai wrote:
> > > Ok, I'm now using the spin_trylock check in the trigger function and
> > > RX_LOOP and TX_LOOP protectors are removed now. Hopefully, it finally
> > > solves our problem.
> >
> > i'm afraid no.  as mentioned before, we need to consider to avoid two
> > things:
> >
> > 1. (infinite) recursive call of trigger() (via input_read())
> > 2. double spinlocks around input_read() from irq handler and
> >    the suceeding call of trigger()
> >
> > introducing spin_trylock() solves the case #2 but not case #1,
> > since spin_trylock() always returns 1 on UP.
> > for #1, an extra (formerly RX_LOOP bit) flag is still necessary.
>
> I see. So the protectors are back in trigger() callbacks.

Now the recursive call sequence describe by Takashi is still possible:

>	irq
>	-> snd_mpu401_uart_interrupt()
>	-> lock
>	-> snd_mpu401_uart_input_read()
>		-> snd_rawmidi_receive()
>			-> rawmidi->event()
>				-> rawmidi->trigger() (in seq_midi.c)
>					-> snd_mpu401_uart_input_trigger()

... so the interrupt probably should set the bit, too. OTOH the recursion
isn't infinite anymore, so the current code really doesn't need any more
fixing.


Regards,
Clemens




-------------------------------------------------------
Enterprise Linux Forum Conference & Expo, June 4-6, 2003, Santa Clara
The only event dedicated to issues related to Linux enterprise solutions
www.enterpriselinuxforum.com

Re: [PATCH] MPU-401 fixes

[Takashi Iwai]

At Wed, 07 May 2003 10:31:46 +0200 (METDST),
Clemens Ladisch wrote:
> 
> Jaroslav Kysela wrote:
> > On Tue, 6 May 2003, Takashi Iwai wrote:
> > > > Ok, I'm now using the spin_trylock check in the trigger function and
> > > > RX_LOOP and TX_LOOP protectors are removed now. Hopefully, it finally
> > > > solves our problem.
> > >
> > > i'm afraid no.  as mentioned before, we need to consider to avoid two
> > > things:
> > >
> > > 1. (infinite) recursive call of trigger() (via input_read())
> > > 2. double spinlocks around input_read() from irq handler and
> > >    the suceeding call of trigger()
> > >
> > > introducing spin_trylock() solves the case #2 but not case #1,
> > > since spin_trylock() always returns 1 on UP.
> > > for #1, an extra (formerly RX_LOOP bit) flag is still necessary.
> >
> > I see. So the protectors are back in trigger() callbacks.
> 
> Now the recursive call sequence describe by Takashi is still possible:
> 
> >	irq
> >	-> snd_mpu401_uart_interrupt()
> >	-> lock
> >	-> snd_mpu401_uart_input_read()
> >		-> snd_rawmidi_receive()
> >			-> rawmidi->event()
> >				-> rawmidi->trigger() (in seq_midi.c)
> >					-> snd_mpu401_uart_input_trigger()
> 
> ... so the interrupt probably should set the bit, too. OTOH the recursion
> isn't infinite anymore, so the current code really doesn't need any more
> fixing.

yep, i agree that the flag should be set in the interrupt route, too.

i believe my last change (rev. 1.25) should work fine.
it sets the flag in input_read() (i.e. in spinlock).  instead of
spin_trylock() the bit is checked but it's safe because
- input_read is protected by spinlock between interrupt() and
  input_trigger()
- recursive call and double spinlocks of input_read() is avoided
  by checking the bit


ciao,

Takashi


-------------------------------------------------------
Enterprise Linux Forum Conference & Expo, June 4-6, 2003, Santa Clara
The only event dedicated to issues related to Linux enterprise solutions
www.enterpriselinuxforum.com