defmt_rtt can panic on multicore

[jannic]

The code at

defmt/firmware/defmt-rtt/src/lib.rs

Lines 50 to 52 in 964c8e7

	if TAKEN.load(Ordering::Relaxed) {
	panic!("defmt logger taken reentrantly")
	}

is not multi-core safe: If two cores both try to acquire the lock, the second one will panic.

[Dirbaio]

defmt-rtt assumes singlecore, it uses interrupt::disable() like cortex_m::interrupt::free().

You'd have to change it to use a custom critical section for your chip (RP2040 I guess?). I have a version of defmt-rtt that uses critical-section here, together with the impl here rp-rs/rp-hal#151 it should work.

Perhaps changing defmt-rtt to use critical-section would be a welcome addition to upstream defmt-rtt?

[jannic]

Ok, that makes sense. However I wonder why TAKEN is an atomic, if it's only used on single core while interrupts are disabled.

Yes, my use case is rp2040, how did you guess? :-)

[jonathanpallant]

Can defmt be generic over some type T: DefmtLock? You can have a default implementation for systems with atomic values, and a user can substitute in an implementation that uses a stronger lock (e.g. RP2040 has spin-locks) without having to fork the code.

[jannic]

If defmt-rtt used critical-section like in the version @Dirbaio mentioned, individual firmwares could replace the actual implementation using the custom-impl feature of critical-section. It would not be necessary for defmt-rtt to directly support different implementations.
Not sure if the additional dependency is justified, though.

[Dirbaio]

@jannic Ok, that makes sense. However I wonder why TAKEN is an atomic, if it's only used on single core while interrupts are disabled.

I think it could be a static mut as well, but relaxed atomics require less unsafe and have no extra overhead.

@jonathanpallant Can defmt be generic over some type T: DefmtLock?

Unfortunately the Logger is global, it can't have generics. defmt-rtt could offer some macro for you to set your own lock impl, but then the UX is the same as critical-section.

[jannic]

Fixed by #640