Skip to content

RP2040: avoid XIP hangs during flash operations with scheduler=cores - #5411

Open
rdon-key wants to merge 7 commits into
tinygo-org:devfrom
rdon-key:rp2040-flashsafe-section
Open

rdon-key wants to merge 7 commits into
tinygo-org:devfrom
rdon-key:rp2040-flashsafe-section

Conversation

@rdon-key

@rdon-key rdon-key commented May 20, 2026

Copy link
Copy Markdown
Contributor

Fixes #5288

RP2040 flash program / erase / command operations temporarily disable XIP.
With scheduler=cores, the other core may continue executing instructions from XIP flash during that window, which can cause a system hang.

What was done

This change adds an RP2040-specific flash-safe section.

Runtime changes:

  • For scheduler=cores, send a SIO FIFO command to the other core before starting a flash operation.
  • The interrupted core enters a RAM-resident flash-safe handler and waits there until the flash operation is complete.
  • The core performing the flash operation waits until the other core has entered the handler before disabling XIP.
  • For non-cores schedulers, keep the existing local interrupt disable / restore behavior.
  • Add a build-only RP2350 stub handler so shared RP2 runtime code continues to compile without changing RP2350 behavior.

Machine changes:

  • Wrap flash_range_write
  • Wrap flash_erase_blocks
  • Wrap flash_do_cmd

These flash operations now run inside rp2040EnterFlashSafeSection / rp2040ExitFlashSafeSection.

Notes

This is intentionally limited to RP2040 flash operations that temporarily disable XIP.
It is not intended to be a general multicore lock.

Other shared peripherals should be protected by their own ownership or locking rules.

RP2350 behavior is intentionally left unchanged.
The RP2350 handler added here is only a build-only stub for shared RP2 runtime code.

If the monitor output becomes corrupted under scheduler=cores, for example:

sta100
multi-core scheduler:
slected flash : 0
seedflashs0
run0oe: at
p e
ease re

please also test this reproducer with pull request #5391:

https://github.com/tinygo-org/tinygo/pull/5391

That output corruption appears to be a separate USB CDC multicore output issue, and it can make the flash test result difficult to read.

Reproducer

Warning: Flash memory has a limited number of program/erase cycles. This reproducer repeatedly erases and writes flash, so run it only when needed.

main.go
//go:build tinygo && rp2040

package main

import (
	"machine"
	"runtime"
	"sync/atomic"
	"time"
	"unsafe"
)

const maxRounds uint32 = 100
const writeSize = 4096

const sioCPUID = uintptr(0xd0000000)

func coreID() uint32 {
	return *(*uint32)(unsafe.Pointer(sioCPUID))
}

func alignDown(v uintptr, align uintptr) uintptr {
	return v & ^(align - 1)
}

func roundUp(v int64, align int64) int64 {
	return (v + align - 1) &^ (align - 1)
}

func safeFlashOffset(round uint32) int64 {
	start := uintptr(machine.FlashDataStart())
	end := uintptr(machine.FlashDataEnd())
	eraseSize := uintptr(machine.Flash.EraseBlockSize())
	testSpan := uintptr(roundUp(writeSize, int64(eraseSize)))

	if eraseSize == 0 || eraseSize&(eraseSize-1) != 0 {
		println("invalid erase block size")
		for {
		}
	}

	if end <= start || end-start < testSpan {
		println("not enough writable flash data area")
		for {
		}
	}

	// Compute the number of usable erase-aligned regions and rotate the offset
	// per round so that we don't repeatedly erase/program the same sector.
	// main rotates DOWN from the top of the data region.
	lastAligned := alignDown(end-testSpan, eraseSize)
	available := (lastAligned-start)/eraseSize + 1

	rotated := uintptr(round) % available
	absOffset := lastAligned - rotated*eraseSize
	relOffset := absOffset - start

	println("selected flash abs offset:", absOffset)
	println("selected flash rel offset:", relOffset)

	return int64(relOffset)
}

var writeBuf [writeSize]byte
var readBuf [writeSize]byte

// Worker observability.
//   workerDone:   0 = continue, 1 = stop requested, 2 = stopped
//   workerCore:   99 = not started, otherwise the core id worker is running on
//   workerWrites: number of successful flash writes performed by the worker
var (
	workerDone   uint32
	workerCore   uint32 = 99
	workerWrites uint32
)

// Worker writes to a fixed low offset, well separated from main's rotating
// range (main covers the top sectors of the data region). The two never write
// to the same sector during a single test run.
const workerWriteOffset = int64(0)

var workerWriteBuf [writeSize]byte

// Exactly 256 bytes: 4 lines x 64 chars.
const chunk256 = "" +
	"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +
	"fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210" +
	"00112233445566778899aabbccddeeffffeeddccbbaa99887766554433221100" +
	"rp2040xipcacheflashworkerrandomaccesstestdataAAAAAAAAAAAAAAAAAAA"

// 4096 bytes (4 KB).
const block4k = chunk256 + chunk256 + chunk256 + chunk256 +
	chunk256 + chunk256 + chunk256 + chunk256 +
	chunk256 + chunk256 + chunk256 + chunk256 +
	chunk256 + chunk256 + chunk256 + chunk256

// 65536 bytes (64 KB). Large enough to overflow the 16 KB XIP cache on RP2040.
const flashData = block4k + block4k + block4k + block4k +
	block4k + block4k + block4k + block4k +
	block4k + block4k + block4k + block4k +
	block4k + block4k + block4k + block4k

func fillWriteBuffer(round uint32) {
	seed := byte(0x5a ^ byte(round*37))
	for i := range writeBuf {
		writeBuf[i] = byte(i) ^ seed
	}
}

