locking in a device driver

[Julian Elischer]

Julian Elischer wrote:

> Dinesh Nair wrote:
>
>>
>> On 11/02/05 00:03 Scott Long said the following:
>>
>>> I think this thread has gone too far into hyperbole and conjecture. 
>>> What is your code trying to do, and what problems are you seeing?
>>
>>
>>
>> apologies, scott. i'm actually trying to get a driver written for 
>> freebsd 5.x backported to 4.x. the driver works like a charm on 5.x.
>>
>> under 4.x, i seem to be getting problems with synchronization/locking.
>>
>> the driver set consists of two drivers, a pseudo driver which the 
>> userland reads/writes/ioctls to, and the actual device driver which 
>> reads/writes from the device thru DMA and places the data into a ring 
>> buffer.
>>
>> this is the sequence of events for data from the device:
>>
>> 1. interrupt handler of device driver called
>
>
> handler called at splxxx() level   (see later)
>
>> 2. device driver reads data from DMA memory space
>
>
> still at splxxx level
>
>> 3. device driver writes to a shared buffer
>
>
> still at splxxx level
>
>>
>> 4. device driver calls a function inside pseudo driver
>
>
> still at splxxx level
>
>> 5. pseudo driver copies data from shared buffer to another buffer
>
>
> still at splxx level
>
>> 6. wakeup() is called
>
>
> still at splxxx level
>
>> 7. device driver waits for next interrupt
>
>
> drops to splzero or similar,..
> woken process called,
> starts manipulating "another buffer"
> collides with next interrupt.
>
> what ever is woken up needs to call splxxx() to block the interupt 
> routine while
> manipulating "another buffer" for as long as it takes to get the data out
> or swap the b uffer pointers or whatever it does.
>
>
>>
>> the interrupt handler uses splhigh()/splx() to mask out interrupts 
>> during the time it's executing. interrupts happen 1000 times a second 
>> consistently.
>

the interrupt handler is ALREADY blocking it's own interupt. you don't 
need to do anything.


You need to block interrupts for a momen tin #8, not in the interrupt 
handler.

>
>
> When you register an interrupt handler, you specify which of several 
> groups you are a part of..
> Network, disk io, etc.
>
> once you specify you are part of a  particular set, then you should only
> block interrupts from that set..
>
> e.g, a netowrk driver would use  s = splimp(); ...; splx(s)
>
> Try not to use splhigh()..
> it is ok for getting your driver going but may block too much.
>
>>
>> when a read on the pseudo device is called from a -lc_r threaded 
>> userland process, the following happens in pseudo device driver:
>>
>> 7. tsleep() is called, (with the same ident as the wakeup() in #6)
>> 8. pseudo device reads from buffer in #5 and uses uiomove to return 
>> data to calling process
>
>
>
> it needs to call splxxx() while it is doing it..
> I would suggest having two buffers and swapping them under splxxx() so 
> that
> the one that the driver is accessing is not the one you are draining.
> that  way teh splxxx() levle needs to only be held for the small time 
> you are doing the swap.
>
>>
>> exactly the reverse happens for a write.
>>
>> i believe that steps 3,5,8 need to be protected/synchronized with locks.
>
>
>
> not locks, but spl,
> and only step 8 needs to be changed because all teh rest are already 
> done at high spl.
>
>>
>> the code uses mtx_lock/mtx_unlock in the 5.x version, and uses 
>> simple_lock in the 4.x version. digging thru the include files, i've 
>> noticed that simple_lock is a NOOP in 4.x if you're on a single 
>> processor.
>>
>> could i replace the mtx_lock/mtx_unlock with 
>> lockmgr(...,LK_EXCLUSIVE/LK_RELEASE) instead ? the earlier notion of 
>> using
>> splhigh()/splx() to protect the common areas didnt seem like the 
>> right way to do it since the pseudo device driver is not interrupt 
>> driven, but rather is driven by open/read/write/ioctl from the userland.
>>
>> also, when the threaded userland reads, it's first put to a tsleep 
>> with PZERO|PCATCH and this tsleep is surrounded by a mtx_lock(&Giant) 
>> and mtx_unlock(&Giant). what should this be replaced with in 4.x ?
>>
>> when the real device driver has data from the device, it uses 
>> wakeup() to wake the tsleep'ed bits in the pseudo device driver. is 
>> this the correct way to do this ?
>>
>> also, is there any equivalent for PROC_LOCK()/PROC_UNLOCK() in 4.x or 
>> is it unnecessary ?
>>
>
> use spl???()/splx  uinstead.
> your top end needs to raise to the same level as the interrupt you have
> registered for as long as it is manupulating data that the bottom end
> might manipulate, (e.g. buffer pointers or linked lists, etc.)
> _______________________________________________
> freebsd-hackers at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to 
> "freebsd-hackers-unsubscribe at freebsd.org"