walkround for samd21 setup_packet overflow

[hathach]

Describe the PR
increase setup packet size from 8 to 12 10, since USB DMA controller is suspected to overflow the buffer with 2 extra bytes for CRC (somehow I decide there is enough space for CRC). Issue is discovered while troubleshooting adafruit/circuitpython#3912 . This is considered a walkaround/hack rather than actual fix. It may probably overflow more than 4 bytes in the future, currently 4 2 bytes is too cheap for a fix considering the amount of work to trace it down so far. We may take a look at it later on if needed (like figuring out why USB DMA decide to do so).

@dhalbert

[dhalbert]

In https://www.avrfreaks.net/forum/getting-extrange-data-enumeration-process-usb-uc3c?skey=multi_packet_size, someone asks about two extra bytes they are getting, and the reply is that those are the CRC bytes. This is for AVR, but it's a clue.

I think this may be the issue. From the data sheet, 32.6.2.7, page 799:

If data was successfully received, an ACK handshake is returned to the host if the endpoint is not
isochronous, and the number of received data bytes, excluding CRC, is written to
PCKSIZE.BYTE_COUNT. If the number of received data bytes is the maximum data payload specified by
PCKSIZE.SIZE no CRC data bytes are written to the data buffer. If the number of received data bytes is
the maximum data payload specified by PCKSIZE.SIZE minus one, only the first CRC data byte is written
to the data buffer If the number of received data is equal or less than the data payload specified by
PCKSIZE.SIZE minus two, both CRC data bytes are written to the data buffer.

Size PCKSIZE is 0x3, which means 64 bytes, and since the incoming packet is 8 bytes, then the USB peripheral thinks there's room for the two CRC bytes, and it writes 8 data bytes and the 2 CRC bytes, total 10.

The code does this:

  sram_registers[0][0].PCKSIZE.bit.MULTI_PACKET_SIZE = sizeof(_setup_packet);

but the next section explains the purpose of MULTI_PACKET_SIZE, which I think does not figure into the size calculation above:

32.6.2.8
Multi-Packet Transfers for OUT Endpoint
The number of data bytes received is stored in endpoint PCKSIZE.BYTE_COUNT as for normal
operation. Since PCKSIZE.BYTE_COUNT is updated after each transaction, it must be set to zero when
setting up a new transfer. The total number of bytes to be received must be written to
PCKSIZE.MULTI_PACKET_SIZE. This value must be a multiple of PCKSIZE.SIZE, otherwise excess
data may be written to SRAM locations used by other parts of the application.

So it seems like this is the correct fix, though it could be 8+2 instead of 8+4.

But what about SAMD51? I don't know why we don't see 10 bytes written there. The SAMD51 data sheet has what looks like identical language about writing the CRC bytes.

[hathach]

The total number of bytes to be received must be written to
PCKSIZE.MULTI_PACKET_SIZE. This value must be a multiple of PCKSIZE.SIZE, otherwise excess
data may be written to SRAM locations used by other parts of the application.

Thanks @dhalbert, the CRC part does make lots of sense. In case of setup packet, MULTI_PACKET_SIZE = 8 which is not multiple of 64, I am not sure if this would be counted as this.

Also there is a scenario when scheduling the ZLP ACK, we have to also prepare for the next setup
https://github.com/hathach/tinyusb/blob/master/src/portable/microchip/samd/dcd_samd.c#L248

The reason is next setup packet can occur just right after the ACK, before the IRQ is trigger and give tinyusb stack chance to call prepare_setup(). The interesting part is since we are doing this when scheduling the ZLP, the MULTI_PACKET_SIZE=0, maybe in that brief of time, when SETUP received, the incorrect value of MULTI_PACKET_SIZE confuse USB DMA logic making it thing that there is enough space to also write out the CRC. SAM51 doesn't behave the same is possibly due to timing/racing condition, since it run at much faster rate and have less chance for this to happen (tinyusb irq handler is trigger and able to correct the MULTI_PACKET_SIZE value to 8).

So it seems like this is the correct fix, though it could be 8+2 instead of 8+4.

Yeah, though since the memory pointer must be aligned of 4, it is more or less the same, I will update code with comment just to clear this up.

[dhalbert]

Do you think that MULTI_PACKET_SIZE=8 is wrong and it should be 64? I don't understand this well enough to comment.

[dhalbert]

If you merge this, in whatever form, I'll make a PR for CircuitPython and then test this more thoroughly, including on WIndows 7, etc. Thanks.

[hathach]

Do you think that MULTI_PACKET_SIZE=8 is wrong and it should be 64? I don't understand this well enough to comment.

I just poke at microchip driver, they use MULTI_PACKET_SIZE=8, so I think we are good, increase it has a greater chance for greater overflow I guess.

If you merge this, in whatever form, I'll make a PR for CircuitPython and then test this more thoroughly, including on WIndows 7, etc. Thanks.

merging now

PS: update comment to remove walkaround, it seems to be a "correct" fix.