RE: Race conditions in serial_select

["Gaer, A." <Andreas dot Gaer at baslerweb dot com>]

Hello!

As the DSR IMHO shouldn't try to accquire the mutex lock anyway, I would
guess that it is save to do the locking in the proposed way. But I must
admit that I intended to do it exactly the other way around like it was done
in all the other functions of the serial.c file, simply to keep the logic a
bit consistent. Don't ask me why I got it wrong, I must have been completely
blind that afternoon. Sorry.

I can resend the patch with the locks swapped, if this is wanted.

As I understand it, the mutex lock is used to synchronize access to the
read/write buffers from concurrent threads. As the logic of the select
depends on the consitency of the buffers during it's exectution, I would say
the mutex lock is neccessary. Though I can't imagine any usecase where the
serial select and read/write calls are placed into different threads - but
who knows.

Best regards,
 Andreas.

> -----Original Message-----
> From: Jay Foster [mailto:jay@systech.com]
> Sent: Tuesday, June 21, 2005 12:00 AM
> To: 'Andrew Lunn'; Gaer, A.
> Cc: ecos-patches@ecos.sourceware.org
> Subject: RE: Race conditions in serial_select
> 
> 
> I took a look at this change for considering whether to apply 
> it to my code
> base.  It looks like there may now be a deadlock problem with 
> the new code.
> In the serial.c file, the code obtains and releases the locks 
> as follows:
> 
> 	cyg_drv_mutex_lock(...);
> 	...
> 	cyg_drv_dsr_lock(...);
> 
> 	...
> 
> 	cyg_drv_dsr_unlock(...);
> 	...
> 	cyg_drv_mutex_unlock(...);
> 
> With the change in serial_select(), the order is reversed 
> (i.e. the DSR lock
> is aquired before the mutex lock).  This can lead to a 
> deadlock condition.
> Would not just the DSR lock be sufficient?
> 
> Jay Foster
> 
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Monday, June 20, 2005 11:15 AM
> To: Gaer, A.
> Cc: ecos-patches@ecos.sourceware.org
> Subject: Re: Race conditions in serial_select
> 
> 
> On Fri, Jun 17, 2005 at 11:47:09AM +0200, Gaer, A. wrote:
> > Hello all!
> > 
> > The serial_select() implementation from
> > "io/serial/current/src/common/serial.c" doesn't care about 
> concurrent
> access
> > to the cbuf structure. This leads to race conditions when 
> the serial DSR
> > routine changes the counters in this structure while select 
> checks for
> > "cbuf->nb == 0". As a result, user threads that sleep 
> because of a select
> > call will not be woken up, even if data arrives. Also access to this
> > structure isn't implemented thread-safe because of missing 
> mutex locks.
> The
> > attached patch fixes this problems.
> 
>         Thanks
>                 Andrew
>