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

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

[Steve deRosier]

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.
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.
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.

This fixes a bug that we were encountering in the driver,
and I'm pretty sure it shouldn't cause any problems 
with the other devices that use this driver.  If anyone has 
the other devices to test on, please test away.

Thanks,

- Steve


------

Change log entry:  Fixes problem where buffers fill up with SNDRV_SERIAL_GENERIC adaptor
                   when device doesn't signal CTS.

Index: serial-u16550.c
===================================================================
RCS file: /home/cvsroot/alsa/alsa-kernel/drivers/serial-u16550.c,v
retrieving revision 1.22
diff -c -3 -p -r1.22 serial-u16550.c
*** serial-u16550.c     2003/10/14 13:08:13     1.22
--- serial-u16550.c     2003/10/29 21:19:08
*************** inline static void snd_uart16550_del_tim
*** 194,205 ****
  inline static void snd_uart16550_buffer_output(snd_uart16550_t *uart)
  {
        unsigned short buff_out = uart->buff_out;
!       outb(uart->tx_buff[buff_out], uart->base + UART_TX);
!       uart->fifo_count++;
!       buff_out++;
!       buff_out &= TX_BUFF_MASK;
!       uart->buff_out = buff_out;
!       uart->buff_in_count--;
  }

  /* This loop should be called with interrupts disabled
--- 194,208 ----
  inline static void snd_uart16550_buffer_output(snd_uart16550_t *uart)
  {
        unsigned short buff_out = uart->buff_out;
!       if( uart->buff_in_count > 0 )
!       {
!               outb(uart->tx_buff[buff_out], uart->base + UART_TX);
!               uart->fifo_count++;
!               buff_out++;
!               buff_out &= TX_BUFF_MASK;
!               uart->buff_out = buff_out;
!               uart->buff_in_count--;
!       }
  }

  /* This loop should be called with interrupts disabled
*************** static void snd_uart16550_io_loop(snd_ua
*** 257,265 ****
           || uart->adaptor == SNDRV_SERIAL_GENERIC) {
                /* Can't use FIFO, must send only when CTS is true */
                status = inb(uart->base + UART_MSR);
