Re: [PATCH] serial-u16550 driver. Fixes buffer blocking bug

[Takashi Iwai <tiwai@suse.de>]

Hi,

thanks for the patch!

At Wed, 29 Oct 2003 13:47:49 -0800,
Steve deRosier wrote:
> 
> I am submitting a patch to the serial-u16550 driver fix a bug 
> that we have encountered.  Basically, when using the 
> SNDRV_SERIAL_GENERIC adaptor in the serial-u16550 MIDI driver,
> if the device becomes unplugged or some other condition where
> the device stops signaling CTS, the send buffer backs up.
> This is fine and expected opperation, except on a prolonged
> problem (with our music, about 30-40 minutes typically) the
> buffer itself gets full.  It is supposed to then just throw 
> away data, but it seems that it doesn't operate quite right
> and it ends up backing up data in rawmidi.  When this 
> happens, the driver basically locks up (though "send direct"
> data still gets out), and only on a close and reopen of the
> device will recover.  
> 
> This patch fixes the problem, by modifiying how it checks
> for and handles its internal buffer:
> 
> 1. Move all checks of buffer overflow and such to the 
> actual buffer write and read routines.  This makes
> the buffer routines more robust and encaspulates buffer
> opperations better.

the check in the caller may be still needed for the multi-bytes write
(e.g. for switching the port).  otherwise, the port-switch command
can be mixed and messed up when the device is accessed concurrently.


> 2. Always try to send data to the buffers.  Don't interupt
> normal opperation of the algoritim because buffers are full.
> This was what was actually causing the data to be left in 
> rawmidi and thus causing it to lock up.  This is helped
> by moving the buffer size checks into the routines.

this is the questionary behavior.
the fact that the local buffer is full means that the transfer doesn't
work.  so, it is quite correct behavior that rawmidi blocks the
further write (in blocking mode), i think.  and, it should return
-EAGAIN in non-blocking mode.
or, at least, we can make the behavior selectable: abandon or block.

the problem is that there is no way to reset the device except for
reopening the device.  although there is a drop ioctl for rawmidi, it
doesn't reset the device as expected but only flushes the rawmidi
buffer.


> 3. When able to write, drain output as necessary instead of
> only writing one byte at a time.  This was causing us to 
> backup eventually anyway since we usually write more than 
> one byte of data.

looking at the code, only MS124W_SA and GENERIC types don't do loop.
so, it might be intentional, although i don't see any reason against
the looping.


btw, please use the unified diff (-u) as the patch at the next time.
it's much more readable.


ciao,

Takashi


-------------------------------------------------------
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive?  Does it
help you create better code?   SHARE THE LOVE, and help us help
YOU!  Click Here: http://sourceforge.net/donate/