linux-kernel - Re: [PATCH 2/5] usb: xhci: Clean up the TD skipping loop

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <be026374-91d2-4b1d-9eb7-568b376f6e72@linux.intel.com>
Date: Sat, 22 Feb 2025 14:37:58 +0200
From: "Neronin, Niklas" <niklas.neronin@...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/5] usb: xhci: Clean up the TD skipping loop


On 10/02/2025 9.39, Michal Pecio wrote:
> Half of this loop is code which only executes once to deal with cases
> where no TD matches the event and then immediately returns. This code
> has no need to be in any kind of loop, so get it out.
> 
> Shuffle the remaining conditionals a little to improve readability.
> 
> No functional change.
> 
> Signed-off-by: Michal Pecio <michal.pecio@...il.com>
> ---
>  
> -		if (!ep_seg) {
> +		if (ep->skip) {
>  
> -			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
> +			if (!ep_seg && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
>  				skip_isoc_td(xhci, td, ep, status);
>  				if (!list_empty(&ep_ring->td_list))
>  					continue;
> @@ -2880,38 +2880,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>  				goto check_endpoint_halted;
>  			}
>  
> -			/*
> -			 * Skip the Force Stopped Event. The 'ep_trb' of FSE is not in the current
> -			 * TD pointed by 'ep_ring->dequeue' because that the hardware dequeue
> -			 * pointer still at the previous TRB of the current TD. The previous TRB
> -			 * maybe a Link TD or the last TRB of the previous TD. The command
> -			 * completion handle will take care the rest.
> -			 */
> -			if (trb_comp_code == COMP_STOPPED ||
> -			    trb_comp_code == COMP_STOPPED_LENGTH_INVALID) {
> -				return 0;
> -			}
> -
> -			/*
> -			 * Some hosts give a spurious success event after a short
> -			 * transfer. Ignore it.
> -			 */
> -			if ((xhci->quirks & XHCI_SPURIOUS_SUCCESS) &&
> -			    ep_ring->last_td_was_short) {
> -				ep_ring->last_td_was_short = false;
> -				return 0;
> -			}
> -
> -			/* HC is busted, give up! */
> -			xhci_err(xhci,
> -				 "ERROR Transfer event TRB DMA ptr not part of current TD ep_index %d comp_code %u\n",
> -				 ep_index, trb_comp_code);
> -			trb_in_td(xhci, td, ep_trb_dma, true);
> -
> -			return -ESHUTDOWN;
> -		}
> -
> -		if (ep->skip) {
>  			xhci_dbg(xhci,
>  				 "Found td. Clear skip flag for slot %u ep %u.\n",
>  				 slot_id, ep_index);

This debug message is now misleading, the TD way or may not be found on non-isochronous.

Before:
	if (ep_seg && ep->skip)
		xhci_dbg(xhci, "Found td. ...
After:
	if (ep->skip && (ep_seg || !isoc))
		xhci_dbg(xhci, "Found td. ...


With Best Regards,
Niklas