Skip to content

usb/msc: erase blocks before writing to them #5017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

usb/msc: erase blocks before writing to them #5017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions src/machine/usb/msc/msc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package msc

import (
"errors"
"machine"
"machine/usb"
"machine/usb/descriptor"
"machine/usb/msc/csw"
"machine/usb/msc/scsi"
"math/bits"
"time"
)

Expand All @@ -23,6 +25,8 @@ const (
mscInterface = 2
)

var errInvalidBlockSize = errors.New("usb/msc: invalid block size")

var MSC *msc

type msc struct {
Expand Down Expand Up @@ -60,20 +64,41 @@ type msc struct {
}

// Port returns the USB Mass Storage port
func Port(dev machine.BlockDevice) *msc {
func Port(dev machine.BlockDevice) (*msc, error) {
if MSC == nil {
MSC = newMSC(dev)
msc, err := newMSC(dev)
if err != nil {
return nil, err
}
MSC = msc
}
return MSC
return MSC, nil
}

func newMSC(dev machine.BlockDevice) *msc {
func newMSC(dev machine.BlockDevice) (*msc, error) {
// Size our buffer to match the maximum packet size of the IN endpoint
maxPacketSize := descriptor.EndpointMSCIN.GetMaxPacketSize()

// Windows only supports block sizes of 512 or 4096 bytes, other systems are
// probably similar.
blockSize := max(dev.EraseBlockSize(), dev.WriteBlockSize())
if bits.OnesCount32(uint32(blockSize)) != 1 {
return nil, errInvalidBlockSize // not a power of two
}
var blockSizeUSB uint32
switch {
case blockSize <= 512:
blockSizeUSB = 512
case blockSize <= 4096:
blockSizeUSB = 4096
default:
return nil, errInvalidBlockSize
}

m := &msc{
// Some platforms require reads/writes to be aligned to the full underlying hardware block
blockCache: make([]byte, dev.WriteBlockSize()),
blockSizeUSB: 512,
blockSizeUSB: blockSizeUSB,
Copy link
Contributor

@mikesmitty mikesmitty Sep 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It would probably be better to leave the default blockSizeUSB here to 512 and move the new block size checking logic to RegisterBlockDevice() in disk.go (called a few lines below here). I put the logic around setting up the block device in there with the idea that it could be swapped out during runtime if someone wanted to.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean, you'd like to still emulate a block size of 512 even if the underlying flash has an erase block size of 4096?
I think that's a bad idea:

  1. It takes up more space.
  2. It causes write amplification (up to 8x as many erase operations - remember: flash has a finite amount of erase cycles!)
  3. It seems that all modern operating systems support a block size of 4096. So I don't see the point of emulating a smaller block size.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was more thinking in the context that it'd be unusual for TinyGo to see a device with 4k erase blocks so 512 would be a safe default, but after the unexpected edge case that just bit me with the nrf52840 buffer thing, might as well plan to handle it better

Copy link
Member Author

@aykevl aykevl Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

4k is pretty common, it's what the nrf52* devices uses and IIRC also what rp2040 uses. And the code in this PR uses 512 bytes when possible anyway, so devices with a smaller erase block size can use that.

buf: make([]byte, dev.WriteBlockSize()),
cswBuf: make([]byte, csw.MsgLen),
cbw: &CBW{Data: make([]byte, 31)},
Expand Down Expand Up @@ -114,7 +139,7 @@ func newMSC(dev machine.BlockDevice) *msc {

go m.processTasks()

return m
return m, nil
}

func (m *msc) processTasks() {
Expand Down
10 changes: 10 additions & 0 deletions src/machine/usb/msc/scsi_readwrite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,16 @@ func (m *msc) writeBlock(b []byte, lba, offset uint32) (n int, err error) {
return 0, invalidWriteError
}

// Erase the block first if needed.
// Data packets arrive in order, so if we want to write to the start of a
// block, that means it's the first write to this erase block and it needs
// to be erased first.
if uint32(blockStart)%m.blockSizeUSB == 0 {
firstBlock := uint32(blockStart) / uint32(m.dev.EraseBlockSize())
numBlocks := (m.blockSizeUSB + uint32(m.dev.EraseBlockSize()) - 1) / uint32(m.dev.EraseBlockSize())
m.dev.EraseBlocks(int64(firstBlock), int64(numBlocks))
}
Comment on lines +98 to +102
Copy link
Member Author

@aykevl aykevl Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This makes the assumption that data is always written in full blocks, and that packets arrive sequentially. @mikesmitty can you confirm that this is true?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's how the protocol operates, yeah. A read or write command is sent from the host in a CDB packet along with the transfer length and then nothing but un-encapsulated raw data is sent/received until the transfer length is met

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again, it might make sense to refactor the block erase logic like in #5027, then you could simplify this like so:

if offset == 0 && (lba % m.blockSizeUSB) == m.eraseBlockOffset {
    err := m.unmapUSBBlocks(lba, 1)
    // Error handling
}

writeBlock() gets called when a full flash device write block is buffered and offset here is that flash block's byte offset within the emulated USB sector, so it would depend on the usb block size being at least as large as the advertised usb block, but you've got that case covered now with the checks above


// Write the full block to the underlying device
n, err = m.dev.WriteAt(b, blockStart)
n -= int(blockOffset)
Expand Down