Handle the closing of endpoints on RP2040 #1233
Conversation
There was a problem hiding this comment.
Thank you for your PR, all changes look great, except for the one with hw_endpoint_xfer_continue(). I think we should return true instead to notify actual happened event previously, even though endpoint is closed now (it probably doesn't make any difference with ISO endpoint). Though I am open to all suggestion.
PS: On a side note, I actually have an plan to remove dcd_edpt_close() since it causes quite a bit of trouble with other dcd. It is better with just reserving the highest bandwidth then disable/enable endpoint bit for SET_INTERFACE rather than going through the full close/open config cycle. But that could take time to test on all ports though.
| pico_info("Ignore xfer on inactive ep %d %s", tu_edpt_number(ep->ep_addr), ep_dir_string[tu_edpt_dir(ep->ep_addr)]); | ||
| _hw_endpoint_lock_update(ep, -1); | ||
| return false; |
There was a problem hiding this comment.
as far as I understand, this happens when an in-progress transfer complete on a newly closed endpoint. Though I think we need to return true instead to notify transfer complete to usbd.
There was a problem hiding this comment.
Returning true here would call driver->xfer_cb() with the closed EP, not sure if driver supports that.
My understanding is endpoints are closed in response to a control transfer on EP0, by which time all transfers on the endpoint being closed should have completed. So handling USB_INTS_BUFF_STATUS before USB_INTS_SETUP_REQ in the IRQ should prevent the transfer-after-close situation.
There was a problem hiding this comment.
If data is actually transferred via bus, it should be submitted to higher layer usbd/class driver. It would be their choice to whether process or ignore data. dcd shouldn't assume that.
My understanding is endpoints are closed in response to a control transfer on EP0, by which time all transfers on the endpoint being closed should have completed. So handling USB_INTS_BUFF_STATUS before USB_INTS_SETUP_REQ in the IRQ should prevent the transfer-after-close situation.
Yes, but since dcd_edpt_close() is called within tud_task() and controller may be already in the middle of transferring by the time we process the SET_INTERFACE. It is a race, there is always chance. To be honest, I don't actually see any issue with that.
Btw did you experience this often enough, and which callback it triggers ?
There was a problem hiding this comment.
It's a difficult topic. I've never seen a transfer event being delivered after the EP was closed. This change was a precaution based on the comment from usb.c. Do you agree to roll back to original panic()? If it comes up in practice, then it will be loud and clear and easier to deal with.
I propose also updating IRQ to handle buff_status before setup_req. This should take care of transfer completing immediately before EP close, even if there's still a chance for races like you said.
There was a problem hiding this comment.
It's a difficult topic. I've never seen a transfer event being delivered after the EP was closed. This change was a precaution based on the comment from usb.c. Do you agree to roll back to original panic()? If it comes up in practice, then it will be loud and clear and easier to deal with.
yeah sure, if you haven't run into this scenario. It is best to leave it as it is. I have an tendency to not write code for event that isn't happened yet (to avoid having code that isn't run).
I propose also updating IRQ to handle buff_status before setup_req. This should take care of transfer completing immediately before EP close, even if there's still a chance for races like you said.
Sure, if you could make an PR, I will be happy to review.
There was a problem hiding this comment.
Patch submitted: ae970ba
| pico_trace("dcd_edpt_close %02x\n", ep_addr); | ||
|
|
||
| // usbd.c says: In progress transfers on this EP may be delivered after this call. | ||
| // If the endpoint is no longer active when the transfer event is delivered, it will be ignored. |
There was a problem hiding this comment.
I think it should still be processed if data successfully travel through usb bus.
| bool reclaim_buffers = true; | ||
| for ( uint8_t i = 1; i < USB_MAX_ENDPOINTS; i++ ) | ||
| { | ||
| if (hw_endpoint_get_by_num(i, TUSB_DIR_OUT)->hw_data_buf != NULL || hw_endpoint_get_by_num(i, TUSB_DIR_IN)->hw_data_buf != NULL) | ||
| { | ||
| reclaim_buffers = false; | ||
| break; | ||
| } | ||
| } | ||
| if (reclaim_buffers) | ||
| { | ||
| next_buffer_ptr = &usb_dpram->epx_data[0]; | ||
| } |
There was a problem hiding this comment.
I have an question, We only reclaim buffer when all endpoints are closed, this does help with audio only class. But the issue still exists if we are using composite with additional endpoints right ?
PS: You don't have to update it work with composite class, since I have an plan to remove/refactor the dcd_edpt_close() in the near future anyway.
There was a problem hiding this comment.
That's true. A better solution would be to track and recycle individual chunks of memory. Or simply not deallocate when endpoints are closed.
There was a problem hiding this comment.
thank you for confirmation, it is still better than current state. It could buy me some time since I don't know when I could start refactoring.
Describe the PR
Close endpoints and reclaim buffer space on RP2040.
Additional context
Proposed fix for #1232.