Re: [PATCH] MPU-401 fixes

[Takashi Iwai <tiwai@suse.de>]

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