lowRISC / lowRISC/opentitan

[usbdev] Race condition, SETUP packets

Open
#24,023 8 comments 0 reactions 2 assignees View on GitHub

@alees24 is already working on this.

Since Jul 16, 2024.

IP:usbdev
Dominant language
SystemVerilog
Stars
3.6k
Forks
1.1k
Avg merge
2d 22h
Merged PRs (30d)
141

Description

### Description

The documentation says the following about the [configin . rdy](https://opentitan.org/book/hw/ip/usbdev/doc/registers.html#configin--rdy) bit:

> This bit should be set to indicate the buffer is ready for sending. It will be cleared when the ACK is received indicating the host has accepted the data.
>
> This bit will also be cleared if an enabled SETUP transaction is received on the endpoint. This allows use of the IN channel for transfer of SETUP information. The original buffer must be resubmitted after the SETUP sequence is complete. A link reset also clears the bit. In either of the cases where the hardware cancels the transaction it will also set the pend bit.

A SETUP transaction typically consists of the USB host sending a SETUP packet, and then retrieving one or more IN packets in response. Software on OT will handle the reception of a SETUP packet in the RXFIFO by preparing (part of) a response and then setting the above `configin.rdy` bit to allow the host to retrieve the data. It appears that the functionality for the hardware to clear the `rdy` bit upon reception of a SETUP packet is meant for cases where the USB host will issue a new SETUP request before having retrieved every part of the previous response. This is a good feature to have, however, it seems to me that there is a gap: If a second SETUP is received while processing of the first one has not yet completed. In that case, when processing of the first SETUP packet completes and is ready to set the `rdy` bit, the second SETUP packet has already been received, and hence the second SETUP will not cause clearing of the `rdy` bit. Instead, the USB host will get confused, as it receives stale data and thinks that they were in response to the most recent request.

In my view, we need a hardware feature similar to how the i2cdev treats non-empty ACQ fifo. That is, if the USB rxfifo contains one or more SETUP packets (meaning that there are already requests which the software has not yet seen or processed), then an attempt by software to set the `rdy` bit should immediately result in the `pend` bit being set (and the `rdy` bit not actually being set). This way, the end result will be the same, whether the second SETUP packet arrives one clock cycle before or after the software attempts to set the `rdy` bit based on a previous SETUP packet.

I am wondering if we can find a software workaround for the A1 hardware. The only thing I can think of is something like every time you want to set the `rdy` bit on the control endpoint, then you have to disable reception of SETUP packets, inspect the rxfifo to verify that there are none in queue, then set the `rdy` bit, and then re-enable processing of SETUP packets.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.