Re: Bug in serial-u16550.c

["Bill Adair" <adair@gotadsl.co.uk>]

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