| Message ID | 20250310093748.201e87cd@foxbook (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] usb: xhci: Document endpoint state management | expand |
On 10.3.2025 10.37, Michal Pecio wrote: > xhci_ring_endpoint_doorbell() needs a list of flags which prohibit > running the endpoint. > > xhci_urb_dequeue() needs the same list, split in two parts, to know > whether the endpoint is running and how to cancel TDs. > > Define the two partial lists in xhci.h and use them in both functions. > > Add a comment about the AMD Stop Endpoint bug, see commit 28a2369f7d72 > ("usb: xhci: Issue stop EP command only when the EP state is running") > > Signed-off-by: Michal Pecio <michal.pecio@gmail.com> > ---> drivers/usb/host/xhci-ring.c | 10 ++-------- > drivers/usb/host/xhci.c | 16 +++++++++++----- > drivers/usb/host/xhci.h | 16 +++++++++++++++- > 3 files changed, 28 insertions(+), 14 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 0f8acbb9cd21..8aab077d6183 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -555,14 +555,8 @@ void xhci_ring_ep_doorbell(struct xhci_hcd *xhci, > struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index]; > unsigned int ep_state = ep->ep_state; > > - /* Don't ring the doorbell for this endpoint if there are pending > - * cancellations because we don't want to interrupt processing. > - * We don't want to restart any stream rings if there's a set dequeue > - * pointer command pending because the device can choose to start any > - * stream once the endpoint is on the HW schedule. > - */ > - if (ep_state & (EP_STOP_CMD_PENDING | SET_DEQ_PENDING | EP_HALTED | > - EP_CLEARING_TT | EP_STALLED)) > + /* Don't start yet if certain endpoint operations are ongoing */ > + if (ep_state & (EP_CANCEL_PENDING | EP_MISC_OPS_PENDING)) > return; > > trace_xhci_ring_ep_doorbell(slot_id, DB_VALUE(ep_index, stream_id)); > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 7492090fad5f..c33134a3003a 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -1762,16 +1762,22 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) > } > } > > - /* These completion handlers will sort out cancelled TDs for us */ > - if (ep->ep_state & (EP_STOP_CMD_PENDING | EP_HALTED | SET_DEQ_PENDING)) { > + /* > + * We have a few strategies to give back the cancelled TDs. If the endpoint is running, > + * no other choice - it must be stopped. But if it's not, we avoid queuing Stop Endpoint > + * because this triggers a bug in "AMD SNPS 3.1 xHC" and because our completion handler > + * is complex enough already without having to worry about such things. > + */ > + > + /* If cancellation is already running, giveback of all cancelled TDs is guaranteed */ > + if (ep->ep_state & EP_CANCEL_PENDING) { > xhci_dbg(xhci, "Not queuing Stop Endpoint on slot %d ep %d in state 0x%x\n", > urb->dev->slot_id, ep_index, ep->ep_state); > goto done; > } > > - /* In this case no commands are pending but the endpoint is stopped */ > - if (ep->ep_state & (EP_CLEARING_TT | EP_STALLED)) { > - /* and cancelled TDs can be given back right away */ > + /* Cancel immediately if no commands are pending but the endpoint is held stopped */ > + if (ep->ep_state & EP_MISC_OPS_PENDING) { > xhci_dbg(xhci, "Invalidating TDs instantly on slot %d ep %d in state 0x%x\n", > urb->dev->slot_id, ep_index, ep->ep_state); > xhci_process_cancelled_tds(ep); > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 46bbdc97cc4b..87d87ed08b8b 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -718,9 +718,23 @@ struct xhci_virt_ep { > * xhci_ring_ep_doorbell() inspects the flags to decide if the endpoint can be restarted. Another > * user is xhci_urb_dequeue(), which must not attempt to stop a Stopped endpoint, due to HW bugs. > * An endpoint with pending URBs and no flags preventing restart must be Running for this to work. > - * Call xhci_ring_doorbell_for_active_rings() or similar after clearing any such flag. > + * Call xhci_ring_doorbell_for_active_rings() or similar after clearing flags on the lists below. > */ > > +/* > + * TD cancellation is in progress. New TDs can be marked as cancelled without further action and > + * indeed no such action is possible until these commands complete. Their handlers must check for > + * more cancelled TDs and continue until all are given back. The endpoint must not be restarted. > + */ > +#define EP_CANCEL_PENDING (SET_DEQ_PENDING | EP_HALTED | EP_STOP_CMD_PENDING) > + > +/* > + * Some other operations are pending which preclude restarting the endpoint. If the endpoint isn't > + * transitioning to the Stopped state, it has already reached this state and stays in it. > + */ > +#define EP_MISC_OPS_PENDING (EP_CLEARING_TT | EP_STALLED) > + Not sure this helps readability It defines even more macros to abstract away something that is not complex enough. It also gives false impression that EP_HALTED would somehow be more part of cancelling a TD than EP_STALLED, when both of those are about returning a TD with an error due to transfer issues detected by host, not class driver cancelling URBs Thanks Mathias
On Mon, 10 Mar 2025 11:51:30 +0200, Mathias Nyman wrote: > Not sure this helps readability > > It defines even more macros to abstract away something that is not > complex enough. It was less about readability, but keeping these lists in one place so that they don't get out of sync and trigger the double-stop bug. With this change, a new flag like EP_STALLED only needs to be added in one place and it's picked up by both functions which need it. OTOH, maybe such flags aren't being added very often... > It also gives false impression that EP_HALTED would somehow be more > part of cancelling a TD than EP_STALLED, when both of those are about > returning a TD with an error due to transfer issues detected by host, > not class driver cancelling URBs. I think EP_HALTED is about cancellation (among other things), because it indicates that Reset EP handler will run and finish cancellation of the halted TD and also any other TDs unlinked by class driver. EP_STALLED and EP_CLEARING_TT are less about the halted TD and more about ensuring full reset of the pipe before new TDs are executed. Michal
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 0f8acbb9cd21..8aab077d6183 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -555,14 +555,8 @@ void xhci_ring_ep_doorbell(struct xhci_hcd *xhci, struct xhci_virt_ep *ep = &xhci->devs[slot_id]->eps[ep_index]; unsigned int ep_state = ep->ep_state; - /* Don't ring the doorbell for this endpoint if there are pending - * cancellations because we don't want to interrupt processing. - * We don't want to restart any stream rings if there's a set dequeue - * pointer command pending because the device can choose to start any - * stream once the endpoint is on the HW schedule. - */ - if (ep_state & (EP_STOP_CMD_PENDING | SET_DEQ_PENDING | EP_HALTED | - EP_CLEARING_TT | EP_STALLED)) + /* Don't start yet if certain endpoint operations are ongoing */ + if (ep_state & (EP_CANCEL_PENDING | EP_MISC_OPS_PENDING)) return; trace_xhci_ring_ep_doorbell(slot_id, DB_VALUE(ep_index, stream_id)); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 7492090fad5f..c33134a3003a 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -1762,16 +1762,22 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) } } - /* These completion handlers will sort out cancelled TDs for us */ - if (ep->ep_state & (EP_STOP_CMD_PENDING | EP_HALTED | SET_DEQ_PENDING)) { + /* + * We have a few strategies to give back the cancelled TDs. If the endpoint is running, + * no other choice - it must be stopped. But if it's not, we avoid queuing Stop Endpoint + * because this triggers a bug in "AMD SNPS 3.1 xHC" and because our completion handler + * is complex enough already without having to worry about such things. + */ + + /* If cancellation is already running, giveback of all cancelled TDs is guaranteed */ + if (ep->ep_state & EP_CANCEL_PENDING) { xhci_dbg(xhci, "Not queuing Stop Endpoint on slot %d ep %d in state 0x%x\n", urb->dev->slot_id, ep_index, ep->ep_state); goto done; } - /* In this case no commands are pending but the endpoint is stopped */ - if (ep->ep_state & (EP_CLEARING_TT | EP_STALLED)) { - /* and cancelled TDs can be given back right away */ + /* Cancel immediately if no commands are pending but the endpoint is held stopped */ + if (ep->ep_state & EP_MISC_OPS_PENDING) { xhci_dbg(xhci, "Invalidating TDs instantly on slot %d ep %d in state 0x%x\n", urb->dev->slot_id, ep_index, ep->ep_state); xhci_process_cancelled_tds(ep); diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 46bbdc97cc4b..87d87ed08b8b 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -718,9 +718,23 @@ struct xhci_virt_ep { * xhci_ring_ep_doorbell() inspects the flags to decide if the endpoint can be restarted. Another * user is xhci_urb_dequeue(), which must not attempt to stop a Stopped endpoint, due to HW bugs. * An endpoint with pending URBs and no flags preventing restart must be Running for this to work. - * Call xhci_ring_doorbell_for_active_rings() or similar after clearing any such flag. + * Call xhci_ring_doorbell_for_active_rings() or similar after clearing flags on the lists below. */ +/* + * TD cancellation is in progress. New TDs can be marked as cancelled without further action and + * indeed no such action is possible until these commands complete. Their handlers must check for + * more cancelled TDs and continue until all are given back. The endpoint must not be restarted. + */ +#define EP_CANCEL_PENDING (SET_DEQ_PENDING | EP_HALTED | EP_STOP_CMD_PENDING) + +/* + * Some other operations are pending which preclude restarting the endpoint. If the endpoint isn't + * transitioning to the Stopped state, it has already reached this state and stays in it. + */ +#define EP_MISC_OPS_PENDING (EP_CLEARING_TT | EP_STALLED) + + enum xhci_overhead_type { LS_OVERHEAD_TYPE = 0, FS_OVERHEAD_TYPE,
xhci_ring_endpoint_doorbell() needs a list of flags which prohibit running the endpoint. xhci_urb_dequeue() needs the same list, split in two parts, to know whether the endpoint is running and how to cancel TDs. Define the two partial lists in xhci.h and use them in both functions. Add a comment about the AMD Stop Endpoint bug, see commit 28a2369f7d72 ("usb: xhci: Issue stop EP command only when the EP state is running") Signed-off-by: Michal Pecio <michal.pecio@gmail.com> --- drivers/usb/host/xhci-ring.c | 10 ++-------- drivers/usb/host/xhci.c | 16 +++++++++++----- drivers/usb/host/xhci.h | 16 +++++++++++++++- 3 files changed, 28 insertions(+), 14 deletions(-)