fix(dmac): DMAC handler now waits for channel to report as disabled #938
Conversation
| }); | ||
|
|
||
| while dmac.channel(channel).chctrla().read().enable().bit_is_set() { | ||
| core::hint::spin_loop(); |
There was a problem hiding this comment.
Is spin_loop necessary? I thought that bug had been fixed. Don't we use an empty while body in tons of other places?
There was a problem hiding this comment.
there was a bug where spin_loop was necessary? it seems like sometimes there's an empty body, sometimes spin_loop is used, and sometimes cortex_m::asm::nop() is used
There was a problem hiding this comment.
Indeed maybe we should standardize at some point, but it's not a source of bugs or anything so I'm not super concerned about it for now
| // Prevent the compiler from re-ordering read/write | ||
| // operations beyond this fence. | ||
| // (see https://docs.rust-embedded.org/embedonomicon/dma.html#compiler-misoptimizations) | ||
| atomic::fence(atomic::Ordering::Acquire); // ▼ |
There was a problem hiding this comment.
Is there another atomic::fence with Release ordering somewhere else? I thought fences like this were only meaningful when paired across two concurrent functions? I think it's already guaranteed by the language that the enable bit will read true before wake() is called. The reads are volatile, so they can't be removed, and the read is observed, so you can't reorder wake() before it.
There was a problem hiding this comment.
Yeah, this seems more or less unnecessary. I think the intention was to protect reordering reads/writes to the memory chunk the DMA is accessing, although we already have fences for that at a lower level.
That being said, we had a discussion a while ago about how atomic::fence isn't even enough to guarantee no reordering will take place... IIRC the verdict was that we should use an empty asm! black box instead.
There was a problem hiding this comment.
Channel::_enable_private has the corresponding Release fence + is called in the TransferFuture::poll implementation that this ISR interacts with
|
Nice, thank you! One small comment, I think this fix should also extend to D11/D21 targets. This should also take care of the clippy warning by the same occasion. |
|
oh good catch @jbeaurivage , added in 2d466f1 |
Summary
I was running into an issue with using multiple SPI+DMA transactions in a very tight control loop: sometimes the
spi.transferwould hang indefinitely because the RX DMA channel was enabled, but somehow the TX DMA channel was disabled.I managed to figure out that this was happening because the DMAC interrupt handler does not wait for the channel to finish disabling itself, meaning that sometimes the disable doesn't finish until after my loop's next SPI enable is dispatched.
The fix is for the DMAC interrupt handler to wait for the channel to finish disabling before waking the future up again.
--
Note that
TransferFutureis not a public item, so it should be OK for its return type to change.Checklist
#[allow]certain lints where reasonable, but ideally justify those with a short comment.