rp2: Disable DMA IRQ before clearing handler function. - #18914

@pdw-mb @agatti Thanks for the very clear bug report and the stack trace, made this very easy to fix.

@agatti One slight niggle, your stack trace in the issue shows a crash happening earlier than mine did - and without the tell-tale unhandled_user_irq_num_in_r0 in the trace. If you have time, could you please try and reproduce the crash you saw with this fix?

Code size report:

Reference:  README: Add a section describing MicroPython's values. [4625f97]
Comparison: rp2: Disable DMA IRQ before clearing handler function. [merge of 08ffbd3]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
      esp32:    +0 +0.000% ESP32_GENERIC
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +8 +0.001% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

FWIW I'm not exactly sure why the DMA IRQ triggers at this point, because dma_channel_set_irq0_enabled(channel, false); is called before dma_channel_unclaim(channel); so you'd think no IRQ should trigger from this channel... but it does appear to, anyway.

Thanks for looking into this.

I verified the fix in this PR works on a RPI_PICO_W. And the fix here does seem like a very sensible thing to do even regardless of it fixing a known bug.

I also found another patch which seems to fix it:

--- a/ports/rp2/rp2_dma.c
+++ b/ports/rp2/rp2_dma.c
@@ -427,6 +427,8 @@ static mp_obj_t rp2_dma_close(mp_obj_t self_in) {
     uint8_t channel = self->channel;

     if (channel != CHANNEL_CLOSED) {
+        dma_channel_set_irq0_enabled(channel, false);
+
         // Reset this channel's registers to their default values (zeros).
         dma_channel_config config = { .ctrl = 0 };
         dma_channel_configure(channel, &config, NULL, NULL, 0, false);
@@ -441,7 +443,6 @@ static mp_obj_t rp2_dma_close(mp_obj_t self_in) {
         if (irq) {
             irq->parent = MP_OBJ_NULL;
             irq->handler = MP_OBJ_NULL;
-            dma_channel_set_irq0_enabled(channel, false);
         }
         dma_channel_unclaim(channel);
         self->channel = CHANNEL_CLOSED;

and that patch also arguably makes sense regardless of it fixing any known bugs (ie disable the IRQ before resetting and aborting the DMA).

FWIW I'm not exactly sure why the DMA IRQ triggers at this point, because dma_channel_set_irq0_enabled(channel, false); is called before dma_channel_unclaim(channel); so you'd think no IRQ should trigger from this channel... but it does appear to, anyway.

The patch I show just above maybe gives a hint here: the call to dma_channel_abort() may be triggering an IRQ. I think in @agatti 's stack trace that function is the one which crashes/hangs.

Yet another idea I had was to run rp2_dma_deinit() after gc_sweep_all() (because you probably want to shut down the subsystems after running finalisers). That also seems to fix the original bug report.

What do you think is the best way to fix this? Maybe applying this PR and my patch above?

What do you think is the best way to fix this? Maybe applying this PR and my patch above?

Sounds good. I've done this and re-ran the rp2_dma.py unit test on a RPI_PICO board.

@agatti One slight niggle, your stack trace in the issue shows a crash happening earlier than mine did - and without the tell-tale unhandled_user_irq_num_in_r0 in the trace. If you have time, could you please try and reproduce the crash you saw with this fix?

For what it's worth, your earlier patch did fix the issue in the same scenario (debug build, swd debugger attached) :)