Re: [ECOS] probable bug in io/serial/common/serial.c!

[Gary Thomas <gthomas@cygnus.co.uk>]

On 05-Jan-00 Mohammed Illyas Mansoor wrote:
> Gary Thomas wrote:
> 
>> I don't think there is a chance for deadlock here since the affected
>> region [while (size > 0)] is protected by a "DSR lock".  The call to
>> 'cyg_drv_dsr_lock()' effectively says "don't allow DSRs to run while
>> executing the following code".  Since it is only the DSR that can
>> signal the condition variable, this should be safe.
> 
>     This is true only if xmt_char() is called only by the DSR fucntion,
> but cogent_serial_with_interrupts calls xmt_char() directly from
> start_xmit(),
> after enabling xmt interrupts, in this implementation at least would it not
> cause a deadlock??
> 

Indeed, the 'start_xmit' function can call back to 'xmt_char' which potentially
_could_ signal the variable.  However, it will only do this when there is
at least "low water" space in the queue which is defined to be 25% of the
total queue/buffer length.  If you set the buffer small enough, then it 
could fail.

The defaults (and indeed most any realistic configuration) would not allow
this error to occur.

However, it seems prudent to add the check you suggested.  I will commit this
change to the sources:

Index: io/serial/current/ChangeLog
===================================================================
RCS file: /local/cvsfiles/ecc/ecc/io/serial/current/ChangeLog,v
retrieving revision 1.149
diff -u -5 -p -r1.149 ChangeLog
--- io/serial/current/ChangeLog 2000/01/03 20:52:00     1.149
+++ io/serial/current/ChangeLog 2000/01/05 14:47:12
@@ -1,5 +1,10 @@
+2000-01-05  Gary Thomas  <gthomas@cygnus.co.uk>
+
+       * src/common/serial.c (serial_write): Avoid potential deadlock if
+       transmit start actually sends enough characters to signal cond wait.
+
 2000-01-03  Gary Thomas  <gthomas@cygnus.co.uk>
 
        * include/serial.h: Fix namespace pollution - 
        serial_devio => cyg_io_serial_devio
        serial_callbacks => cyg_io_serial_callbacks
Index: io/serial/current/src/common/serial.c
===================================================================
RCS file: /local/cvsfiles/ecc/ecc/io/serial/current/src/common/serial.c,v
retrieving revision 1.12
diff -u -5 -p -r1.12 serial.c
--- io/serial/current/src/common/serial.c       2000/01/03 20:52:02     1.12
+++ io/serial/current/src/common/serial.c       2000/01/05 14:47:13
@@ -119,13 +119,16 @@ serial_write(cyg_io_handle_t handle, con
             if (next == cbuf->len) next = 0;
             if (next == cbuf->get) {                
                 cbuf->waiting = true;
                 // Buffer full - wait for space
                 (funs->start_xmit)(chan);  // Make sure xmit is running
-                cbuf->pending += size;  // Have this much more to send [eventually]
-                cyg_drv_cond_wait(&cbuf->wait);
-                cbuf->pending -= size;
+                if (cbuf->waiting) {
+                    // Note: 'start_xmit' may have obviated the need to wait :-)
+                    cbuf->pending += size;  // Have this much more to send [eventually]
+                    cyg_drv_cond_wait(&cbuf->wait);
+                    cbuf->pending -= size;
+                }
                 if (cbuf->abort) {
                     // Give up!
                     cbuf->abort = false;
                     cbuf->waiting = false;
                     res = -EINTR;