| Message ID | 20240626124835.1023046-20-mathias.nyman@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1b349f214ac7b87fd2b854db0d4ce34895776642 |
| Headers | show |
| Series | xhci features for usb-next | expand |
Hi, This commit has now landed in 6.11-r1 and it appears to have a side effect of performing the halted endpoint check after every handled event, which wasn't done before. >+ /* update the urb's actual_length and give back to the core */ >+ if (usb_endpoint_xfer_control(&td->urb->ep->desc)) >+ process_ctrl_td(xhci, ep, ep_ring, td, ep_trb, event); >+ else if (usb_endpoint_xfer_isoc(&td->urb->ep->desc)) >+ process_isoc_td(xhci, ep, ep_ring, td, ep_trb, event); >+ else >+ process_bulk_intr_td(xhci, ep, ep_ring, td, ep_trb, event); >+ >+check_endpoint_halted: >+ if (xhci_halted_host_endpoint(ep_ctx, trb_comp_code)) >+ xhci_handle_halted_endpoint(xhci, ep, td, EP_HARD_RESET); > > return 0; Since stall handling is already present in functions like finish_td() or process_bulk_intr_td() called from the above snippet, and this change of behavior is not documented in the changelog, I doubt if it's intended? Regards, Michal
On 02/08/2024 12.21, Michał Pecio wrote: > Hi, > > This commit has now landed in 6.11-r1 and it appears to have a side > effect of performing the halted endpoint check after every handled > event, which wasn't done before. > >> + /* update the urb's actual_length and give back to the core */ >> + if (usb_endpoint_xfer_control(&td->urb->ep->desc)) >> + process_ctrl_td(xhci, ep, ep_ring, td, ep_trb, event); >> + else if (usb_endpoint_xfer_isoc(&td->urb->ep->desc)) >> + process_isoc_td(xhci, ep, ep_ring, td, ep_trb, event); >> + else >> + process_bulk_intr_td(xhci, ep, ep_ring, td, ep_trb, event); >> + >> +check_endpoint_halted: >> + if (xhci_halted_host_endpoint(ep_ctx, trb_comp_code)) >> + xhci_handle_halted_endpoint(xhci, ep, td, EP_HARD_RESET); >> >> return 0; > > Since stall handling is already present in functions like finish_td() or > process_bulk_intr_td() called from the above snippet, and this change of > behavior is not documented in the changelog, I doubt if it's intended? Hi, Thanks, this is indeed unintended. There definitely should be a return before the goto. Thanks, Niklas > > Regards, > Michal >
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index d037d3bbc767..49f8f980776b 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -2788,10 +2788,9 @@ static int handle_tx_event(struct xhci_hcd *xhci, xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n", slot_id, ep_index); } - if (xhci_halted_host_endpoint(ep_ctx, trb_comp_code)) - xhci_handle_halted_endpoint(xhci, ep, NULL, EP_HARD_RESET); - return 0; + td = NULL; + goto check_endpoint_halted; } td = list_first_entry(&ep_ring->td_list, struct xhci_td, @@ -2899,20 +2898,22 @@ static int handle_tx_event(struct xhci_hcd *xhci, * indefinitely. */ - if (trb_is_noop(ep_trb)) { - if (xhci_halted_host_endpoint(ep_ctx, trb_comp_code)) - xhci_handle_halted_endpoint(xhci, ep, td, EP_HARD_RESET); - } else { - td->status = status; + if (trb_is_noop(ep_trb)) + goto check_endpoint_halted; - /* update the urb's actual_length and give back to the core */ - if (usb_endpoint_xfer_control(&td->urb->ep->desc)) - process_ctrl_td(xhci, ep, ep_ring, td, ep_trb, event); - else if (usb_endpoint_xfer_isoc(&td->urb->ep->desc)) - process_isoc_td(xhci, ep, ep_ring, td, ep_trb, event); - else - process_bulk_intr_td(xhci, ep, ep_ring, td, ep_trb, event); - } + td->status = status; + + /* update the urb's actual_length and give back to the core */ + if (usb_endpoint_xfer_control(&td->urb->ep->desc)) + process_ctrl_td(xhci, ep, ep_ring, td, ep_trb, event); + else if (usb_endpoint_xfer_isoc(&td->urb->ep->desc)) + process_isoc_td(xhci, ep, ep_ring, td, ep_trb, event); + else + process_bulk_intr_td(xhci, ep, ep_ring, td, ep_trb, event); + +check_endpoint_halted: + if (xhci_halted_host_endpoint(ep_ctx, trb_comp_code)) + xhci_handle_halted_endpoint(xhci, ep, td, EP_HARD_RESET); return 0;