[PATCH 0/4] xhci fixes for usb-linus

[PATCH 0/4] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

This series fixes gaps discovered in isochronous transfer error
handling, and a couple small issues in the newly added secondary
interrupter code (6.8-rc1).

Thanks
-Mathias

Mathias Nyman (3):
  xhci: fix possible null pointer dereference at secondary interrupter
    removal
  xhci: fix off by one check when adding a secondary interrupter.
  xhci: process isoc TD properly when there was a transaction error mid
    TD.

Michal Pecio (1):
  xhci: handle isoc Babble and Buffer Overrun events properly

 drivers/usb/host/xhci-mem.c  | 14 +++----
 drivers/usb/host/xhci-ring.c | 80 +++++++++++++++++++++++++++++-------
 drivers/usb/host/xhci.h      |  1 +
 3 files changed, 73 insertions(+), 22 deletions(-)

-- 
2.25.1

[PATCH 1/4] xhci: fix possible null pointer dereference at secondary interrupter removal

[Mathias Nyman]

Don't try to remove a secondary interrupter that is known to be invalid.
Also check if the interrupter is valid inside the spinlock that protects
the array of interrupters.

Found by smatch static checker

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-usb/ffaa0a1b-5984-4a1f-bfd3-9184630a97b9@moroto.mountain/
Fixes: c99b38c41234 ("xhci: add support to allocate several interrupters")
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 4460fa7e9fab..d00d4d937236 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1861,14 +1861,14 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	unsigned int intr_num;
 
+	spin_lock_irq(&xhci->lock);
+
 	/* interrupter 0 is primary interrupter, don't touch it */
-	if (!ir || !ir->intr_num || ir->intr_num >= xhci->max_interrupters)
+	if (!ir || !ir->intr_num || ir->intr_num >= xhci->max_interrupters) {
 		xhci_dbg(xhci, "Invalid secondary interrupter, can't remove\n");
-
-	/* fixme, should we check xhci->interrupter[intr_num] == ir */
-	/* fixme locking */
-
-	spin_lock_irq(&xhci->lock);
+		spin_unlock_irq(&xhci->lock);
+		return;
+	}
 
 	intr_num = ir->intr_num;
 
-- 
2.25.1

[PATCH 2/4] xhci: fix off by one check when adding a secondary interrupter.

[Mathias Nyman]

The sanity check of interrupter index when adding a new interrupter is
off by one. intr_num needs to be smaller than xhci->max_interrupter to
fit the array of interrupters.

Luckily this doesn't cause any real word harm as xhci_add_interrupter()
is always called with a intr_num value smaller than xhci->max_interrupters
in any current kernel.

Should not be needed for stable as 6.7 kernel and older only supports
one interrupter, with intr_num always being zero.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-usb/e9771296-586d-456a-ac24-a82de79bb2e6@moroto.mountain/
Fixes: 4bf398e15aa4 ("xhci: split allocate interrupter into separate alloacte and add parts")
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index d00d4d937236..a7716202a8dd 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2322,7 +2322,7 @@ xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
 	u64 erst_base;
 	u32 erst_size;
 
