This is the mail archive of the
ecos-discuss@sourceware.cygnus.com
mailing list for the eCos project.
RE: probable bug in io/serial/common/serial.c!
- To: Mohammed Illyas Mansoor <mansoor at cdotb dot ernet dot in>
- Subject: RE: [ECOS] probable bug in io/serial/common/serial.c!
- From: Gary Thomas <gthomas at cygnus dot co dot uk>
- Date: Wed, 05 Jan 2000 06:35:15 -0700 (MST)
- Cc: jskov at cynus dot co dot uk dot cdotb dot ernet dot in, ecos-discuss at sourceware dot cygnus dot com
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.
>