func readFlash(label string, off int64) {
	for i := range readBuf {
		readBuf[i] = 0
	}
	n, err := machine.Flash.ReadAt(readBuf[:], off)
	if err != nil {
		println(label, "readback error:", err.Error())
		for {
		}
	}
	if n != len(readBuf) {
		println(label, "readback length mismatch:", n)
		for {
		}
	}
}

func verifyWrite(round uint32) {
	for i := range readBuf {
		if readBuf[i] != writeBuf[i] {
			println("write readback mismatch round:", round, "index:", i, "got:", readBuf[i], "want:", writeBuf[i])
			for {
			}
		}
	}
	println("write readback ok")
}

func verifyErase(round uint32) {
	for i := range readBuf {
		if readBuf[i] != 0xff {
			println("erase readback mismatch round:", round, "index:", i, "got:", readBuf[i])
			for {
			}
		}
	}
	println("erase readback ok")
}

func eraseFlash(label string, off int64) {
	eraseSize := machine.Flash.EraseBlockSize()
	eraseStart := off / eraseSize
	eraseBlocks := roundUp(writeSize, eraseSize) / eraseSize

	println(label, "erase start")
	err := machine.Flash.EraseBlocks(eraseStart, eraseBlocks)
	if err != nil {
		println(label, "erase error:", err.Error())
		for {
		}
	}
	println(label, "erase done")
}

// worker exercises the system from the second core:
//   1. Continuous random reads from a 64 KB flash-resident const array.
//      This defeats the 16 KB XIP cache and produces sustained flash
//      instruction/data traffic, which is required to make the bug
//      observable on stock TinyGo.
//   2. Periodic flash writes to a fixed low offset. This forces the
//      worker and main to race for rp2040EnterFlashSafeSection, which
//      is what the flash-safe spinlock fix is supposed to serialize.
func worker() {
	core := coreID()
	atomic.StoreUint32(&workerCore, core)
	println("worker started on core:", core)

	led := machine.LED
	led.Configure(machine.PinConfig{Mode: machine.PinOutput})

	// Distinct pattern so the worker's flash content differs from main's.
	for i := range workerWriteBuf {
		workerWriteBuf[i] = byte(i) ^ 0xa5
	}

	eraseSize := machine.Flash.EraseBlockSize()

	x := uint32(0x12345678)
	sum := uint32(0)
	writes := uint32(0)

	for atomic.LoadUint32(&workerDone) == 0 {
		// Hammer the XIP cache with random reads.
		for n := 0; n < 8192; n++ {
			x = x*1664525 + 1013904223
			i := int(x % uint32(len(flashData)))
			sum += uint32(flashData[i])
		}
		led.Set((sum & 0x800000) != 0)

		// Periodically write to flash. (sum & 0xff) == 0 fires roughly
		// once every couple hundred outer iterations -> enough overlap
		// chance with main's flash ops without burning the sector too
		// hard.
		if (sum & 0xff) == 0 {
			if err := machine.Flash.EraseBlocks(workerWriteOffset/eraseSize, 1); err == nil {
				if _, err := machine.Flash.WriteAt(workerWriteBuf[:], workerWriteOffset); err == nil {
					writes++
					atomic.StoreUint32(&workerWrites, writes)
				}
			}
		}
	}

	led.Low()
	atomic.StoreUint32(&workerDone, 2)
}

func main() {
	time.Sleep(2 * time.Second)

	numCPU := runtime.NumCPU()
	workerStarted := numCPU > 1

	println("start")
	println("NumCPU:", numCPU)
	println("main core:", coreID())
	println("flashData size:", len(flashData))
	println("max rounds:", maxRounds)

	if workerStarted {
		println("multi-core scheduler: worker may run concurrently (with concurrent flash writes)")
	} else {
		println("single-core scheduler: worker will not be started")
	}

	// Sanity-check sizes at runtime as well.
	if len(chunk256) != 256 {
		println("WARN: chunk256 size unexpected:", len(chunk256))
	}
	if len(flashData) != 65536 {
		println("WARN: flashData size unexpected:", len(flashData))
	}

	for round := uint32(0); round < maxRounds; round++ {
		fillWriteBuffer(round)
	}

	if workerStarted {
		go worker()
	}

	for round := uint32(0); round < maxRounds; round++ {
		flashOffset := safeFlashOffset(round)
		fillWriteBuffer(round)

		println("round:", round,
			"main core:", coreID(),
			"worker core:", atomic.LoadUint32(&workerCore),
			"worker writes:", atomic.LoadUint32(&workerWrites),
		)

		eraseFlash("prepare", flashOffset)
		readFlash("prepare", flashOffset)
		verifyErase(round)

		println("write start")
		n, err := machine.Flash.WriteAt(writeBuf[:], flashOffset)
		if err != nil {
			println("write error:", err.Error())
			for {
			}
		}
		println("write done:", n)
		if n != len(writeBuf) {
			println("write length mismatch:", n)
			for {
			}
		}

		readFlash("write", flashOffset)
		verifyWrite(round)

		eraseFlash("final", flashOffset)
		readFlash("final", flashOffset)
		verifyErase(round)
	}

	if workerStarted {
		println("requesting worker stop")
		atomic.StoreUint32(&workerDone, 1)

		const maxSpin = 100_000_000
		spun := 0
		for atomic.LoadUint32(&workerDone) != 2 && spun < maxSpin {
			spun++
		}

		if atomic.LoadUint32(&workerDone) == 2 {
			println("worker stopped cleanly")
		} else {
			println("worker did not stop within bound")
		}

		println("worker core:", atomic.LoadUint32(&workerCore))
		println("worker writes:", atomic.LoadUint32(&workerWrites))
	} else {
		println("worker was not started")
	}

	println("test finished")

	// Pure busy loop: keep main pinned to its current core.
	for {
	}
}

