[PATCH 00/12] xhci features for usb-next

[PATCH 00/12] xhci features for usb-next

[Mathias Nyman]

Hi Greg

This series for usb-next has several small xhci cleanups, a couple DbC
improvements, one small secondary interrupter feature, and a fix for
setting PCI D3 state correctly during remove callback on older haswell
hosts.

Thanks
Mathias


Mathias Nyman (4):
  xhci: dbc: Fix STALL transfer event handling
  xhci: dbc: add dbgtty request to end of list once it completes
  xhci: Set quirky xHC PCI hosts to D3 _after_ stopping and freeing
    them.
  xhci: support setting interrupt moderation IMOD for secondary
    interrupters

Niklas Neronin (7):
  usb: xhci: remove excessive isoc frame debug message spam
  usb: xhci: remove excessive Bulk short packet debug message
  usb: xhci: remove unused variables from struct 'xhci_hcd'
  usb: xhci: make 'sbrn' a local variable
  usb: xhci: add comments explaining specific interrupt behaviour
  usb: xhci: remove 'retval' from xhci_pci_resume()
  usb: xhci: adjust empty TD list handling in handle_tx_event()

Yue Haibing (1):
  xhci: Remove unused function declarations

 drivers/usb/host/xhci-dbgcap.c | 133 ++++++++++++++++++++-------------
 drivers/usb/host/xhci-dbgcap.h |   2 +-
 drivers/usb/host/xhci-dbgtty.c |   2 +-
 drivers/usb/host/xhci-mem.c    |   8 +-
 drivers/usb/host/xhci-pci.c    |  25 ++++---
 drivers/usb/host/xhci-ring.c   |  58 ++++++--------
 drivers/usb/host/xhci.c        |   4 +-
 drivers/usb/host/xhci.h        |  14 +---
 8 files changed, 135 insertions(+), 111 deletions(-)

-- 
2.25.1

[PATCH 01/12] xhci: dbc: Fix STALL transfer event handling

[Mathias Nyman]

Don't flush all pending DbC data requests when an endpoint halts.

An endpoint may halt and xHC DbC triggers a STALL error event if there's
an issue with a bulk data transfer. The transfer should restart once xHC
DbC receives a ClearFeature(ENDPOINT_HALT) request from the host.

Once xHC DbC restarts it will start from the TRB pointed to by dequeue
field in the endpoint context, which might be the same TRB we got the
STALL event for. Turn the TRB to a no-op in this case to make sure xHC
DbC doesn't reuse and tries to retransmit this same TRB after we already
handled it, and gave its corresponding data request back.

Other STALL events might be completely bogus.
Lukasz Bartosik discovered that xHC DbC might issue spurious STALL events
if hosts sends a ClearFeature(ENDPOINT_HALT) request to non-halted
endpoints even without any active bulk transfers.

Assume STALL event is spurious if it reports 0 bytes transferred, and
the endpoint stopped on the STALLED TRB.
Don't give back the data request corresponding to the TRB in this case.

The halted status is per endpoint. Track it with a per endpoint flag
instead of the driver invented DbC wide DS_STALLED state.
DbC remains in DbC-Configured state even if endpoints halt. There is no
Stalled state in the DbC Port state Machine (xhci section 7.6.6)

Reported-by: Łukasz Bartosik <ukaszb@chromium.org>
Closes: https://lore.kernel.org/linux-usb/20240725074857.623299-1-ukaszb@chromium.org/
Tested-by: Łukasz Bartosik <ukaszb@chromium.org>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-dbgcap.c | 133 ++++++++++++++++++++-------------
 drivers/usb/host/xhci-dbgcap.h |   2 +-
 2 files changed, 83 insertions(+), 52 deletions(-)

diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index 161c09953c4e..241d7aa1fbc2 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -173,16 +173,18 @@ static void xhci_dbc_giveback(struct dbc_request *req, int status)
 	spin_lock(&dbc->lock);
 }
 
-static void xhci_dbc_flush_single_request(struct dbc_request *req)
+static void trb_to_noop(union xhci_trb *trb)
 {
-	union xhci_trb	*trb = req->trb;
-
 	trb->generic.field[0]	= 0;
 	trb->generic.field[1]	= 0;
 	trb->generic.field[2]	= 0;
 	trb->generic.field[3]	&= cpu_to_le32(TRB_CYCLE);
 	trb->generic.field[3]	|= cpu_to_le32(TRB_TYPE(TRB_TR_NOOP));
+}
 
+static void xhci_dbc_flush_single_request(struct dbc_request *req)
+{
+	trb_to_noop(req->trb);
 	xhci_dbc_giveback(req, -ESHUTDOWN);
 }
 
@@ -649,7 +651,6 @@ static void xhci_dbc_stop(struct xhci_dbc *dbc)
 	case DS_DISABLED:
 		return;
 	case DS_CONFIGURED:
-	case DS_STALLED:
 		if (dbc->driver->disconnect)
 			dbc->driver->disconnect(dbc);
 		break;
@@ -669,6 +670,23 @@ static void xhci_dbc_stop(struct xhci_dbc *dbc)
 	pm_runtime_put_sync(dbc->dev); /* note, was self.controller */
 }
 
