Hi KR,
your patch has been submitted
https://github.com/apache/nuttx/pull/16498
Best regards
Alin
________________________________
Från: kr....@kerogit.eu <kr....@kerogit.eu>
Skickat: den 5 juni 2025 16:44
Till: dev@nuttx.apache.org <dev@nuttx.apache.org>
Ämne: RFC: fix of atomic functions on AVR
Hello, I would like to submit a patch fixing atomic functions defined in
arch_atomic. c when compiled for AVR When trying to fix a race condition in the
serial driver (PR #16466) using __atomic_load_N, I noticed that the generated
code was incorrect
Hello,
I would like to submit a patch fixing atomic functions defined in
arch_atomic.c when compiled for AVR
When trying to fix a race condition in the serial driver (PR #16466)
using __atomic_load_N, I noticed that the generated code was incorrect
for AVR architecture. The disassembly looked like this (for
__atomic_load_4):
in r18, 0x3f ; store interrupts enabled flag
cli ; disable interrupts
out 0x3f, r18 ; restore the flag
movw r30, r24 ; copy parameter (address) to pointer register
ld r22, Z ; indirect load to return value registers
ldd r23, Z+1
ldd r24, Z+2
ldd r25, Z+3
ret ; return
As can be seen, the interrupts are disabled to be immediately
re-enabled, the load only takes place after that and is not atomic.
After some examination I found that for AVR, the LOAD macro surrounds
the load with calls to up_irq_save and up_irq_restore. I compared these
with their counterparts from other architectures (x86/486, Risc-V) and
discovered that the inline assembly is always marked with clobbers:
memory.
Doing the same thing for AVR fixed the problem:
in r18, 0x3f ; store interrupts enabled flag
cli ; disable interrupts
movw r30, r24 ; copy address
ld r22, Z ; load
ldd r23, Z+1
ldd r24, Z+2
ldd r25, Z+3
out 0x3f, r18 ; restore interrupts enabled flag
ret ; return
The other option I considered but did not do in the end was a change
like this:
diff --git a/libs/libc/machine/arch_atomic.c
b/libs/libc/machine/arch_atomic.c
index 87d68008d5..e5e213963f 100644
--- a/libs/libc/machine/arch_atomic.c
+++ b/libs/libc/machine/arch_atomic.c
@@ -60,7 +60,7 @@ static spinlock_t g_atomic_lock = SP_UNLOCKED;
{ \
irqstate_t irqstate = spin_lock_irqsave_notrace(&g_atomic_lock); \
\
- type ret = *(FAR type *)ptr; \
+ type ret = *(FAR volatile type *)ptr; \
\
spin_unlock_irqrestore_notrace(&g_atomic_lock, irqstate); \
return ret; \
I decided against this solution because I do not know if losing the
volatile property of the pointer parameter before the assignment is done
on purpose here and this change would affect other architectures.
Additionally - judging from that the compiled code changed in more
aspects than only in the __atomic_load_4, I suspect there were other
similar errors hidden in the compiled code.
Aside from this change, the patch also marks up_irq_enable the same way.
Moreover, functions up_irq_disabled and putsreg are removed, they are
not used in any part of the tree.
The changes was tested by comparing the assembly and by simple stress
application.
The patch is attached to this message and also available in a git
repository nuttx.git at git.kerogit.eu accessible through HTTP/S.
(Trying to prevent bot traffic by not posting the URL in
machine-readable form.) The relevant branch is called avr_fixes3_v1. If
the change is acceptable, I would like to ask someone with GitHub
account to open a PR (I don't have a GH account.) Any comments or
suggestions are welcome.
