Re: [Alsa-devel] [PATCH] serial-u16550 driver. Fixes buffer blocking bug (2nd try)
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/ ___ Alsa-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/alsa-devel
[Alsa-devel] [PATCH] serial-u16550 driver. Fixes buffer blocking bug (2nd try)
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 - 1.22
+++ serial-u16550.c 3 Dec 2003 01:14:42 -
@@ -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 */
Re: [Alsa-devel] [PATCH] serial-u16550 driver. Fixes buffer blocking bug
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
Re: [Alsa-devel] [PATCH] serial-u16550 driver. Fixes buffer blocking bug
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
Re: [Alsa-devel] [PATCH] serial-u16550 driver. Fixes buffer blocking bug
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
[Alsa-devel] [PATCH] serial-u16550 driver. Fixes buffer blocking bug
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