+static void
+handle_ep_halt_changes(struct xhci_dbc *dbc, struct dbc_ep *dep, bool halted)
+{
+	if (halted) {
+		dev_info(dbc->dev, "DbC Endpoint halted\n");
+		dep->halted = 1;
+
+	} else if (dep->halted) {
+		dev_info(dbc->dev, "DbC Endpoint halt cleared\n");
+		dep->halted = 0;
+
+		if (!list_empty(&dep->list_pending))
+			writel(DBC_DOOR_BELL_TARGET(dep->direction),
+			       &dbc->regs->doorbell);
+	}
+}
+
 static void
 dbc_handle_port_status(struct xhci_dbc *dbc, union xhci_trb *event)
 {
@@ -697,6 +715,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
 	struct xhci_ring	*ring;
 	int			ep_id;
 	int			status;
+	struct xhci_ep_ctx	*ep_ctx;
 	u32			comp_code;
 	size_t			remain_length;
 	struct dbc_request	*req = NULL, *r;
@@ -706,8 +725,30 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
 	ep_id		= TRB_TO_EP_ID(le32_to_cpu(event->generic.field[3]));
 	dep		= (ep_id == EPID_OUT) ?
 				get_out_ep(dbc) : get_in_ep(dbc);
+	ep_ctx		= (ep_id == EPID_OUT) ?
+				dbc_bulkout_ctx(dbc) : dbc_bulkin_ctx(dbc);
 	ring		= dep->ring;
 
+	/* Match the pending request: */
+	list_for_each_entry(r, &dep->list_pending, list_pending) {
+		if (r->trb_dma == event->trans_event.buffer) {
+			req = r;
+			break;
+		}
+		if (r->status == -COMP_STALL_ERROR) {
+			dev_warn(dbc->dev, "Give back stale stalled req\n");
+			ring->num_trbs_free++;
+			xhci_dbc_giveback(r, 0);
+		}
+	}
+
+	if (!req) {
+		dev_warn(dbc->dev, "no matched request\n");
+		return;
+	}
+
+	trace_xhci_dbc_handle_transfer(ring, &req->trb->generic);
+
 	switch (comp_code) {
 	case COMP_SUCCESS:
 		remain_length = 0;
@@ -718,31 +759,49 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
 	case COMP_TRB_ERROR:
 	case COMP_BABBLE_DETECTED_ERROR:
 	case COMP_USB_TRANSACTION_ERROR:
-	case COMP_STALL_ERROR:
 		dev_warn(dbc->dev, "tx error %d detected\n", comp_code);
 		status = -comp_code;
 		break;
+	case COMP_STALL_ERROR:
+		dev_warn(dbc->dev, "Stall error at bulk TRB %llx, remaining %zu, ep deq %llx\n",
+			 event->trans_event.buffer, remain_length, ep_ctx->deq);
+		status = 0;
+		dep->halted = 1;
+
+		/*
+		 * xHC DbC may trigger a STALL bulk xfer event when host sends a
+		 * ClearFeature(ENDPOINT_HALT) request even if there wasn't an
+		 * active bulk transfer.
+		 *
+		 * Don't give back this transfer request as hardware will later
+		 * start processing TRBs starting from this 'STALLED' TRB,
+		 * causing TRBs and requests to be out of sync.
+		 *
+		 * If STALL event shows some bytes were transferred then assume
+		 * it's an actual transfer issue and give back the request.
+		 * In this case mark the TRB as No-Op to avoid hw from using the
+		 * TRB again.
+		 */
+
+		if ((ep_ctx->deq & ~TRB_CYCLE) == event->trans_event.buffer) {
+			dev_dbg(dbc->dev, "Ep stopped on Stalled TRB\n");
+			if (remain_length == req->length) {
+				dev_dbg(dbc->dev, "Spurious stall event, keep req\n");
+				req->status = -COMP_STALL_ERROR;
+				req->actual = 0;
+				return;
+			}
+			dev_dbg(dbc->dev, "Give back stalled req, but turn TRB to No-op\n");
+			trb_to_noop(req->trb);
+		}
+		break;
+
 	default:
 		dev_err(dbc->dev, "unknown tx error %d\n", comp_code);
 		status = -comp_code;
 		break;
 	}
 
-	/* Match the pending request: */
-	list_for_each_entry(r, &dep->list_pending, list_pending) {
-		if (r->trb_dma == event->trans_event.buffer) {
-			req = r;
-			break;
-		}
-	}
-
-	if (!req) {
-		dev_warn(dbc->dev, "no matched request\n");
-		return;
-	}
-
-	trace_xhci_dbc_handle_transfer(ring, &req->trb->generic);
-
 	ring->num_trbs_free++;
 	req->actual = req->length - remain_length;
 	xhci_dbc_giveback(req, status);
@@ -762,7 +821,6 @@ static void inc_evt_deq(struct xhci_ring *ring)
 static enum evtreturn xhci_dbc_do_handle_events(struct xhci_dbc *dbc)
 {
 	dma_addr_t		deq;
-	struct dbc_ep		*dep;
 	union xhci_trb		*evt;
 	u32			ctrl, portsc;
 	bool			update_erdp = false;
@@ -814,43 +872,17 @@ static enum evtreturn xhci_dbc_do_handle_events(struct xhci_dbc *dbc)
 			return EVT_DISC;
 		}
 
-		/* Handle endpoint stall event: */
+		/* Check and handle changes in endpoint halt status */
 		ctrl = readl(&dbc->regs->control);
-		if ((ctrl & DBC_CTRL_HALT_IN_TR) ||
-		    (ctrl & DBC_CTRL_HALT_OUT_TR)) {
-			dev_info(dbc->dev, "DbC Endpoint stall\n");
-			dbc->state = DS_STALLED;
-
-			if (ctrl & DBC_CTRL_HALT_IN_TR) {
-				dep = get_in_ep(dbc);
-				xhci_dbc_flush_endpoint_requests(dep);
-			}
-
-			if (ctrl & DBC_CTRL_HALT_OUT_TR) {
-				dep = get_out_ep(dbc);
-				xhci_dbc_flush_endpoint_requests(dep);
-			}
-
-			return EVT_DONE;
-		}
+		handle_ep_halt_changes(dbc, get_in_ep(dbc), ctrl & DBC_CTRL_HALT_IN_TR);
+		handle_ep_halt_changes(dbc, get_out_ep(dbc), ctrl & DBC_CTRL_HALT_OUT_TR);
 
 		/* Clear DbC run change bit: */
 		if (ctrl & DBC_CTRL_DBC_RUN_CHANGE) {
 			writel(ctrl, &dbc->regs->control);
 			ctrl = readl(&dbc->regs->control);
 		}
-
 		break;
-	case DS_STALLED:
-		ctrl = readl(&dbc->regs->control);
-		if (!(ctrl & DBC_CTRL_HALT_IN_TR) &&
-		    !(ctrl & DBC_CTRL_HALT_OUT_TR) &&
-		    (ctrl & DBC_CTRL_DBC_RUN)) {
-			dbc->state = DS_CONFIGURED;
-			break;
-		}
-
-		return EVT_DONE;
 	default:
 		dev_err(dbc->dev, "Unknown DbC state %d\n", dbc->state);
 		break;
@@ -939,7 +971,6 @@ static const char * const dbc_state_strings[DS_MAX] = {
 	[DS_ENABLED] = "enabled",
 	[DS_CONNECTED] = "connected",
 	[DS_CONFIGURED] = "configured",
-	[DS_STALLED] = "stalled",
 };
 
 static ssize_t dbc_show(struct device *dev,
diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
index 0118c6288a3c..97c5dc290138 100644
--- a/drivers/usb/host/xhci-dbgcap.h
+++ b/drivers/usb/host/xhci-dbgcap.h
@@ -81,7 +81,6 @@ enum dbc_state {
 	DS_ENABLED,
 	DS_CONNECTED,
 	DS_CONFIGURED,
-	DS_STALLED,
 	DS_MAX
 };
 
@@ -90,6 +89,7 @@ struct dbc_ep {
 	struct list_head		list_pending;
 	struct xhci_ring		*ring;
 	unsigned int			direction:1;
+	unsigned int			halted:1;
 };
 
 #define DBC_QUEUE_SIZE			16
-- 
2.25.1

[PATCH 02/12] xhci: dbc: add dbgtty request to end of list once it completes

[Mathias Nyman]

Make sure we move the requests from the read_queue to the end of the
read_pool list, avoiding looping and using the same one request all
the time.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-dbgtty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
index b74e98e94393..5e567b3eb4d9 100644
--- a/drivers/usb/host/xhci-dbgtty.c
+++ b/drivers/usb/host/xhci-dbgtty.c
@@ -346,7 +346,7 @@ static void dbc_rx_push(struct tasklet_struct *t)
 			port->n_read = 0;
 		}
 
-		list_move(&req->list_pool, &port->read_pool);
+		list_move_tail(&req->list_pool, &port->read_pool);
 	}
 
 	if (do_push)
-- 
2.25.1

[PATCH 03/12] xhci: Remove unused function declarations

[Mathias Nyman]

From: Yue Haibing <yuehaibing@huawei.com>

