drivers: udc: follow-up, fix race conditions in nRF USBD and Kinetis UDC drivers #53489
Conversation
Immediately return NULL if endpoint configuration is not available. Signed-off-by: Johann Fischer <[email protected]>
jfischer-no
added
area: Drivers
area: USB
jfischer-no
requested review from
nashif,
galak and
MaureenHelm
as code owners
drivers/usb/udc/udc_common.c
Outdated
| if (unlikely(ep_cfg == NULL)) { | ||
| __ASSERT(false, "ep 0x%02x is not available", ep); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I don't like such concepts. I'd prefer to replace it with __ASSERT(ep_cfg != NULL, "ep 0x%02x is not available", ep);. The thing is, if this is called with invalid ep, then the code is already known to be wrong and continuting execution is going no good. It is better if it just crashes with NULL pointer dereference if the code is wrong and asserts are disabled instead of masking the issue (which essentially makes debugging harder and allows bugs to go unnoticed for long time - or even worse, worked around in higher layers).
There was a problem hiding this comment.
It is better if it just crashes with NULL pointer dereference if the code is wrong and asserts are disabled instead of masking the issue...
That actually makes perfect sense in this case. I will change it.
drivers/usb/udc/udc_common.c
Outdated
| if (unlikely(ep_cfg == NULL)) { | ||
| __ASSERT(false, "ep 0x%02x is not available", ep); | ||
| } else { | ||
| ep_cfg->stat.busy = busy; | ||
| } | ||
| } |
There was a problem hiding this comment.
I would definitely prefer __ASSERT(ep_cfg != NULL, "ep 0x%02x is not available", ep); followed by unconditional ep_cfg->stat.busy = busy;. This way if asserts are disabled the code will simply fail writing to NULL pointer and will make the issue obvious (which will save a lot of time debugging it later).
| LOG_DBG("ep 0x%02x buf busy", cfg->addr); | ||
| if (unlikely(usbfsotg_bd_is_busy(bd))) { | ||
| LOG_ERR("ep 0x%02x buf busy", cfg->addr); | ||
| __ASSERT_NO_MSG(false); |
There was a problem hiding this comment.
That checks controller buffer flag, here the buffer should not be owned by the controller.
There was a problem hiding this comment.
Then I would prefer the assert to be moved before the if, e.g. __ASSERT_NO_MSG(!usbfsotg_bd_is_busy(bd));
Add helper functions to set and check endpoint busy state. Signed-off-by: Johann Fischer <[email protected]>
Periodic enqeueu of buffers can cause a attempt to start a new transfer to host even though an IN endpoint is already busy. Use busy state flags to explicitly mark an endpoint busy. Signed-off-by: Johann Fischer <[email protected]>
Pending state flag was only used by the UDC nRF USBD driver. With the introduction of busy state flag it is no longer needed and can be removed. Signed-off-by: Johann Fischer <[email protected]>
Periodic enqeueu of buffers can cause a attempt to start a new transfer even though an endpoint is already busy. Split usbfsotg_xfer_start() into two function, one to start next transfer and another to continue the transfers, and use busy state flags to explicitly mark an endpoint busy. Signed-off-by: Johann Fischer <[email protected]>
Remove workaround for the UDC enqueue bug in experimental CDC ACM implementation. Signed-off-by: Johann Fischer <[email protected]>
The description was still from the early stages of development. Signed-off-by: Johann Fischer <[email protected]>
jfischer-no
force-pushed
the
pr-udc-follow-up
branch
from
ae3b924 to
6570896
Compare
| LOG_DBG("ep 0x%02x buf busy", cfg->addr); | ||
| if (unlikely(usbfsotg_bd_is_busy(bd))) { | ||
| LOG_ERR("ep 0x%02x buf busy", cfg->addr); | ||
| __ASSERT_NO_MSG(false); |
There was a problem hiding this comment.
Then I would prefer the assert to be moved before the if, e.g. __ASSERT_NO_MSG(!usbfsotg_bd_is_busy(bd));
follow-up on #38679 , fix race conditions in nRF USBD and Kinetis UDC drivers