[BUG] DMA channel functions are not thread-safe

[JonathonReinhart]

Background

While access to global DMA controller configuration should of course be single-threaded, it is reasonable to expect that configuration of individual channels can happen safely on different threads. For example, I might have one FreeRTOS thread managing a SPI interface while another thread deals with I2C.

Most of the per-channel configuration happens in per-channel registers (DMA_Type.CHANNEL[i]), so as long as only one thread manages one channel, there is no concern here.

But there are also per-channel bits in a few registers in the common (DMA_Type.COMMON) area, e.g. DMA_Type.COMMON[0].ENABLESET) Accessing these bits with a read-modify-write sequence is racy and could potentially inadvertently re-set bits which were recently cleared. That's why either:

• Separate SET/CLR registers are provided, where only 1 bits being written have any effect on the underlying register.
  • e.g. ENABLESET, ENABLECLR
• Registers are write-only with the same semantics (e.g. ABORT)

Problem

Some DMA functions use |= in an attempt to only set the desired bit. As described above, this is unnecessary and can actually set bits which were cleared on another thread.

E.g. DMA_EnableChannel():

mcux-sdk/drivers/lpc_dma/fsl_dma.h

Lines 405 to 410 in cf224e2

	static inline void DMA_EnableChannel(DMA_Type *base, uint32_t channel)
	{
	assert((FSL_FEATURE_DMA_NUMBER_OF_CHANNELSn(base) != -1) &&
	(channel < (uint32_t)FSL_FEATURE_DMA_NUMBER_OF_CHANNELSn(base)));
	DMA_COMMON_REG_GET(base, channel, ENABLESET) |= 1UL << DMA_CHANNEL_INDEX(base, channel);
	}

This will compile to a read-modify-write sequence like this:

ldr     r1, DMA0_ENABLESET0
ldr     r2, [r1]
orr     r2, r2, #1
; ----
str     r2, [r1]

If this thread is preempted on the indicated line, and another thread clears bits in the register, they will be re-set when this thread resumes.

To Reproduce
I haven't actually observed this yet.

Solution

The code should not care about the current value of the registers. (For write-only registers, it is all zero anyway.)

It should simply write a 1 in the bit to be modified, and let the SET/CLR semantics do the right thing.

[zejiang0jason]

Hi @JonathonReinhart , you are right, the |= shall be =. Thanks for point out this. Will fix.

[JonathonReinhart]

Hi @JonathonReinhart , you are right, the |= shall be =. Thanks for point out this. Will fix.

Or use the DMA_COMMON_REG_SET macro, e.g.:

DMA_COMMON_REG_SET(base, channel, ENABLESET, 1UL << DMA_CHANNEL_INDEX(base, channel));

[JonathonReinhart]

Here's my fix: #174

[JonathonReinhart]

@zejiang0jason I've tested #174 in my target application but haven't evaluated in any of the EVK sample apps. If you could help test, that would be great.