Bug in serial-u16550.c

Bug in serial-u16550.c

[Bill Adair]

There is a bug in serial-u16550.c that has been causing my PC to lock up  
solid. The three chunks of code
should explain the nature of it. Basically a lock is taken out on  
interrupt and is still held on calling
snd_rawmidi_receive (). snd_rawmidi_receive () eventually calls back into  
the driver code at snd_uart16550_input_trigger ()
where an attempt is made to take out the same lock again. Other pieces of  
code using the same construction avoid
this problem by unlocking before calling snd_rawmidi_receive () and  
relocking e.g. mtpav.c - though this does
not seem a good solution ;-).

The bug would only affect users who receive data from their MIDI  
interfaces.

So two questions -

Do you want a patch for this? In the Opcode driver I've been writing
based on this I've checked to see if the kernel is in an interrupt using  
the in_interrupt ()
call before locking or unlocking in snd_uart16550_input_trigger ().

Can anyone describe the function of the up parameter passed to  
snd_uart16550_input_trigger ()?
Should I be using this to determine if the call came from an interrupt?

static irqreturn_t snd_uart16550_interrupt(int irq, void *dev_id, struct  
pt_regs *regs)
{
         snd_uart16550_t *uart;

         uart = (snd_uart16550_t *) dev_id;
         spin_lock(&uart->open_lock);
         if (uart->filemode == SERIAL_MODE_NOT_OPENED) {
                 spin_unlock(&uart->open_lock);
                 return IRQ_NONE;
         }
         inb(uart->base + UART_IIR);             /* indicate to the UART  
that the interrupt has been serviced */
         snd_uart16550_io_loop(uart);
         spin_unlock(&uart->open_lock);
         return IRQ_HANDLED;
}

static void snd_uart16550_io_loop(snd_uart16550_t * uart)
{
		// deleted stuff here...
		/* Read Loop */
		while ((status = inb(uart->base + UART_LSR)) & UART_LSR_DR) {
			/* while receive data ready */
			c = inb(uart->base + UART_RX);

			// deleted stuff here...
			snd_rawmidi_receive(uart->midi_input[substream], &c, 1);                 
}

			// deleted stuff here...
		}

		// deleted stuff here...
}

static void snd_uart16550_input_trigger(snd_rawmidi_substream_t *  
substream, int up)
{
         unsigned long flags;
         snd_uart16550_t *uart = substream->rmidi->private_data;

         spin_lock_irqsave(&uart->open_lock, flags);
         if (up) {
                 uart->filemode |= SERIAL_MODE_INPUT_TRIGGERED;
         } else {
                 uart->filemode &= ~SERIAL_MODE_INPUT_TRIGGERED;
         }
         spin_unlock_irqrestore(&uart->open_lock, flags);
}


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_ide95&alloc_id\x14396&op=click

Re: Bug in serial-u16550.c

[Clemens Ladisch]

Bill Adair wrote:
> There is a bug in serial-u16550.c that has been causing my PC to lock up
> solid. The three chunks of code
> should explain the nature of it. Basically a lock is taken out on
> interrupt and is still held on calling
> snd_rawmidi_receive (). snd_rawmidi_receive () eventually calls back into
> the driver code at snd_uart16550_input_trigger ()
> where an attempt is made to take out the same lock again. Other pieces of
> code using the same construction avoid
> this problem by unlocking before calling snd_rawmidi_receive () and
> relocking e.g. mtpav.c - though this does
> not seem a good solution ;-).

This depends on what data the lock is supposed to protect.

> Do you want a patch for this? In the Opcode driver I've been writing
> based on this I've checked to see if the kernel is in an interrupt using
> the in_interrupt ()
> call before locking or unlocking in snd_uart16550_input_trigger ().

Not locking could, in theory, lead to problems on SMP machines.

However, it filemode is the only variable that is protected, it might
be possible to change it to use atomic bit operations.

> Can anyone describe the function of the up parameter passed to
> snd_uart16550_input_trigger ()?

It enables/disables input from the port.

> Should I be using this to determine if the call came from an interrupt?

It has nothing to do with in-interruptness.


HTH
Clemens



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Re: Bug in serial-u16550.c

[Bill Adair]

On Thu, 24 Feb 2005 13:48:24 +0100 (MET), Clemens Ladisch  
<clemens@ladisch.de> wrote:

