Bug in irq_remove_handler() and/or irq_add_shared_handler()? #823

New issue

Closed

opened 2022-05-12 07:50:47 -05:00 by bruelltuete · 3 comments

bruelltuete commented

2022-05-12 07:50:47 -05:00

(Migrated from github.com)

The code looks equivalent to:

irq_add_shared_handler(DMA_IRQ_0, handler1, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
// do stuff with dma
irq_remove_handler(DMA_IRQ_0, handler1);
irq_remove_handler(DMA_IRQ_0, handler2);
// sleep or do something unrelated
irq_add_shared_handler(DMA_IRQ_0, handler3 /*!!!*/, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY);
// do stuff to trigger dma

At this point handler3 is nowhere to be found in irq_handler_chain_slots. And handler2 is looping on itself.

I'm still trying to figure out whats going on but the shared handler code looks really complicated and its hard to tell when something goes wrong. Here is my symptom: I have a few shared DMA handlers, all on the same DMA IRQ. After a round of add/remove-shared-handler one of those handlers is forever called in an endless loop, without any DMA interrupt pending. Looks like one of the shared handlers is just returning to itself over and over again. After poking around it bit, I've come to the conclusion that the contents of `irq_handler_chain_slots` look fishy. The code looks equivalent to: ``` irq_add_shared_handler(DMA_IRQ_0, handler1, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY); irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY); // do stuff with dma irq_remove_handler(DMA_IRQ_0, handler1); irq_remove_handler(DMA_IRQ_0, handler2); // sleep or do something unrelated irq_add_shared_handler(DMA_IRQ_0, handler3 /*!!!*/, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY); irq_add_shared_handler(DMA_IRQ_0, handler2, PICO_SHARED_IRQ_HANDLER_DEFAULT_ORDER_PRIORITY); // do stuff to trigger dma ``` At this point `handler3` is nowhere to be found in `irq_handler_chain_slots`. And `handler2` is looping on itself.

kilograham commented

2022-05-12 16:46:25 -05:00

(Migrated from github.com)

I'm still trying to figure out whats going on but the shared handler code looks really complicated and its hard to tell when something goes wrong.

Fair; the code was written that way in attempt to minimize the amount of runtime overhead vs raw IRQs, but also with a view to not spending too much memory on tracking lists of handlers, and finally of course avoiding any locking in the IRQ path (particularly with respect to removing an IRQ handler during itself which seemed like a useful feature)

Here is my symptom: I have a few shared DMA handlers, all on the same DMA IRQ. After a round of add/remove-shared-handler one of those handlers is forever called in an endless loop, without any DMA interrupt pending. Looks like one of the shared handlers is just returning to itself over and over again.
After poking around it bit, I've come to the conclusion that the contents of irq_handler_chain_slots look fishy.

Yes, there is a bug - thanks for spotting it. Dynamically adding/removing handlers is evidently not something people do much, and our spot testing of it went down a happy path!. I will fix and add an actual test case!

> I'm still trying to figure out whats going on but the shared handler code looks really complicated and its hard to tell when something goes wrong. Fair; the code was written that way in attempt to minimize the amount of runtime overhead vs raw IRQs, but also with a view to not spending too much memory on tracking lists of handlers, and finally of course avoiding any locking in the IRQ path (particularly with respect to removing an IRQ handler during itself which seemed like a useful feature) > Here is my symptom: I have a few shared DMA handlers, all on the same DMA IRQ. After a round of add/remove-shared-handler one of those handlers is forever called in an endless loop, without any DMA interrupt pending. Looks like one of the shared handlers is just returning to itself over and over again. After poking around it bit, I've come to the conclusion that the contents of irq_handler_chain_slots look fishy. Yes, there is a bug - thanks for spotting it. Dynamically adding/removing handlers is evidently not something people do much, and our spot testing of it went down a happy path!. I will fix and add an actual test case!

bruelltuete commented

2022-05-14 11:43:51 -05:00

(Migrated from github.com)