Commit 674f8438c121 ("xhci: split handling halted endpoints into two
steps") removed xhci_cleanup_stalled_ring() but left declaration.

Commit 25355e046d29 ("xhci: use generic command timer for stop endpoint
commands.") left behind xhci_stop_endpoint_command_watchdog().

Signed-off-by: Yue Haibing <yuehaibing@huawei.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 40aebdfb37c9..ae4b50b01284 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1904,10 +1904,6 @@ int xhci_queue_reset_ep(struct xhci_hcd *xhci, struct xhci_command *cmd,
 		enum xhci_ep_reset_type reset_type);
 int xhci_queue_reset_device(struct xhci_hcd *xhci, struct xhci_command *cmd,
 		u32 slot_id);
-void xhci_cleanup_stalled_ring(struct xhci_hcd *xhci, unsigned int slot_id,
-			       unsigned int ep_index, unsigned int stream_id,
-			       struct xhci_td *td);
-void xhci_stop_endpoint_command_watchdog(struct timer_list *t);
 void xhci_handle_command_timeout(struct work_struct *work);
 
 void xhci_ring_ep_doorbell(struct xhci_hcd *xhci, unsigned int slot_id,
-- 
2.25.1

[PATCH 04/12] usb: xhci: remove excessive isoc frame debug message spam

[Mathias Nyman]

From: Niklas Neronin <niklas.neronin@linux.intel.com>

The removed debug messages trigger each time an isoc frame is handled.
In case of an error, a dedicated debug message exists.

For example, a 60fps USB camera will trigger the debug message every 0.6s.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 4ea2c3e072a9..e1c9838084bf 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3941,10 +3941,6 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci,
 	start_frame_id = (start_frame_id >> 3) & 0x7ff;
 	end_frame_id = (end_frame_id >> 3) & 0x7ff;
 
-	xhci_dbg(xhci, "%s: index %d, reg 0x%x start_frame_id 0x%x, end_frame_id 0x%x, start_frame 0x%x\n",
-		 __func__, index, readl(&xhci->run_regs->microframe_index),
-		 start_frame_id, end_frame_id, start_frame);
-
 	if (start_frame_id < end_frame_id) {
 		if (start_frame > end_frame_id ||
 				start_frame < start_frame_id)
-- 
2.25.1

[PATCH 05/12] usb: xhci: remove excessive Bulk short packet debug message

[Mathias Nyman]

From: Niklas Neronin <niklas.neronin@linux.intel.com>

Completion codes 'COMP_SUCCESS' and 'COMP_SHORT_PACKET' are the most
frequently encountered completion codes. Typically, these codes do not
trigger a default debug message but rather a warning that indicates a
potential issue. This behavior is consistent across all transfer types
with the exception of Bulk transfers. To reduce unnecessary log clutter,
remove the Bulk 'COMP_SHORT_PACKET' debug message.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index e1c9838084bf..d37eeee74960 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2521,9 +2521,6 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		td->status = 0;
 		break;
 	case COMP_SHORT_PACKET:
-		xhci_dbg(xhci, "ep %#x - asked for %d bytes, %d bytes untransferred\n",
-			 td->urb->ep->desc.bEndpointAddress,
-			 requested, remaining);
 		td->status = 0;
 		break;
 	case COMP_STOPPED_SHORT_PACKET:
-- 
2.25.1

[PATCH 06/12] usb: xhci: remove unused variables from struct 'xhci_hcd'

[Mathias Nyman]

From: Niklas Neronin <niklas.neronin@linux.intel.com>

Variables 'max_slots', 'max_ports', 'isoc_threshold' and 'event_ring_max'
are never set or used. Thus, remove them.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index ae4b50b01284..6f8cecc789d6 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1500,13 +1500,9 @@ struct xhci_hcd {
 	/* packed release number */
 	u8		sbrn;
 	u16		hci_version;
-	u8		max_slots;
 	u16		max_interrupters;
-	u8		max_ports;
-	u8		isoc_threshold;
 	/* imod_interval in ns (I * 250ns) */
 	u32		imod_interval;
-	int		event_ring_max;
 	/* 4KB min, 128MB max */
 	int		page_size;
 	/* Valid values are 12 to 20, inclusive */
-- 
2.25.1

[PATCH 07/12] usb: xhci: make 'sbrn' a local variable

[Mathias Nyman]

From: Niklas Neronin <niklas.neronin@linux.intel.com>

Variable 'sbrn' is used to store the Serial Bus Release Number, which is
then only used for a debug message. Thus, 'sbrn' can be a local variable
and assigned after the primary HCD check. The SBRN debug message is only
printed when a primary HCD is setup.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 6 +++---
 drivers/usb/host/xhci.h     | 1 -
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index b5705ed01d83..b405cff2d600 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -525,10 +525,9 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
 	struct xhci_hcd		*xhci;
 	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
 	int			retval;
+	u8			sbrn;
 
 	xhci = hcd_to_xhci(hcd);
-	if (!xhci->sbrn)
-		pci_read_config_byte(pdev, XHCI_SBRN_OFFSET, &xhci->sbrn);
 
 	/* imod_interval is the interrupt moderation value in nanoseconds. */
 	xhci->imod_interval = 40000;
@@ -543,7 +542,8 @@ static int xhci_pci_setup(struct usb_hcd *hcd)
 	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
 		xhci_pme_acpi_rtd3_enable(pdev);
 
-	xhci_dbg(xhci, "Got SBRN %u\n", (unsigned int) xhci->sbrn);
+	pci_read_config_byte(pdev, XHCI_SBRN_OFFSET, &sbrn);
+	xhci_dbg(xhci, "Got SBRN %u\n", (unsigned int)sbrn);
 
 	/* Find any debug ports */
 	return xhci_pci_reinit(xhci, pdev);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 6f8cecc789d6..d41523c9b15c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1498,7 +1498,6 @@ struct xhci_hcd {
 	spinlock_t	lock;
 
 	/* packed release number */
-	u8		sbrn;
 	u16		hci_version;
 	u16		max_interrupters;
 	/* imod_interval in ns (I * 250ns) */
-- 
2.25.1

[PATCH 08/12] usb: xhci: add comments explaining specific interrupt behaviour

[Mathias Nyman]

From: Niklas Neronin <niklas.neronin@linux.intel.com>

HCD does not allocate or request interrupt for the xhci driver, but HCD
does free and sync xhci interrupts in some cases. Add comment detailing
in which cases HCD will free/sync xhci interrupts.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index b405cff2d600..d6196c08ea87 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -93,6 +93,10 @@ static const struct xhci_driver_overrides xhci_pci_overrides __initconst = {
 	.update_hub_device = xhci_pci_update_hub_device,
 };
 
+/*
+ * Primary Legacy and MSI IRQ are synced in suspend_common().
+ * All MSI-X IRQs and secondary MSI IRQs should be synced here.
+ */
 static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 {
 	struct usb_hcd *hcd = xhci_to_hcd(xhci);
@@ -105,13 +109,12 @@ static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 	}
 }
 
-/* Free any IRQs and disable MSI-X */
+/* Legacy IRQ is freed by usb_remove_hcd() or usb_hcd_pci_shutdown() */
 static void xhci_cleanup_msix(struct xhci_hcd *xhci)
 {
 	struct usb_hcd *hcd = xhci_to_hcd(xhci);
 	struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
 
-	/* return if using legacy interrupt */
 	if (hcd->irq > 0)
 		return;
 
-- 
2.25.1

[PATCH 09/12] usb: xhci: remove 'retval' from xhci_pci_resume()

[Mathias Nyman]

From: Niklas Neronin <niklas.neronin@linux.intel.com>

Remove unnecessary local 'retval' argument.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index d6196c08ea87..526739af2070 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -786,7 +786,6 @@ static int xhci_pci_resume(struct usb_hcd *hcd, pm_message_t msg)
 {
 	struct xhci_hcd		*xhci = hcd_to_xhci(hcd);
 	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
-	int			retval = 0;
 
 	reset_control_reset(xhci->reset);
 
@@ -817,8 +816,7 @@ static int xhci_pci_resume(struct usb_hcd *hcd, pm_message_t msg)
 	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
 		xhci_pme_quirk(hcd);
 
-	retval = xhci_resume(xhci, msg);
-	return retval;
+	return xhci_resume(xhci, msg);
 }
 
 static int xhci_pci_poweroff_late(struct usb_hcd *hcd, bool do_wakeup)
-- 
2.25.1

[PATCH 10/12] usb: xhci: adjust empty TD list handling in handle_tx_event()

[Mathias Nyman]

From: Niklas Neronin <niklas.neronin@linux.intel.com>

Introduce an initial check for an empty list prior to entering the while
loop. Which enables, the implementation of distinct warnings to
differentiate between scenarios where the list is initially empty and
when it has been emptied during processing skipped isoc TDs.

These adjustments not only simplifies the large while loop, but also
facilitates future enhancements to the handle_tx_event() function.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 51 +++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d37eeee74960..a4383735b16c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2761,35 +2761,25 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 		return 0;
 	}
 
-	do {
-		/* This TRB should be in the TD at the head of this ring's
-		 * TD list.
+	if (list_empty(&ep_ring->td_list)) {
+		/*
+		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
+		 * extra completion event if the device was suspended. Or, a event for the last TRB
+		 * of a short TD we already got a short event for. The short TD is already removed
+		 * from the TD list.
 		 */
-		if (list_empty(&ep_ring->td_list)) {
-			/*
-			 * Don't print wanings if it's due to a stopped endpoint
-			 * generating an extra completion event if the device
-			 * was suspended. Or, a event for the last TRB of a
-			 * short TD we already got a short event for.
-			 * The short TD is already removed from the TD list.
-			 */
-
-			if (!(trb_comp_code == COMP_STOPPED ||
-			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
-			      ep_ring->last_td_was_short)) {
-				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
-					  slot_id, ep_index);
-			}
-			if (ep->skip) {
-				ep->skip = false;
-				xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
-					 slot_id, ep_index);
-			}
-
-			td = NULL;
-			goto check_endpoint_halted;
+		if (trb_comp_code != COMP_STOPPED &&
+		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
+		    !ep_ring->last_td_was_short) {
+			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
+				  slot_id, ep_index);
 		}
 
+		ep->skip = false;
+		goto check_endpoint_halted;
+	}
+
+	do {
 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
 				      td_list);
 
@@ -2800,7 +2790,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
 				skip_isoc_td(xhci, td, ep, status);
-				continue;
+				if (!list_empty(&ep_ring->td_list))
+					continue;
+
+				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
+					 slot_id, ep_index);
+				ep->skip = false;
+				td = NULL;
+				goto check_endpoint_halted;
 			}
 
 			/*
-- 
2.25.1

[PATCH 11/12] xhci: Set quirky xHC PCI hosts to D3 _after_ stopping and freeing them.

[Mathias Nyman]

PCI xHC host should be stopped and xhci driver memory freed before putting
host to PCI D3 state during PCI remove callback.

Hosts with XHCI_SPURIOUS_WAKEUP quirk did this the wrong way around
and set the host to D3 before calling usb_hcd_pci_remove(dev), which will
access the host to stop it, and then free xhci.

Fixes: f1f6d9a8b540 ("xhci: don't dereference a xhci member after removing xhci")
Cc: stable@vger.kernel.org
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-pci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 526739af2070..de50f5ba60df 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -657,8 +657,10 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 void xhci_pci_remove(struct pci_dev *dev)
 {
 	struct xhci_hcd *xhci;
+	bool set_power_d3;
 
 	xhci = hcd_to_xhci(pci_get_drvdata(dev));
+	set_power_d3 = xhci->quirks & XHCI_SPURIOUS_WAKEUP;
 
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
 
@@ -671,11 +673,11 @@ void xhci_pci_remove(struct pci_dev *dev)
 		xhci->shared_hcd = NULL;
 	}
 
+	usb_hcd_pci_remove(dev);
+
 	/* Workaround for spurious wakeups at shutdown with HSW */
-	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
+	if (set_power_d3)
 		pci_set_power_state(dev, PCI_D3hot);
-
-	usb_hcd_pci_remove(dev);
 }
 EXPORT_SYMBOL_NS_GPL(xhci_pci_remove, xhci);
 
