usb: device_next: Allocate control buffers in stack#103493
usb: device_next: Allocate control buffers in stack#103493carlescufi merged 2 commits intozephyrproject-rtos:mainfrom
Conversation
zephyrbot
added
area: USB
zephyrbot
requested review from
GeorgeCGV,
JarmouniA,
MarkWangChinese,
RichardSWheatley,
aaronyegx,
etienne-lms,
fabiobaltieri,
jfischer-no,
josuah,
juickar,
keith-zephyr,
mathieuchopstm,
maxd-nordic,
mmahadevan108 and
nashif
tmon-nordic
force-pushed
the
rework-udc
branch
from
2473bf0 to
caa9224
Compare
zephyrbot
requested review from
AlessandroLuo,
Chenhongren,
Dino-Li,
GTLin08 and
erwango
|
@mathieuchopstm can you please go through new udc_stm32 commits and confirm that it is fine to squash them all together? |
mathieuchopstm
left a comment
mathieuchopstm
left a comment
There was a problem hiding this comment.
I will do some testing on my side and report back.
tmon-nordic
force-pushed
the
rework-udc
branch
3 times, most recently
from
b56d4da to
ef19dce
Compare
|
Patches for udp, usbc, usbhs are ready. Just waiting #102041 be merged to push patches here. |
| bi->zlp = false; | ||
| } | ||
|
|
||
| void udc_setup_received(const struct device *dev, void *setup, bool valid) |
There was a problem hiding this comment.
It looks like the 'valid' parameter can be omitted and the "setup" parameter can be used directly to indicate a non-valid setup packet by setting it to NULL.
void udc_setup_received(const struct device *dev, const void *const setup)There was a problem hiding this comment.
Removed the bool valid parameter.
| while ((buf = udc_buf_get(cfg_out))) { | ||
| struct udc_buf_info *bi = udc_get_buf_info(buf); | ||
|
|
||
| if (bi->setup) { | ||
| break; | ||
| } | ||
|
|
||
| udc_submit_ep_event(dev, buf, -ECONNRESET); | ||
| } |
There was a problem hiding this comment.
Similar as in the other PR review, result of the assignment operator should not be used, this needs to be
buf = udc_buf_get(cfg_out);
while (buf != NULL) {
struct udc_buf_info *bi = udc_get_buf_info(buf);
if (bi->setup) {
break;
}
udc_submit_ep_event(dev, buf, -ECONNRESET);
buf = udc_buf_get(cfg_out);
}There was a problem hiding this comment.
I replaced the while loops with for loops. However I truly I disagree with your statement. There are numerous places in Zephyr source code where this pattern is used, for example
zephyr/subsys/bluetooth/host/iso.c
Line 469 in 37eb5ad
zephyr/subsys/bluetooth/host/l2cap.c
Lines 2399 to 2406 in 37eb5ad
zephyr/subsys/bluetooth/host/l2cap.c
Line 281 in 37eb5ad
I would appreciate if you updated checkpatch.pl so there could be a definitive rule to follow on this matter.
| const struct udc_api *api = dev->api; | ||
| struct udc_ep_config *cfg; | ||
| struct udc_buf_info *bi; | ||
| struct udc_data *data = dev->data; |
There was a problem hiding this comment.
const struct udc_api *api = dev->api;
struct udc_data *data = dev->data;
struct udc_ep_config *cfg;
struct udc_buf_info *bi;There was a problem hiding this comment.
The code did follow the Reverse Christmas trees, which I thought was also the case in Zephyr. I have no idea what the rule here is that you are enforcing, but I applied your suggestion.
| if (cfg) { | ||
| while ((buf = udc_buf_get(cfg))) { | ||
| net_buf_unref(buf); | ||
| } | ||
| } | ||
|
|
||
| cfg = udc_get_ep_cfg(dev, USB_CONTROL_EP_OUT); | ||
| if (cfg) { | ||
| while ((buf = udc_buf_get(cfg))) { | ||
| net_buf_unref(buf); | ||
| } | ||
| } |
There was a problem hiding this comment.
Result of the assignment operator should not be used in while(). Similar as in the other PR review, where I provided example how it should be implemented.
There was a problem hiding this comment.
Replaced with for loops.
| config->bdt[out_even].set.bd_ctrl = 0; | ||
| config->bdt[out_odd].set.bd_ctrl = 0; | ||
| config->bdt[in_even].set.bd_ctrl = 0; | ||
| config->bdt[in_odd].set.bd_ctrl = 0; |
There was a problem hiding this comment.
const struct usbfsotg_config *config = dev->config;
config->bdt[0].set.bd_ctrl = 0;
config->bdt[1].set.bd_ctrl = 0;
config->bdt[2].set.bd_ctrl = 0;
config->bdt[3].set.bd_ctrl = 0;There was a problem hiding this comment.
Replaced to adhere to your comment.
| struct net_buf *out_buf[2]; | ||
| bool busy[2]; | ||
| struct usb_setup_packet setup; | ||
| uint8_t setup_bytes; |
There was a problem hiding this comment.
Looks like there is no need to count the bytes and it can be just bool setup_valid; if you want to keep valid parameter.
There was a problem hiding this comment.
Replaced with bool setup_valid
| if (ep == USB_CONTROL_EP_IN) { | ||
| const struct udc_buf_info *bi = udc_get_buf_info(buf); |
There was a problem hiding this comment.
if (buf == NULL) {
return;
}
if (ep == USB_CONTROL_EP_IN) {
const struct udc_buf_info *bi = udc_get_buf_info(buf);
...There was a problem hiding this comment.
Changed to adhere to your comment (note that the nesting was there prior to this commit).
| } else if (ep_cfg->addr == USB_CONTROL_EP_IN) { | ||
| struct udc_buf_info *bi = udc_get_buf_info(buf); |
There was a problem hiding this comment.
Unnecessary else if
}
if (ep_cfg->addr == USB_CONTROL_EP_IN) {
struct udc_buf_info *bi = udc_get_buf_info(buf);There was a problem hiding this comment.
Changed to adhere to your comment.
| /** | ||
| * Controller performs Status OUT stage automatically after Data IN. | ||
| * | ||
| * When set, USB stack will not enqueue Status OUT buffer. | ||
| */ | ||
| uint32_t autostatus_after_data_in : 1; |
There was a problem hiding this comment.
Looks like pointless renaming. Keep out_ack variable name.
There was a problem hiding this comment.
Renamed, but I believe out_ack is extremely confusing and actually a bad name.
henrikbrixandersen
left a comment
henrikbrixandersen
left a comment
There was a problem hiding this comment.
This could use an entry in the migration guide.
If there are no more review rounds I suggest we add it later in a follow-up PR @tmon-nordic @jfischer-no |
| bi->zlp = false; | ||
| } | ||
|
|
||
| void udc_setup_received(const struct device *dev, void *setup) |
There was a problem hiding this comment.
I missed it last time, non-blocking but would be nice to have
void udc_setup_received(const struct device *dev, const void *const setup)
We do not break any USB device stack applications. |
Allocating buffer in response to control transfer is part of processing
and therefore should not be done in UDC driver but rather in the stack.
Simplify UDC driver design by moving all control transfer buffer
allocations and processing to USB stack.
New control transfer handling flow is as follows:
1. USB stack allocates and queues buffer for Setup stage. This informs
UDC driver that USB stack is ready to process new transfer.
2. UDC driver completes enqueued Setup buffer. SETUP data may have
been received by device before Setup buffer was enqueued.
If multiple SETUP data was received, UDC driver must complete
request using last received SETUP data.
3. USB stack processes request. USB stack is responsible for:
* Stalling control endpoint when request cannot be processed
* Allocating and enqueueing Data stage buffer if necessary
- Depending on enqueued endpoint (IN/OUT), UDC driver is
expected to complete the transfer. If host sends new SETUP
data for any reason (e.g. timeout), then Data stage buffer
must be completed with -ECONNRESET code.
- UDC driver may postpone processing Data IN until USB stack
enqueues new Setup stage buffer.
* Handling status stage if applicable
- If Data IN was enqueued, stack immediately allocates and
enqueues Status OUT stage buffer.
- If Data OUT was enqueued, stack waits for Data OUT buffer
completion before doing any further processing.
- If control transfer handling fails, stack is expected to
STALL control endpoint.
* Allocating and enqueuing Setup buffer.
- This informs UDC driver that stack has finished processing
control transfer. UDC driver may choose to start processing
Data IN and/or Status OUT only after new Setup buffer is
enqueued.
4. UDC driver must fail (complete with -ECONNRESET) any enqueued and
not completed Data and Status buffers if host sends new SETUP data.
UDC driver must be able to buffer last received SETUP data until
USB stack is ready (enqueues Setup buffer).
5. UDC driver must complete all Data and Status buffers before it
completes Setup buffer.
6. UDC driver may keep ownership of Setup, Data and/or Status buffers
across USB bus resets. USB stack does not attempt to dequeue any
control transfer buffer it enqueued.
This approach implicitly synchronizes UDC driver against USB stack,
which ensures that only one set of Setup/Data/Status buffers is
allocated at a time.
Another advantage of the rework is drawing a clear line on buffer
ownership. The buffer responsibilities are as follows:
* USB stack is the only entity that both allocates and frees the
buffers. Only USB stack is allowed to set "setup", "data" and
"status" fields in struct udc_buf_info.
* UDC takes ownership of buffers handed to it in udc_ep_enqueue().
* UDC releases buffer ownership by calling udc_submit_ep_event().
Because there is just a single place where the buffer ownership changes,
and all buffers must go USB stack (alloc) -> UDC (perform requests on
the bus) -> USB stack (free) route it would be possible in the future to
implement a "tap" similar to Linux usbmon that would ease debugging.
This commit significantly changes how USB stack communicates with UDC
drivers. It was decided that supporting both the old and new model
simultaneously would require way too much effort. Therefore all UDC
drivers were reworked. Following people worked on driver rework:
* Tomasz Moń - ambiq, dwc2, kinetis, mcux ip3511, nrf, numaker,
renesas ra, rpi pico, smartbond, virtual
* Mathieu Choplain - stm32
* Mark Wang - mcux ehci
* Johann Fischer - sam0, stm32
* Ren Chen - it82xx2
* Brandon Hurst - max32
* Gerson Fernando Budke - sam udp, sam usbc, sam usbhs
Co-authored-by: Mathieu Choplain <mathieu.choplain-ext@st.com>
Signed-off-by: Mathieu Choplain <mathieu.choplain-ext@st.com>
Co-authored-by: Johann Fischer <johann.fischer@nordicsemi.no>
Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Co-authored-by: Mark Wang <yichang.wang@nxp.com>
Signed-off-by: Mark Wang <yichang.wang@nxp.com>
Co-authored-by: Ren Chen <Ren.Chen@ite.com.tw>
Signed-off-by: Ren Chen <Ren.Chen@ite.com.tw>
Co-authored-by: Brandon Hurst <brandon.hurst@analog.com>
Signed-off-by: Brandon Hurst <brandon.hurst@analog.com>
Co-authored-by: Gerson Fernando Budke <nandojve@gmail.com>
Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Signed-off-by: Tomasz Moń <tomasz.mon@nordicsemi.no>
Preallocate and keep reusing the same buffer for SETUP data. This ensures that USB stack will always be able to handle SETUP stage regardless of UDC memory pool usage. Signed-off-by: Tomasz Moń <tomasz.mon@nordicsemi.no>
|
What is required to satisfy the "block: HW Test"? I have run it successfully on following boards: While originally there were "blind reworks" for few drivers (ambiq, numaker, renesas ra, smartbond, it82xx2), now I believe all are have been successfully run on hardware. From the "blind reworks", I ordered After @nandojve submitted the rework, I also run it on For the contributed driver changes, I believe the respective contributors did run it on their platforms and are satisfied with the results. |
Both before and after this PR:
Unstable API changes are not announced zephyr/doc/develop/api/api_lifecycle.rst Lines 69 to 71 in 5741b61 Does this qualify as exception? What exactly should be mentioned (preferably to be added in separate PR to save CI runs)? |
|
Applications may include drivers :) I'll probably send a PR clarifying what is meant by "Anything else that can affect the compilation or runtime behavior of an existing application" since it is certainly not the first time I see folks thinking "but this doesn't break the application!"
True. I guess it's at the discretion of the people responsible for the change to decide if they want to announce them. But for an unstable feature to have any chance to take off and get broadly adopted and eventually graduate, I think it's fair to say that users should be kept aware of any change that might break them. Maybe that's also wording that should be clarified in the linked section of the API lifecycle document.
+1, I like that wording. |
"USB device support consists of the USB device controller (UDC) drivers , USB device controller (UDC) driver API, and USB device stack, USB device stack API. The USB device controller (UDC) driver API provides a generic and vendor independent interface to USB device controllers, and although, there a is clear separation between these layers, the purpose of USB device controller (UDC) driver API is to serve Zephyr’s USB device stack exclusively." https://docs.zephyrproject.org/latest/connectivity/usb/device_next/usb_device.html |
It was for me. I ran a few more tests on the hardware and with USB/IP. |
nandojve
left a comment
nandojve
left a comment
There was a problem hiding this comment.
From my side, I fully tested on all HW that I worked. I did not tested on sam0 because I assume that @jfischer-no make the port and was using for their own testing, including this PR. Please, advice otherwise.
I tested sam0 (best USB device controller I have seen so far), stm32, kinetis, DWC2, mcux_ehci. Not all controllers pass all testusb tests, but this PR does not introduce these issues. |
13 participants
Includes commits from #102491. This PR is limited to UDC handling rework (starts at "usb: device_next: Allocate control OUT buffers in stack" commit).Reworked drivers:
nrf54h20dk/nrf54h20/cpuappandnrf54lm20dk/nrf54lm20a/cpuappnrf5340dk/nrf5340/cpuappfrdm_k64frpi_picomimxrt685_evkapollo4p_blue_kxr_evbnumaker_m2l31kiandnumaker_m5531ek_ra4m3(needs manifest: update hal_renesas revision #105197)da14695_dk_usbUDC drivers that need help/assignment - please add your name here if you are going to work on any of these:
Please open PR against source branch (tmon-nordic/rework-udc) once you have reworked any of above mentioned drivers.
Allocating buffer in response to control transfer is part of processing and therefore should not be done in UDC driver but rather in the stack.
Simplify UDC driver design by moving all control transfer buffer allocations and processing to USB stack.
New control transfer handling flow is as follows:
This approach implicitly synchronizes UDC driver against USB stack, which ensures that only one set of Setup/Data/Status buffers is allocated at a time.
Another advantage of the rework is drawing a clear line on buffer ownership. The buffer responsibilities are as follows:
Because there is just a single place where the buffer ownership changes, and all buffers must go USB stack (alloc) -> UDC (perform requests on the bus) -> USB stack (free) route it would be possible in the future to implement a "tap" similar to Linux usbmon that would ease debugging.