Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle the closing of endpoints on RP2040 #1233

Merged
merged 3 commits into from Dec 9, 2021
Merged

Handle the closing of endpoints on RP2040 #1233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48e1f6d
Handle the closing of endpoints on RP2040
vmilea Dec 4, 2021
36e69b8
Remove buffer reclaim logs
vmilea Dec 7, 2021
ae970ba
Handle xfer events before closing EP
vmilea Dec 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 23 additions & 5 deletions src/portable/raspberrypi/rp2040/dcd_rp2040.c
View file
Open in desktop
Expand Up @@ -83,7 +83,7 @@ static void _hw_endpoint_alloc(struct hw_endpoint *ep, uint8_t transfer_type)

assert(((uintptr_t )next_buffer_ptr & 0b111111u) == 0);
uint dpram_offset = hw_data_offset(ep->hw_data_buf);
assert(hw_data_offset(next_buffer_ptr) <= USB_DPRAM_MAX);
hard_assert(hw_data_offset(next_buffer_ptr) <= USB_DPRAM_MAX);

pico_info(" Alloced %d bytes at offset 0x%x (0x%p)\r\n", size, dpram_offset, ep->hw_data_buf);

Expand All @@ -93,7 +93,6 @@ static void _hw_endpoint_alloc(struct hw_endpoint *ep, uint8_t transfer_type)
*ep->endpoint_control = reg;
}

#if 0 // todo unused
static void _hw_endpoint_close(struct hw_endpoint *ep)
{
// Clear hardware registers and then zero the struct
Expand All @@ -103,14 +102,29 @@ static void _hw_endpoint_close(struct hw_endpoint *ep)
*ep->buffer_control = 0;
// Clear any endpoint state
memset(ep, 0, sizeof(struct hw_endpoint));

// Reclaim buffer space if all endpoints are closed
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)
{
pico_info(" reclaim buffer space\n");
next_buffer_ptr = &usb_dpram->epx_data[0];
}
}

static void hw_endpoint_close(uint8_t ep_addr)
{
struct hw_endpoint *ep = hw_endpoint_get_by_addr(ep_addr);
_hw_endpoint_close(ep);
}
#endif

static void hw_endpoint_init(uint8_t ep_addr, uint16_t wMaxPacketSize, uint8_t transfer_type)
{
Expand Down Expand Up @@ -224,6 +238,8 @@ static void reset_non_control_endpoints(void)

// clear non-control hw endpoints
tu_memclr(hw_endpoints[1], sizeof(hw_endpoints) - 2*sizeof(hw_endpoint_t));

pico_info(" reclaim buffer space\n");
hathach marked this conversation as resolved.
Show resolved Hide resolved
next_buffer_ptr = &usb_dpram->epx_data[0];
}

Expand Down Expand Up @@ -479,10 +495,12 @@ void dcd_edpt_clear_stall(uint8_t rhport, uint8_t ep_addr)
void dcd_edpt_close (uint8_t rhport, uint8_t ep_addr)
{
(void) rhport;
(void) ep_addr;

// usbd.c says: In progress transfers on this EP may be delivered after this call
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.
Copy link
Owner

@hathach hathach Dec 7, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should still be processed if data successfully travel through usb bus.

hw_endpoint_close(ep_addr);
}

void dcd_int_handler(uint8_t rhport)
Expand Down
4 changes: 3 additions & 1 deletion src/portable/raspberrypi/rp2040/rp2040_usb.c
View file
Open in desktop
Expand Up @@ -294,7 +294,9 @@ bool hw_endpoint_xfer_continue(struct hw_endpoint *ep)
// Part way through a transfer
if (!ep->active)
{
panic("Can't continue xfer on inactive ep %d %s", tu_edpt_number(ep->ep_addr), ep_dir_string);
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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@hathach hathach Dec 8, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch submitted: ae970ba

}

// Update EP struct from hardware state
Expand Down