usbus/msc: add initial support for Mass Storage Class #15941

dylad · 2021-02-07T21:24:30Z

Contribution description

This PR adds the initial support for Mass Storage Class in USBUS. This PR relies on the RIOT MTD implementation to implement the Mass Storage Class support. With the provided test application, a MTD device will be accessible as a normal storage device on your host computer ~~(Linux only for now)~~.
Read and Write operations are allowed.
The MSC relies on SCSI protocol to operate. SCSI is a beast and only a part of it has been implemented.

Currently there are some limitations:

Supported host : Linux & Windows
MSC cannot be used if MTD page size > 512
Only mtd0 is supported.

~~Looks like some additional requests code must be supported to get it running on Windows~~ -> Done.
Regarding the page size, on the host side, Linux expects at least 512 bytes as page size. (512 is standard. 1024/2048 are accepted as some recent SSD make use of these values). This means that some logic was added in case the MTD page size is less than 512 bytes like SPI-NOR devices.
More logic must be added to handle page size superior to 512 bytes (e.g some flashpage implementation) but I think this should belongs to another PR.

Please be aware that performance are not so great. Be patient if you are using a big SD card.
And also, don't use a sd card if you have important data on it :)

Comments, especially on the MTD part, are very welcome !

Testing procedure

Flash tests/usbus_msc, on a board with a MTD device.
Access your memory as a standard memory through /dev/sdX
You can use dd to read/write data or even format a FS on your memory device so you can access it the same way as a USB stick.

Issues/PRs references

None

benpicco · 2021-02-07T21:40:47Z

sys/usb/usbus/msc/msc.c

+
+/* Internal buffer to handle size difference between MTD layer and USB
+   endpoint size as some MTD implementation (like mtd_sdcard) doesn't allow
+   endpoint size transfer */


Do you mean it does not support reading at an offset?
mtd_sdcard might actually be the only driver that does not support that.

Yes mtd_sdcard doesn't support the offset argument so I had to workaround this.

mtd_mci, which is also used to drive SD-cards, has an internal page buffer to support offsets.

