Skip to content

Conversation

Wren6991
Copy link
Contributor

@Wren6991 Wren6991 commented Mar 4, 2022

See: raspberrypi/pico-feedback#224

Aborting a channel can cause a spurious interrupt, contrary to documentation. This patch adds a workaround to the dma_abort() function which:

  • Saves and clears the interrupt enable for the aborted channel
  • Triggers the abort
  • Polls for completion using CTRL.BUSY rather than the ABORT bit which can trigger prematurely
  • Clears the channel interrupt and restores the interrupt enable

@Wren6991 Wren6991 requested a review from kilograham March 4, 2022 13:30
@kilograham
Copy link
Contributor

kilograham commented Mar 22, 2022

Am suggesting we not do this:

  • This enlarges/slows down the implementation a bit
  • The fix has to guess what the caller is doing (which IRQ etc)
  • Often the caller may have already protected themselves against this because the whole DMA abort/restart is inherently race prone.

instead we should document this with some sample code

@kilograham kilograham added the documentation Improvements or additions to documentation label Mar 22, 2022
@ZodiusInfuser
Copy link
Contributor

Hi. Just want to chip in here and say thanks to @Wren6991 for presenting the workaround.

I have been having similar issues these past weeks at Pimoroni with tearing down DMA (something that becomes quite a frequent task when dealing with our Micropython bindings it turns out), and this workaround appears to fix them.

It also explains why calling dma_channel_unclaim() following an abort had issues.

For now I have just copied in the implementation, but if this does ever get merged into master as dma_channel_abort() or another function, it would be great to know!

@lurch
Copy link
Contributor

lurch commented Mar 24, 2022

@kilograham Rather than merging this fix into the current dma_channel_abort function (and slowing down existing code), could it possibly be worth adding it as a new dma_channel_abort_safe or dma_channel_abort_no_irq or something? (obviously with appropriate doxygen comments!)

@ZodiusInfuser
Copy link
Contributor

I assume there is a use case for DMA that does not involve irq, hence this slowdown concern?

@kilograham
Copy link
Contributor

I assume there is a use case for DMA that does not involve irq, hence this slowdown concern?

yes, and also you don't really know which IRQs to disable or indeed which core it is enabled on

@kilograham
Copy link
Contributor

closing in favor of #816

@kilograham kilograham closed this May 9, 2022
@kilograham kilograham deleted the dma-abort-fix branch May 17, 2022 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants