Since you are writing a full page, you can use mtd_write_page_raw() and avoid the read-modify-write cycle

Ah I see you actually want this for cases where the block size is > 4096.
We should probably add a fast-path to mtd_write_page() then to skip the read-modify-write cycle if the whole sector is written - #19252

I wanted to implement both mtd_write_page() and mtd_write_page_raw() for some time but since this PR stalled I think we should just stick to one of them for now.
But I don't mind to switch to mtd_write_page_raw() if you think it is more suitable.

Huh the MTD layer should take care of splitting up the transfers.
What do you mean here?

This buffer is used to store data coming from the endpoint. Since USB endpoints transfers (for Full-Speed device) are capped at 64 bytes, this buffer stores each transfer until it reaches the MTD device flash page size.
For example, if the MTD page size is 512 bytes. We need to have 8 endpoints transfer of 64 bytes to fill this buffer so we can trigger a write to the MTD device.
Moreover, since I cannot report the real page size of the MTD device if it's less than 512 bytes (e.g for EEPROM). This buffer will then have to store multiples pages at once.
But I agree that the wording is weird. I can come up with something better.

You don't need to do this if you erase the sector on the first block and always write a whole sector. (mtd_write_page_raw() will not to the read-modify-write but just write and you don't need to write the whole sector at once).

There might be a slight performance benefit to it, but I'm not sure.

If you are the sole user of the MTD device you could also use mtd->work_area which is a per-device, dynamically allocated sector-sized buffer.

That would be a bit hacky though - but I also don't see a problem with just writing 64 bytes at a time.

You don't need to do this if you erase the sector on the first block and always write a whole sector.

Unfortunately, I am not sure I can guarantee I'll always write a whole sector. USB Host will use the reported page size and doesn't know about the sector size of the MTD device. So it may write only one or a few pages.

Then how do you decide when to flush?
Or are those writes just lost if the sector is not completed?

USB will always transfer a page¹ (in 64 byte chunks) but since we can only erase a sector, we have to do a read-modify write cycle.

Very true.

Buffering the page brings that down to one RMW cycle.

That's the idea.

To sum up, there are 3 cases to properly handle:

• USB reported page size == MTD page size

  In this case, (512, 1024, 2048, 4096), this is pretty easy. We store the 64 bytes chunks into a buffer (allocated dynamically or not) Since we have a whole MTD page (not a MTD sector) we can write it directly to the memory. (And we rely on the MTD implementation to handle the erase part).

• USB reported page size > MTD page size

This is the case for a few MTD type like EEPROM. In this case, we report an USB page size of 512 (the minimum allowed). So if MTD page size is 256. We have to write two MTD pages for each USB page.
This is also why we cannot export MTD device which have less than 512 bytes of memory. (Well we could cheat but this will just add more complexity for a small corner case and I believe this PR is already complex enough...)

• USB reported page size < MTD page size

This is a more tricky one, which is not yet support by this PR (even though there is some code laying around). For this case, we need to do some RMW cycle. But this will belong to a followup PR.

I would propose the following:
if the 'USB page' is smaller than the sector size, allocate a temporary buffer as you do here, but you may use malloc (as that's already used by mtd_pagewise - I think malloc on init is fine.
if the 'USB page' is greater or equal the sector size, don't allocate a buffer and write the bytes as you get them with mtd_write_page_raw(), ereasing the sector at the first block.

That way you don't have the memory overhead in the common case and don't have to tune config variables to fit the storage.

What do you think?

Performance-wise, the second idea is great but I need to check if it won't make the code unreadable because of the corner cases.
For the first idea, I think this will the most common pattern. I can switch to dynamically allocated buffer since we let the user chose which MTD to export through the test application. malloc() can then be call only and only if the user request the MTD device to be exported. This may save some memory on boards which have a several MTD that one can export but only need one to be exported.

But why tie this to page size? A page is just the largest transfer unit, but the MTD will already take care of that, so on the application/interface level it's rather arbitrary.

Sector size is the much more interesting value as it's the erase unit. If you report the sector size (and you will always get a full sector through MSC) you can just erase the sector at the start of the transfer and then write the 64 byte blocks as they come (without the RMW).

I see your point. This will increase performance for some MTD devices. So you suggest to erase the sector from USBUS then call mtd_write_page_raw() for each USB endpoints chunks received to avoid storing the content of a page ?
I can try to give it a go. I don't think such rework would be too painful.
Obviously, this won't work at all for sector size > 4096.

I am making progress here, I have a working version using sector size instead of page size.
I am now looking at the buffer side and I think we shouldn't drop it even if the USB block == MTD sector size:
This might impact performance on read operation. Currently, a single mtd_read() is performed and stored into the buffer. Then we slice the buffer through USB chunks. Dropping the buffer would force us to call mtd_read() for each USB transfer chunks and it seems sub-optimal to me.

Regarding the dynamical allocation of the buffer, I am not against. This can be done during the USB_MSC_SETUP_REQ_GML request. This way we can iterate through all exported MTD devices to USB. Determine which device has the greatest sector size and allocate this sector size to the buffer. No need for user configuration.

Note that even if multiple LUNs are used at the same time. Operation can only be done one LUN at a time. So having one buffer is completely fine for all LUNs (read or write operations).

Why - 4?

This is the additional packet length minus the "default" data of this packet.
a SCSI inquiry response will always include the 4 first bytes, but you may have to pass additional information. Thus, you pass the number additional bytes through this field.

I'll add a comment.

What are those magic numbers?

Is undocumented black magic numbers, a valid answer here ?

Where did you get them from?

Hmm, I think it was on some source code provided by ST for a STM32F1 that I used back in 2019.
I did have a look at Zephyr a few months ago too, but it was the same. (Just hexadecimal value used to fill their buffer).
I'll have a look at it tonight. I have a bunch of USB MSC related document on my home laptop. I'll see if I can give you more insight then.

This will also need a Makefile.ci
You can create it with

make generate-Makefile.ci

Why is this needed here if we already have auto_init_usbus?

Same here

Should this be moved to auto_init_usb.c?