drivers: usb: udc: numaker: fix Control transfer issues#105824
drivers: usb: udc: numaker: fix Control transfer issues#105824ccli8 wants to merge 2 commits intozephyrproject-rtos:mainfrom
Conversation
For HSUSBD, in Control transfer, the IN/OUT triggers for Statue In/OUT stages are redundant because the hardware will handle them via NAKCLR bit. Signed-off-by: Chun-Chieh Li <ccli8@nuvoton.com>
Remove the error check because it may misjudge some right cases. For example, for HSUSBD, in the case Setup-DataIn-StatusOut and immediately following Setup-DataOut-StatusIn, next DataOut has arrived but user buffer for OUT transfer may only ready for previous StatusOut at this time. Signed-off-by: Chun-Chieh Li <ccli8@nuvoton.com>
|
|
Needs to be reworked based on #103493 |
tmon-nordic
left a comment
tmon-nordic
left a comment
There was a problem hiding this comment.
I think this PR can be closed, because I have fixed these issues (and few others) in rework-udc. I recommend that you look into the "host timeouts during control transfers" which was not covered with my fixes. My fixes only made the normal control transfers working (before rework-udc, the control transfer handling would hang after just a few quick control read followed by control write pairs).
| /* Enable CEP interrupt */ | ||
| base->CEPINTEN |= HSUSBD_CEPINTEN_TXPKIEN_Msk; | ||
| /* Enable CEP interrupt */ | ||
| base->CEPINTEN |= HSUSBD_CEPINTEN_TXPKIEN_Msk; |
There was a problem hiding this comment.
base->CEPINTEN |= and base->CEPINTEN &= ~ are not atomic. Both these essentially translate to:
- read CEPINTEN value to a register
- modify value in a register
- write value to CEPINTEN
In the driver, the CEPINTEN is modified in both thread context and interrupt context. This leads to race conditions.
In rework-udc I have removed the dynamic CEPINTEN masking for everything except HSUSBD_CEPINTEN_RXPKIEN_Msk. When there is just one interrupt bit changed like this the race consequences are significantly lowered. I would recommend you to try to also eliminate the need for dynamically masking HSUSBD_CEPINTEN_RXPKIEN_Msk.
| if (len == 0) { | ||
| base->CEPCTL = HSUSBD_CEPCTL_ZEROLEN | | ||
| HSUSBD_CEPCTL_NAKCLR_Msk; | ||
| } else { | ||
| __ASSERT_NO_MSG(len <= ep_cur->mps); | ||
| base->CEPTXCNT = len; | ||
| } |
There was a problem hiding this comment.
This is another problematic part that I fixed in rework-udc. Hopefully when len is zero, it is possible to just write 0 to base->CEPTXCNT. This eliminates the race related to accessing base->CEPCTL NAKCLR bit.
|
@tmon-nordic Many thanks for your rework and comment. I will do regression test and maybe re-raise related PR based on my test result. |
4 participants
For Nuvoton NuMaker SoC series, for UDC driver, this fixes some Control transfer issues: