detachInterrupt() from within ISR (freeRTOS) runs into Mutex

[caveman99]

We are using Radiolib in Meshtastic. While porting the existing code (that runs fine on ESP32* an nRF52) to RP2040 we're running into a mutex deadlock when calling detachInterrupt through the Radiolib clearDio1Action() from a running ISR. This seems to be a similar issue as #878

There is more comments and a debug trace in meshtastic/firmware#2299

The calling code is as simple as

void INTERRUPT_ATTR RadioLibInterface::isrLevel0Common(PendingISR cause)
{
    instance->disableInterrupt(); // this calls lora.clearDio1Action()

    BaseType_t xHigherPriorityTaskWoken;
    instance->notifyFromISR(&xHigherPriorityTaskWoken, cause, true);

    /* Force a context switch if xHigherPriorityTaskWoken is now set to pdTRUE.
    The macro used to do this is dependent on the port and may be called
    portEND_SWITCHING_ISR. */
    YIELD_FROM_ISR(xHigherPriorityTaskWoken);
}

[maxgerhardt]

Hmm

arduino-pico/cores/rp2040/wiring_private.cpp

Lines 145 to 154 in 6e52b72

	extern "C" void detachInterrupt(pin_size_t pin) {
	CoreMutex m(&_irqMutex);
	if (!m) {
	return;
	}
	noInterrupts();
	_detachInterruptInternal(pin);
	interrupts();
	}

You think the right path here would to check if we're inside an interrupt when detachInterrupt() is called and then not take the mutex?

[caveman99]

a speculative fix i came up with was adding portCHECK_IF_IN_ISR() to the CoreMutex.cpp so it would call __freertos_mutex_take_from_isr(m); instead of the vanilla version. I am not sure just skipping the mutex at all is safe.

Problem with that approach is, i don't know how the include system in the framework is supposed to work, so i shoehorned relative path includes to FreeRTOSConfig.h and portmacro.h to the CoreMutex.cpp file, and i am positive that's not how it's supposed to work :-)

[earlephilhower]

@caveman99 your suggestion sounds good. Do you have a PR you'd like to throw out there?

The mutex can't be skipped because the underlying call uses a 32b bitmask which is a 3-cycle read/modify/write operation. So you can't safely clear or set a bit w/o it. Removing the mutex would require turning the 4-byte value into an array of 32 ints taking 128 bytes.

[earlephilhower]

So I just read the 2nd part of your comment, @caveman99 . The FreeRTOS includes in the core are kind of obtuse since we don't use FreeRTOS and its in a library.

I've done a proposed change implementing what you said. Could you give #1442 a try and report back, or offer suggestions if it's not what you had done?

[caveman99]

Thanks, that was exactly what i had in mind. I will test asap, was tied up today...

[earlephilhower]

If you pulled #1442 earlier, please re-pull the PR and try again. I forgot to handle the mutex_give_from_isr() portion in the destructor. That's been cleaned up, along with the manual special case I did for GPIO IRQ handling (taken care of by the real API am-i-in-isr call).

[earlephilhower]

Ping @caveman99 . Any update?

[caveman99]

i'm sorry, i have a surprise workload this week. will get to this asap monday or tuesday.

[GUVWAF]

I see that you merged the PR already, but I wanted to let you know that I just tested the Meshtastic firmware with this and it doesn't run into a deadlock upon receiving anymore. Thank you very much for the quick implementation! Looks like Meshtastic has another supported board :)

[earlephilhower]

Awesome, thanks for the feedback! Normally I'd wait for feedback, but the problem and solution were pretty obvious here so I' figured I'd take a shot to get out a new release this week.