Test results

Tested on RP2040/Pico.

With -scheduler=cores (the failing case before this PR)

Reproducer: a worker goroutine on core 1 performs continuous random reads
from a 64 KB flash-resident const slice (which defeats the 16 KB XIP cache)
and periodic flash writes to a fixed low offset. Main on core 0 does
100 rounds of erase / write / read-back / verify on the top sectors of the
data region.

Before this PR: hangs at the first write start on core 0.

After this PR:

$ tinygo flash -target=pico -scheduler=cores -monitor main.go
start
NumCPU: 2
main core: 0
flashData size: 65536
max rounds: 100
multi-core scheduler: worker may run concurrently (with concurrent flash writes)
selected flash abs offset: 0x101ff000
worker started on core: 1
selected flash rel offset: 0x001eb000
round: 0 main core: 0 worker core: 1 worker writes: 0
prepare erase start
prepare erase done
erase readback ok
write start
write done: 4096
write readback ok
final erase start
final erase done
erase readback ok
...
round: 50 main core: 0 worker core: 1 worker writes: 79
prepare erase start
prepare erase done
erase readback ok
write start
write done: 4096
write readback ok
final erase start
final erase done
erase readback ok
...
round: 99 main core: 0 worker core: 1 worker writes: 155
prepare erase start
prepare erase done
erase readback ok
write start
write done: 4096
write readback ok
final erase start
final erase done
erase readback ok
requesting worker stop
worker stopped cleanly
worker core: 1
worker writes: 156
test finished

Evidence:

  • worker started on core: 1 and worker core: 1 in every round header
    confirm the test condition (worker is actually running on core 1, not 0).
  • worker writes: N grows from 0 to 156, meaning the worker successfully
    performed 156 flash writes concurrently with main's flash ops. Each of
    those is a moment when both cores were contending for the flash-safe
    section, exercising the spinlock added in this PR.
  • All 100 rounds of main's erase / write / read-back complete without
    any mismatch or error, so the cross-core lockout protocol did not
    corrupt either core's data.
  • worker stopped cleanly confirms no deadlock at shutdown.

With -scheduler=tasks (non-regression check)

$ tinygo flash -target=pico -scheduler=tasks -monitor main.go
start
NumCPU: 1
main core: 0
flashData size: 65536
max rounds: 100
single-core scheduler: worker will not be started
selected flash abs offset: 0x101ff000
selected flash rel offset: 0x001fb000
round: 0 main core: 0 worker core: 99 worker writes: 0
prepare erase start
prepare erase done
erase readback ok
write start
write done: 4096
write readback ok
final erase start
final erase done
erase readback ok
...
round: 99 main core: 0 worker core: 99 worker writes: 0
prepare erase start
prepare erase done
erase readback ok
write start
write done: 4096
write readback ok
final erase start
final erase done
erase readback ok
worker was not started
test finished

Single-core builds go through runtime_rp2040_flashsafe_single.go, which
is a plain interrupt.Disable() / interrupt.Restore() pair. 100 rounds
complete with no regression. worker core: 99 is the sentinel value
meaning the worker goroutine was never started (NumCPU == 1).

Symbol placement (//go:section .ramfuncs)

$ llvm-nm test.elf | grep rp2FlashSafe
10003314 t __Thumbv6MABSLongThunk_runtime.rp2FlashSafeInterruptHandler
20001184 t runtime.rp2FlashSafeInterruptHandler

rp2FlashSafeInterruptHandler is placed at 0x20001184 (RP2040 SRAM),
not in the XIP-mapped flash region. The flash-side symbol is the long-branch
thunk LLVM auto-generates for Cortex-M0; it is fetched while XIP is still
enabled, so it is safe.

@rdon-key
rdon-key force-pushed the rp2040-flashsafe-section branch from 59911c7 to 4c44c4a Compare June 9, 2026 12:50
@rdon-key

rdon-key commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the latest dev after #5391 (usb/cdc: fix RP2 USB CDC TX race with cores scheduler) was merged.

Re-ran the RP2040 Pico reproducer:

  • scheduler=cores: 100 rounds completed without hang, mismatch, or deadlock
  • scheduler=tasks: 100 rounds completed without regression

interrupt.Restore(state)
}

func rp2FlashSafeInterruptHandler(core uint32) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You could add a comment here that it is a no-op on single core.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.

}

