As it turns out, the RP2350 has hardware spinlocks that can be unlocked by writes to nearby addresses, the lower spinlocks currently in use in TinyGo happen to be unlocked by writes to the doorbell interrupt registers used to signal between cores, very possibly leading to some unexpected unlocks. This was not corrected in the A3 or A4 steppings and instead software spinlocks are used by default on RP2350 in pico-sdk:

RP2350 Warning. Due to erratum RP2350-E2, writes to new SIO registers above an offset of +0x180 alias the spinlocks, causing spurious lock releases. This SDK by default use atomic memory accesses to implement the hardware_sync_spin_lock API, as a workaround on RP2350 A2.

https://www.raspberrypi.com/documentation/pico-sdk/hardware.html#group_hardware_sync

Another thing I noticed in that section, they also always disable interrupts when the spinlocks are being held, we may want to do the same:

[...] the default spinlock related methods here (e.g.spin_lock_blocking) always disable interrupts while the lock is held as use by IRQ handlers and user code is common/desirable, and spin locks are only expected to be held for brief periods.

These are the software spinlock macros ported over:
https://github.com/raspberrypi/pico-sdk/blob/2.2.0/src/rp2_common/hardware_sync_spin_lock/include/hardware/sync/spin_lock.h#L112
https://github.com/raspberrypi/pico-sdk/blob/2.2.0/src/rp2_common/hardware_sync_spin_lock/include/hardware/sync/spin_lock.h#L197

Copy link

ContributorAuthor

mikesmitty commentedSep 10, 2025

Oh, whoops. My go fmt extension has been flaking out on me. Will have the missing rp2040 imports updated in a moment. Here's the lock/unlock disassembled output with inlining disabled:

   0x10001178 <(*runtime.spinLock).Lock+0>:     cbz     r0, 0x10001192 <(*runtime.spinLock).Lock+26>   0x1000117a <(*runtime.spinLock).Lock+2>:     ldaexb  r2, [r0]   0x1000117e <(*runtime.spinLock).Lock+6>:     movs    r1, #1   0x10001180 <(*runtime.spinLock).Lock+8>:     cmp     r2, #0   0x10001182 <(*runtime.spinLock).Lock+10>:    bne.n   0x1000117a <(*runtime.spinLock).Lock+2>   0x10001184 <(*runtime.spinLock).Lock+12>:    strexb  r2, r1, [r0]   0x10001188 <(*runtime.spinLock).Lock+16>:    cmp     r2, #0   0x1000118a <(*runtime.spinLock).Lock+18>:    bne.n   0x1000117a <(*runtime.spinLock).Lock+2>   0x1000118c <(*runtime.spinLock).Lock+20>:    dmb     sy   0x10001190 <(*runtime.spinLock).Lock+24>:    bx      lr   0x10001192 <(*runtime.spinLock).Lock+26>:    bl      0x100013f4 <runtime.nilPanic>

   0x100013e4 <(*runtime.spinLock).Unlock+0>:   cbz     r0, 0x100013ee <(*runtime.spinLock).Unlock+10>   0x100013e6 <(*runtime.spinLock).Unlock+2>:   movs    r1, #0   0x100013e8 <(*runtime.spinLock).Unlock+4>:   stlb    r1, [r0]   0x100013ec <(*runtime.spinLock).Unlock+8>:   bx      lr   0x100013ee <(*runtime.spinLock).Unlock+10>:  bl      0x100013f4 <runtime.nilPanic>

eliasnaur reviewed

Sep 10, 2025

View reviewed changes

Copy link

Contributor

eliasnaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I support the switch to atomic instructions, but if you need something that works right away, RP2350-E2 mentions that some spinlocks are not affected:

The following SIO spinlocks can be used normally because they don’t alias with writable registers: 5, 6, 7,
10, 11, and 18 through 31. Some of the other lock addresses may be used safely depending on which of
the high-addressed SIO registers are in use.
Locks 18 through 24 alias with some read-only TMDS encoder registers, which is safe as only writes are
mis-decoded.

src/runtime/runtime_rp2350.go OutdatedShow resolvedHide resolved

aykevl requested changes

Sep 10, 2025

View reviewed changes

Copy link

Member

aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

See my comment below.

Also,

Another thing I noticed in that section, they also always disable interrupts when the spinlocks are being held, we may want to do the same: [...]

The runtime does this in various places if needed. The spinlock implementation doesn't need to disable interrupts too.

src/runtime/runtime_rp2350.go OutdatedShow resolvedHide resolved

aykevl reviewed

Sep 10, 2025

View reviewed changes

src/runtime/runtime_rp2350.go OutdatedShow resolvedHide resolved

eliasnaur reviewed

Sep 10, 2025

View reviewed changes

src/runtime/runtime_rp2350.go OutdatedShow resolvedHide resolved

Copy link

ContributorAuthor

mikesmitty commentedSep 10, 2025

I support the switch to atomic instructions, but if you need something that works right away, RP2350-E2 mentions that some spinlocks are not affected:
The following SIO spinlocks can be used normally because they don’t alias with writable registers: 5, 6, 7,
10, 11, and 18 through 31. Some of the other lock addresses may be used safely depending on which of
the high-addressed SIO registers are in use.
Locks 18 through 24 alias with some read-only TMDS encoder registers, which is safe as only writes are
mis-decoded.

Thank you for tracking those down. I figured there would probably be some, but I couldn't find them

mikesmitty requested review fromaykevl andeliasnaur

September 10, 2025 23:24

eliasnaur approved these changes

Sep 17, 2025

View reviewed changes

Copy link

Contributor

eliasnaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Other than a nit, this looks good to me.@aykevl WDYT?

