Re: [ECOS] How to debug synchronisation in the usbs.c in a new usb-driver for the ARM at91sam7s...

[Bart Veer <bartv at ecoscentric dot com>]

>>>>> "Nick" == Nick Garnett <nickg@ecoscentric.com> writes:

    <snip>
    >> Does anybody remember why counting semaphores were left out of the
    >> driver API?

    Nick> In eCos device drivers are responsible for their own
    Nick> concurrency control. While some drivers might be able to
    Nick> cope with two threads executing in them concurrently, most
    Nick> cannot. They must therefore implement some sort of mutual
    Nick> exclusion to serialize the threads. By remarkable
    Nick> coincidence, a mutex does exactly the right thing.

    Nick> Once a thread is in the driver it examines its state.
    Nick> Depending on what it finds, it may need to wait for data to
    Nick> arrive, or start data transmission, and then wait for it to
    Nick> finish. The wait takes the form of a loop, testing some
    Nick> condition of the device driver and calling
    Nick> cyg_dev_cond_wait().

    Nick> Waiting on a condition variable unlocks the mutex, which may
    Nick> allow a second thread through. This thread may want to do
    Nick> something else in the device (transmit rather than receive,
    Nick> select, non-blocking IO, ioctl etc.) and may either quickly
    Nick> exit or wait elsewhere. If it follows the same path as the
    Nick> first thread it will end up queued behind it on the same
    Nick> condition variable.

    Nick> A semaphore just would not work in this situation. For a
    Nick> start it does not atomically release the mutex, and just
    Nick> unlocking the mutex before waiting on the semaphore (and
    Nick> relocking after) opens up a race window. Also, the semaphore
    Nick> count is not very useful. The fact that the ISR/DSR have run
    Nick> is encoded in the state of the device driver, which is what
    Nick> the loop around the condition variable wait should be
    Nick> testing. For example, in the serial drivers, threads wait
    Nick> for characters to be inserted into the receive queue, or for
    Nick> space to become available in the transmit queue.

    Nick> If the USB drivers are losing wakeups I can only assume that
    Nick> the wait loops are testing the wrong conditions. It should
    Nick> not be too hard to work out what the correct condition is
    Nick> and fix it.

OK, I think I understand your reasoning. It makes a great deal of
sense in the context of serial drivers where e.g. data may arrive
asynchronously and end up in ring buffers. However I am not sure it
makes as much sense for other kinds of devices where no I/O happens
until it is explicitly initiated. USB, I2C and SPI all fail into that
category.

To get things working right with condition variables the code would
have to do something like the following:

1) in the device structure:

       cyg_drv_mutex_t	lock;
       cyg_drv_cond_t   wait;
       cyg_bool		busy;
       cyg_bool		completed;

2) at the start of an operation, typically somewhere in the
   device-specific I/O layer:

       cyg_drv_mutex_lock(&lock);
       while (busy) {
           cyg_drv_cond_wait(&wait);
       }
       busy = 1;
       (call into the device driver to perform the actual I/O)
       busy = 0;
       cyg_drv_mutex_unlock(&lock);
       cyg_drv_cond_signal(&wait);

3) inside the device driver, called with lock claimed:

       completed = 0;
       (start the I/O operation)
       cyg_drv_dsr_lock();
       while (!completed) {
         cyg_drv_cond_wait(&wait);
       }
       cyg_drv_dsr_unlock();
       (perform any clean-ups that may be necessary)

4) in the DSR:

       completed = 1;
       cyg_drv_cond_broadcast(&wait);

(For simplicity this ignores incoming signals aborting the drv
routines. Typically for something like USB or SPI there is no sensible
way to abort a transfer that is in progress, so the I/O is not
interruptible and signals have to be ignored).

What I would prefer to see is:

1) in the device structure:

       cyg_drv_mutex_t		lock;
       cyg_drv_sem_t		wait;

2) at the start of an I/O operation:

       cyg_drv_mutex_lock(&lock);
       (call into the device driver to perform the actual I/O)
       cyg_drv_mutex_unlock(&lock);

3) inside the device driver:

       (start the I/O operation)
       cyg_drv_sem_wait(&wait);
       (perform any clean-ups that may be necessary);

4) in the DSR:

       cyg_drv_sem_post(&wait);

(again allowing for signals will complicate this a bit).

A semaphore and a condition variable are the same size (assuming
32-bit pointers), so the second approach saves space for the busy and
completed flags. That may be two ints, two bytes, or just two bits in
a flags field (although testing bits is likely to impose a code size
penalty). It is still an overhead.

Re. code size, the second approach eliminates four function calls,
four assignments, and two loops. A fairly clear gain. If there are
multiple threads attempting to perform I/O to the same device then
the cyg_drv_cond_broadcast() call will also result in threads waking
up and having to go to sleep again immediately because the device is
still busy. This could be eliminated using a separate condition
variable, but that adds other overhead.

In terms of real-time behaviour, the condition variable approach has
to lock the scheduler albeit for only a short time. More worryingly
mutex priority inversion protection is not going to help. If you have
a low priority thread currently performing I/O, a high priority thread
wanting access to the same device, and an intermediate thread hogging
the cpu, then when the I/O has completed you have a priority inversion
situation: the high priority thread is blocked on a condition
variable, not a semaphore.

So although I accept that there are situations where general purpose
condition variables will be more appropriate, I believe the semaphore
approach is better for many types of device. Hence we should
reconsider adding semaphores to the driver API.

Bart

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