func rp2FlashSafeInterruptHandler(core uint32) {
_ = core

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you add the above comment you can remove this line.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.


package runtime

func rp2FlashSafeInterruptHandler(core uint32) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same feedback as above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, done.

@rdon-key

Copy link
Copy Markdown
Contributor Author

Thank you for your review. I addressed the comments.

Also, the println / monitor output appears to be affected by a separate issue, so I attached a reduced test case with fewer messages.

Details
//go:build tinygo && rp2040

package main

import (
	"machine"
	"runtime"
	"sync/atomic"
	"time"
	"unsafe"
)

const maxRounds uint32 = 100
const writeSize = 4096

// Keep hot-loop progress off USB CDC. This test uses GP14/GP15 as the
// primary progress indicator so USB CDC problems don't hide as flash hangs.
const usbProgressLog = false

const (
	debugPhaseIdle    uint8 = iota
	debugPhasePrepare       // GP14 on
	debugPhaseWrite         // GP15 on
	debugPhaseFinal         // GP14 + GP15 on
)

var (
	debugLED0 = machine.GPIO14
	debugLED1 = machine.GPIO15
)

func initDebugLEDs() {
	debugLED0.Configure(machine.PinConfig{Mode: machine.PinOutput})
	debugLED1.Configure(machine.PinConfig{Mode: machine.PinOutput})
	setDebugPhase(debugPhaseIdle)
}

func setDebugPhase(phase uint8) {
	debugLED0.Set((phase & 1) != 0)
	debugLED1.Set((phase & 2) != 0)
}

func blinkDebug(phase uint8, count int) {
	for i := 0; i < count; i++ {
		setDebugPhase(phase)
		time.Sleep(100 * time.Millisecond)
		setDebugPhase(debugPhaseIdle)
		time.Sleep(100 * time.Millisecond)
	}
}

const sioCPUID = uintptr(0xd0000000)

func coreID() uint32 {
	return *(*uint32)(unsafe.Pointer(sioCPUID))
}

func alignDown(v uintptr, align uintptr) uintptr {
	return v & ^(align - 1)
}

func roundUp(v int64, align int64) int64 {
	return (v + align - 1) &^ (align - 1)
}

func safeFlashOffset(round uint32) int64 {
	start := uintptr(machine.FlashDataStart())
	end := uintptr(machine.FlashDataEnd())
	eraseSize := uintptr(machine.Flash.EraseBlockSize())
	testSpan := uintptr(roundUp(writeSize, int64(eraseSize)))

	if eraseSize == 0 || eraseSize&(eraseSize-1) != 0 {
		println("invalid erase block size")
		for {
		}
	}

	if end <= start || end-start < testSpan {
		println("not enough writable flash data area")
		for {
		}
	}

	// Compute the number of usable erase-aligned regions and rotate the offset
	// per round so that we don't repeatedly erase/program the same sector.
	// main rotates DOWN from the top of the data region.
	lastAligned := alignDown(end-testSpan, eraseSize)
	available := (lastAligned-start)/eraseSize + 1

	rotated := uintptr(round) % available
	absOffset := lastAligned - rotated*eraseSize
	relOffset := absOffset - start

	if usbProgressLog {
		println("selected flash abs offset:", absOffset)
		println("selected flash rel offset:", relOffset)
	}

	return int64(relOffset)
}

var writeBuf [writeSize]byte
var readBuf [writeSize]byte

// Worker observability.
//
//	workerDone:   0 = continue, 1 = stop requested, 2 = stopped
//	workerCore:   99 = not started, otherwise the core id worker is running on
//	workerWrites: number of successful flash writes performed by the worker
var (
	workerDone   uint32
	workerCore   uint32 = 99
	workerWrites uint32
)

// Worker writes to a fixed low offset, well separated from main's rotating
// range (main covers the top sectors of the data region). The two never write
// to the same sector during a single test run.
const workerWriteOffset = int64(0)

var workerWriteBuf [writeSize]byte

// Exactly 256 bytes: 4 lines x 64 chars.
const chunk256 = "" +
	"0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +
	"fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210" +
	"00112233445566778899aabbccddeeffffeeddccbbaa99887766554433221100" +
	"rp2040xipcacheflashworkerrandomaccesstestdataAAAAAAAAAAAAAAAAAAA"

// 4096 bytes (4 KB).
const block4k = chunk256 + chunk256 + chunk256 + chunk256 +
	chunk256 + chunk256 + chunk256 + chunk256 +
	chunk256 + chunk256 + chunk256 + chunk256 +
	chunk256 + chunk256 + chunk256 + chunk256

// 65536 bytes (64 KB). Large enough to overflow the 16 KB XIP cache on RP2040.
const flashData = block4k + block4k + block4k + block4k +
	block4k + block4k + block4k + block4k +
	block4k + block4k + block4k + block4k +
	block4k + block4k + block4k + block4k

func fillWriteBuffer(round uint32) {
	seed := byte(0x5a ^ byte(round*37))
	for i := range writeBuf {
		writeBuf[i] = byte(i) ^ seed
	}
}

func readFlash(label string, off int64) {
	for i := range readBuf {
		readBuf[i] = 0
	}
	n, err := machine.Flash.ReadAt(readBuf[:], off)
	if err != nil {
		println(label, "readback error:", err.Error())
		for {
		}
	}
	if n != len(readBuf) {
		println(label, "readback length mismatch:", n)
		for {
		}
	}
}

func verifyWrite(round uint32) {
	for i := range readBuf {
		if readBuf[i] != writeBuf[i] {
			println("write readback mismatch round:", round, "index:", i, "got:", readBuf[i], "want:", writeBuf[i])
			for {
			}
		}
	}
	if usbProgressLog {
		println("write readback ok")
	}
}

func verifyErase(round uint32) {
	for i := range readBuf {
		if readBuf[i] != 0xff {
			println("erase readback mismatch round:", round, "index:", i, "got:", readBuf[i])
			for {
			}
		}
	}
	if usbProgressLog {
		println("erase readback ok")
	}
}

func eraseFlash(label string, off int64) {
	eraseSize := machine.Flash.EraseBlockSize()
	eraseStart := off / eraseSize
	eraseBlocks := roundUp(writeSize, eraseSize) / eraseSize

	if usbProgressLog {
		println(label, "erase start")
	}
	err := machine.Flash.EraseBlocks(eraseStart, eraseBlocks)
	if err != nil {
		println(label, "erase error:", err.Error())
		for {
		}
	}
	if usbProgressLog {
		println(label, "erase done")
	}
}

// worker exercises the system from the second core:
//  1. Continuous random reads from a 64 KB flash-resident const array.
//     This defeats the 16 KB XIP cache and produces sustained flash
//     instruction/data traffic, which is required to make the bug
//     observable on stock TinyGo.
//  2. Periodic flash writes to a fixed low offset. This forces the
//     worker and main to race for rp2040EnterFlashSafeSection, which
//     is what the flash-safe spinlock fix is supposed to serialize.
func worker() {
	core := coreID()
	atomic.StoreUint32(&workerCore, core)
	println("worker started on core:", core)

	led := machine.LED
	led.Configure(machine.PinConfig{Mode: machine.PinOutput})

	// Distinct pattern so the worker's flash content differs from main's.
	for i := range workerWriteBuf {
		workerWriteBuf[i] = byte(i) ^ 0xa5
	}

	eraseSize := machine.Flash.EraseBlockSize()

	x := uint32(0x12345678)
	sum := uint32(0)
	writes := uint32(0)

	for atomic.LoadUint32(&workerDone) == 0 {
		// Hammer the XIP cache with random reads.
		for n := 0; n < 8192; n++ {
			x = x*1664525 + 1013904223
			i := int(x % uint32(len(flashData)))
			sum += uint32(flashData[i])
		}
		led.Set((sum & 0x800000) != 0)

		// Periodically write to flash. (sum & 0xff) == 0 fires roughly
		// once every couple hundred outer iterations -> enough overlap
		// chance with main's flash ops without burning the sector too
		// hard.
		if (sum & 0xff) == 0 {
			if err := machine.Flash.EraseBlocks(workerWriteOffset/eraseSize, 1); err == nil {
				if _, err := machine.Flash.WriteAt(workerWriteBuf[:], workerWriteOffset); err == nil {
					writes++
					atomic.StoreUint32(&workerWrites, writes)
				}
			}
		}
	}

	led.Low()
	atomic.StoreUint32(&workerDone, 2)
}

func main() {
	time.Sleep(2 * time.Second)
	initDebugLEDs()
	blinkDebug(debugPhasePrepare, 1)
	blinkDebug(debugPhaseWrite, 1)

	numCPU := runtime.NumCPU()
	workerStarted := numCPU > 1

	println("start")
	println("NumCPU:", numCPU)
	println("main core:", coreID())
	println("flashData size:", len(flashData))
	println("max rounds:", maxRounds)

	if workerStarted {
		println("multi-core scheduler: worker may run concurrently (with concurrent flash writes)")
	} else {
		println("single-core scheduler: worker will not be started")
	}

	// Sanity-check sizes at runtime as well.
	if len(chunk256) != 256 {
		println("WARN: chunk256 size unexpected:", len(chunk256))
	}
	if len(flashData) != 65536 {
		println("WARN: flashData size unexpected:", len(flashData))
	}

	for round := uint32(0); round < maxRounds; round++ {
		fillWriteBuffer(round)
	}

	if workerStarted {
		go worker()
	}

	for round := uint32(0); round < maxRounds; round++ {
		flashOffset := safeFlashOffset(round)
		fillWriteBuffer(round)

		if usbProgressLog {
			println("round:", round,
				"main core:", coreID(),
				"worker core:", atomic.LoadUint32(&workerCore),
				"worker writes:", atomic.LoadUint32(&workerWrites),
			)
		}

		setDebugPhase(debugPhasePrepare)
		eraseFlash("prepare", flashOffset)
		readFlash("prepare", flashOffset)
		verifyErase(round)

		setDebugPhase(debugPhaseWrite)
		if usbProgressLog {
			println("write start")
		}
		n, err := machine.Flash.WriteAt(writeBuf[:], flashOffset)
		if err != nil {
			println("write error:", err.Error())
			for {
			}
		}
		setDebugPhase(debugPhaseFinal)
		if usbProgressLog {
			println("write done:", n)
		}
		if n != len(writeBuf) {
			println("write length mismatch:", n)
			for {
			}
		}

		readFlash("write", flashOffset)
		verifyWrite(round)

		eraseFlash("final", flashOffset)
		readFlash("final", flashOffset)
		verifyErase(round)
		setDebugPhase(debugPhaseIdle)
	}

	if workerStarted {
		println("requesting worker stop")
		atomic.StoreUint32(&workerDone, 1)

		const maxSpin = 100_000_000
		spun := 0
		for atomic.LoadUint32(&workerDone) != 2 && spun < maxSpin {
			spun++
		}

		if atomic.LoadUint32(&workerDone) == 2 {
			println("worker stopped cleanly")
		} else {
			println("worker did not stop within bound")
		}

		println("worker core:", atomic.LoadUint32(&workerCore))
		println("worker writes:", atomic.LoadUint32(&workerWrites))
	} else {
		println("worker was not started")
	}

	println("test finished")
	blinkDebug(debugPhaseFinal, 5)
	setDebugPhase(debugPhaseIdle)

	// Pure busy loop: keep main pinned to its current core.
	for {
	}
}

@rdon-key
rdon-key force-pushed the rp2040-flashsafe-section branch from e3dc1bd to 27373a0 Compare July 4, 2026 12:05
@rdon-key

rdon-key commented Jul 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased this branch onto current dev.

This branch conflicted with #5482, runtime/rp2: handle RP2350 shared FIFO IRQ for GC. During the rebase, I resolved the conflict by keeping the current per-chip SIO FIFO IRQ handler split introduced by #5482, and applying the flash-safe FIFO command handling to that structure instead of restoring the older inline handlers in runtime_rp2.go.

The resolved structure is:

Tested after the rebase:

GC stress test

  • pico / scheduler=tasks: PASS
  • pico / scheduler=cores: PASS
  • pico2 / scheduler=tasks: PASS
  • pico2 / scheduler=cores: PASS

Each target survived 10,000 cycles of runtime.Gosched() + runtime.GC().

RP2040 flash-safe stress test

  • pico / scheduler=tasks: PASS
  • pico / scheduler=cores: PASS

The flash-safe stress test completed 100 rounds successfully. Each round erased, read back, wrote, verified, and erased a flash sector again.

For scheduler=cores, another goroutine was running on the other core. It continuously generated XIP pressure by randomly reading from a 64 KB flash-resident const array, and also periodically performed flash erase/write operations to a separate sector.

Result:

  • all 100 rounds completed
  • all erase readbacks verified as 0xff
  • all write readbacks matched
  • worker stopped cleanly
  • worker completed 75 flash writes

I also ran the flash test on pico2 with scheduler=tasks as a sanity check. I did not run the pico2 scheduler=cores flash test, because this PR does not implement RP2350 flash-safe handling; the RP2350 flash-safe handler is intentionally a no-op stub, and the pico2 GC stress test already covers the shared FIFO IRQ path.

@aykevl aykevl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks reasonable, a few comments below.

//
// id: 24 is reserved here. ids 20-23 are already used by printLock,
// schedulerLock, atomicsLock, futexLock (see runtime_rp2.go).
var flashSafeLock = spinLock{id: 24}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please either put this spinLock in runtime_rp2.go along with the rest (preferably), or add a comment there referring here that ID 24 is in use. Otherwise it's easy to miss this other spinlock when adding a new spinlock in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I moved flashSafeLock next to the other RP2 runtime spinLock definitions in runtime_rp2.go, so the spinLock IDs remain visible in one place.

Comment thread src/machine/machine_rp2040_rom.go Outdated
Comment on lines +212 to +213
txbuf := make([]byte, len(tx))
copy(txbuf, tx)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does this need to be copied to a newly heap-allocated array, instead of just using tx directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this is an important concern. I removed the unconditional RAM copy here.

I originally added it after seeing failures when the source buffer itself was placed in flash, but that is a separate pre-existing limitation: using an XIP flash address as the source buffer is not safe even with the tasks scheduler.

For this PR, I think it is better to rely on the caller passing a RAM-resident buffer, as before, and avoid adding an unconditional heap allocation. XIP-flash source buffers should be handled explicitly in a separate change, either by copying only when the source overlaps XIP flash or by returning an error for that case.

Comment thread src/machine/machine_rp2040_rom.go Outdated
Comment on lines +239 to +240
buf := make([]byte, len(padded))
copy(buf, padded)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here, this is potentially very expensive. In fact, flashPad already can heap allocate if it isn't aligned and you're allocating even more here (where p can be a potentially large buffer).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same here. I removed the unconditional RAM copy and now use the padded slice directly, so flashPad's buffer is not copied again.

}

