-
Notifications
You must be signed in to change notification settings - Fork 8.1k
STM32 UDC: fix transmission on halted endpoints #93796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
STM32 UDC: fix transmission on halted endpoints #93796
Conversation
6045959
to
8872a8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message does not look nice. You can have up to 75 characters per line in the commit message.
Manage endpoint halt state explicitly in set/clear halt functions.
Prevent transmissions on halted endpoints in enqueue function.
Trigger retransmission post-halt clearance if applicable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
if (buf && USB_EP_DIR_IS_IN(cfg->addr) && !udc_ep_is_busy(cfg)) { | ||
udc_stm32_tx(dev, cfg, buf); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the OUT direction? Do you need to restart anything explicitly here? I did not have time to look at the OUT path in my suggestion, so I just added a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point regarding the OUT direction. For OUT endpoints, I will add logic to re-arm the endpoint (e.g., by calling udc_stm32_rx()) to ensure it is ready to receive new data.
I’ll update the code accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking comments.
drivers/usb/udc/udc_stm32.c
Outdated
if (USB_EP_GET_IDX(cfg->addr)) { | ||
struct net_buf *buf = udc_buf_peek(cfg); | ||
|
||
if (buf && USB_EP_DIR_IS_IN(cfg->addr) && !udc_ep_is_busy(cfg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (buf && USB_EP_DIR_IS_IN(cfg->addr) && !udc_ep_is_busy(cfg)) { | |
if (buf != NULL && USB_EP_DIR_IS_IN(cfg->addr) && !udc_ep_is_busy(cfg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
drivers/usb/udc/udc_stm32.c
Outdated
* If endpoint is non-control, has pending data, is IN direction, | ||
* and not busy, trigger retransmission to continue data flow | ||
*/ | ||
if (USB_EP_GET_IDX(cfg->addr)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (USB_EP_GET_IDX(cfg->addr)) { | |
if (USB_EP_GET_IDX(cfg->addr) != 0U) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
drivers/usb/udc/udc_stm32.c
Outdated
* Endpoint is halted (stalled) by the host | ||
* Skip transmission to avoid protocol violations | ||
* and potential data corruption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer with terminal dots to explicitly end each sentences.
* Endpoint is halted (stalled) by the host | |
* Skip transmission to avoid protocol violations | |
* and potential data corruption | |
* Endpoint is halted (stalled) by the host. | |
* Skip transmission to avoid protocol violations | |
* and potential data corruption. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
8872a8f
to
027ca86
Compare
Hi @marwaiehm-st , Can you confirm that with this patch the |
027ca86
to
9f003e7
Compare
Hi @nandojve , In my testing, I observed the following results: With v4.1.0 (sudo ./testusb -v 512 -D /dev/bus/usb/001/021): ./testusb: /dev/bus/usb/001/021 may see only control tests
high speed /dev/bus/usb/001/021 0
/dev/bus/usb/001/021 test 0, 0.000008 secs
/dev/bus/usb/001/021 test 9 --> 32 (error 32)
/dev/bus/usb/001/021 test 10 --> 32 (error 32) With the main branch (sudo ./testusb -v 512 -D /dev/bus/usb/001/022): ./testusb: /dev/bus/usb/001/022 may see only control tests
full speed /dev/bus/usb/001/022 0
/dev/bus/usb/001/025 test 0, 0.000007 secs
/dev/bus/usb/001/025 test 9, 1.186878 secs
/dev/bus/usb/001/025 test 10, 4.333058 secs Based on these results, the issue is no longer reproduced on the latest main branch. However, this patch does not resolve the issue; the fix appears to be present in main independently of this patch. |
Hi @marwaiehm-st , Just to be sure, did you tested high-speed, right ? In the #94001 I provided a sequence that show to pass on all tests for full/high-speed and the only fails is test #13 in high-speed. |
@nandojve, yes i tested high-speed using this PR #89866, there are no errors: sudo ./testusb -v 512 -D /dev/bus/usb/001/034
./testusb: /dev/bus/usb/001/034 may see only control tests
high speed /dev/bus/usb/001/034 0
/dev/bus/usb/001/034 test 0, 0.000007 secs
/dev/bus/usb/001/034 test 9, 0.623093 secs
/dev/bus/usb/001/034 test 10, 3.154529 secs
|
Hi @marwaiehm-st , I noted that I can see only 3 tests in your list and the 13 is not in. Are you enabling all the test list using the I don't want to bother you but I really want to make sure USB is working 100% on the U5A. |
0daa232
to
9f003e7
Compare
Manage endpoint halt state explicitly in set/clear halt functions. Prevent transmissions on halted endpoints in enqueue function. Trigger retransmission post-halt clearance if applicable. Signed-off-by: IBEN EL HADJ MESSAOUD Marwa <[email protected]>
9f003e7
to
07c7d16
Compare
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
@nandojve, i hadn’t used that command before connecting the device. After applying all the patches you mentioned in the #94001 and then running the command before connecting the USB device, I noticed a difference in the test results. Now, I can see a more complete list of tests, including test 13. ![]() |
Thank you for looking on this, I really appreciate it. Yes, usually when a important feature fails, test 13 for instance, many of the remaining can fail because device stay in a bad state. You can then run one by one if you want to check the other tests by add I notice that something changed in mainline that bring a regression in the 14 and 21 in regards to my branch: HS
FS
So, 13 still failing as expected in the HS and passing in the FS but both 14 and 21 started to fail where:
You can reproduce above using the https://github.com/nandojve/zephyr/tree/stm/fix_usb_fs_hs_linux_compliance_tests_more_fixes Do you know any changes in mainline about ep0 maintenance ? |
Superseded by 95857 |
Fixes: #91483
This fix is based on the patch by @jfischer-no : #91067 (comment)