src/runtime/runtime_rp2.go Outdated

		printLock=spinLock{id:0}
		schedulerLock=spinLock{id:1}
		atomicsLock=spinLock{id:2}
		futexLock=spinLock{id:3}

Copy link

Contributor

eliasnaurSep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

id is an implementation detail of rp2040 spinlocks, whereas on rp2350ids have some meaning but are unusued. I suggest moving the spinlock variables to the rpXXXX.go files and avoid theid field on rp2350.

src/runtime/runtime_rp2350.go Outdated


		typespinLockstruct {
		atomic.Uint32
		iduint8

Copy link

Contributor

eliasnaurSep 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Delete field.

Copy link

ContributorAuthor

mikesmittySep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

That one's still needed for compatibility with how the rp2040 hardware spinlocks are initialized here:

tinygo/src/runtime/runtime_rp2.go

Lines 295 to 298 in109e076

	printLock=spinLock{id:20}
	schedulerLock=spinLock{id:21}
	atomicsLock=spinLock{id:22}
	futexLock=spinLock{id:23}

Copy link

Contributor

eliasnaurSep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Indeed. Above, I suggested moving thatvar () block to theruntime_rp2xxx.go files to get rid of that dependency.

Copy link

ContributorAuthor

mikesmittySep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah yep, I missed that

Copy link

ContributorAuthor

mikesmitty commentedSep 18, 2025

After doing some testing, I think this might not actually be locking properly. Going to do some more thorough testing

Copy link

Member

aykevl commentedSep 23, 2025•
edited
Loading

Yes, this looks good.@mikesmitty can you apply the changes proposed by@eliasnaur?

Also, you may want to rebase on the dev branch to hopefully fix the assert-test-linux CI failure.

mikesmitty added6 commits

September 24, 2025 08:39

fix(rp2350): add software spinlocks

d1eca84

chore: clarify comments

fc81160

chore: use atomics instead of artisinal assembly

31696c2

chore: comment cleanup

09fbdc0

fix: add id field for hw spinlock compatibility

ab8bf89

chore: switch to cgo inline asm

cee4537

mikesmitty force-pushed thems/rp2350-sw-mutex branch from1f2642c tocee4537Compare

September 24, 2025 12:43

Copy link

ContributorAuthor

mikesmitty commentedSep 24, 2025

Sure, here's the rebase. I haven't had time to test or diagnose it yet, been stuck working on a project that's consumed all my free time, but I'm pretty sure it's not functioning properly as confirmed by that CI failure

Copy link

ContributorAuthor

mikesmitty commentedSep 24, 2025

Hmm, no I guess it is actually locking. I'm not sure what's happening with that CI test though

aykevl changed the base branch fromrelease todev

September 29, 2025 12:59

aykevl approved these changes

Sep 29, 2025

View reviewed changes

Copy link

Member

aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM, thank you!

Copy link

Member

aykevl commentedSep 29, 2025

Restarting CI, these are probably flaky tests.

eliasnaur reviewed

Sep 29, 2025

View reviewed changes

src/runtime/runtime_rp2350.go


		// #include <stdint.h>
		//
		// static void spinlock_lock(void *lock) {

Copy link

Contributor

eliasnaurSep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What was wrong with thesync/atomic operations? Custom assembly for just one target (rp2350) seems to me like a maintenance headache for little gain.

Copy link

ContributorAuthor

mikesmittyOct 3, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I forget, but I'll take a second look at them. One reason was the size of them, but I think there was another reason also. In hindsight, the size difference is fairly small all things considered

Copy link

ContributorAuthor

mikesmitty commentedOct 3, 2025•
edited
Loading

Ok, finally coming back to take a look at this again. I still have some reservations about the stability of this one. I'm getting mysterious panics in various runtime locations that I haven't tracked down yet. I suspect some race conditions are going on that I haven't narrowed down yet

For example:

[2025-10-03T13:28:06.865Z, +005191ms] panipanic: runtime  ee[2025-10-03T13:28:06.877Z, +005203ms] rrrroorr  aatt  0x100036ed0x10003a5d: unsafe.Slice/String: len out of range

aykevl requested changes

Oct 3, 2025

View reviewed changes

Copy link

Member

aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just realized there are some issues with the assembly code.

src/runtime/runtime_rp2350.go

		// "cmp %1, #0\n" \
		// /* Claim failed due to intervening write, so retry */ \
		// "bne 1b\n" \
		// : "=&r" (_tmp0), "=&r" (_tmp1) : "r" (lock) \

Copy link

Member

aykevlOct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Just realized this is incomplete. You also need to clobber flags and memory at least.

src/runtime/runtime_rp2350.go

		// uint32_t zero = 0; \
		// __asm volatile ( \
		// "stlb %0, [%1]\n" \
		// : : "r" (zero), "r" (lock) \

Copy link

Member

aykevlOct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Same here. It modifies memory, though I don't think it updates any flags.

Labels

None yet

Movatterモバイル変換

fix(rp2350): add software spinlocks#5034

Are you sure you want to change the base?

fix(rp2350): add software spinlocks#5034

Uh oh!

Conversation

mikesmitty commentedSep 10, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mikesmitty commentedSep 10, 2025

Uh oh!

eliasnaur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mikesmitty commentedSep 10, 2025

Uh oh!

eliasnaur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesmitty commentedSep 18, 2025

Uh oh!

aykevl commentedSep 23, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mikesmitty commentedSep 24, 2025

Uh oh!

mikesmitty commentedSep 24, 2025

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

aykevl commentedSep 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesmittyOct 3, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesmitty commentedOct 3, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

aykevl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mikesmitty commentedSep 10, 2025•
edited
Loading

aykevl commentedSep 23, 2025•
edited
Loading

mikesmittyOct 3, 2025•
edited
Loading

mikesmitty commentedOct 3, 2025•
edited
Loading