| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dabb1140-b26e-4f90-8e65-85e16d99aa49@linux.intel.com>
Date: Mon, 10 Mar 2025 11:51:30 +0200
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Michal Pecio <michal.pecio@...il.com>,
Mathias Nyman <mathias.nyman@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/6] usb: xhci: Deduplicate some endpoint state flag lists
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@...il.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
Powered by blists - more mailing lists