-- 
2.25.1

[PATCH 12/12] xhci: support setting interrupt moderation IMOD for secondary interrupters

[Mathias Nyman]

Allow creators of seconday interrupters to specify the interrupt
moderation interval value in nanoseconds when creating the interrupter.

If not sure what value to use then use the xhci driver default
xhci->imod_interval

Suggested-by: Wesley Cheng <quic_wcheng@quicinc.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c | 8 +++++++-
 drivers/usb/host/xhci.c     | 4 ++--
 drivers/usb/host/xhci.h     | 5 ++++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 937ce5fd5809..d2900197a49e 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2332,7 +2332,8 @@ xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
 }
 
 struct xhci_interrupter *
-xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs)
+xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs,
+				  u32 imod_interval)
 {
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	struct xhci_interrupter *ir;
@@ -2365,6 +2366,11 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs)
 		return NULL;
 	}
 
+	err = xhci_set_interrupter_moderation(ir, imod_interval);
+	if (err)
+		xhci_warn(xhci, "Failed to set interrupter %d moderation to %uns\n",
+			  i, imod_interval);
+
 	xhci_dbg(xhci, "Add secondary interrupter %d, max interrupters %d\n",
 		 i, xhci->max_interrupters);
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a69245074395..899c0effb5d3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -347,8 +347,8 @@ static int xhci_disable_interrupter(struct xhci_interrupter *ir)
 }
 
 /* interrupt moderation interval imod_interval in nanoseconds */
-static int xhci_set_interrupter_moderation(struct xhci_interrupter *ir,
-					   u32 imod_interval)
+int xhci_set_interrupter_moderation(struct xhci_interrupter *ir,
+				    u32 imod_interval)
 {
 	u32 imod;
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index d41523c9b15c..1e50ebbe9514 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1826,7 +1826,8 @@ struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci,
 void xhci_free_container_ctx(struct xhci_hcd *xhci,
 		struct xhci_container_ctx *ctx);
 struct xhci_interrupter *
-xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs);
+xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs,
+				  u32 imod_interval);
 void xhci_remove_secondary_interrupter(struct usb_hcd
 				       *hcd, struct xhci_interrupter *ir);
 
@@ -1866,6 +1867,8 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci,
 		struct xhci_virt_device *virt_dev,
 		struct usb_device *hdev,
 		struct usb_tt *tt, gfp_t mem_flags);
+int xhci_set_interrupter_moderation(struct xhci_interrupter *ir,
+				    u32 imod_interval);
 
 /* xHCI ring, segment, TRB, and TD functions */
 dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, union xhci_trb *trb);
-- 
2.25.1

Re: [PATCH 04/12] usb: xhci: remove excessive isoc frame debug message spam

[Thinh Nguyen]