-	if (intr_num > xhci->max_interrupters) {
+	if (intr_num >= xhci->max_interrupters) {
 		xhci_warn(xhci, "Can't add interrupter %d, max interrupters %d\n",
 			  intr_num, xhci->max_interrupters);
 		return -EINVAL;
-- 
2.25.1

[PATCH 3/4] xhci: process isoc TD properly when there was a transaction error mid TD.

[Mathias Nyman]

The last TRB of a isoc TD might not trigger an event if there was
an error event for a TRB mid TD. This is seen on a NEC Corporation
uPD720200 USB 3.0 Host

After an error mid a multi-TRB TD the xHC should according to xhci 4.9.1
generate events for passed TRBs with IOC flag set if it proceeds to the
next TD. This event is either a copy of the original error, or a
"success" transfer event.

If that event is missing then the driver and xHC host get out of sync as
the driver is still expecting a transfer event for that first TD, while
xHC host is already sending events for the next TD in the list.
This leads to
"Transfer event TRB DMA ptr not part of current TD" messages.

As a solution we tag the isoc TDs that get error events mid TD.
If an event doesn't match the first TD, then check if the tag is
set, and event points to the next TD.
In that case give back the fist TD and process the next TD normally

Make sure TD status and transferred length stay valid in both cases
with and without final TD completion event.

Reported-by: Michał Pecio <michal.pecio@gmail.com>
Closes: https://lore.kernel.org/linux-usb/20240112235205.1259f60c@foxbook/
Tested-by: Michał Pecio <michal.pecio@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 74 +++++++++++++++++++++++++++++-------
 drivers/usb/host/xhci.h      |  1 +
 2 files changed, 61 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 33806ae966f9..41be7d31a36e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2376,6 +2376,9 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	/* handle completion code */
 	switch (trb_comp_code) {
 	case COMP_SUCCESS:
+		/* Don't overwrite status if TD had an error, see xHCI 4.9.1 */
+		if (td->error_mid_td)
+			break;
 		if (remaining) {
 			frame->status = short_framestatus;
 			if (xhci->quirks & XHCI_TRUST_TX_LENGTH)
@@ -2401,8 +2404,9 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		break;
 	case COMP_USB_TRANSACTION_ERROR:
 		frame->status = -EPROTO;
+		sum_trbs_for_length = true;
 		if (ep_trb != td->last_trb)
-			return 0;
+			td->error_mid_td = true;
 		break;
 	case COMP_STOPPED:
 		sum_trbs_for_length = true;
@@ -2422,6 +2426,9 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		break;
 	}
 
+	if (td->urb_length_set)
+		goto finish_td;
+
 	if (sum_trbs_for_length)
 		frame->actual_length = sum_trb_lengths(xhci, ep->ring, ep_trb) +
 			ep_trb_len - remaining;
@@ -2430,6 +2437,14 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 
 	td->urb->actual_length += frame->actual_length;
 
+finish_td:
+	/* Don't give back TD yet if we encountered an error mid TD */
+	if (td->error_mid_td && ep_trb != td->last_trb) {
+		xhci_dbg(xhci, "Error mid isoc TD, wait for final completion event\n");
+		td->urb_length_set = true;
+		return 0;
+	}
+
 	return finish_td(xhci, ep, ep_ring, td, trb_comp_code);
 }
 
@@ -2808,17 +2823,51 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		}
 
 		if (!ep_seg) {
-			if (!ep->skip ||
-			    !usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
-				/* Some host controllers give a spurious
-				 * successful 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;
-					goto cleanup;
+
+			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
+				skip_isoc_td(xhci, td, ep, status);
+				goto cleanup;
+			}
+
+			/*
+			 * 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;
+				goto cleanup;
+			}
+
+			/*
+			 * xhci 4.10.2 states isoc endpoints should continue
+			 * processing the next TD if there was an error mid TD.
+			 * So host like NEC don't generate an event for the last
+			 * isoc TRB even if the IOC flag is set.
+			 * xhci 4.9.1 states that if there are errors in mult-TRB
+			 * TDs xHC should generate an error for that TRB, and if xHC
+			 * proceeds to the next TD it should genete an event for
+			 * any TRB with IOC flag on the way. Other host follow this.
+			 * So this event might be for the next TD.
+			 */
+			if (td->error_mid_td &&
+			    !list_is_last(&td->td_list, &ep_ring->td_list)) {
+				struct xhci_td *td_next = list_next_entry(td, td_list);
+
+				ep_seg = trb_in_td(xhci, td_next->start_seg, td_next->first_trb,
+						   td_next->last_trb, ep_trb_dma, false);
+				if (ep_seg) {
+					/* give back previous TD, start handling new */
+					xhci_dbg(xhci, "Missing TD completion event after mid TD error\n");
+					ep_ring->dequeue = td->last_trb;
+					ep_ring->deq_seg = td->last_trb_seg;
+					inc_deq(xhci, ep_ring);
+					xhci_td_cleanup(xhci, td, ep_ring, td->status);
+					td = td_next;
 				}
+			}
+
+			if (!ep_seg) {
 				/* HC is busted, give up! */
 				xhci_err(xhci,
 					"ERROR Transfer event TRB DMA ptr not "
@@ -2830,9 +2879,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 					  ep_trb_dma, true);
 				return -ESHUTDOWN;
 			}
-
-			skip_isoc_td(xhci, td, ep, status);
-			goto cleanup;
 		}
 		if (trb_comp_code == COMP_SHORT_PACKET)
 			ep_ring->last_td_was_short = true;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index a5c72a634e6a..6f82d404883f 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1549,6 +1549,7 @@ struct xhci_td {
 	struct xhci_segment	*bounce_seg;
 	/* actual_length of the URB has already been set */
 	bool			urb_length_set;
+	bool			error_mid_td;
 	unsigned int		num_trbs;
 };
 
-- 
2.25.1

[PATCH 4/4] xhci: handle isoc Babble and Buffer Overrun events properly

[Mathias Nyman]

From: Michal Pecio <michal.pecio@gmail.com>

xHCI 4.9 explicitly forbids assuming that the xHC has released its
ownership of a multi-TRB TD when it reports an error on one of the
early TRBs. Yet the driver makes such assumption and releases the TD,
allowing the remaining TRBs to be freed or overwritten by new TDs.

The xHC should also report completion of the final TRB due to its IOC
flag being set by us, regardless of prior errors. This event cannot
be recognized if the TD has already been freed earlier, resulting in
"Transfer event TRB DMA ptr not part of current TD" error message.

Fix this by reusing the logic for processing isoc Transaction Errors.
This also handles hosts which fail to report the final completion.

Fix transfer length reporting on Babble errors. They may be caused by
device malfunction, no guarantee that the buffer has been filled.

Signed-off-by: Michal Pecio <michal.pecio@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 41be7d31a36e..f0d8a607ff21 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2394,9 +2394,13 @@ static int process_isoc_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 	case COMP_BANDWIDTH_OVERRUN_ERROR:
 		frame->status = -ECOMM;
 		break;
-	case COMP_ISOCH_BUFFER_OVERRUN:
 	case COMP_BABBLE_DETECTED_ERROR:
+		sum_trbs_for_length = true;
+		fallthrough;
+	case COMP_ISOCH_BUFFER_OVERRUN:
 		frame->status = -EOVERFLOW;
+		if (ep_trb != td->last_trb)
+			td->error_mid_td = true;
 		break;
 	case COMP_INCOMPATIBLE_DEVICE_ERROR:
 	case COMP_STALL_ERROR:
-- 
2.25.1

[PATCH 0/4] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

A few small xhci fixes for 6.6

Use correct dev in dma bounce buffer mapping, fix event handling
issues, and track suspended ports correctly in software

Thanks
Mathias

Lukas Wunner (2):
  xhci: Clear EHB bit only at end of interrupt handler
  xhci: Preserve RsvdP bits in ERSTBA register correctly

Mathias Nyman (1):
  xhci: track port suspend state correctly in unsuccessful resume cases

Wesley Cheng (1):
  usb: xhci: xhci-ring: Use sysdev for mapping bounce buffer

 drivers/usb/host/xhci-hub.c  | 19 ++++++++++---------
 drivers/usb/host/xhci-mem.c  |  4 ++--
 drivers/usb/host/xhci-ring.c | 16 +++++++++-------
 drivers/usb/host/xhci.h      |  2 +-
 4 files changed, 22 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH 0/4] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

Some xhci fixes for usb-linus, mostly tuning quirks, but also fixing
one linked list issue.

Thanks
Mathias

Jens Glathe (1):
  usb: xhci: add XHCI_SPURIOUS_SUCCESS to ASM1042 despite being a V0.96
    controller

Mario Limonciello (1):
  xhci-pci: Set runtime PM as default policy on all xHC 1.2 or later
    devices

Mathias Nyman (2):
  xhci: Add quirk to reset host back to default state at shutdown
  xhci: Remove device endpoints from bandwidth list when freeing the
    device

 drivers/usb/host/xhci-mem.c | 20 ++++++++++-------
 drivers/usb/host/xhci-pci.c | 44 +++++++++++++------------------------
 drivers/usb/host/xhci.c     | 10 +++++++--
 drivers/usb/host/xhci.h     |  1 +
 4 files changed, 36 insertions(+), 39 deletions(-)

-- 
2.25.1

[PATCH 0/4] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

A few xhci fixes for 5.19
Adding a couple PCI IDs, turning off port power in shutdown, and delay
xHC interrupt generation until host is running.

Thanks
Mathias

Hongyu Xie (1):
  xhci: Keep interrupt disabled in initialization until host is running.

Mathias Nyman (1):
  xhci: turn off port power in shutdown

Tanveer Alam (1):
  xhci-pci: Allow host runtime PM as default for Intel Raptor Lake xHCI

Utkarsh Patel (1):
  xhci-pci: Allow host runtime PM as default for Intel Meteor Lake xHCI

 drivers/usb/host/xhci-hub.c |  2 +-
 drivers/usb/host/xhci-pci.c |  6 ++++-
 drivers/usb/host/xhci.c     | 50 ++++++++++++++++++++++++++-----------
 drivers/usb/host/xhci.h     |  2 ++
 4 files changed, 43 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH 0/4] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

