drivers: usb: udc: stm32: handle endpoint halt properly#95857
drivers: usb: udc: stm32: handle endpoint halt properly#95857fabiobaltieri merged 1 commit intozephyrproject-rtos:mainfrom
Conversation
zephyrbot
requested review from
FRASTM,
GeorgeCGV,
djiatsaf-st,
erwango,
etienne-lms,
gautierg-st,
jfischer-no,
marwaiehm-st and
tmon-nordic
|
FTR, USB3CV MSC Tests pass without errors on Build command: |
This is the GetDescriptor() for String descriptor index 0 (wLANGID array) that is called with wLength=4096 during the USB3CV MSC Tests. I think USB device stack should allow passing such requests, given that the maximum real length will fit in memory. However, it is not clear to how to implement the limiting (and it is out of scope here). |
jfischer-no
left a comment
jfischer-no
left a comment
There was a problem hiding this comment.
@nandojve Can you please check it?
jfischer-no
requested review from
nandojve
and removed request for
GeorgeCGV
|
Hi @mathieuchopstm , @jfischer-no , The outcome is that it recovers when an error happens in the test 13 and not fail in the 21 (HS mode) . What I can not understand is why it works in HSFSSetup |
mathieuchopstm
dismissed stale reviews from jfischer-no, erwango, and etienne-lms
via
b78b0c4
mathieuchopstm
force-pushed
the
stm32_usb_fix_ep_halt
branch
from
b78b0c4 to
4c8d68f
Compare
|
🤦 pushed to wrong branch... sorry. (note to self: @nandojve I have only tested on testusb results(my - dmesg logs
+ Zephyr logs
./testusb: /dev/bus/usb/003/017 may see only control tests
full speed /dev/bus/usb/003/017 0
/dev/bus/usb/003/017 test 0, 0.000005 secs
/dev/bus/usb/003/017 test 1, 1.657647 secs
/dev/bus/usb/003/017 test 2, 1.300067 secs
/dev/bus/usb/003/017 test 3, 0.935144 secs
/dev/bus/usb/003/017 test 4, 0.787059 secs
/dev/bus/usb/003/017 test 5, 48.896797 secs
/dev/bus/usb/003/017 test 6, 39.109974 secs
/dev/bus/usb/003/017 test 7, 25.250139 secs
/dev/bus/usb/003/017 test 8, 20.278973 secs
/dev/bus/usb/003/017 test 9, 1.938179 secs
/dev/bus/usb/003/017 test 10, 6.831558 secs
/dev/bus/usb/003/017 test 11, 8.277230 secs
- unlink retry [many times]
/dev/bus/usb/003/017 test 12, 8.077155 secs
- unlink retry [once]
/dev/bus/usb/003/017 test 13, 7.142753 secs
/dev/bus/usb/003/017 test 14 --> 110 (error 110)
- ctrl_out write failed, code -110, count 0
/dev/bus/usb/003/017 test 15 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 16 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 17 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 18 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 19 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 20 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 21 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 22 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 23 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 24 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 25 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 26 --> 110 (error 110)
- set altsetting to 0 failed, -110
/dev/bus/usb/003/017 test 27, 48.742084 secs
+ [00:04:26.258,000] <err> udc: Failed to allocate net_buf 0, ep 0x80
+ [00:04:26.265,000] <err> usbd_ch9: Buffer for data|status is missing
+ [00:04:26.272,000] <err> usbd_ch9: Malformed setup packet
+ [00:04:26.278,000] <err> udc: Failed to allocate net_buf 0, ep 0x80
+ [00:04:26.285,000] <err> usbd_ch9: Buffer for data|status is missing
+ [00:04:26.292,000] <err> usbd_ch9: Malformed setup packet
+ [00:04:26.299,000] <err> udc: Failed to allocate net_buf 0, ep 0x80
+ [00:04:26.305,000] <err> usbd_ch9: Buffer for data|status is missing
+ [00:04:26.312,000] <err> usbd_ch9: Malformed setup packet
/dev/bus/usb/003/017 test 28, 38.911322 secs
+ [00:05:14.318,000] <err> udc: Failed to allocate net_buf 0, ep 0x80
+ [00:05:14.325,000] <err> usbd_ch9: Buffer for data|status is missing
+ [00:05:14.332,000] <err> usbd_ch9: Malformed setup packet
+ [00:05:14.338,000] <err> udc: Failed to allocate net_buf 0, ep 0x80
+ [00:05:14.345,000] <err> usbd_ch9: Buffer for data|status is missing
+ [00:05:14.352,000] <err> usbd_ch9: Malformed setup packet
+ [00:05:14.359,000] <err> udc: Failed to allocate net_buf 0, ep 0x80
+ [00:05:14.365,000] <err> usbd_ch9: Buffer for data|status is missing
+ [00:05:14.372,000] <err> usbd_ch9: Malformed setup packet
/dev/bus/usb/003/017 test 29 --> 32 (error 32)
- ep 01 couldn't clear halt, -32
- toggle sync failed, iterations left 999On second run, during test 13 or 14: We'd like to integrate (more or less) small fixes one by one rather than try to fix everything at once, so hopefully this is fine. |
|
FTR, there seems to be an issue on the HAL side as it declares If you try to run the zephyr/drivers/usb/udc/udc_stm32.c Line 985 in 67f2464 with #define EP_MPS 512
|
|
Hi @mathieuchopstm ,
As per USB 2.0 specs the max endpoint is 1024 HS and 1023 FS in the Isochronous mode. If the interface is a high speed and high bandwidth endpoint it can specifies whether it requires two or three transactions per microframe which can led to use 3072 bytes max or 2|3 times 1024. This means that the EP max size depends on Besides, this patch still don't pass HS |
|
@nandojve Even if some test failures remain, do you think this should block the PR ? Edit: For correctness, maybe we should remove |
|
Hi @erwango ,
I have the same impression. The PR already improved the situation and I don't want to block it. This is why I report the details about EP size and latest test status.
Yes, I think it makes sense to remove the linked issue till a complete fix is made. |
Definitely, this is not a correct fix - but it allows the
A more appropriate fix is upcoming (although I did not take the multi-transactions into account... I'll make sure to pay attention to it) For the issue, I seem unable to unlink it from this PR. I'll re-open it manually if merging closes it. |
drivers/usb/udc/udc_stm32.c
Outdated
| * Hardware will clear halt bit of EP0 automatically such | ||
| * that software never sees control endpoints as halted. |
There was a problem hiding this comment.
I think that is not quite correct, and I suggest removing it. For example, if the controller is DWC2, then the controller writes received setup packet to the FIFO regardless of NAK, STALL, or available FIFO size. It is up to the driver, then, to handle it properly, see dwc2_handle_evt_setup() in udc_dwc2.c.
There was a problem hiding this comment.
@jfischer-no Don't hesitate to NACK if you really mean to. Otherwise I take it as a non blocking comment.
There was a problem hiding this comment.
I was referring to the following sentence from STM32 RefMan (for SNPS DWC2 IP):
The ST USB IP has a similar behavior:
There was a problem hiding this comment.
Simplified the comment to merely state we ignore control EP, without getting into details. PTAL.
drivers/usb/udc/udc_stm32.c
Outdated
| /* | ||
| * Endpoint is currently halted and should not | ||
| * transmit any data until host clears halt bit. | ||
| */ |
There was a problem hiding this comment.
This comment is redundant, similar is stated in the logging message.
drivers/usb/udc/udc_stm32.c
Outdated
| */ | ||
| LOG_DBG("skip enqueue for halted ep 0x%02x", epcfg->addr); | ||
| } else { | ||
| /* Endpoint not halted: we can transmit */ |
jfischer-no
left a comment
jfischer-no
left a comment
There was a problem hiding this comment.
I think it is better to fix #95857 (comment)
Handle endpoints in halted state properly by marking endpoints as halted when appropriate, and inhibiting transfers involving halted endpoints. Co-authored-by: IBEN EL HADJ MESSAOUD Marwa <marwa.ibenelhadjmessaoud-ext@st.com> Signed-off-by: Mathieu CHOPLAIN <mathieu.choplain-ext@st.com>
mathieuchopstm
dismissed stale reviews from nandojve and erwango
via
b968d45
mathieuchopstm
force-pushed
the
stm32_usb_fix_ep_halt
branch
from
4c8d68f to
b968d45
Compare
|
8 participants
Cleaned up version of #93796. Handle endpoints in halted state properly by marking endpoints as halted when appropriate, and inhibiting transfers involving halted endpoints.
Partial fix for #91483