I wonder if we should rather add that to mtd_sdcard too (maybe as a module, so it can be left out if e.g. fatfs is used, that will only ever write pages and has it's own page buffer for that.)

If there is a way to use the offset for sdcard, I would be happy to adapt the code.
Nevertheless, I think we should keep an internal buffer to store a page in memory for write operations to avoid multiple write (64 bytes) for a single page.

sys/usb/usbus/msc/msc.c

benpicco · 2021-02-07T21:51:11Z

sys/usb/usbus/msc/scsi.c

+    assert(mtd0->page_size <= HOST_MINIMAL_PAGE_SIZE);
+
+    /* Host side expects to have at least 512 bytes per page, otherwise it will reject
+       the setup. Thus add some logic here to be able to use smaller page size mtd device */


Can't we report a larger page size to the host?
I suppose then the statically allocated buff would be too small to act as a page cache.

(could we use malloc() here if we need the page cache at all? 😇)

Why would you report a larger page size to the host ?

Internal flash can have 4k or 8k pages.
If we want to support that too we would have to report larger pages - unless we also want to map that to virtual 512 byte pages.

Reporting larger page size is somehow limited.
Linux rejects me when I was testing SAME54 flashpage because I reported 8192 bytes as page size.
So supported values are 512, 1024, 2048. (maybe 4096 I didn't try).

sys/usb/usbus/msc/msc.c

dylad · 2021-02-08T08:36:00Z

To give a little more details.
MSC relies on the SCSI protocol over USB.
So a basic transfer looks like the following:

Host send CBW (Command Block Wrapper) to the device
Device may answer back depending on the CBW command
Device send a CSW (Command Status Wrapper) to indicate the CBW was processed and is now ready for a new command.

In a read/write command case:

Host send the CBW
Device read or write data by endpoint size chunk: (If the advertised page size is 512), the device will read/write 64 bytes * 8 transfers (=512) * the number of page the host asked. If the host asked for only one page, the device will then process 8 endpoint transfer.
Once all bytes has been written or read, the device can send the CSW to the host.

Since our hardware only support USB full-speed. We are limited to 64 bytes for Bulk Endpoints by the spec and thus, we need to slice the page size into 64 bytes chunks.

dylad · 2021-02-18T20:57:35Z

Pushed Kconfig support

dylad · 2021-04-08T20:12:47Z

Finally got it working on Windows.
This is pretty slow but I finally managed to access my SDCARD through a Windows computer. Both read & write work.
I'll update the PR description.

chrysn

Just a few things I noticed when skimming this for how tied it is to mtd (not that that tie would be a bad thing, was just curious).

chrysn · 2021-11-18T13:44:26Z

tests/usbus_msc/Makefile

+
+ifeq ($(USB_VID):$(USB_PID), 1209:0001)
+  $(shell $(COLOR_ECHO) "$(COLOR_RED)Private testing pid.codes USB VID/PID used!, do not use it outside of test environments!$(COLOR_RESET)" 1>&2)
+  $(shell $(COLOR_ECHO) "$(COLOR_RED)MUST NOT be used on any device redistributed, sold or manufactured, VID/PID is not unique!$(COLOR_RESET)" 1>&2)
+endif


This hasn't been needed any more for quite some while (see #13382)

Suggested change

ifeq ($(USB_VID):$(USB_PID), 1209:0001)

$(shell $(COLOR_ECHO) "$(COLOR_RED)Private testing pid.codes USB VID/PID used!, do not use it outside of test environments!$(COLOR_RESET)" 1>&2)

$(shell $(COLOR_ECHO) "$(COLOR_RED)MUST NOT be used on any device redistributed, sold or manufactured, VID/PID is not unique!$(COLOR_RESET)" 1>&2)

endif

chrysn · 2021-11-18T13:49:10Z

tests/usbus_msc/Makefile

+USB_VID ?= 1209
+USB_PID ?= 0001


These are predefined now:

Suggested change

USB_VID ?= 1209

USB_PID ?= 0001

USB_VID ?= $(USB_VID_TESTING)

USB_PID ?= $(USB_PID_TESTING)

chrysn · 2021-11-18T13:49:58Z

tests/usbus_msc/main.c

+ #include <stdio.h>
+
+ #include "shell.h"


Odd indentation

Suggested change

#include <stdio.h>

#include "shell.h"

#include <stdio.h>

#include "shell.h"

dylad · 2021-11-18T14:03:40Z

Yes this PR is a bit staging and need a rebase.
Since there is ongoing rework on USBUS, I'll wait before rebasing this one because it will benefit from it and speedup transfer rate (in theory ?). Then I'll ping @bergzand to death so he can review it :)

dylad · 2022-01-24T20:46:52Z

Revive this PR. Rebased to lastest master and squash at the same time.
I've also adapt this PR to recent USBUS changes.
I've tested it with mtd_flashpage, mtd_sdcard and mtd_spi_nor. (Mostly RAW access through /dev/sdX) and works like a charm on my side.
@bergzand It's time to summon you !

OlegHahm · 2022-03-09T12:47:36Z

Apart from some documentation issues, this seems to be fine. What's up regarding the dependencies on USBUS? @dylad? @bergzand?

dylad · 2022-03-19T17:40:38Z

What's up regarding the dependencies on USBUS?

@OlegHahm Not sure I've understand your question.

Right now this PR is only waiting some review and testing.

benpicco · 2022-03-20T21:18:12Z

Somehow an offset member when missing in the usbus_msc_device_t struct?

I tried it with my sam0_sdhc branch (MTD_MSC is MTD_2) but for the SD card I only get

[ 9378.268306] sd 9:0:0:0: [sdd] Attached SCSI removable disk
[16279.907089] usb 2-1.3.3.3: new full-speed USB device number 16 using ehci-pci
[16280.017391] usb 2-1.3.3.3: New USB device found, idVendor=1209, idProduct=7d01, bcdDevice= 1.00
[16280.017400] usb 2-1.3.3.3: New USB device strings: Mfr=3, Product=2, SerialNumber=4
[16280.017403] usb 2-1.3.3.3: Product: USB device
[16280.017406] usb 2-1.3.3.3: Manufacturer: RIOT-os.org
[16280.017408] usb 2-1.3.3.3: SerialNumber: ABECE68BF9BAAEC8
[16280.017896] usb-storage 2-1.3.3.3:1.0: USB Mass Storage device detected
[16280.018166] scsi host10: usb-storage 2-1.3.3.3:1.0
[16281.048085] scsi 10:0:0:0: Direct-Access     RIOT-OS  RIOT_MSC_DISK     1.0 PQ: 0 ANSI: 1
[16281.048513] sd 10:0:0:0: Attached scsi generic sg5 type 0
[16281.049159] sd 10:0:0:0: [sde] Very big device. Trying to use READ CAPACITY(16).
[16281.049563] sd 10:0:0:0: [sde] Sector size 0 reported, assuming 512.
[16281.049574] sd 10:0:0:0: [sde] 1 512-byte logical blocks: (512 B/512 B)
[16281.049578] sd 10:0:0:0: [sde] 0-byte physical blocks
[16281.050412] sd 10:0:0:0: [sde] Write Protect is off
[16281.050419] sd 10:0:0:0: [sde] Mode Sense: 03 00 00 00
[16281.051246] sd 10:0:0:0: [sde] No Caching mode page found
[16281.051250] sd 10:0:0:0: [sde] Assuming drive cache: write through
[16281.108115] sd 10:0:0:0: [sde] Very big device. Trying to use READ CAPACITY(16).
[16281.108365] sd 10:0:0:0: [sde] Sector size 0 reported, assuming 512.

Reading the external flash works:

% sudo dd if=/dev/sde of=/tmp/flash.img
65536+0 records in
65536+0 records out
33554432 bytes (34 MB, 32 MiB) copied, 54,0845 s, 620 kB/s

benpicco · 2022-03-20T21:26:15Z

sys/include/usb/usbus/msc.h

+                                         WRITE operations */
+    uint16_t block_offset;          /**< Internal offset for endpoint size chunk transfer */
+    uint8_t  pages_per_vpage;       /**< Number of pages to fill host expected page size */
+} usbus_msc_device_t;