On Thu, Sep 05, 2024, Mathias Nyman wrote:
> From: Niklas Neronin <niklas.neronin@linux.intel.com>
> 
> The removed debug messages trigger each time an isoc frame is handled.
> In case of an error, a dedicated debug message exists.
> 
> For example, a 60fps USB camera will trigger the debug message every 0.6s.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/host/xhci-ring.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 4ea2c3e072a9..e1c9838084bf 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3941,10 +3941,6 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci,
>  	start_frame_id = (start_frame_id >> 3) & 0x7ff;
>  	end_frame_id = (end_frame_id >> 3) & 0x7ff;
>  
> -	xhci_dbg(xhci, "%s: index %d, reg 0x%x start_frame_id 0x%x, end_frame_id 0x%x, start_frame 0x%x\n",
> -		 __func__, index, readl(&xhci->run_regs->microframe_index),
> -		 start_frame_id, end_frame_id, start_frame);
> -
>  	if (start_frame_id < end_frame_id) {
>  		if (start_frame > end_frame_id ||
>  				start_frame < start_frame_id)
> -- 
> 2.25.1
> 

Please capture this info in the tracepoint instead. Otherwise we have no
idea if the isoc is scheduled as SIA or CFI. If it's CFI, I want to know
the start frame value. Currently, I don't think you're decoding this in
the TRB tracepoints.

BR,
Thinh

Re: [PATCH 10/12] usb: xhci: adjust empty TD list handling in handle_tx_event()

[fps]

On September 5, 2024 4:32:58 PM GMT+02:00, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
>From: Niklas Neronin <niklas.neronin@linux.intel.com>
>
>Introduce an initial check for an empty list prior to entering the while
>loop. Which enables, the implementation of distinct warnings to
>differentiate between scenarios where the list is initially empty and
>when it has been emptied during processing skipped isoc TDs.
>
>These adjustments not only simplifies the large while loop, but also
>facilitates future enhancements to the handle_tx_event() function.
>
>Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
>Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>---
> drivers/usb/host/xhci-ring.c | 51 +++++++++++++++++-------------------
> 1 file changed, 24 insertions(+), 27 deletions(-)
>
>diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>index d37eeee74960..a4383735b16c 100644
>--- a/drivers/usb/host/xhci-ring.c
>+++ b/drivers/usb/host/xhci-ring.c
>@@ -2761,35 +2761,25 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> 		return 0;
> 	}
> 
>-	do {
>-		/* This TRB should be in the TD at the head of this ring's
>-		 * TD list.
>+	if (list_empty(&ep_ring->td_list)) {
>+		/*
>+		 * Don't print wanings if ring is empty due to a stopped endpoint generating an

"wanings" should be "warnings", no?


>+		 * extra completion event if the device was suspended. Or, a event for the last TRB
>+		 * of a short TD we already got a short event for. The short TD is already removed
>+		 * from the TD list.
> 		 */
>-		if (list_empty(&ep_ring->td_list)) {
>-			/*
>-			 * Don't print wanings if it's due to a stopped endpoint
>-			 * generating an extra completion event if the device
>-			 * was suspended. Or, a event for the last TRB of a
>-			 * short TD we already got a short event for.
>-			 * The short TD is already removed from the TD list.
>-			 */
>-
>-			if (!(trb_comp_code == COMP_STOPPED ||
>-			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
>-			      ep_ring->last_td_was_short)) {
>-				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
>-					  slot_id, ep_index);
>-			}
>-			if (ep->skip) {
>-				ep->skip = false;
>-				xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
>-					 slot_id, ep_index);
>-			}
>-
>-			td = NULL;
>-			goto check_endpoint_halted;
>+		if (trb_comp_code != COMP_STOPPED &&
>+		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
>+		    !ep_ring->last_td_was_short) {
>+			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
>+				  slot_id, ep_index);
> 		}
> 
>+		ep->skip = false;
>+		goto check_endpoint_halted;
>+	}
>+
>+	do {
> 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
> 				      td_list);
> 
>@@ -2800,7 +2790,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> 
> 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
> 				skip_isoc_td(xhci, td, ep, status);
>-				continue;
>+				if (!list_empty(&ep_ring->td_list))
>+					continue;
>+
>+				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
>+					 slot_id, ep_index);
>+				ep->skip = false;
>+				td = NULL;
>+				goto check_endpoint_halted;
> 			}
> 
> 			/*

Kind regards,
FPS

Re: [PATCH 10/12] usb: xhci: adjust empty TD list handling in handle_tx_event()

[Neronin, Niklas]

On 06/09/2024 7.44, fps wrote:
> On September 5, 2024 4:32:58 PM GMT+02:00, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:
>> From: Niklas Neronin <niklas.neronin@linux.intel.com>
>>
>> Introduce an initial check for an empty list prior to entering the while
>> loop. Which enables, the implementation of distinct warnings to
>> differentiate between scenarios where the list is initially empty and
>> when it has been emptied during processing skipped isoc TDs.
>>
>> These adjustments not only simplifies the large while loop, but also
>> facilitates future enhancements to the handle_tx_event() function.
>>
>> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>> drivers/usb/host/xhci-ring.c | 51 +++++++++++++++++-------------------
>> 1 file changed, 24 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index d37eeee74960..a4383735b16c 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -2761,35 +2761,25 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>> 		return 0;
>> 	}
>>
>> -	do {
>> -		/* This TRB should be in the TD at the head of this ring's
>> -		 * TD list.
>> +	if (list_empty(&ep_ring->td_list)) {
>> +		/*
>> +		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
> 
> "wanings" should be "warnings", no?

Thanks, yes it should be the latter.
It will fix it in a handle_tx_event() cleanup patch.

Thanks,
Niklas
> 
> 
>> +		 * extra completion event if the device was suspended. Or, a event for the last TRB
>> +		 * of a short TD we already got a short event for. The short TD is already removed
>> +		 * from the TD list.
>> 		 */
>> -		if (list_empty(&ep_ring->td_list)) {
>> -			/*
>> -			 * Don't print wanings if it's due to a stopped endpoint
>> -			 * generating an extra completion event if the device
>> -			 * was suspended. Or, a event for the last TRB of a
>> -			 * short TD we already got a short event for.
>> -			 * The short TD is already removed from the TD list.
>> -			 */
>> -
>> -			if (!(trb_comp_code == COMP_STOPPED ||
>> -			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
>> -			      ep_ring->last_td_was_short)) {
>> -				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
>> -					  slot_id, ep_index);
>> -			}
>> -			if (ep->skip) {
>> -				ep->skip = false;
>> -				xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
>> -					 slot_id, ep_index);
>> -			}
>> -
>> -			td = NULL;
>> -			goto check_endpoint_halted;
>> +		if (trb_comp_code != COMP_STOPPED &&
>> +		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
>> +		    !ep_ring->last_td_was_short) {
>> +			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
>> +				  slot_id, ep_index);
>> 		}
>>
>> +		ep->skip = false;
>> +		goto check_endpoint_halted;
>> +	}
>> +
>> +	do {
>> 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
>> 				      td_list);
>>
>> @@ -2800,7 +2790,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>>
>> 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
>> 				skip_isoc_td(xhci, td, ep, status);
>> -				continue;
>> +				if (!list_empty(&ep_ring->td_list))
>> +					continue;
>> +
>> +				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
>> +					 slot_id, ep_index);
>> +				ep->skip = false;
>> +				td = NULL;
>> +				goto check_endpoint_halted;
>> 			}
>>
>> 			/*
> 
> Kind regards,
> FPS

Re: [PATCH 10/12] usb: xhci: adjust empty TD list handling in handle_tx_event()

[Michał Pecio]

>@@ -2761,35 +2761,25 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> 		return 0;
> 	}
> 
>-	do {
>-		/* This TRB should be in the TD at the head of this ring's
>-		 * TD list.
>+	if (list_empty(&ep_ring->td_list)) {
>+		/*
>+		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
>+		 * extra completion event if the device was suspended. Or, a event for the last TRB
Is changing this code perhaps an opportunity to clarify its comments?

This is just confusing. A stopped endpoint doesn't generate any "extra"
events since it can't be stopped again. Commit message of a83d6755814e4
suggests that this was about stopping running but idle EPs (as is the
case of EP0 before suspend). So briefly and to the point:

/* Ignore Force Stopped Event on an empty ring,
   or one containing only NOPs and Links */