!               if (uart->fifo_count == 0 && (status & UART_MSR_CTS)
!                   && uart->buff_in_count > 0)
!                       snd_uart16550_buffer_output(uart);
        } else {
                /* Write loop */
                while (uart->fifo_count < uart->fifo_limit      /* Can we write ? */
--- 260,271 ----
           || uart->adaptor == SNDRV_SERIAL_GENERIC) {
                /* Can't use FIFO, must send only when CTS is true */
                status = inb(uart->base + UART_MSR);
!               while( (uart->fifo_count == 0) && (status & UART_MSR_CTS) &&
!                        (uart->buff_in_count > 0) )
!               {
!                       snd_uart16550_buffer_output(uart);
!                       status = inb( uart->base + UART_MSR );
!               }
        } else {
                /* Write loop */
                while (uart->fifo_count < uart->fifo_limit      /* Can we write ? */
*************** static int snd_uart16550_output_close(sn
*** 579,591 ****
  inline static void snd_uart16550_write_buffer(snd_uart16550_t *uart, unsigned char byte)
  {
        unsigned short buff_in = uart->buff_in;
!       uart->tx_buff[buff_in] = byte;
!       buff_in++;
!       buff_in &= TX_BUFF_MASK;
!       uart->buff_in = buff_in;
!       uart->buff_in_count++;
!       if (uart->irq < 0) /* polling mode */
!               snd_uart16550_add_timer(uart);
  }

  static void snd_uart16550_output_byte(snd_uart16550_t *uart, snd_rawmidi_substream_t * substream, unsigned char midi_byte)
--- 585,600 ----
  inline static void snd_uart16550_write_buffer(snd_uart16550_t *uart, unsigned char byte)
  {
        unsigned short buff_in = uart->buff_in;
!       if( uart->buff_in_count < TX_BUFF_SIZE )
!       {
!               uart->tx_buff[buff_in] = byte;
!               buff_in++;
!               buff_in &= TX_BUFF_MASK;
!               uart->buff_in = buff_in;
!               uart->buff_in_count++;
!               if (uart->irq < 0) /* polling mode */
!                       snd_uart16550_add_timer(uart);
!       }
  }

  static void snd_uart16550_output_byte(snd_uart16550_t *uart, snd_rawmidi_substream_t * substream, unsigned char midi_byte)
*************** static void snd_uart16550_output_byte(sn
*** 614,620 ****
                if (uart->buff_in_count >= TX_BUFF_SIZE) {
                        snd_printk("%s: Buffer overrun on device at 0x%lx\n",
                                   uart->rmidi->name, uart->base);
-                       return;
                }
                snd_uart16550_write_buffer(uart, midi_byte);
        }
--- 623,628 ----
*************** static void snd_uart16550_output_write(s
*** 669,678 ****
                                uart->adaptor == SNDRV_SERIAL_GENERIC) &&
                           (uart->prev_out != substream->number || jiffies-lasttime > 3*HZ)) {

-                               /* We will need three bytes of data here (worst case). */
-                               if (uart->buff_in_count >= TX_BUFF_SIZE - 3)
-                                       break;
-
                                /* Roland Soundcanvas part selection */
                                /* If this substream of the data is different previous
                                   substream in this uart, send the change part event */
--- 677,682 ----
*************** static void snd_uart16550_output_write(s
*** 685,694 ****
                                if ((midi_byte < 0x80) && (uart->adaptor == SNDRV_SERIAL_SOUNDCANVAS))
                                        snd_uart16550_output_byte(uart, substream, uart->prev_status[uart->prev_out]);
                        }
-
-                       /* buffer full? */
-                       if (uart->buff_in_count >= TX_BUFF_SIZE)
-                               break;

                        /* send midi byte */
                        snd_uart16550_output_byte(uart, substream, midi_byte);
--- 689,694 ----



-------------------------------------------------------
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/

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

[Takashi Iwai]

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/

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

[Steve deRosier]

Takashi,

Thanks for your response.  I've addressed your issues below.  Let's 
discuss this and if necessary I'll modify my fix.

>>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.
> 

I thought about this, but decided it's not a large issue since it is 
mitigated by the natural operation of the driver.
For normal opperation:  As this is called with interupts off, the 
process can't be interupted.  The function doesn't return until after 
we've drained the snd_rawmidi_transmit(), and if we do decide to insert 
a "f5..." msg, we do the whole msg before we can return.  So, no issue 
durring normal opperation.  So concurrent access doesn't apear to be a 
large issue.

If buffer gets full:  Yes, it is possible for an "f5..." msg to get 
interupted.  BUT, I don't see this as a large problem.  On return to 
normal opperation (ie. the buffer begins draining again), the proper 
port-switch command will be written in correctly when the port gets 
switched, or in the next three seconds, whichever comes first.  Perhaps 
some data gets sent to the wrong port, but it autocorrects very quickly. 
While this condition technically is possible, I haven't seen it happen 
in practice, and even if it does it would correct itself quickly. For 
another mittigating factor, please see my answer to #2 below.

If you're still concerned about this, I have an alternate fix that would 
keep a check, but not interfeare with opperation.

> 
>>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.
> 

Well, the driver already was trying to opperate in the mode where it was 
abandoning bytes, it was just doing a bad job of it.  All I did was fix 
it so it actually abandoned all bytes that didn't fit in the buffers.

Before it was grabbing a byte from snd_rawmidi_transmit() and if no room 
was in the buffer it would break out of the while(1) loop.  So, 
basically it would abandon the first byte on its execution, but leave 
some data there for later.  This caused a problem with rawmidi's buffers 
seeming to back up and eventually hanging up the driver (recoverable 
only via reopening the device).

The program (call it pdr) we have using this device operates in blocking 
mode.  So, if things worked properly, pdr would attempt to write the 
midi msg, and the snd_seq_event_output() just wouldn't return until it 
was able to send the data.  That would be fine.  BUT, if we physically 
disconnect the serial cable from the device, after awhile, the buffer in 
serial-u16550.c fills up, then one byte is retrieved from 
snd_rawmidi_transmit() (and then discarded by the driver) for every 
three bytes put into the rawmidi buffers, these buffers fill up and 
eventually cause the lockup of the driver, all while never reporting a 
problem back to pdr (snd_seq_event_output() seems not to block even 
though we're in blocking mode).  As far as pdr seems to know, it's able 
to always send data, but eventually the data stops getting to the 
physical device (as a side note, midi data that avoids the rawmidi 
buffers via snd_seq_event_output_direct() will get written to the device 
properly even in this case).

So, now the driver reliably drops all data given to it if the buffers 
are full, not just some of the data and rawmidi buffers never get backed 
up, the driver doesn't lockup and everything works properly when the 
device gets pluged back in.

Personally, I feel it IS proper behavior.  Simple fact is, if the 
condition happens where this buffer gets filled in, the midi events have 
been "released" by alsa to go out to the device immedately, but have now 
been delayed by up to 30-40 minutes.  They're past their prime and are 
really no longer relevant.  If we discard data, well, tough.  At this 
point it becomes about error recovery, not valid data.  This code 
recovers proper opperation, where the old code didn't.  This method 
works best for our application.

However, if we want different behavior, let me know how to do this.  I 
was able to fix the problem here, without digging further into the 
rawmidi code or even further up the chain.  Frankly I'm not educated 
enough on that code to safely mess with it.  To handle a "blocking" 
mode, the driver would need some way to properly communicate it is full 
to rawmidi.  This could be either some method to do this specfically, or 
just that rawmidi's buffer doesn't get emptied.  But, at this time, that 
seems to cause a problem.

Granted, my opinion that this is the proper operation is just my 
perspective--it does what we want for our devices.  Is it generally 
applicable to everyone that uses this driver?  Maybe or maybe not.  I'll 
trust that you know more than me about the general purposes of this 
stuff and how it fits in the overall alsa archetecture.

> 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.
> 

Earlier while I was working to solve this problem with the driver, I 
hacked in a solultion that fixed it, but it was a horrible hack. 
Basically, in snd_uart16550_write_buffer() I made it so if the buffer 
was full and it attempted to write a byte into it, the driver would 
reset the buffer.  Basically:

inline static void snd_uart16550_write_buffer(snd_uart16550_t *uart, 
unsigned char byte)
{
	unsigned short buff_in = uart->buff_in;
	if( uart->buff_in_count < TX_BUFF_SIZE )
	{
	        uart->tx_buff[buff_in] = byte;
	        buff_in++;
	        buff_in &= TX_BUFF_MASK;
	        uart->buff_in = buff_in;
	        uart->buff_in_count++;
	        if (uart->irq < 0) /* polling mode */
	                snd_uart16550_add_timer(uart);
	}
         else
         {
           uart->buff_in = 0;
           uart->buf_in_count = 0;
           uart->buff_out = 0;
         }
}

As I said, this was an ugly HACK.  But, it fixed the immedate problem we 
were having by resetting buffers, and gave me one of my first hints as 
to the real problems.  I went on from there and eventually came up with 
the patch I sent in.  (I've been working on this specific problem for 
roughly 4-6 weeks now, so I've gone through a lot of detail on what 
works or doesn't and why.  And I'm very familar with the snd_uart16550.c 
code now.)

Maybe if there was some hook so that the snd_uart16550 driver itself 
could get reset?

>>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.
> 

I talked w/ the person who said he added that if(...) in there in the 
first place and I was told it wasn't intentional.  So I think that the 
loop should be fine and it makes it more consistant with the second 
clause.  BTW, we're using the GENERIC type.  Not having the loop 
actually caused a secondary problem.

> 
> btw, please use the unified diff (-u) as the patch at the next time.
> it's much more readable.
> 
Sure, not a problem.  BTW, there doesn't seem to be anywhere on the alsa 
web site (and no mention in alsa docs or readme) of what patch format to 
use.  I researched it for quite awhile and then gave up and just used 
the form that GNU's GCC project says to use 
(http://www.gnu.org/software/gcc/contribute.html#patches).  I'm a past 
contributer of GCC and Binutils, so I just fell back on what I'm used 
to.  What is the Alsa project's offical format?  -u? -up?

I know that this is alot of info, but I've been working on this problem 
for quite awhile now and frankly it is simply a lot of info.  I'm hoping 
that it will allow us to either banter this back and forth enough to 
come to an agrement and then another acceptable fix that works for both 
of us, or reassure you that my patch is good enough to commit.

Thanks,
- Steve



-------------------------------------------------------
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/

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

[Takashi Iwai]

At Thu, 30 Oct 2003 13:33:37 -0800,
Steve deRosier wrote:
> 
> Takashi,
> 
> Thanks for your response.  I've addressed your issues below.  Let's 
> discuss this and if necessary I'll modify my fix.
> 
> >>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.
> > 
> 
> I thought about this, but decided it's not a large issue since it is 
> mitigated by the natural operation of the driver.
> For normal opperation:  As this is called with interupts off, the 
> process can't be interupted.  The function doesn't return until after 
> we've drained the snd_rawmidi_transmit(), and if we do decide to insert 
> a "f5..." msg, we do the whole msg before we can return.  So, no issue 
> durring normal opperation.  So concurrent access doesn't apear to be a 
> large issue.
 
yes, normally it's ok.  the problem would be only the full-buffer
state.

> If buffer gets full:  Yes, it is possible for an "f5..." msg to get 
> interupted.  BUT, I don't see this as a large problem.  On return to 
> normal opperation (ie. the buffer begins draining again), the proper 
> port-switch command will be written in correctly when the port gets 
> switched, or in the next three seconds, whichever comes first.  Perhaps 
> some data gets sent to the wrong port, but it autocorrects very quickly. 
> While this condition technically is possible, I haven't seen it happen 
> in practice, and even if it does it would correct itself quickly. For 
> another mittigating factor, please see my answer to #2 below.
> 
> If you're still concerned about this, I have an alternate fix that would 
> keep a check, but not interfeare with opperation.
> 
> > 
> >>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.
> > 
> 
> Well, the driver already was trying to opperate in the mode where it was 
> abandoning bytes, it was just doing a bad job of it.  All I did was fix 
> it so it actually abandoned all bytes that didn't fit in the buffers.
> 
> Before it was grabbing a byte from snd_rawmidi_transmit() and if no room 
> was in the buffer it would break out of the while(1) loop.  So, 
> basically it would abandon the first byte on its execution, but leave 
> some data there for later.  This caused a problem with rawmidi's buffers 
> seeming to back up and eventually hanging up the driver (recoverable 
> only via reopening the device).

basically, the data should NOT be abandoned as long as it can be
held.  the current code is not good from this perspective.
it should be something like:

	while (snd_rawmidi_transmit_peek(substream, &byte, 1) == 1) {
		if (! writable())
			break;
		write_byte(byte);
		snd_rawmidi_transmit_ack(substream, 1);
	}

> The program (call it pdr) we have using this device operates in blocking 
> mode.  So, if things worked properly, pdr would attempt to write the 
> midi msg, and the snd_seq_event_output() just wouldn't return until it 
> was able to send the data.  That would be fine.  BUT, if we physically 
> disconnect the serial cable from the device, after awhile, the buffer in 
> serial-u16550.c fills up, then one byte is retrieved from 
> snd_rawmidi_transmit() (and then discarded by the driver) for every 
> three bytes put into the rawmidi buffers, these buffers fill up and 
> eventually cause the lockup of the driver, all while never reporting a 
> problem back to pdr (snd_seq_event_output() seems not to block even 
> though we're in blocking mode).

in this case, the operation is blocked because it's really blocked.
sure, it doesn't report errors because it's just the blocking
behavior.  the driver doesn't detect the disconnection, and it simply
waits until the next transfer is available.

(snip)
> However, if we want different behavior, let me know how to do this.  I 
> was able to fix the problem here, without digging further into the 
> rawmidi code or even further up the chain.  Frankly I'm not educated 
> enough on that code to safely mess with it.  To handle a "blocking" 
> mode, the driver would need some way to properly communicate it is full 
> to rawmidi.  This could be either some method to do this specfically, or 
> just that rawmidi's buffer doesn't get emptied.  But, at this time, that 
> seems to cause a problem.

my opinion is that it's bad behavior to abandon the data IMMEDIATELY
when the local buffer is full.  this situation can happen quite easily
when you send a large bulk data.  the uart's local buffer will be full
soon, then the successding data will be simply lost.

it's true that in some special cases, we need to reset the device.  
but, as far as i understand, the buffer-full is not the criteria of
the reset, since it may happen even with the normal operation.

maybe it'd be better to add a timeout for the sanity check.  firstly
after the time out is detected, we can do some reset work on the
device, e.g. flush the buffer, etc.
(the communication over the ALSA sequencer is a different story,
though...)

well, until this is implemented, we can add the switch (e.g. a module
parameter) for your behavior (ignore-buffer-overflow), so that the
driver works at least for your purpose...


> >>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.
> > 
> 
> I talked w/ the person who said he added that if(...) in there in the 
> first place and I was told it wasn't intentional.  So I think that the 
> loop should be fine and it makes it more consistant with the second 
> clause.  BTW, we're using the GENERIC type.  Not having the loop 
> actually caused a secondary problem.

ok, thanks for the confirmation.

> > 
> > btw, please use the unified diff (-u) as the patch at the next time.
> > it's much more readable.
> > 
> Sure, not a problem.  BTW, there doesn't seem to be anywhere on the alsa 
> web site (and no mention in alsa docs or readme) of what patch format to 
> use.  I researched it for quite awhile and then gave up and just used 
> the form that GNU's GCC project says to use 
> (http://www.gnu.org/software/gcc/contribute.html#patches).  I'm a past 
> contributer of GCC and Binutils, so I just fell back on what I'm used 
> to.  What is the Alsa project's offical format?  -u? -up?

we follow the linux-kernel style.  in general, -u would suffice, but
of course -up would be nicer in many cases.


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/

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

[Steve deRosier]

Takashi Iwai wrote:
> At Thu, 30 Oct 2003 13:33:37 -0800,
> Steve deRosier wrote:
> 
>>Takashi,
>>
>>Thanks for your response.  I've addressed your issues below.  Let's 
>>discuss this and if necessary I'll modify my fix.
>>
>>
>>>>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.
>>>
>>
>>I thought about this, but decided it's not a large issue since it is 
>>mitigated by the natural operation of the driver.
>>For normal opperation:  As this is called with interupts off, the 
>>process can't be interupted.  The function doesn't return until after 
>>we've drained the snd_rawmidi_transmit(), and if we do decide to insert 
>>a "f5..." msg, we do the whole msg before we can return.  So, no issue 
>>durring normal opperation.  So concurrent access doesn't apear to be a 
>>large issue.
> 
>  
> yes, normally it's ok.  the problem would be only the full-buffer
> state.
> 

Ok, as you don't seem swayed by my argument, I'll go ahead and put a 
check in that will address the issue, but not break other things.

>>>>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.
>>>
>>
>>Well, the driver already was trying to opperate in the mode where it was 
>>abandoning bytes, it was just doing a bad job of it.  All I did was fix 
>>it so it actually abandoned all bytes that didn't fit in the buffers.
>>
>>Before it was grabbing a byte from snd_rawmidi_transmit() and if no room 
>>was in the buffer it would break out of the while(1) loop.  So, 
>>basically it would abandon the first byte on its execution, but leave 
>>some data there for later.  This caused a problem with rawmidi's buffers 
>>seeming to back up and eventually hanging up the driver (recoverable 
>>only via reopening the device).
> 
> 
> basically, the data should NOT be abandoned as long as it can be
> held.  the current code is not good from this perspective.
> it should be something like:
> 
> 	while (snd_rawmidi_transmit_peek(substream, &byte, 1) == 1) {
> 		if (! writable())
> 			break;
> 		write_byte(byte);
> 		snd_rawmidi_transmit_ack(substream, 1);
> 	}
> 

So, should the driver not be using the snd_rawmidi_transmit() function, 
and instead doing its own thing.  Perhaps this would be more proper and 
would make it work better.  It'll require a larger re-work of the 
relevant driver function, but I think it'll be worth it.

> 
>>The program (call it pdr) we have using this device operates in blocking 
>>mode.  So, if things worked properly, pdr would attempt to write the 
>>midi msg, and the snd_seq_event_output() just wouldn't return until it 
>>was able to send the data.  That would be fine.  BUT, if we physically 
>>disconnect the serial cable from the device, after awhile, the buffer in 
>>serial-u16550.c fills up, then one byte is retrieved from 
>>snd_rawmidi_transmit() (and then discarded by the driver) for every 
>>three bytes put into the rawmidi buffers, these buffers fill up and 
>>eventually cause the lockup of the driver, all while never reporting a 
>>problem back to pdr (snd_seq_event_output() seems not to block even 
>>though we're in blocking mode).
> 
> 
> in this case, the operation is blocked because it's really blocked.
> sure, it doesn't report errors because it's just the blocking
> behavior.  the driver doesn't detect the disconnection, and it simply
> waits until the next transfer is available.

I don't think you're understanding what I'm telling you.  pdr doesn't 
get blocked even though we don't set SND_SEQ_NONBLOCK mode on.  It calls 
the snd_seq_event_output() and that ALWAYS returns.  In the condition 
that the buffer fills up, and then the system is reconnected and drains 
the buffer, after that any more writes that pdr sends out that go 
through rawmidi's buffers never happen.  All send direct output will 
still work however.

It really doesn't matter to our app if it gets blocked or is not blocked 
or if bytes get discarded during a physical cable disconnection, the way 
it is designed it continues to grind on anyway and as such, when 
reconnected and the buffer drains eventually all would be right with the 
world again.  BUT, something in Alsa breaks down in this instance and 
doesn't just clear up.  It drains the buffers, but after that no more 
data that gets routed through the buffers ever ends up in the driver. 
Send direct events do get through to the driver and sent out over the wire.

So, right now, having the driver not discard bytes when it's buffer is 
full will cause rawmidi's buffers to fill up.  At that point the 
subsystem is locked up forever, at least until it is reset.  Even the 
reconnection and draining of the buffers keeps it locked up.

Basically, what I've done is as much a work-around as anything else.  I 
make it so that data never can back up in rawmidi's buffers.  Hence, 
when we plug the system back in, after the serial driver buffers get 
cleared out, new data can get through to the device.

>>However, if we want different behavior, let me know how to do this.  I 
>>was able to fix the problem here, without digging further into the 
>>rawmidi code or even further up the chain.  Frankly I'm not educated 
>>enough on that code to safely mess with it.  To handle a "blocking" 
>>mode, the driver would need some way to properly communicate it is full 
>>to rawmidi.  This could be either some method to do this specfically, or 
>>just that rawmidi's buffer doesn't get emptied.  But, at this time, that 
>>seems to cause a problem.
> 
> 
> my opinion is that it's bad behavior to abandon the data IMMEDIATELY
> when the local buffer is full.  this situation can happen quite easily
> when you send a large bulk data.  the uart's local buffer will be full
> soon, then the successding data will be simply lost.

This is a good point.  Though as the buffer is about 32k, we don't ever 
see this problem, I do acknowledge  that it is a potential problem. 
Didn't think about it since we don't see this issue.

> 
> it's true that in some special cases, we need to reset the device.  
> but, as far as i understand, the buffer-full is not the criteria of
> the reset, since it may happen even with the normal operation.
> 

I agree, hence why I called it a _hack_ and looked for a better solution.

> maybe it'd be better to add a timeout for the sanity check.  firstly
> after the time out is detected, we can do some reset work on the
> device, e.g. flush the buffer, etc.
> (the communication over the ALSA sequencer is a different story,
> though...)
> 

eep!  I'm not sure I want to get into figuring out a timeout thingy... 
Maybe better left to the application level, where the app can decide 
that after getting some count of -EAGAIN responses in non-blocking mode 
it can give up or send some sort of flush/reset command to the alsa system.

> well, until this is implemented, we can add the switch (e.g. a module
> parameter) for your behavior (ignore-buffer-overflow), so that the
> driver works at least for your purpose...
> 

Ok, but I'd rather find the root of the problem and come up with a clean 
solution than depend on a special switch.  I'll work on it a little and 
propose a slightly different patch.  Maybe I'll rework 
snd_uart16550_output_write() a bit more drastically so that it works 
better.  Though I'm not sure what I can do to it that doesn't drop bytes 
yet doesn't seem to lockup the rawmidi stuff by not removing data from 
rawmidi buffers.  If I can't find another way, I'll put in the parameter 
as a work around until we can find a better fix.

I'll work on this for a bit this week and propose a second patch.  If 
you can come up with any ideas of things to look at or other issues, let 
me know.

Thanks,
- Steve



-------------------------------------------------------
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/

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

[Takashi Iwai]

At Mon, 03 Nov 2003 17:44:21 -0800,
Steve deRosier wrote:
> 
> It really doesn't matter to our app if it gets blocked or is not blocked 
> or if bytes get discarded during a physical cable disconnection, the way 
> it is designed it continues to grind on anyway and as such, when 
> reconnected and the buffer drains eventually all would be right with the 
> world again.  BUT, something in Alsa breaks down in this instance and 
> doesn't just clear up.  It drains the buffers, but after that no more 
> data that gets routed through the buffers ever ends up in the driver. 

that's true.  the drop call doesn't reset the local buffer at all.
we should add a callback to reset the local device at the drop, if
any.

(snip)
> > maybe it'd be better to add a timeout for the sanity check.  firstly
> > after the time out is detected, we can do some reset work on the
> > device, e.g. flush the buffer, etc.
> > (the communication over the ALSA sequencer is a different story,
> > though...)
> > 
> 
> eep!  I'm not sure I want to get into figuring out a timeout thingy... 

well, it's not so terrifying stuff :)

> Maybe better left to the application level, where the app can decide 
> that after getting some count of -EAGAIN responses in non-blocking mode 
> it can give up or send some sort of flush/reset command to the alsa system.
 
that's also an alternative solution.
still the proper method to reset fails currently.  as you wrote, only
reopening the device is the proper way.  that should be fixed,
too (as written above).

> > well, until this is implemented, we can add the switch (e.g. a module
> > parameter) for your behavior (ignore-buffer-overflow), so that the
> > driver works at least for your purpose...
> > 
> 
> Ok, but I'd rather find the root of the problem and come up with a clean 
> solution than depend on a special switch.  I'll work on it a little and 
> propose a slightly different patch.  Maybe I'll rework 
> snd_uart16550_output_write() a bit more drastically so that it works 
> better.  Though I'm not sure what I can do to it that doesn't drop bytes 
> yet doesn't seem to lockup the rawmidi stuff by not removing data from 
> rawmidi buffers.  If I can't find another way, I'll put in the parameter 
> as a work around until we can find a better fix.
> 
> I'll work on this for a bit this week and propose a second patch.  If 
> you can come up with any ideas of things to look at or other issues, let 
> me know.

thanks.   please note that we're reaching to the feature/code freeze
for 1.0 right now (it's planned in this month).  but, of course, the
patch can be put soon after 1.0 is released.


ciao,

Takashi


-------------------------------------------------------
This SF.Net email sponsored by: ApacheCon 2003,
16-19 November in Las Vegas. Learn firsthand the latest
developments in Apache, PHP, Perl, XML, Java, MySQL,
WebDAV, and more! http://www.apachecon.com/

[PATCH] serial-u16550 driver. Fixes buffer blocking bug (2nd try)

[Steve deRosier]

Takashi,

I've completed my changes on the serial-u16550 driver.  I have 
implemented most of your suggestions to my previous patch.  Now:

* There is a user selectable flag "droponfull".  Set to 1 and
  any new bytes delivered to the driver after the buffer fills
  up will be discarded until the buffer is able to flush some
  bytes.
* If droponfull==0 (or is not set, the default is 0), the driver
  proceeds to block further input by not calling 
  snd_rawmidi_transmit_ack() and aborting the attempt.  It will
  try again later.
* I've redone a bit of the interface for the buffer routines.  
  This was done to support the proper blocking/non-blocking methods
  as spelled out above, and to try to protect the buffer data a bit.  

So, if droponfull==0, we operate as originally desired (but the 
original implementation was broken a bit and this fixes it).  If
droponfull==1 we now discard new bytes if our buffer is full; this
behavior fixes the driver "hang" problem we were having.

The changes should have no material effect on the "SNDRV_SERIAL_MS124W_MB"
device, and all others should still work well, though we can only
test with the SNDRV_SERIAL_GENERIC. 

Please commit this at your earliest opportunity.

Thanks,
- Steve

----------------
Change log entry:  Fixes problem where buffers fill up with SNDRV_SERIAL_GENERIC adaptor
                   when device doesn't signal CTS by dropping new bytes if the module
                   parameter "droponfull" == 1.


Index: serial-u16550.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/drivers/serial-u16550.c,v
retrieving revision 1.22
diff -u -p -r1.22 serial-u16550.c
--- serial-u16550.c     14 Oct 2003 13:08:13 -0000      1.22
+++ serial-u16550.c     3 Dec 2003 01:14:42 -0000
@@ -63,6 +63,9 @@ static char *adaptor_names[] = {
        "Generic"
 };

+#define SNDRV_SERIAL_NORMALBUFF 0 /* Normal blocking buffer operation */
+#define SNDRV_SERIAL_DROPBUFF   1 /* Non-blocking discard operation */
+
 static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;     /* Index 0-MAX */
 static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;      /* ID for this card */
 static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE; /* Enable this card */
@@ -73,6 +76,7 @@ static int base[SNDRV_CARDS] = {[0 ... (
 static int outs[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = 1};         /* 1 to 16 */
 static int ins[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = 1}; /* 1 to 16 */
 static int adaptor[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS - 1)] = SNDRV_SERIAL_SOUNDCANVAS};
+static int droponfull[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS -1)] = SNDRV_SERIAL_NORMALBUFF };

 MODULE_PARM(index, "1-" __MODULE_STRING(SNDRV_CARDS) "i");
 MODULE_PARM_DESC(index, "Index value for Serial MIDI.");
@@ -99,6 +103,9 @@ MODULE_PARM(outs, "1-" __MODULE_STRING(S
 MODULE_PARM_DESC(outs, "Number of MIDI outputs.");
 MODULE_PARM(ins, "1-" __MODULE_STRING(SNDRV_CARDS) "i");
 MODULE_PARM_DESC(ins, "Number of MIDI inputs.");
+MODULE_PARM(droponfull, "1-" __MODULE_STRING(SNDRV_CARDS) "i");
+MODULE_PARM_DESC(droponfull, "Flag to enable drop-on-full buffer mode");
+MODULE_PARM_SYNTAX(droponfull, SNDRV_ENABLED "," SNDRV_BOOLEAN_FALSE_DESC);

 MODULE_PARM_SYNTAX(outs, SNDRV_ENABLED ",allows:{{1,16}},dialog:list");
 MODULE_PARM_SYNTAX(ins, SNDRV_ENABLED ",allows:{{1,16}},dialog:list");
@@ -163,6 +170,7 @@ typedef struct _snd_uart16550 {
        int buff_in_count;
         int buff_in;
         int buff_out;
+        int drop_on_full;

        // wait timer
        unsigned int timer_running:1;
@@ -194,12 +202,15 @@ inline static void snd_uart16550_del_tim
 inline static void snd_uart16550_buffer_output(snd_uart16550_t *uart)
 {
        unsigned short buff_out = uart->buff_out;
-       outb(uart->tx_buff[buff_out], uart->base + UART_TX);
-       uart->fifo_count++;
-       buff_out++;
-       buff_out &= TX_BUFF_MASK;
-       uart->buff_out = buff_out;
-       uart->buff_in_count--;
+       if( uart->buff_in_count > 0 )
+       {
+               outb(uart->tx_buff[buff_out], uart->base + UART_TX);
+               uart->fifo_count++;
+               buff_out++;
+               buff_out &= TX_BUFF_MASK;
+               uart->buff_out = buff_out;
+               uart->buff_in_count--;
+       }
 }

 /* This loop should be called with interrupts disabled
@@ -257,9 +268,12 @@ static void snd_uart16550_io_loop(snd_ua
           || uart->adaptor == SNDRV_SERIAL_GENERIC) {
                /* Can't use FIFO, must send only when CTS is true */
                status = inb(uart->base + UART_MSR);
-               if (uart->fifo_count == 0 && (status & UART_MSR_CTS)
-                   && uart->buff_in_count > 0)
-                       snd_uart16550_buffer_output(uart);
+               while( (uart->fifo_count == 0) && (status & UART_MSR_CTS) &&
+                      (uart->buff_in_count > 0) )
+               {
+                       snd_uart16550_buffer_output(uart);
+                       status = inb( uart->base + UART_MSR );
+               }
        } else {
                /* Write loop */
                while (uart->fifo_count < uart->fifo_limit      /* Can we write ? */
@@ -576,19 +590,33 @@ static int snd_uart16550_output_close(sn
        return 0;
 };

-inline static void snd_uart16550_write_buffer(snd_uart16550_t *uart, unsigned char byte)
+inline static int snd_uart16550_buffer_can_write( snd_uart16550_t *uart, int Num )
 {
-       unsigned short buff_in = uart->buff_in;
-       uart->tx_buff[buff_in] = byte;
-       buff_in++;
-       buff_in &= TX_BUFF_MASK;
-       uart->buff_in = buff_in;
-       uart->buff_in_count++;
-       if (uart->irq < 0) /* polling mode */
-               snd_uart16550_add_timer(uart);
+       if( uart->buff_in_count + Num < TX_BUFF_SIZE )
+               return 1;
+       else
+               return 0;
 }

-static void snd_uart16550_output_byte(snd_uart16550_t *uart, snd_rawmidi_substream_t * substream, unsigned char midi_byte)
+inline static int snd_uart16550_write_buffer(snd_uart16550_t *uart, unsigned char byte)
+{
+       unsigned short buff_in = uart->buff_in;
+       if( uart->buff_in_count < TX_BUFF_SIZE )
+       {
+               uart->tx_buff[buff_in] = byte;
+               buff_in++;
+               buff_in &= TX_BUFF_MASK;
+               uart->buff_in = buff_in;
+               uart->buff_in_count++;
+               if (uart->irq < 0) /* polling mode */
+                       snd_uart16550_add_timer(uart);
+               return 1;
+       }
+       else
+               return 0;
+}
+
+static int snd_uart16550_output_byte(snd_uart16550_t *uart, snd_rawmidi_substream_t * substream, unsigned char midi_byte)
 {
        if (uart->buff_in_count == 0                            /* Buffer empty? */
            && ((uart->adaptor != SNDRV_SERIAL_MS124W_SA &&
@@ -611,13 +639,14 @@ static void snd_uart16550_output_byte(sn
                        }
                }
        } else {
-               if (uart->buff_in_count >= TX_BUFF_SIZE) {
+               if( !snd_uart16550_write_buffer(uart, midi_byte) ) {
                        snd_printk("%s: Buffer overrun on device at 0x%lx\n",
                                   uart->rmidi->name, uart->base);
-                       return;
+                       return 0;
                }
-               snd_uart16550_write_buffer(uart, midi_byte);
        }
+
+       return 1;
 }

 static void snd_uart16550_output_write(snd_rawmidi_substream_t * substream)
@@ -661,40 +690,41 @@ static void snd_uart16550_output_write(s
                }
        } else {
                first = 0;
-               while (1) {
-                       if (snd_rawmidi_transmit(substream, &midi_byte, 1) != 1)
-                               break;
+               while( 1 == snd_rawmidi_transmit_peek(substream, &midi_byte, 1) )
+               {
                        /* Also send F5 after 3 seconds with no data to handle device disconnect */
                        if (first == 0 && (uart->adaptor == SNDRV_SERIAL_SOUNDCANVAS ||
                                uart->adaptor == SNDRV_SERIAL_GENERIC) &&
                           (uart->prev_out != substream->number || jiffies-lasttime > 3*HZ)) {

-                               /* We will need three bytes of data here (worst case). */
-                               if (uart->buff_in_count >= TX_BUFF_SIZE - 3)
-                                       break;
-
-                               /* Roland Soundcanvas part selection */
-                               /* If this substream of the data is different previous
-                                  substream in this uart, send the change part event */
-                               uart->prev_out = substream->number;
-                               /* change part */
-                               snd_uart16550_output_byte(uart, substream, 0xf5);
-                               /* data */
-                               snd_uart16550_output_byte(uart, substream, uart->prev_out + 1);
-                               /* If midi_byte is a data byte, send the previous status byte */
-                               if ((midi_byte < 0x80) && (uart->adaptor == SNDRV_SERIAL_SOUNDCANVAS))
-                                       snd_uart16550_output_byte(uart, substream, uart->prev_status[uart->prev_out]);
-                       }
+                              if( snd_uart16550_buffer_can_write( uart, 3 ) )
+                              {
+                                      /* Roland Soundcanvas part selection */
+                                      /* If this substream of the data is different previous
+                                         substream in this uart, send the change part event */
+                                      uart->prev_out = substream->number;
+                                      /* change part */
+                                      snd_uart16550_output_byte(uart, substream, 0xf5);
+                                      /* data */
+                                      snd_uart16550_output_byte(uart, substream, uart->prev_out + 1);
+                                      /* If midi_byte is a data byte, send the previous status byte */
+                                      if ((midi_byte < 0x80) && (uart->adaptor == SNDRV_SERIAL_SOUNDCANVAS))
+                                            snd_uart16550_output_byte(uart, substream, uart->prev_status[uart->prev_out]);
+                              }
+                              else if( !uart->drop_on_full )
+                                      break;

-                       /* buffer full? */
-                       if (uart->buff_in_count >= TX_BUFF_SIZE)
-                               break;
+                       }

                        /* send midi byte */
-                       snd_uart16550_output_byte(uart, substream, midi_byte);
+                       if( !snd_uart16550_output_byte(uart, substream, midi_byte) && !uart->drop_on_full )
+                               break;
+
                        if (midi_byte >= 0x80 && midi_byte < 0xf0)
                                uart->prev_status[uart->prev_out] = midi_byte;
                        first = 1;
+
+                       snd_rawmidi_transmit_ack( substream, 1 );
                }
                lasttime = jiffies;
        }
@@ -755,6 +785,7 @@ static int __init snd_uart16550_create(s
                                       unsigned int speed,
                                       unsigned int base,
                                       int adaptor,
+                                      int droponfull,
                                       snd_uart16550_t **ruart)
 {
        static snd_device_ops_t ops = {
@@ -771,6 +802,7 @@ static int __init snd_uart16550_create(s
        spin_lock_init(&uart->open_lock);
        uart->irq = -1;
        uart->base = iobase;
+       uart->drop_on_full = droponfull;

        if ((err = snd_uart16550_detect(uart)) <= 0) {
                printk(KERN_ERR "no UART detected at 0x%lx\n", iobase);
@@ -900,6 +932,7 @@ static int __init snd_serial_probe(int d
                                        speed[dev],
                                        base[dev],
                                        adaptor[dev],
+                                       droponfull[dev],
                                        &uart)) < 0) {
                snd_card_free(card);
                return err;
@@ -910,7 +943,7 @@ static int __init snd_serial_probe(int d
                return err;
        }

-       sprintf(card->longname, "%s at 0x%lx, irq %d speed %d div %d outs %d ins %d adaptor %s",
+       sprintf(card->longname, "%s at 0x%lx, irq %d speed %d div %d outs %d ins %d adaptor %s droponfull %d",
                card->shortname,
                uart->base,
                uart->irq,
@@ -918,7 +951,8 @@ static int __init snd_serial_probe(int d
                (int)uart->divisor,
                outs[dev],
                ins[dev],
-               adaptor_names[uart->adaptor]);
+               adaptor_names[uart->adaptor],
+               uart->drop_on_full);

        if ((err = snd_card_register(card)) < 0) {
                snd_card_free(card);
@@ -964,7 +998,7 @@ module_exit(alsa_card_serial_exit)

 /* format is: snd-serial=enable,index,id,
                         port,irq,speed,base,outs,
-                        ins,adaptor */
+                        ins,adaptor,droponfull */

 static int __init alsa_card_serial_setup(char *str)
 {
@@ -981,7 +1015,8 @@ static int __init alsa_card_serial_setup
               get_option(&str,&base[nr_dev]) == 2 &&
               get_option(&str,&outs[nr_dev]) == 2 &&
               get_option(&str,&ins[nr_dev]) == 2 &&
-              get_option(&str,&adaptor[nr_dev]) == 2);
+              get_option(&str,&adaptor[nr_dev]) == 2 &&
+              get_option(&str,&droponfull[nr_dev]) == 2 );
        nr_dev++;
        return 1;
 }



-------------------------------------------------------
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/

Re: [PATCH] serial-u16550 driver. Fixes buffer blocking bug (2nd try)

[Takashi Iwai]

At Tue, 02 Dec 2003 17:41:13 -0800,
Steve deRosier wrote:
> 
> Takashi,
> 
> I've completed my changes on the serial-u16550 driver.  I have 
> implemented most of your suggestions to my previous patch.  Now:
> 
> * There is a user selectable flag "droponfull".  Set to 1 and
>   any new bytes delivered to the driver after the buffer fills
>   up will be discarded until the buffer is able to flush some
>   bytes.
> * If droponfull==0 (or is not set, the default is 0), the driver
>   proceeds to block further input by not calling 
>   snd_rawmidi_transmit_ack() and aborting the attempt.  It will
>   try again later.
> * I've redone a bit of the interface for the buffer routines.  
>   This was done to support the proper blocking/non-blocking methods
>   as spelled out above, and to try to protect the buffer data a bit.  
> 
> So, if droponfull==0, we operate as originally desired (but the 
> original implementation was broken a bit and this fixes it).  If
> droponfull==1 we now discard new bytes if our buffer is full; this
> behavior fixes the driver "hang" problem we were having.
> 
> The changes should have no material effect on the "SNDRV_SERIAL_MS124W_MB"
> device, and all others should still work well, though we can only
> test with the SNDRV_SERIAL_GENERIC. 

thanks, the patch looks nice.

unfortunately, the patch was broken.  perhaps your mailer converted
tab to spaces.  so i applied it manually and committed to cvs.
i hope i didn't copy wrongly.

please test whether it works when rc2 appears.


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/