> Bill Adair wrote:
>> There is a bug in serial-u16550.c that has been causing my PC to lock up
>> solid. The three chunks of code
>> should explain the nature of it. Basically a lock is taken out on
>> interrupt and is still held on calling
>> snd_rawmidi_receive (). snd_rawmidi_receive () eventually calls back  
>> into
>> the driver code at snd_uart16550_input_trigger ()
>> where an attempt is made to take out the same lock again. Other pieces  
>> of
>> code using the same construction avoid
>> this problem by unlocking before calling snd_rawmidi_receive () and
>> relocking e.g. mtpav.c - though this does
>> not seem a good solution ;-).
>
> This depends on what data the lock is supposed to protect.
>

The different flags used in filemode are below. There are all seperate  
bits of a bitfield i.e.
setting SERIAL_MODE_INPUT_TRIGGERED cannot effect SERIAL_MODE_INPUT_OPEN.  
The only flag
ever tested in the driver is SERIAL_MODE_INPUT_OPEN, the others are just  
set.

So as it happens the code does nothing at all except - as a by blow -  
check that a lock
can be taken out on the uart structure (which is can't because it's  
already locked).

>> Do you want a patch for this? In the Opcode driver I've been writing
>> based on this I've checked to see if the kernel is in an interrupt using
>> the in_interrupt ()
>> call before locking or unlocking in snd_uart16550_input_trigger ().
>
> Not locking could, in theory, lead to problems on SMP machines.
>

Locking against an already held lock is problematical on my machine ;-).
My machine is SMP and from my reading of the kernel programming guides  
when an interrupt is
received it is _guaranteed_ to be serviced on the processor it was picked  
up by. Presumably
this would include the alsa code executed until the interrupt is marked as  
serviced.

> However, it filemode is the only variable that is protected, it might
> be possible to change it to use atomic bit operations.
>

Have you a sample of code for this?

>> Can anyone describe the function of the up parameter passed to
>> snd_uart16550_input_trigger ()?
>
> It enables/disables input from the port.
>

This is interesting - can the sequencer code in alsa tell the driver not  
to accept further
input? There is no support in this driver for this if that is the case.

>> Should I be using this to determine if the call came from an interrupt?
>
> It has nothing to do with in-interruptness.
>
>
> HTH
> Clemens
>

#define SERIAL_MODE_NOT_OPENED          (0)
#define SERIAL_MODE_INPUT_OPEN          (1 << 0)
#define SERIAL_MODE_OUTPUT_OPEN         (1 << 1)
#define SERIAL_MODE_INPUT_TRIGGERED     (1 << 2)
#define SERIAL_MODE_OUTPUT_TRIGGERED    (1 << 3)

static void snd_uart16550_input_trigger(snd_rawmidi_substream_t  
*substream, int up)
{
         unsigned long flags = 0;
         snd_uart16550_t *uart = substream->rmidi->private_data;

         if (!in_interrupt ()) {
                 spin_lock_irqsave (&uart->open_lock, flags);
         }
         if (up) {
                 uart->filemode |= SERIAL_MODE_INPUT_TRIGGERED;
         }
         else {
                 uart->filemode &= ~SERIAL_MODE_INPUT_TRIGGERED;
         }
         if (!in_interrupt ()) {
                 spin_unlock_irqrestore (&uart->open_lock, flags);
         }
}


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_ide95&alloc_id\x14396&op=click

Re: Bug in serial-u16550.c

[Clemens Ladisch]

Bill Adair wrote:
> The only flag ever tested in the driver is SERIAL_MODE_INPUT_OPEN,
> the others are just set.

The other flags are checked when the driver compares against
SERIAL_MODE_NOT_OPEN, i.e., it checks that none of these flags is set.

> > However, it filemode is the only variable that is protected, it might
> > be possible to change it to use atomic bit operations.
>
> Have you a sample of code for this?

msnd_midi.c and mpu401_uart.c do something like this.

> >> Can anyone describe the function of the up parameter passed to
> >> snd_uart16550_input_trigger ()?
> >
> > It enables/disables input from the port.
>
> This is interesting - can the sequencer code in alsa tell the
> driver not to accept further input?

Yes.  (Well, no -- it's the rawmidi code, but that could be called by
the sequencer code. :-)

> There is no support in this driver for this if that is the case.

snd_uart16550_input_trigger clears the SERIAL_MODE_INPUT_TRIGGERED
bit, which is checked ...

Oops.  Which should be checked by the io_loop.


Oh, just BTW:  I fixed the deadlock by moving the event call into a
tasklet.  The change should be on the public CVS server shortly.


Regards,
Clemens



-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click