>+		 * of a short TD we already got a short event for. The short TD is already removed
>+		 * from the TD list.
> 		 */
>-		if (list_empty(&ep_ring->td_list)) {
>-			/*
>-			 * Don't print wanings if it's due to a stopped endpoint
>-			 * generating an extra completion event if the device
>-			 * was suspended. Or, a event for the last TRB of a
>-			 * short TD we already got a short event for.
>-			 * The short TD is already removed from the TD list.
>-			 */
>-
>-			if (!(trb_comp_code == COMP_STOPPED ||
>-			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
>-			      ep_ring->last_td_was_short)) {
>-				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
>-					  slot_id, ep_index);
>-			}
>-			if (ep->skip) {
>-				ep->skip = false;
>-				xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
>-					 slot_id, ep_index);
>-			}
>-
>-			td = NULL;
>-			goto check_endpoint_halted;
>+		if (trb_comp_code != COMP_STOPPED &&
>+		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
>+		    !ep_ring->last_td_was_short) {
>+			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
>+				  slot_id, ep_index);
I would add trb_comp_code here if touching this line.

> 		}
> 
>+		ep->skip = false;
I don't like that the xhci_dbg() has been removed. If skip debugging is
to be reliable, it should report all state transitions. And this is an
unusual one, so maybe very interesting. Skip debugging is valuable, as
the logic is tricky and has known problem cases. More below.

>+		goto check_endpoint_halted;
>+	}
>+
>+	do {
> 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
> 				      td_list);
> 
>@@ -2800,7 +2790,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> 
> 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
> 				skip_isoc_td(xhci, td, ep, status);
>-				continue;
>+				if (!list_empty(&ep_ring->td_list))
>+					continue;
>+
>+				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
>+					 slot_id, ep_index);
This used to get the empty list warning, but now it's mere xhci_dbg().
Throwing out all queued TDs is not the common case and it may easily
be a bug. Indeed, I can readily name two cases when it is a bug today:

1. Force Stopped Event on a NOP or Link following the missed TD. Then
trb_in_td() doesn't match subsequent TD and the rest is trashed.

Actually, this is a v6.11 regression since d56b0b2ab1429. Although past
behavior was bad and broken too, it was broken differently.

2. Ring Underrun/Overrun if new TDs were queued before we handled it.
If ep_trb_dma is NULL, nothing ever matches and everything goes out.

Arguably, these are rare events and I haven't observed them yet.
And one more problem that I don't think currently exists, but:

3. If you ever find yourself doing it on an ordinary event (Success,
Transaction Error, Babble, etc.) then, seriously, WTF?

Bottom line, empty list is a very suspicious thing to see here. I can
only think of two legitimate cases:

1. Ring X-run, only if nothing new was queued since it occurred.
2. FSE outside transfer TDs, if no transfer TDs existed after it.

>+				ep->skip = false;
>+				td = NULL;
>+				goto check_endpoint_halted;
Isoch EPs can't stall and aren't supposed to halt on errors, 4.10.2.

> 			}
> 
> 			/*

Re: [PATCH 10/12] usb: xhci: adjust empty TD list handling in handle_tx_event()

[Neronin, Niklas]

On 06/09/2024 15.23, Michał Pecio wrote:
>> @@ -2761,35 +2761,25 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>> 		return 0;
>> 	}
>>
>> -	do {
>> -		/* This TRB should be in the TD at the head of this ring's
>> -		 * TD list.
>> +	if (list_empty(&ep_ring->td_list)) {
>> +		/*
>> +		 * Don't print wanings if ring is empty due to a stopped endpoint generating an
>> +		 * extra completion event if the device was suspended. Or, a event for the last TRB
> Is changing this code perhaps an opportunity to clarify its comments?
> 
> This is just confusing. A stopped endpoint doesn't generate any "extra"
> events since it can't be stopped again. Commit message of a83d6755814e4
> suggests that this was about stopping running but idle EPs (as is the
> case of EP0 before suspend). So briefly and to the point:
> 
> /* Ignore Force Stopped Event on an empty ring,
>    or one containing only NOPs and Links */

Thanks, for the suggestion. Indeed the comment should be updated.

