I tested the below patch and it works. The throughput performance change compared to using a semaphore (in our system) is usbs_devtab_cwrite : 17% drop usbs_devtab_cread : 53% drop which is quite significant!
Derek -----Original Message----- From: Bart Veer [mailto:[EMAIL PROTECTED] Sent: Monday, February 20, 2006 8:42 AM To: [EMAIL PROTECTED] Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED] Subject: Re: [ECOS] How to debug synchronisation in the usbs.c in a new usb-driver for the ARM at91sam7s... >>>>> "Derek" == Derek Bouius <[EMAIL PROTECTED]> writes: Derek> The problem is a race condition between checking the completed flag and executing the callback. Derek> I will describe the flow of the usbs_devtab_write: Derek> 1. Initialize the variables (signal, mutex, etc) Derek> 2. Setup the callback function - (Callback sets the signal) Derek> 3. Start the transfer Derek> 4. Lock the mutex. Derek> 5. Check if wait.completed Derek> ...race condition.... Derek> 6. Wait for signal -- waits here forever in some cases if the callback was executed after #5 and before #6 Derek> 7. Unlock the mutex. Derek> ... Derek> .. Derek> What happens in the lower level driver is that the (#3) Derek> start_tx_fn() enables the interrupt. That can fire Derek> immediately and call the callback from within the dsr. Derek> Executing #5 above can happen inbetween the enable of the Derek> interrupt and the actual interrupt. Derek> The same process can occur in the read. I agree there is a race condition in the current code. My preferred solution would be to replace the mutex/condition variable combo in this code with a semaphore, as per other ecos-discuss postings. An alternative approach for now, which works with the existing driver API, is to add some scheduler locking as well. The patch below is untested (I don't have any USB-capable hardware setup right now) but should do the trick. Bart Index: usbs.c =================================================================== RCS file: /cvs/ecos/ecos/packages/io/usb/slave/current/src/usbs.c,v retrieving revision 1.5 diff -u -r1.5 usbs.c --- usbs.c 23 May 2002 23:06:36 -0000 1.5 +++ usbs.c 20 Feb 2006 13:37:06 -0000 @@ -111,9 +111,11 @@ (*endpoint->start_tx_fn)(endpoint); cyg_drv_mutex_lock(&wait.lock); + cyg_drv_dsr_lock(); while (!wait.completed) { cyg_drv_cond_wait(&wait.signal); } + cyg_drv_dsr_unlock(); cyg_drv_mutex_unlock(&wait.lock); if (wait.result < 0) { result = wait.result; @@ -155,9 +157,11 @@ endpoint->complete_data = (void*) &wait; (*endpoint->start_rx_fn)(endpoint); cyg_drv_mutex_lock(&wait.lock); + cyg_drv_dsr_lock(); while (!wait.completed) { cyg_drv_cond_wait(&wait.signal); } + cyg_drv_dsr_unlock(); cyg_drv_mutex_unlock(&wait.lock); if (wait.result < 0) { result = wait.result; -- Bart Veer eCos Configuration Architect http://www.ecoscentric.com/ The eCos and RedBoot experts -- Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss
