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

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

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