Re: [PATCH 2/6] usb: xhci: Deduplicate some endpoint state flag lists

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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