Why not add a mtd_dev_t *mtd member here?

That's ongoing.

benpicco · 2022-03-20T21:27:13Z

sys/usb/usbus/msc/scsi.c

+{
+    usbus_msc_device_t *msc = (usbus_msc_device_t*)handler;
+    msc_read_fmt_capa_pkt_t *pkt = (msc_read_fmt_capa_pkt_t*)msc->in_buf;
+    uint32_t blk_nb = (mtd0->sector_count * mtd0->pages_per_sector);


Then you can

Suggested change

uint32_t blk_nb = (mtd0->sector_count * mtd0->pages_per_sector);

uint32_t blk_nb = (msc->mtd->sector_count * msc->mtd->pages_per_sector);

benpicco · 2022-03-20T21:29:52Z

MSC cannot be used if MTD page size > 512

Is that a protocol limitation? HDDs with 4k sectors do exist for more then a decade now and Linux supports 4k native mode since 2.6.31, Windows since version 8

dylad · 2022-03-21T08:27:12Z

Is that a protocol limitation?

No, this was an implementation limitation.
I started some rework again this weekend. This will be fixed at the same time as MTD device selection.

Currently the page size limitation can be found here

dylad · 2022-03-21T08:30:45Z

[16281.049159] sd 10:0:0:0: [sde] Very big device. Trying to use READ CAPACITY(16).

Huh, I wasn't expecting to implement READ16 and WRITE16 operations any time soon due to poor performance.
Currently only READ10 and WRITE10 are implemented. What's your SD card size ? I'll check if I have a big enough one so I can reproduce and implement these functions.

benpicco · 2022-03-21T08:50:55Z

I think the problem was that my SD card was mtd2, but mtd0 is hard-coded so setting MTD_MCI is not enough

benpicco · 2022-03-21T08:56:48Z

sys/usb/usbus/msc/msc.c

+
+    /* buffer is full, write it and point to new block if any */
+    if (msc->block_offset >= (MTD_MSC->page_size * msc->pages_per_vpage)) {
+        mtd_write_page(MTD_MSC, msc->buffer, msc->block * msc->pages_per_vpage,


If you write whole pages you might also use mtd_write_page_raw.
Then you could even use the per-mtd device sector sized work buffer instead of allocating your own.

Alternatively you can deselect the mtd_pagewise module if you do raw writes

The problem with this approach is for devices with more than 4096 bytes as page size. I'll be force to report 4096 as page size so Host can be happy but I'll need to write less than a page.

dylad · 2022-03-21T09:02:05Z

I think the problem was that my SD card was mtd2, but mtd0 is hard-coded so setting MTD_MCI is not enough

I'll let you know when I finish this rework.

stale · 2022-11-02T05:37:26Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

gschorcht · 2022-11-30T19:01:05Z

I'll let you know when I finish this rework.

@dylad Is there anything new on this rework?

dylad · 2022-11-30T19:30:09Z

Yeah I've polished it a bit and added multiple LUNs support. remove auto-init so users can select which MTD devices to export at runtime.
Some more work is still needed for some cases (flashpage > 4096 bytes for instance).
Anyways, I was planning on closing this PR at some point and open a brand-new one.
I just need time but I struggle to find enough during my spare-time lately. I'll do my best to see this PR merged for 2023.01 release.

dylad · 2023-02-03T20:48:31Z

Closing this one.
It will by replaced by a new PR a bit later tonight.

19242: usbus/msc: add initial Mass Storage Class support r=benpicco a=dylad ### Contribution description This PR adds the initial support for Mass Storage Class in USBUS. This PR relies on the RIOT MTD implementation to implement the Mass Storage Class support. With the provided test application, a MTD device will be accessible as a normal storage device on your host computer. Read and Write operations are allowed. Multiple LUNs are supported so several MTD devices can be exported through USB. The MSC relies on SCSI protocol to operate. Currently there are some limitations: Supported host : Linux & Windows (macOS is untested) MSC cannot be used if MTD page size > 4096 MTD device must have at least 512 bytes of memory to be exported. Please be aware that performance are not so great. ### Testing procedure Flash `tests/usbus_msc` application on a board with at least one MTD device. Once the shell has started, prepare one or several MTD devices to be exported using `add_lun` command. Once ready, start the USB connection with `usb_attach` All MTD exported should appear as` /dev/sdX` on Linux. ### Issues/PRs references Supersede #15941 Co-authored-by: Dylan Laduranty <dylan.laduranty@mesotic.com>

dylad added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: USB Area: Universal Serial Bus labels Feb 7, 2021

dylad requested review from aabadie, bergzand, fjmolinas, leandrolanzieri, MichelRottleuthner and smlng as code owners February 7, 2021 21:24

benpicco reviewed Feb 7, 2021

View reviewed changes

dylad requested review from cgundogan and jia200x as code owners February 18, 2021 20:54

MrKevinWeiss added this to the Release 2021.07 milestone Jun 22, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

benpicco added this to the Release 2022.01 milestone Sep 22, 2021

chrysn reviewed Nov 18, 2021

View reviewed changes

dylad added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 18, 2021

dylad added 4 commits January 24, 2022 21:43

usbus/msc: add initial support for Mass Storage Class

eb252e3

sys/auto_init: add support for USB MSC

5a4febd

tests: add test application for usbus msc

d666b81

Kconfig: add Kconfig for USB MSC

0804230

dylad force-pushed the pr/usbus_msc branch from 0b91efb to 0804230 Compare January 24, 2022 20:43

github-actions bot added the Area: Kconfig Area: Kconfig integration label Jan 24, 2022

github-actions bot added the Area: tests Area: tests and testing framework label Jan 24, 2022

dylad removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for author State: Action by the author of the PR is required labels Jan 24, 2022

dylad modified the milestones: Release 2022.01, Release 2022.04 Jan 24, 2022

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 20, 2022

benpicco reviewed Mar 20, 2022

View reviewed changes

benpicco reviewed Mar 21, 2022

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 2, 2022

maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 2, 2022

gschorcht removed the State: stale State: The issue / PR has no activity for >185 days label Nov 30, 2022

dylad closed this Feb 3, 2023

dylad mentioned this pull request Feb 3, 2023

usbus/msc: add initial Mass Storage Class support #19242

Merged

dylad deleted the pr/usbus_msc branch April 7, 2023 10:50

	uint32_t blk_nb = (mtd0->sector_count * mtd0->pages_per_sector);
	uint32_t blk_nb = (msc->mtd->sector_count * msc->mtd->pages_per_sector);

usbus/msc: add initial support for Mass Storage Class #15941

usbus/msc: add initial support for Mass Storage Class #15941

Uh oh!

Conversation

dylad commented Feb 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

dylad commented Feb 8, 2021

Uh oh!

dylad commented Feb 18, 2021

Uh oh!

dylad commented Apr 8, 2021

Uh oh!

chrysn 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!

dylad commented Nov 18, 2021

Uh oh!

dylad commented Jan 24, 2022

Uh oh!

OlegHahm commented Mar 9, 2022

Uh oh!

dylad commented Mar 19, 2022

Uh oh!

benpicco commented Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

benpicco commented Mar 20, 2022

Uh oh!

dylad commented Mar 21, 2022

Uh oh!

dylad commented Mar 21, 2022

Uh oh!

benpicco commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented Mar 21, 2022

Uh oh!

dylad commented Feb 7, 2021 •

edited

Loading

benpicco Feb 8, 2021 •

edited

Loading

benpicco commented Mar 20, 2022 •

edited

Loading

benpicco commented Mar 21, 2022 •

edited

Loading

benpicco Mar 21, 2022 •

edited

Loading