Skip to content

machine/rp2: fix USB buffer status race that can stall CDC TX pump#5472

Open
rdon-key wants to merge 1 commit into
tinygo-org:devfrom
rdon-key:fix-rp2-usb-buff-status-clear
Open

machine/rp2: fix USB buffer status race that can stall CDC TX pump#5472
rdon-key wants to merge 1 commit into
tinygo-org:devfrom
rdon-key:fix-rp2-usb-buff-status-clear

Conversation

@rdon-key

Copy link
Copy Markdown
Contributor

This fixes an RP2 USB CDC TX pump stall observed with -scheduler=cores under XIP pressure.

The problem was reproduced on RP2040 as an intermittent USB CDC monitor hang: the program continued to run, but CDC output stopped before reaching test finished.

The underlying issue appears to be a race in the RP2040/RP2350 USB IRQ handlers around BUFF_STATUS.

Previously, the IRQ handler did this:

1. Read BUFF_STATUS
2. Invoke endpoint handlers
3. Clear BUFF_STATUS using the old snapshot

However, the USB CDC TX handler can immediately arm the next IN transfer. If that newly armed IN transfer completes while the endpoint handler is still running, the later clear of the old BUFF_STATUS snapshot can also clear the newly completed transfer bit.

That can cause the CDC TX pump to miss a TX completion event and remain stuck waiting for a completion that has already been cleared.

This change makes the IRQ handler acknowledge the observed BUFF_STATUS bits before invoking endpoint handlers:

1. Read BUFF_STATUS
2. Clear the observed BUFF_STATUS bits
3. Invoke endpoint handlers

With this ordering, any new completion that happens while endpoint handlers are running remains pending and can be handled by a later IRQ.

Background

This was observed after the RP2 bidirectional endpoint changes made the issue reproducible with USB CDC output under XIP pressure.

The bidirectional endpoint changes appear to have made an existing IRQ ordering race visible, rather than being the direct source of the BUFF_STATUS clear ordering itself.

Reproducer

The following program repeatedly writes USB CDC output while another goroutine continuously reads from flash to create XIP/cache pressure.

Details
//go:build tinygo

package main

import (
        "sync/atomic"
        "time"
)

const lines = 5000
const reads = 8192
const payload = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ--usb-xip-atomic-use-min--"

const chunk256 = "" +
        "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" +
        "fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210" +
        "00112233445566778899aabbccddeeffffeeddccbbaa99887766554433221100" +
        "rp2040xipcacheflashworkerrandomaccesstestdataAAAAAAAAAAAAAAAAAAA"

const block4k = chunk256 + chunk256 + chunk256 + chunk256 +
        chunk256 + chunk256 + chunk256 + chunk256 +
        chunk256 + chunk256 + chunk256 + chunk256 +
        chunk256 + chunk256 + chunk256 + chunk256

const flashData = block4k + block4k + block4k + block4k +
        block4k + block4k + block4k + block4k +
        block4k + block4k + block4k + block4k +
        block4k + block4k + block4k + block4k

var (
        workerDone uint32
        workerSum  uint32
)

func xipWorker() {
        x := uint32(0x12345678)
        s := uint32(0)
        for {
                v := atomic.LoadUint32(&workerDone)
                s ^= v // use the atomic value, but do not branch on it
                for i := 0; i < reads; i++ {
                        x = x*1664525 + 1013904223
                        s += uint32(flashData[int(x%uint32(len(flashData)))])
                }
                workerSum = s ^ x
        }
}

func main() {
        time.Sleep(2 * time.Second)
        println("start usb xip stall atomic-use")
        println("flashData size:", len(flashData))
        println("lines:", lines)

        go xipWorker()
        time.Sleep(10 * time.Millisecond)

        for i := 0; i < lines; i++ {
                println("line:", i, payload)
        }

        workerDone = 1
        println("worker sum:", workerSum)
        println("test finished")

        for {
        }
}

Example commands:

tinygo flash -target=pico -scheduler=cores -monitor ./25_min_usb_xip_atomic_use_64kb.go
tinygo flash -target=pico2 -scheduler=cores -monitor ./25_min_usb_xip_atomic_use_64kb.go

Testing

Using the reproducer above:

  • RP2040: completed successfully 10/10 runs
  • RP2350: completed successfully 10/10 runs

Before this change, the RP2040 test could intermittently hang before printing test finished.

This does not claim to fix every possible USB CDC stall, but it fixes the observed RP2 USB CDC TX pump stall under -scheduler=cores with XIP pressure and matches the suspected lost-completion race.

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.

1 participant