func rp2040FlashSafePauseCore(core uint32) {
_ = core // RP2040 SIO FIFO writes to the other core.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: this isn't doing anything (though the comment is helpful and would be useful to keep)

Suggested change
_ = core // RP2040 SIO FIFO writes to the other core.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. I removed the unused assignment and kept the RP2040 SIO FIFO comment.

//
//go:section .ramfuncs
func rp2FlashSafeInterruptHandler(core uint32) {
_ = core

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here, is this for linter reasons? Right not it just adds noise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. It wasn't for linter reasons; removing it doesn't cause any warning, so I removed the unused assignment here as well.

@rdon-key

rdon-key commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

I addressed the review comments and re-ran the RP2040 flash-safe stress test with both scheduler=tasks and scheduler=cores. Both completed 100 rounds successfully after removing the unconditional RAM copies.

This PR is still scoped to RP2040 only; the RP2350 flash-safe handler remains a no-op stub. If this direction looks good, I'm happy to implement similar handling for RP2350 as well — would you prefer that here, or as a separate follow-up PR?

@rdon-key

Copy link
Copy Markdown
Contributor Author

@deadprogram

All review comments have been addressed, and the RP2040 flash-safe stress tests pass with both scheduler=tasks and scheduler=cores.

Since this PR is intentionally scoped to RP2040, I think RP2350 flash-safe handling is better done in a separate follow-up PR.

Unless there are any remaining blockers, could we please get this merged?

@deadprogram

Copy link
Copy Markdown
Member

@rdon-key additional review note.

The GC and flash-safe code share the FIFO and can deadlock.

rp2040EnterFlashSafeSection spins for the ack with interrupts still enabled. If core 1 starts a GC stop-the-world in that window, core 0 takes the GC interrupt and blocks in gcInterruptHandler on gcSignalWait, while core 1 takes the FlashSafe interrupt and parks waiting for Release. Since neither signals the other it would hang.

It is a narrow window, and the reproducer never triggers a GC from core 1 during it. You could fix it perhaps by serializing the flash-safe accesses against the stop-the-world GC.

@rdon-key
rdon-key force-pushed the rp2040-flashsafe-section branch from ddc5485 to 48b3105 Compare August 24, 2026 14:02
@rdon-key

Copy link
Copy Markdown
Contributor Author

Hold on a moment. I found an IRQ-related issue and I’m checking it now.

@rdon-key

Copy link
Copy Markdown
Contributor Author

@deadprogram
Thanks for the additional review — that's an important catch.
While looking into it, I found that the interrupt requirements of the multicore GC are not clearly defined more generally, so I opened #5610. I'd like to settle that first and then come back to this PR, because the right fix here depends on what interrupt state GC callers are allowed to have.
Does that approach make sense to you?

@rdon-key

Copy link
Copy Markdown
Contributor Author

@deadprogram

I've looked into this further and pushed another update.

The flash-safe initiator now disables local interrupts before starting the handshake. This prevents it from entering the GC interrupt handler while waiting for the peer to acknowledge the flash-safe request.

If the peer starts GC during that interval, the GC request to the flash-safe initiator remains pending while its interrupts are disabled. The GC-initiating core itself is still interruptible while waiting for the GC acknowledgement — the wait does not disable interrupts — so it can take the flash-safe interrupt, acknowledge the request, and park until the flash operation completes.

I also adjusted the exit ordering in rp2FlashSafeInterruptHandler. The handler now publishes Idle before restoring its local interrupts:

Release observed
-> publish Idle
-> signal initiator
-> restore local interrupts

This is important because a pending GC interrupt may run immediately after interrupts are restored. If that happened before Idle was published, the peer could block in the GC handler while the flash-safe initiator was still waiting for Idle, creating another circular wait.

So Idle now means that the flash-safe protocol has completed and the initiator no longer needs to wait; it does not require the interrupt handler itself to have fully returned.

The flash-safe section itself performs no allocation — flashPad completes before the section is entered, and doFlashCommand does not allocate — so nothing on this path can trigger GC while local interrupts are disabled.

With these ordering changes, I believe the deadlock path you pointed out is removed.

Could you please take another look when you have a chance?

@deadprogram

deadprogram commented Sep 8, 2026

Copy link
Copy Markdown
Member

Thanks for this @rdon-key .

Some small points from an automated review:

  1. secondaryCoresStarted is read again in rp2040ExitFlashSafeSection. If the value changes between Enter and Exit, Exit unlocks a spinlock that it does not hold. RP2040 Unlock is a plain Set(0), so this breaks mutual exclusion without an error. Please keep the Enter decision in a local variable.

  2. If the other core is in an interrupt handler that waits for this core, the SIO FIFO interrupt cannot start the park handler, and Enter waits forever. An example is a heap allocation in an interrupt handler that starts a GC cycle. Please add one line to the doc comment: do not call flash operations from an interrupt handler or with interrupts disabled.

  3. rp2040FlashSafePauseCore writes FIFO_WR without a check of FIFO_ST.RDY. A full FIFO discards the word and Enter waits forever. gcPauseCore does the same, so this is not new, but the SDK uses multicore_fifo_push_blocking.

  4. The core parameter is not used in rp2040FlashSafePauseCore or rp2FlashSafeInterruptHandler. You can remove it.

  5. Some of the new comments are 6 to 9 lines. The repository style is a maximum of 2 lines. Also, please add a reference for the statement about instruction fetches from 0x10000000, for example RP2040 datasheet section 2.6.3.

  6. The write and command buffers must be in RAM during the flash operation, but flashPad returns the caller buffer when the length is aligned. This is the same as before this PR, so it is not a regression. I mention it only as a known limit.

I think these can be addressed pretty easily and then this would be ready from my POV.

@rdon-key
rdon-key force-pushed the rp2040-flashsafe-section branch from 99e9722 to 65f2c18 Compare September 17, 2026 17:13
@rdon-key

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I've rebased this PR onto the latest dev and addressed the feedback:

  • Keep the Enter decision in a local variable, so Exit no longer re-reads secondaryCoresStarted.
  • Document that flash operations must not be called from an interrupt handler or with interrupts disabled.
  • Remove the unused core parameters.
  • Shorten the comments to at most 2 lines and add a reference to RP2040 datasheet section 2.6.3.
  • Keep setting Idle before restoring interrupts to avoid the GC deadlock.

I left the FIFO RDY handling unchanged for now, since gcPauseCore has the same existing issue. I think it would be better to address that separately for both GC and flash-safe FIFO writes rather than changing only the flash-safe path here.

I also left flashPad as is, since it is a known limit and not a regression.

I also retested on a Pico with -scheduler=cores: 100 rounds of flash erase/write/readback while the other core was running completed successfully.

@deadprogram

Copy link
Copy Markdown
Member

Thanks @rdon-key for this work. The notes below are edited from an automated review.

  1. Flash operations called from an interrupt handler can deadlock. rp2040EnterFlashSafeSection takes flashSafeLock before interrupt.Disable(). If an interrupt handler on the second core calls machine.Flash.WriteAt while the first core holds the lock, the second core spins in flashSafeLock.Lock() inside that handler. The SIO FIFO interrupt has the lowest priority, so the second core cannot take the FlashSafe interrupt, and the first core waits forever for the Locked state. Before this change, interrupt.Disable() made this case safe. The constraint is documented on the runtime function, but not on machine.Flash.WriteAt, machine.Flash.EraseBlocks, or doFlashCommand, where a user can run into it.

  2. There is a startup window in which secondaryCoresStarted is still false. startSecondaryCores() returns after core 1 already runs Go code from XIP, and secondaryCoresStarted = true is set after that (src/runtime/scheduler_cores.go:180). A flash operation in this window takes the single-core path and can hang core 1. The GC has the same pattern, but here the result changes from a missed stack scan to a hang.

  3. The per-core loop in rp2040EnterFlashSafeSection ignores i. rp2040FlashSafePauseCore() takes no argument, so the loop sends the same FIFO message N-1 times. This is correct for numCPU == 2, but the code reads as if it is generic. A single call is clearer.

  4. rp2040FlashSafePauseCore writes FIFO_WR without a wready check, as gcPauseCore does. Only one message is outstanding at a time, so a lost message looks unlikely. It is mentioned because a lost message here gives an unrecoverable hang, not a missed GC step.

@deadprogram

Copy link
Copy Markdown
Member

Thanks @rdon-key for this work.

Some notes, edited from an automated review, about #5709, which changes the same handlers:

  1. The two changes merge clean. This pull request changes the lines inside the switch, runtime/rp2: check FIFO_ST.VLD in the SIO FIFO interrupt handler #5709 adds lines before it. I merged them locally and built for pico and pico2-w with -scheduler=cores. The merged handler is:
func handleSIOFifoInterruptCore0(intr interrupt.Interrupt) {
	rp.SIO.FIFO_ST.Set(rp.SIO_FIFO_ST_ROE | rp.SIO_FIFO_ST_WOF)
	if !multicore_fifo_rvalid() {
		return
	}
	switch rp.SIO.FIFO_RD.Get() {
	case rp2SIOFIFOCommandGC:
		gcInterruptHandler(0)
	case rp2SIOFIFOCommandFlashSafe:
		rp2FlashSafeInterruptHandler()
	}
}
  1. The VLD check is more necessary with this pull request than without it. The SIO FIFO IRQ is the logical OR of the VLD, WOF and ROE bits, so a sticky bit keeps the IRQ asserted with no data to read. During a flash-safe handshake, with XIP disabled, that window is worse than it is for the GC alone.

  2. One read of FIFO_RD per interrupt is correct here. A drain loop is not safe, because gcInterruptHandler and rp2FlashSafeInterruptHandler both block until their phase ends, so a loop can service a second message while parked in the first. A loop is also not needed: a second queued word keeps VLD set, so the interrupt occurs again.

  3. runtime/rp2350: set ACTLR.EXTEXCLALL on each core #5708, below runtime/rp2: check FIFO_ST.VLD in the SIO FIFO interrupt handler #5709 in the stack, is RP2350 only and does not touch RP2040 behavior.

Lastly, could you please squash this PR into a smaller number of commits? It will make it a lot easier to understand.

Thank you!

@rdon-key
rdon-key force-pushed the rp2040-flashsafe-section branch from 65f2c18 to a0b7383 Compare September 19, 2026 17:25
@rdon-key

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up review.

Regarding the four points:

  1. Interrupt-handler / interrupts-disabled constraint
    I added the constraint to machine.Flash.WriteAt, machine.Flash.EraseBlocks, and doFlashCommand.

  2. secondaryCoresStarted startup window
    I agree this window exists. The GC has the same startup-state assumption, so I added a comment documenting it here but left the logic unchanged. I would prefer to fix the startup state for both GC and flash-safe together in a follow-up rather than introduce a flash-safe-only workaround.

  3. Per-core loop
    Fixed. RP2040 has exactly one peer core, so rp2040FlashSafePauseCore() is now called once directly.

  4. FIFO wready check
    I left this unchanged for now. gcPauseCore() has the same direct FIFO_WR write without checking wready. Since both paths use the same FIFO mechanism, I would prefer to switch both GC and flash-safe to the blocking FIFO helper together in a follow-up.

I also squashed this PR into a smaller number of commits as requested.

Regarding #5709:

Thanks for checking the combined changes. I agree the VLD check is important for the flash-safe handshake, and that one FIFO_RD read per interrupt is the correct behavior here. I also understand #5708 is RP2350-only and does not affect this RP2040 change.

Next steps for the GC side:

  1. fix the shared secondaryCoresStarted startup-state issue,
  2. switch both GC and flash-safe FIFO writes to the blocking helper, and
  3. address the GC interrupt-state/entry contract discussed in runtime: define interrupt requirements for GC with scheduler=cores #5610, including allocation-triggered GC.

RP2350 flash-safe handling remains a no-op in this PR. After these three items are addressed, I would like to move on to RP2350 flash-safe support in a separate PR.

@deadprogram

Copy link
Copy Markdown
Member

Thanks @rdon-key for the squash. The notes below are edited from an automated review.

  1. The new push contains no code change. The tree at a0b7383c is the same as the tree at 65f2c18d, the head of the 2026-09-17 push. The squash rewrote the commit dates, so a0b7383c looks new, but it is the comment-only commit that was already there. Please check that the branch you squashed included the new work.

  2. As a result, three of the four items from the last review are not in the branch yet:

    • The interrupt-handler constraint is not on machine.Flash.WriteAt, machine.Flash.EraseBlocks, or doFlashCommand. It is only on rp2040EnterFlashSafeSection.
    • There is no comment about the secondaryCoresStarted startup window.
    • The per-core loop in rp2040EnterFlashSafeSection is still present. rp2040FlashSafePauseCore() now takes no argument, so the loop sends the same message each time.
  3. The branch is behind dev and does not yet include runtime/rp2: check FIFO_ST.VLD in the SIO FIFO interrupt handler #5709, the FIFO_ST.VLD check in the SIO FIFO interrupt handler. The two changes still merge clean. Please rebase so the flash-safe handshake runs with the VLD check in place.

  4. The shortened comment in rp2040EnterFlashSafeSection no longer explains the interrupt order. The reason that local interrupts stay disabled until the acknowledgement arrives is the deadlock fix, and now nothing in the code states it. Please keep a short form of the reason, for example: "Disable local interrupts before the handshake. A GC interrupt here would block this core while the other core is parked."

  5. The datasheet reference for section 2.6.3 is on the handler comment. It is the reason for the //go:section .ramfuncs placement, so it is more useful next to that line.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RP2040: is cross-core synchronization unnecessary in existing flash operations?

3 participants