Fix a couple power management related xHCI issues, and a couple
vendor specific workarounds.

-Mathias

Forest Crossman (1):
  usb: xhci: Fix ASMedia ASM1042A and ASM3242 DMA addressing

Mathias Nyman (2):
  xhci: Improve detection of device initiated wake signal.
  xhci: Fix repeated xhci wake after suspend due to uncleared internal
    wake state

Stanislaw Gruszka (1):
  usb: xhci: do not perform Soft Retry for some xHCI hosts

 drivers/usb/host/xhci-pci.c  | 13 +++++-
 drivers/usb/host/xhci-ring.c |  3 +-
 drivers/usb/host/xhci.c      | 78 ++++++++++++++++++++----------------
 drivers/usb/host/xhci.h      |  1 +
 4 files changed, 57 insertions(+), 38 deletions(-)

-- 
2.25.1

[PATCH 0/4] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

A few xhci fixes resolving an issue entering runtime suspend PCI D3 for
some Intel hosts, fixing a memory leak, and forcing max packet size to
valid values allowing some older FS devices to function with xhci.

-Mathias

Mathias Nyman (4):
  xhci: Force Maximum Packet size for Full-speed bulk devices to valid
    range.
  xhci: Fix memory leak when caching protocol extended capability PSI
    tables
  xhci: fix runtime pm enabling for quirky Intel hosts
  xhci: apply XHCI_PME_STUCK_QUIRK to Intel Comet Lake platforms

 drivers/usb/host/xhci-hub.c | 25 ++++++++-----
 drivers/usb/host/xhci-mem.c | 70 ++++++++++++++++++++++++-------------
 drivers/usb/host/xhci-pci.c | 10 +++---
 drivers/usb/host/xhci.h     | 14 ++++++--
 4 files changed, 79 insertions(+), 40 deletions(-)

-- 
2.17.1