> 
>> +		 * of a short TD we already got a short event for. The short TD is already removed
>> +		 * from the TD list.
>> 		 */
>> -		if (list_empty(&ep_ring->td_list)) {
>> -			/*
>> -			 * Don't print wanings if it's due to a stopped endpoint
>> -			 * generating an extra completion event if the device
>> -			 * was suspended. Or, a event for the last TRB of a
>> -			 * short TD we already got a short event for.
>> -			 * The short TD is already removed from the TD list.
>> -			 */
>> -
>> -			if (!(trb_comp_code == COMP_STOPPED ||
>> -			      trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
>> -			      ep_ring->last_td_was_short)) {
>> -				xhci_warn(xhci, "WARN Event TRB for slot %u ep %d with no TDs queued?\n",
>> -					  slot_id, ep_index);
>> -			}
>> -			if (ep->skip) {
>> -				ep->skip = false;
>> -				xhci_dbg(xhci, "td_list is empty while skip flag set. Clear skip flag for slot %u ep %u.\n",
>> -					 slot_id, ep_index);
>> -			}
>> -
>> -			td = NULL;
>> -			goto check_endpoint_halted;
>> +		if (trb_comp_code != COMP_STOPPED &&
>> +		    trb_comp_code != COMP_STOPPED_LENGTH_INVALID &&
>> +		    !ep_ring->last_td_was_short) {
>> +			xhci_warn(xhci, "Event TRB for slot %u ep %u with no TDs queued\n",
>> +				  slot_id, ep_index);
> I would add trb_comp_code here if touching this line.
> 
>> 		}
>>
>> +		ep->skip = false;
> I don't like that the xhci_dbg() has been removed. If skip debugging is
> to be reliable, it should report all state transitions. And this is an
> unusual one, so maybe very interesting. Skip debugging is valuable, as
> the logic is tricky and has known problem cases. More below.

Sure, I'll add a debug message when the skip flag is toggled.

> 
>> +		goto check_endpoint_halted;
>> +	}
>> +
>> +	do {
>> 		td = list_first_entry(&ep_ring->td_list, struct xhci_td,
>> 				      td_list);
>>
>> @@ -2800,7 +2790,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>>
>> 			if (ep->skip && usb_endpoint_xfer_isoc(&td->urb->ep->desc)) {
>> 				skip_isoc_td(xhci, td, ep, status);
>> -				continue;
>> +				if (!list_empty(&ep_ring->td_list))
>> +					continue;
>> +
>> +				xhci_dbg(xhci, "All TDs skipped for slot %u ep %u. Clear skip flag.\n",
>> +					 slot_id, ep_index);
> This used to get the empty list warning, but now it's mere xhci_dbg().
> Throwing out all queued TDs is not the common case and it may easily
> be a bug. Indeed, I can readily name two cases when it is a bug today:
> 
> 1. Force Stopped Event on a NOP or Link following the missed TD. Then
> trb_in_td() doesn't match subsequent TD and the rest is trashed.
> 
> Actually, this is a v6.11 regression since d56b0b2ab1429. Although past
> behavior was bad and broken too, it was broken differently.
> 
> 2. Ring Underrun/Overrun if new TDs were queued before we handled it.
> If ep_trb_dma is NULL, nothing ever matches and everything goes out.
> 
> Arguably, these are rare events and I haven't observed them yet.
> And one more problem that I don't think currently exists, but:
> 
> 3. If you ever find yourself doing it on an ordinary event (Success,
> Transaction Error, Babble, etc.) then, seriously, WTF?
> 
> Bottom line, empty list is a very suspicious thing to see here. I can
> only think of two legitimate cases:
> 
> 1. Ring X-run, only if nothing new was queued since it occurred.
> 2. FSE outside transfer TDs, if no transfer TDs existed after it.

I can change it from a debug to a warning. Then the edge case should be more visible.

> 
>> +				ep->skip = false;
>> +				td = NULL;
>> +				goto check_endpoint_halted;
> Isoch EPs can't stall and aren't supposed to halt on errors, 4.10.2.

Good point, thanks.

> 
>> 			}
>>
>> 			/*

Re: [PATCH 04/12] usb: xhci: remove excessive isoc frame debug message spam

[Mathias Nyman]

On 6.9.2024 0.30, Thinh Nguyen wrote:
> On Thu, Sep 05, 2024, Mathias Nyman wrote:
>> From: Niklas Neronin <niklas.neronin@linux.intel.com>
>>
>> The removed debug messages trigger each time an isoc frame is handled.
>> In case of an error, a dedicated debug message exists.
>>
>> For example, a 60fps USB camera will trigger the debug message every 0.6s.
>>
>> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>   drivers/usb/host/xhci-ring.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 4ea2c3e072a9..e1c9838084bf 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -3941,10 +3941,6 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci,
>>   	start_frame_id = (start_frame_id >> 3) & 0x7ff;
>>   	end_frame_id = (end_frame_id >> 3) & 0x7ff;
>>   
>> -	xhci_dbg(xhci, "%s: index %d, reg 0x%x start_frame_id 0x%x, end_frame_id 0x%x, start_frame 0x%x\n",
>> -		 __func__, index, readl(&xhci->run_regs->microframe_index),
>> -		 start_frame_id, end_frame_id, start_frame);
>> -
>>   	if (start_frame_id < end_frame_id) {
>>   		if (start_frame > end_frame_id ||
>>   				start_frame < start_frame_id)
>> -- 
>> 2.25.1
>>
> 
> Please capture this info in the tracepoint instead. Otherwise we have no
> idea if the isoc is scheduled as SIA or CFI. If it's CFI, I want to know
> the start frame value. Currently, I don't think you're decoding this in
> the TRB tracepoints.

Good point

Added TBC, TLBPC, frame_id, as SIA enabled 'S' or disabled 's' flag to Isoch TRB tracing.
76.383778: xhci_queue_trb: ISOC: Buffer 0000000118dfa000 length 3 TD size/TBC 0 intr 0 type 'Isoch' TBC 0 TLBPC 0 frame_id 610 flags s:b:i:I:c:s:I:e:C

code:

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 1f6ca0231c84..48b643ae8a40 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1023,9 +1023,6 @@ enum xhci_setup_dev {
  /* Interrupter Target - which MSI-X vector to target the completion event at */
  #define TRB_INTR_TARGET(p)     (((p) & 0x3ff) << 22)
  #define GET_INTR_TARGET(p)     (((p) >> 22) & 0x3ff)
-/* Total burst count field, Rsvdz on xhci 1.1 with Extended TBC enabled (ETE) */
-#define TRB_TBC(p)             (((p) & 0x3) << 7)
-#define TRB_TLBPC(p)           (((p) & 0xf) << 16)
  
  /* Cycle bit - indicates TRB ownership by HC or HCD */
  #define TRB_CYCLE              (1<<0)
@@ -1059,6 +1056,12 @@ enum xhci_setup_dev {
  /* Isochronous TRB specific fields */
  #define TRB_SIA                        (1<<31)
  #define TRB_FRAME_ID(p)                (((p) & 0x7ff) << 20)
+#define GET_FRAME_ID(p)                (((p) >> 20) & 0x7ff)
+/* Total burst count field, Rsvdz on xhci 1.1 with Extended TBC enabled (ETE) */
+#define TRB_TBC(p)             (((p) & 0x3) << 7)
+#define GET_TBC(p)             (((p) >> 7) & 0x3)
+#define TRB_TLBPC(p)           (((p) & 0xf) << 16)
+#define GET_TLBPC(p)           (((p) >> 16) & 0xf)
  
  /* TRB cache size for xHC with TRB cache */
  #define TRB_CACHE_SIZE_HS      8
@@ -2068,7 +2071,6 @@ static inline const char *xhci_decode_trb(char *str, size_t size,
                                 field3 & TRB_CYCLE ? 'C' : 'c');
                 break;
         case TRB_NORMAL:
-       case TRB_ISOC:
         case TRB_EVENT_DATA:
         case TRB_TR_NOOP:
                 snprintf(str, size,
@@ -2085,7 +2087,25 @@ static inline const char *xhci_decode_trb(char *str, size_t size,
                         field3 & TRB_ENT ? 'E' : 'e',
                         field3 & TRB_CYCLE ? 'C' : 'c');
                 break;
-
+       case TRB_ISOC:
+               snprintf(str, size,
+                       "Buffer %08x%08x length %d TD size/TBC %d intr %d type '%s' TBC %u TLBPC %u frame_id %u flags %c:%c:%c:%c:%c:%c:%c:%c:%c",
+                       field1, field0, TRB_LEN(field2), GET_TD_SIZE(field2),
+                       GET_INTR_TARGET(field2),
+                       xhci_trb_type_string(type),
+                       GET_TBC(field3),
+                       GET_TLBPC(field3),
+                       GET_FRAME_ID(field3),
+                       field3 & TRB_SIA ? 'S' : 's',
+                       field3 & TRB_BEI ? 'B' : 'b',
+                       field3 & TRB_IDT ? 'I' : 'i',
+                       field3 & TRB_IOC ? 'I' : 'i',
+                       field3 & TRB_CHAIN ? 'C' : 'c',
+                       field3 & TRB_NO_SNOOP ? 'S' : 's',
+                       field3 & TRB_ISP ? 'I' : 'i',
+                       field3 & TRB_ENT ? 'E' : 'e',
+                       field3 & TRB_CYCLE ? 'C' : 'c');
+               break;



Does this look good to you?

Patch on top of my for-usb-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-next

Thanks
Mathias

Re: [PATCH 04/12] usb: xhci: remove excessive isoc frame debug message spam

[Thinh Nguyen]

On Fri, Sep 06, 2024, Mathias Nyman wrote:
> On 6.9.2024 0.30, Thinh Nguyen wrote:
> > On Thu, Sep 05, 2024, Mathias Nyman wrote:
> > > From: Niklas Neronin <niklas.neronin@linux.intel.com>
> > > 
> > > The removed debug messages trigger each time an isoc frame is handled.
> > > In case of an error, a dedicated debug message exists.
> > > 
> > > For example, a 60fps USB camera will trigger the debug message every 0.6s.
> > > 
> > > Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > ---
> > >   drivers/usb/host/xhci-ring.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > > index 4ea2c3e072a9..e1c9838084bf 100644
> > > --- a/drivers/usb/host/xhci-ring.c
> > > +++ b/drivers/usb/host/xhci-ring.c
> > > @@ -3941,10 +3941,6 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci,
> > >   	start_frame_id = (start_frame_id >> 3) & 0x7ff;
> > >   	end_frame_id = (end_frame_id >> 3) & 0x7ff;
> > > -	xhci_dbg(xhci, "%s: index %d, reg 0x%x start_frame_id 0x%x, end_frame_id 0x%x, start_frame 0x%x\n",
> > > -		 __func__, index, readl(&xhci->run_regs->microframe_index),
> > > -		 start_frame_id, end_frame_id, start_frame);
> > > -
> > >   	if (start_frame_id < end_frame_id) {
> > >   		if (start_frame > end_frame_id ||
> > >   				start_frame < start_frame_id)
> > > -- 
> > > 2.25.1
> > > 
> > 
> > Please capture this info in the tracepoint instead. Otherwise we have no
> > idea if the isoc is scheduled as SIA or CFI. If it's CFI, I want to know
> > the start frame value. Currently, I don't think you're decoding this in
> > the TRB tracepoints.
> 
> Good point
> 
> Added TBC, TLBPC, frame_id, as SIA enabled 'S' or disabled 's' flag to Isoch TRB tracing.
> 76.383778: xhci_queue_trb: ISOC: Buffer 0000000118dfa000 length 3 TD size/TBC 0 intr 0 type 'Isoch' TBC 0 TLBPC 0 frame_id 610 flags s:b:i:I:c:s:I:e:C
> 
> code:
> 
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 1f6ca0231c84..48b643ae8a40 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1023,9 +1023,6 @@ enum xhci_setup_dev {
>  /* Interrupter Target - which MSI-X vector to target the completion event at */
>  #define TRB_INTR_TARGET(p)     (((p) & 0x3ff) << 22)
>  #define GET_INTR_TARGET(p)     (((p) >> 22) & 0x3ff)
> -/* Total burst count field, Rsvdz on xhci 1.1 with Extended TBC enabled (ETE) */
> -#define TRB_TBC(p)             (((p) & 0x3) << 7)
> -#define TRB_TLBPC(p)           (((p) & 0xf) << 16)
>  /* Cycle bit - indicates TRB ownership by HC or HCD */
>  #define TRB_CYCLE              (1<<0)
> @@ -1059,6 +1056,12 @@ enum xhci_setup_dev {
>  /* Isochronous TRB specific fields */
>  #define TRB_SIA                        (1<<31)
>  #define TRB_FRAME_ID(p)                (((p) & 0x7ff) << 20)
> +#define GET_FRAME_ID(p)                (((p) >> 20) & 0x7ff)
> +/* Total burst count field, Rsvdz on xhci 1.1 with Extended TBC enabled (ETE) */
> +#define TRB_TBC(p)             (((p) & 0x3) << 7)
> +#define GET_TBC(p)             (((p) >> 7) & 0x3)
> +#define TRB_TLBPC(p)           (((p) & 0xf) << 16)
> +#define GET_TLBPC(p)           (((p) >> 16) & 0xf)
>  /* TRB cache size for xHC with TRB cache */
>  #define TRB_CACHE_SIZE_HS      8
> @@ -2068,7 +2071,6 @@ static inline const char *xhci_decode_trb(char *str, size_t size,
>                                 field3 & TRB_CYCLE ? 'C' : 'c');
>                 break;
>         case TRB_NORMAL:
> -       case TRB_ISOC:
>         case TRB_EVENT_DATA:
>         case TRB_TR_NOOP:
>                 snprintf(str, size,
> @@ -2085,7 +2087,25 @@ static inline const char *xhci_decode_trb(char *str, size_t size,
>                         field3 & TRB_ENT ? 'E' : 'e',
>                         field3 & TRB_CYCLE ? 'C' : 'c');
>                 break;
> -
> +       case TRB_ISOC:
> +               snprintf(str, size,
> +                       "Buffer %08x%08x length %d TD size/TBC %d intr %d type '%s' TBC %u TLBPC %u frame_id %u flags %c:%c:%c:%c:%c:%c:%c:%c:%c",
> +                       field1, field0, TRB_LEN(field2), GET_TD_SIZE(field2),
> +                       GET_INTR_TARGET(field2),
> +                       xhci_trb_type_string(type),
> +                       GET_TBC(field3),
> +                       GET_TLBPC(field3),
> +                       GET_FRAME_ID(field3),
> +                       field3 & TRB_SIA ? 'S' : 's',
> +                       field3 & TRB_BEI ? 'B' : 'b',
> +                       field3 & TRB_IDT ? 'I' : 'i',
> +                       field3 & TRB_IOC ? 'I' : 'i',
> +                       field3 & TRB_CHAIN ? 'C' : 'c',
> +                       field3 & TRB_NO_SNOOP ? 'S' : 's',
> +                       field3 & TRB_ISP ? 'I' : 'i',
> +                       field3 & TRB_ENT ? 'E' : 'e',
> +                       field3 & TRB_CYCLE ? 'C' : 'c');
> +               break;
> 
> 
> 
> Does this look good to you?
> 
> Patch on top of my for-usb-next branch:
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-next__;!!A4F2R9G_pg!bo3LI3JLGNnmgpFB0wXYpvuY5aWKXUpzASgaQr6NImxYghpqzI3t5abRR8-tcGM0pKPSKiOvIgVg-rRQvkngu3z_q31D9m2C$
> 

Looks good to me!

Thanks,
Thinh

Re: [PATCH 10/12] usb: xhci: adjust empty TD list handling in handle_tx_event()

[Michał Pecio]

Hi,

> > This used to get the empty list warning, but now it's mere
> > xhci_dbg(). Throwing out all queued TDs is not the common case and
> > it may easily be a bug. Indeed, I can readily name two cases when
> > it is a bug today:
> > 
> > 1. Force Stopped Event on a NOP or Link following the missed TD.
> > Then trb_in_td() doesn't match subsequent TD and the rest is
> > trashed.
> > 
> > Actually, this is a v6.11 regression since d56b0b2ab1429. Although
> > past behavior was bad and broken too, it was broken differently.
> > 
> > 2. Ring Underrun/Overrun if new TDs were queued before we handled
> > it. If ep_trb_dma is NULL, nothing ever matches and everything goes
> > out.
> > 
> > Arguably, these are rare events and I haven't observed them yet.
> > And one more problem that I don't think currently exists, but:
> > 
> > 3. If you ever find yourself doing it on an ordinary event (Success,
> > Transaction Error, Babble, etc.) then, seriously, WTF?
> > 
> > Bottom line, empty list is a very suspicious thing to see here. I
> > can only think of two legitimate cases:
> > 
> > 1. Ring X-run, only if nothing new was queued since it occurred.
> > 2. FSE outside transfer TDs, if no transfer TDs existed after it.  
> 
> I can change it from a debug to a warning. Then the edge case should
> be more visible.

Actually, I'm no longer sure. If that ever happens, the user will next
see "WARN list empty" or "ERROR TRB not part of current TD" and this
will show there is a problem, so dyndbg will be enabled to investigate.

I would still like to see more comp_codes printed in those messages,
because on isoc there are some "weird" codes like Missed Service or
Underrun, which have a very different meaning from the usual ones and
are a source of bugs. But I would have to think about which cases it
is useful for, particularly after this change.

Lastly, I'm not sure if this change is worthwile. Over the weekend,
I have produced a fairly simple but dramatic simplification of this
whole code. The 'empty list' branch that your patch deals with can
be completely removed, because it duplicates functionality of other
code. The skipping loop is reduced down to 8 lines of code, as it
should have always been in the first place.

I will try to clean those patches up and send them out today. They
received varying degree of testing (some are two weeks old) and have
not caused any regression so far. They are simple and revieable IMO.

Regards,
Michal