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

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

[Mohammed Illyas Mansoor]

Hi,
    I was going through the common serial io code it seems to me
that there probably is a bug in it, below is a snap shot of the code
in question.

<snap>
static Cyg_ErrNo
serial_write(cyg_io_handle_t handle, const void *_buf, cyg_uint32 *len)
{
    cyg_devtab_entry_t *t = (cyg_devtab_entry_t *)handle;
    serial_channel *chan = (serial_channel *)t->priv;
    serial_funs *funs = chan->funs;
    cyg_int32 size = *len;
    cyg_uint8 *buf = (cyg_uint8 *)_buf;
    int next;
    cbuf_t *cbuf = &chan->out_cbuf;
    Cyg_ErrNo res = ENOERR;

    cbuf->abort = false;
    cyg_drv_mutex_lock(&cbuf->lock);
    if (cbuf->len == 0) {
        // Non interrupt driven (i.e. polled) operation
        while (size-- > 0) {
            while ((funs->putc)(chan, *buf) == false) ;  // Ignore full,
keep trying
            buf++;
        }
    } else {
        cyg_drv_dsr_lock();  // Avoid race condition testing pointers
        while (size > 0) {
            next = cbuf->put + 1;
            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->abort) {
                    // Give up!
                    cbuf->abort = false;
                    cbuf->waiting = false;
                    res = -EINTR;
                    break;
                }
            } else {
                cbuf->data[cbuf->put++] = *buf++;
                cbuf->put = next;
                size--;  // Only count if actually sent!
            }
        }
        cyg_drv_dsr_unlock();
        (funs->start_xmit)(chan);  // Start output as necessary
    }
    cyg_drv_mutex_unlock(&cbuf->lock);
    return res;
}
</snap>

    In the line with ***--->cyg_drv_cond_wait(&cond_wait);<----***,
will cause a deadlock, because before going for a wait on the condition
variable
start_xmt function is called which will trigger the transmitter to send
the char's, if the call to xmt_char finds out that chan->waiting is true

and wakes up the writer thread which was supposed to be waiting by now,
he would also change the chan->waiting to false. Later, when the wait by

the writer is called there will be no waking up of the writer by the
driver
(as it has been already done by the xmt_char() ), causing it to
deadlock.

The fix might be as below,

            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]
            ++if ( cbuf->waiting == true)
                cyg_drv_cond_wait(&cbuf->wait);


--With Regards,

M. I. Mansoor.

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

[Gary Thomas]

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.

On 05-Jan-00 Mohammed Illyas Mansoor wrote:
> Hi,
>     I was going through the common serial io code it seems to me
> that there probably is a bug in it, below is a snap shot of the code
> in question.
> 
> <snap>
> static Cyg_ErrNo
> serial_write(cyg_io_handle_t handle, const void *_buf, cyg_uint32 *len)
> {
>     cyg_devtab_entry_t *t = (cyg_devtab_entry_t *)handle;
>     serial_channel *chan = (serial_channel *)t->priv;
>     serial_funs *funs = chan->funs;
>     cyg_int32 size = *len;
>     cyg_uint8 *buf = (cyg_uint8 *)_buf;
>     int next;
>     cbuf_t *cbuf = &chan->out_cbuf;
>     Cyg_ErrNo res = ENOERR;
> 
>     cbuf->abort = false;
>     cyg_drv_mutex_lock(&cbuf->lock);
>     if (cbuf->len == 0) {
>         // Non interrupt driven (i.e. polled) operation
>         while (size-- > 0) {
>             while ((funs->putc)(chan, *buf) == false) ;  // Ignore full,
> keep trying
>             buf++;
>         }
>     } else {
>         cyg_drv_dsr_lock();  // Avoid race condition testing pointers
>         while (size > 0) {
>             next = cbuf->put + 1;
>             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->abort) {
>                     // Give up!
>                     cbuf->abort = false;
>                     cbuf->waiting = false;
>                     res = -EINTR;
>                     break;
>                 }
>             } else {
>                 cbuf->data[cbuf->put++] = *buf++;
>                 cbuf->put = next;
>                 size--;  // Only count if actually sent!
>             }
>         }
>         cyg_drv_dsr_unlock();
>         (funs->start_xmit)(chan);  // Start output as necessary
>     }
>     cyg_drv_mutex_unlock(&cbuf->lock);
>     return res;
> }
> </snap>
> 
>     In the line with ***--->cyg_drv_cond_wait(&cond_wait);<----***,
> will cause a deadlock, because before going for a wait on the condition
> variable
> start_xmt function is called which will trigger the transmitter to send
> the char's, if the call to xmt_char finds out that chan->waiting is true
> 
> and wakes up the writer thread which was supposed to be waiting by now,
> he would also change the chan->waiting to false. Later, when the wait by
> 
> the writer is called there will be no waking up of the writer by the
> driver
> (as it has been already done by the xmt_char() ), causing it to
> deadlock.
> 
> The fix might be as below,
> 
>             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]
>             ++if ( cbuf->waiting == true)
>                 cyg_drv_cond_wait(&cbuf->wait);
> 
> 
> --With Regards,
> 
> M. I. Mansoor.
> 

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

[Mohammed Illyas Mansoor]

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

--With Regards,

M. I. Mansoor

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

[Gary Thomas]

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;