Incomplete use of memory barriers in driver code

[Araneidae]

Actually, I'm a bit worried about this code now. I see that block->state is modified and read in multiple threads without enough memory barriers or synchronisation. I see some scattered calls to smp_rmb() and smp_wmb(), but am rusty as to their correct use.

Writes

• assign_buffer() set state to BLOCK_DMA, called from ISR. This has no barrier.
• receive_isr_block() sets state to BLOCK_DATA or BLOCK_DATA_END, also called from ISR. This is preceded by smp_wmb().
• allocate_blocks() initialises state to BLOCK_FREE. Not too worried about this one.
• start_hardware() similarly.
• advance_block() sets state to BLOCK_FREE, preceded by smp_wmb(), called from user process.

Reads

• advance_isr_block() checks block is BLOCK_FREE. Called from ISR, no barrier.
• wait_for_block() waits until block is not BLOCK_DMA. I suspect there are the appropriate memory barriers in the implementation of wait_event_interruptible_timeout() which wraps this test. This is called from the user process.
• panda_stream_read() checks for blocks not BLOCK_DMA. No memory barriers, also called from user process.

Originally posted by @Araneidae in #16 (comment)

[Araneidae]

Unfortunately my understanding of how to use memory barriers properly is a bit wobbly. I think it would be good to write out our understanding of the rules here and make sure this code is doing things properly. In particular, any communication between ISR and user-space code must be properly guarded.

Even if this isn't biting us now, I'd expect this to potentially give us trouble with Zynq+ on ARMv8.

[thomascobb]

@Araneidae to discuss this with @lyodaom

[Araneidae]

Relevant documentation:

• Linux kernel programming: Do I need a lock?
• memory-barriers.txt in kernel Documentation directory
• recipes.txt in kernel tools/memory-model/Documentation directory

It looks as if we should be guarding our reads and writes with READ_ONCE() and WRITE_ONCE() as well as the appropriate barriers.