[PATCH 0/7] usb: xhci: enhancements to address printing

[PATCH 0/7] usb: xhci: enhancements to address printing

[Niklas Neronin]

DMA Address Printing:
 All DMA address printing is now standardized using the "%pad" specifier.
 This guarantees correct address formatting regardless of the kernel's
 architecture, whether 32-bit or 64-bit.

Address Formatting:
 Printed hexadecimal values can be ambiguous, making it tricky to
 distinguish between values and addresses. To enhance clarity, all
 trusted DMA addresses are now prefixed with the '@' symbol, ensuring
 immediate recognition as addresses.

 However, the printk() specifier '%p' has had its '@' prefix removed.
 This is because it hashes the address before printing, effectively turning
 it into an address ID. The hashing process does not preserve any relation
 between addresses; for instance, if two addresses are X bytes apart, their
 hashed counterparts will not reflect this distinction.

Read 64-bit register printing:
 Debug messages that print an entire register value containing a DMA
 address have been improved. The register fields are now split and printed
 separately, making it easier for users to interpret the information.

These changes collectively improve the readability and consistency of
address printing in the xHCI driver, making it easier for developers and
maintainers to interpret log outputs accurately.

=== Alternative approach ===
An argument can be made for switching from representing a TRB by its DMA
address to using an index, as most printed DMA addresses are TRB addresses.
While this approach offers potential benefits, it also presents challenges.
Calculating the TRB index is straightforward when the segment and TRB are
known. However, when printing TRB addresses read from xHCI registers,
determining the TRB index becomes complex, increasing the risk of producing
incorrect debug messages. Due to these complexities, I have decided not to
switch to TRB indexes (yet).

Niklas Neronin (7):
  usb: xhci-dbgcap: correct DMA address handling
  usb: xhci: use '%pad' specifier for DMA address printing
  usb: xhci: improve Stream Context register debugging
  usb: xhci: improve Endpoint Context register debugging
  usb: xhci: improve Command Ring Control register debugging
  usb: xhci: improve Event Ring Dequeue Pointer Register debugging
  usb: xhci: standardize address format

 drivers/usb/host/xhci-dbgcap.c  | 11 +++++----
 drivers/usb/host/xhci-debugfs.c | 22 ++++++++++-------
 drivers/usb/host/xhci-mem.c     | 13 +++++-----
 drivers/usb/host/xhci-ring.c    | 42 ++++++++++++++++-----------------
 drivers/usb/host/xhci-trace.h   | 37 +++++++++++++++--------------
 drivers/usb/host/xhci.c         | 37 +++++++++++++++--------------
 drivers/usb/host/xhci.h         |  4 ++--
 7 files changed, 86 insertions(+), 80 deletions(-)

-- 
2.50.1

[PATCH 1/7] usb: xhci-dbgcap: correct DMA address handling

[Niklas Neronin]

Address the handling of addresses within the dbc_handle_xfer_event(),
ensuring accurate extraction and comparison.

Variable details:
'r->trb_dma'		    : A DMA address.
'ep_ctx->deq'		    : A le64, bits 63:4 contain the DMA address.
'event->trans_event.buffer' : A le64, bits 63:0 contain the DMA address.

1. Convert 'ep_ctx->deq' and 'event->trans_event.buffer' addresses to
   the CPU's native byte order.

2. Use mask; TR_DEQ_PTR_MASK (bits 63:4) to extract the address from
   'ep_ctx->deq', replacing the previous use of ~TRB_CYCLE (bits 63:1).

Why not use 'dma_addr_t' for the address?
The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
system architecture or xHCI driver flags, whereas the 64-bit address field
size remains constant. Since hardware cannot be fully trusted, it's better
to print the entire 64-bit address to detect any non-zero values in the
upper 32 bits. This approach ensures that potential issues are easily
detected.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-dbgcap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index 06a2edb9e86e..2f24a3a54439 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -724,6 +724,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
 	u32			comp_code;
 	size_t			remain_length;
 	struct dbc_request	*req = NULL, *r;
+	u64			ep_trb, deq;
 
 	comp_code	= GET_COMP_CODE(le32_to_cpu(event->generic.field[2]));
 	remain_length	= EVENT_TRB_LEN(le32_to_cpu(event->generic.field[2]));
@@ -733,10 +734,11 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
 	ep_ctx		= (ep_id == EPID_OUT) ?
 				dbc_bulkout_ctx(dbc) : dbc_bulkin_ctx(dbc);
 	ring		= dep->ring;
+	ep_trb		= le64_to_cpu(event->trans_event.buffer);
 
 	/* Match the pending request: */
 	list_for_each_entry(r, &dep->list_pending, list_pending) {
-		if (r->trb_dma == event->trans_event.buffer) {
+		if (r->trb_dma == ep_trb) {
 			req = r;
 			break;
 		}
@@ -768,8 +770,9 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
 		status = -comp_code;
 		break;
 	case COMP_STALL_ERROR:
+		deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
 		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);
+			 ep_trb, remain_length, deq);
 		status = 0;
 		dep->halted = 1;
 
@@ -788,7 +791,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
 		 * TRB again.
 		 */
 
-		if ((ep_ctx->deq & ~TRB_CYCLE) == event->trans_event.buffer) {
+		if (deq == ep_trb) {
 			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");
-- 
2.50.1

[PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Niklas Neronin]

Switch all printing of DMA addresses to '%pad' specifier. This specifier
ensures that the address is printed correctly, regardless of whether the
kernel is running in a 32-bit or 64-bit environment.

This patch exclusively modifies variables that are DMA addresses.
Subsequent patches will address variables that partially contain DMA
addresses.

Remove "offset" from dbg trace concerning URBs TD start addresses.
Prefix "0x" is automatically added by '%pad'.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-mem.c   |  4 ++--
 drivers/usb/host/xhci-ring.c  | 33 ++++++++++++++++-----------------
 drivers/usb/host/xhci-trace.h | 30 +++++++++++++++---------------
 drivers/usb/host/xhci.c       | 23 ++++++++++++-----------
 4 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 07289333a1e8..2a414dee7233 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1015,10 +1015,10 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 
 	/* Point to output device context in dcbaa. */
 	xhci->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(dev->out_ctx->dma);
-	xhci_dbg(xhci, "Set slot id %d dcbaa entry %p to 0x%llx\n",
+	xhci_dbg(xhci, "Set slot id %d dcbaa entry %p to %pad\n",
 		 slot_id,
 		 &xhci->dcbaa->dev_context_ptrs[slot_id],
-		 le64_to_cpu(xhci->dcbaa->dev_context_ptrs[slot_id]));
+		 &dev->out_ctx->dma);
 
 	trace_xhci_alloc_virt_device(dev);
 
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d2ccf19292aa..7d57e2a3abf3 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -781,7 +781,7 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
 	ep->queued_deq_ptr = new_deq;
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-		       "Set TR Deq ptr 0x%llx, cycle %u\n", addr, new_cycle);
+		       "Set TR Deq ptr %pad, cycle %u\n", &addr, new_cycle);
 
 	/* Stop the TD queueing code from ringing the doorbell until
 	 * this command completes.  The HC won't set the dequeue pointer
@@ -1035,6 +1035,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 	u64			hw_deq;
 	unsigned int		slot_id = ep->vdev->slot_id;
 	int			err;
+	dma_addr_t		dma;
 
 	/*
 	 * This is not going to work if the hardware is changing its dequeue
@@ -1046,11 +1047,10 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 	xhci = ep->xhci;
 
 	list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) {
+		dma = xhci_trb_virt_to_dma(td->start_seg, td->start_trb);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-			       "Removing canceled TD starting at 0x%llx (dma) in stream %u URB %p",
-			       (unsigned long long)xhci_trb_virt_to_dma(
-				       td->start_seg, td->start_trb),
-			       td->urb->stream_id, td->urb);
+			       "Removing canceled TD starting at %pad (dma) in stream %u URB %p",
+			       &dma, td->urb->stream_id, td->urb);
 		list_del_init(&td->td_list);
 		ring = xhci_urb_to_transfer_ring(xhci, td->urb);
 		if (!ring) {
@@ -2217,6 +2217,7 @@ static void finish_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 		      u32 trb_comp_code)
 {
 	struct xhci_ep_ctx *ep_ctx;
+	dma_addr_t dma;
 
 	ep_ctx = xhci_get_ep_ctx(xhci, ep->vdev->out_ctx, ep->ep_index);
 
@@ -2252,9 +2253,8 @@ static void finish_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 			 */
 			if ((ep->ep_state & EP_HALTED) &&
 			    !list_empty(&td->cancelled_td_list)) {
-				xhci_dbg(xhci, "Already resolving halted ep for 0x%llx\n",
-					 (unsigned long long)xhci_trb_virt_to_dma(
-						 td->start_seg, td->start_trb));
+				dma = xhci_trb_virt_to_dma(td->start_seg, td->start_trb);
+				xhci_dbg(xhci, "Already resolving halted ep for %pad\n", &dma);
 				return;
 			}
 			/* endpoint not halted, don't reset it */
@@ -2654,7 +2654,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	unsigned int slot_id;
 	int ep_index;
 	struct xhci_td *td = NULL;
-	dma_addr_t ep_trb_dma;
+	dma_addr_t ep_trb_dma, deq, td_start, td_end;
 	struct xhci_segment *ep_seg;
 	union xhci_trb *ep_trb;
 	int status = -EINPROGRESS;
@@ -2976,18 +2976,17 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 	return 0;
 
 debug_finding_td:
-	xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n",
-		 &ep_trb_dma, ep_index, trb_comp_code,
-		 (unsigned long long)xhci_trb_virt_to_dma(td->start_seg, td->start_trb),
-		 (unsigned long long)xhci_trb_virt_to_dma(td->end_seg, td->end_trb));
+	td_start = xhci_trb_virt_to_dma(td->start_seg, td->start_trb);
+	td_end = xhci_trb_virt_to_dma(td->end_seg, td->end_trb);
+	xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %pad - %pad\n",
+		 &ep_trb_dma, ep_index, trb_comp_code, &td_start, &td_end);
 
 	return -ESHUTDOWN;
 
 err_out:
-	xhci_err(xhci, "@%016llx %08x %08x %08x %08x\n",
-		 (unsigned long long) xhci_trb_virt_to_dma(
-			 ir->event_ring->deq_seg,
-			 ir->event_ring->dequeue),
+	deq = xhci_trb_virt_to_dma(ir->event_ring->deq_seg, ir->event_ring->dequeue);
+	xhci_err(xhci, "@%pad %08x %08x %08x %08x\n",
+		 &deq,
 		 lower_32_bits(le64_to_cpu(event->buffer)),
 		 upper_32_bits(le64_to_cpu(event->buffer)),
 		 le32_to_cpu(event->transfer_len),
diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
index bfb5c5c17012..8451e9386aa9 100644
--- a/drivers/usb/host/xhci-trace.h
+++ b/drivers/usb/host/xhci-trace.h
@@ -95,9 +95,9 @@ DECLARE_EVENT_CLASS(xhci_log_ctx,
 			((HCC_64BYTE_CONTEXT(xhci->hcc_params) + 1) * 32) *
 			((ctx->type == XHCI_CTX_TYPE_INPUT) + ep_num + 1));
 	),
-	TP_printk("ctx_64=%d, ctx_type=%u, ctx_dma=@%llx, ctx_va=@%p",
+	TP_printk("ctx_64=%d, ctx_type=%u, ctx_dma=@%pad, ctx_va=@%p",
 			__entry->ctx_64, __entry->ctx_type,
-			(unsigned long long) __entry->ctx_dma, __entry->ctx_va
+			&__entry->ctx_dma, __entry->ctx_va
 	)
 );
 
@@ -174,22 +174,22 @@ DECLARE_EVENT_CLASS(xhci_log_free_virt_dev,
 	TP_ARGS(vdev),
 	TP_STRUCT__entry(
 		__field(void *, vdev)
-		__field(unsigned long long, out_ctx)
-		__field(unsigned long long, in_ctx)
+		__field(dma_addr_t, out_ctx)
+		__field(dma_addr_t, in_ctx)
 		__field(int, slot_id)
 		__field(u16, current_mel)
 
 	),
 	TP_fast_assign(
 		__entry->vdev = vdev;
-		__entry->in_ctx = (unsigned long long) vdev->in_ctx->dma;
-		__entry->out_ctx = (unsigned long long) vdev->out_ctx->dma;
+		__entry->in_ctx = vdev->in_ctx->dma;
+		__entry->out_ctx = vdev->out_ctx->dma;
 		__entry->slot_id = (int) vdev->slot_id;
 		__entry->current_mel = (u16) vdev->current_mel;
 		),
-	TP_printk("vdev %p slot %d ctx %llx | %llx current_mel %d",
-		__entry->vdev, __entry->slot_id, __entry->in_ctx,
-		__entry->out_ctx, __entry->current_mel
+	TP_printk("vdev %p slot %d ctx %pad | %pad current_mel %d",
+		__entry->vdev, __entry->slot_id, &__entry->in_ctx,
+		&__entry->out_ctx, __entry->current_mel
 	)
 );
 
@@ -203,8 +203,8 @@ DECLARE_EVENT_CLASS(xhci_log_virt_dev,
 	TP_ARGS(vdev),
 	TP_STRUCT__entry(
 		__field(void *, vdev)
-		__field(unsigned long long, out_ctx)
-		__field(unsigned long long, in_ctx)
+		__field(dma_addr_t, out_ctx)
+		__field(dma_addr_t, in_ctx)
 		__field(int, devnum)
 		__field(int, state)
 		__field(int, speed)
@@ -214,8 +214,8 @@ DECLARE_EVENT_CLASS(xhci_log_virt_dev,
 	),
 	TP_fast_assign(
 		__entry->vdev = vdev;
-		__entry->in_ctx = (unsigned long long) vdev->in_ctx->dma;
-		__entry->out_ctx = (unsigned long long) vdev->out_ctx->dma;
+		__entry->in_ctx = vdev->in_ctx->dma;
+		__entry->out_ctx = vdev->out_ctx->dma;
 		__entry->devnum = vdev->udev->devnum;
 		__entry->state = vdev->udev->state;
 		__entry->speed = vdev->udev->speed;
@@ -223,8 +223,8 @@ DECLARE_EVENT_CLASS(xhci_log_virt_dev,
 		__entry->level = vdev->udev->level;
 		__entry->slot_id = vdev->udev->slot_id;
 	),
-	TP_printk("vdev %p ctx %llx | %llx num %d state %d speed %d port %d level %d slot %d",
-		__entry->vdev, __entry->in_ctx, __entry->out_ctx,
+	TP_printk("vdev %p ctx %pad | %pad num %d state %d speed %d port %d level %d slot %d",
+		__entry->vdev, &__entry->in_ctx, &__entry->out_ctx,
 		__entry->devnum, __entry->state, __entry->speed,
 		__entry->portnum, __entry->level, __entry->slot_id
 	)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 47151ca527bf..5e5681c1eb4e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1818,15 +1818,15 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	}
 
 	i = urb_priv->num_tds_done;
-	if (i < urb_priv->num_tds)
+	if (i < urb_priv->num_tds) {
+		dma_addr_t dma = xhci_trb_virt_to_dma(urb_priv->td[i].start_seg,
+						      urb_priv->td[i].start_trb);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-				"Cancel URB %p, dev %s, ep 0x%x, "
-				"starting at offset 0x%llx",
+				"Cancel URB %p, dev %s, ep 0x%x, starting at %pad",
 				urb, urb->dev->devpath,
 				urb->ep->desc.bEndpointAddress,
-				(unsigned long long) xhci_trb_virt_to_dma(
-					urb_priv->td[i].start_seg,
-					urb_priv->td[i].start_trb));
+				&dma);
+	}
 
 	for (; i < urb_priv->num_tds; i++) {
 		td = &urb_priv->td[i];
@@ -4273,6 +4273,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
 	struct xhci_input_control_ctx *ctrl_ctx;
 	u64 temp_64;
 	struct xhci_command *command = NULL;
+	dma_addr_t dma;
 
 	mutex_lock(&xhci->mutex);
 
@@ -4413,15 +4414,15 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
 	temp_64 = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
 			"Op regs DCBAA ptr = %#016llx", temp_64);
+	dma = le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id]);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
-		"Slot ID %d dcbaa entry @%p = %#016llx",
+		"Slot ID %d dcbaa entry @%p = %pad",
 		udev->slot_id,
 		&xhci->dcbaa->dev_context_ptrs[udev->slot_id],
-		(unsigned long long)
-		le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id]));
+		&dma);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
-			"Output Context DMA address = %#08llx",
-			(unsigned long long)virt_dev->out_ctx->dma);
+			"Output Context DMA address = %pad",
+			&virt_dev->out_ctx->dma);
 	trace_xhci_address_ctx(xhci, virt_dev->in_ctx,
 				le32_to_cpu(slot_ctx->dev_info) >> 27);
 	/*
-- 
2.50.1

[PATCH 3/7] usb: xhci: improve Stream Context register debugging

[Niklas Neronin]

Improve the debugging output for Stream Context registers in the xHCI
driver. The Stream Context registers consist of the following fields:
 bit 0 - Dequeue Cycle State.
 bits 3:1 - Stream Context Type.
 bits 63:4 - TR Dequeue Pointer, is 16-byte aligned.

Instead of printing the entire 64-bit register as a single block, each
field is now printed separately. This approach enhances the readability.

Remove xhci_dbg() in xhci_alloc_stream_info(). A detailed trace message is
printed after xhci_update_stream_mapping() call.

xHCI specification, section 6.2.4.1.

Why not use 'dma_addr_t' for the address?
The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
system architecture or xHCI driver flags, whereas the 64-bit address field
size remains constant. Since hardware cannot be fully trusted, it's better
to print the entire 64-bit address to detect any non-zero values in the
upper 32 bits. This approach ensures that potential issues are easily
detected.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-debugfs.c | 16 ++++++++++------
 drivers/usb/host/xhci-mem.c     |  1 -
 drivers/usb/host/xhci-trace.h   |  5 +++--
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c
index c6d44977193f..35398b95c5a2 100644
--- a/drivers/usb/host/xhci-debugfs.c
+++ b/drivers/usb/host/xhci-debugfs.c
@@ -521,6 +521,7 @@ static int xhci_stream_context_array_show(struct seq_file *s, void *unused)
 	struct xhci_ep_priv	*epriv = s->private;
 	struct xhci_stream_ctx	*stream_ctx;
 	dma_addr_t		dma;
+	u64			ctx;
 	int			id;
 
 	if (!epriv->stream_info)
@@ -533,12 +534,15 @@ static int xhci_stream_context_array_show(struct seq_file *s, void *unused)
 	for (id = 0; id < epriv->stream_info->num_stream_ctxs; id++) {
 		stream_ctx = epriv->stream_info->stream_ctx_array + id;
 		dma = epriv->stream_info->ctx_array_dma + id * 16;
-		if (id < epriv->stream_info->num_streams)
-			seq_printf(s, "%pad stream id %d deq %016llx\n", &dma,
-				   id, le64_to_cpu(stream_ctx->stream_ring));
-		else
-			seq_printf(s, "%pad stream context entry not used deq %016llx\n",
-				   &dma, le64_to_cpu(stream_ctx->stream_ring));
+
+		if (id < epriv->stream_info->num_streams) {
+			ctx = le64_to_cpu(stream_ctx->stream_ring);
+			seq_printf(s, "%pad stream %d: deq %016llx SCT %llu cycle %llu\n",
+				   &dma, id, ctx & TR_DEQ_PTR_MASK, CTX_TO_SCT(ctx),
+				   ctx & EP_CTX_CYCLE_MASK);
+		} else {
+			seq_printf(s, "%pad stream %d: entry not used\n", &dma, id);
+		}
 	}
 
 	return 0;
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 2a414dee7233..9520e7c6e774 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -676,7 +676,6 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
 			cur_ring->cycle_state;
 		stream_info->stream_ctx_array[cur_stream].stream_ring =
 			cpu_to_le64(addr);
-		xhci_dbg(xhci, "Setting stream %d ring ptr to 0x%08llx\n", cur_stream, addr);
 
 		ret = xhci_update_stream_mapping(cur_ring, mem_flags);
 
diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
index 8451e9386aa9..f6a2b4cedb8d 100644
--- a/drivers/usb/host/xhci-trace.h
+++ b/drivers/usb/host/xhci-trace.h
@@ -329,9 +329,10 @@ DECLARE_EVENT_CLASS(xhci_log_stream_ctx,
 		__entry->ctx_array_dma = info->ctx_array_dma + stream_id * 16;
 
 	),
-	TP_printk("stream %u ctx @%pad: SCT %llu deq %llx", __entry->stream_id,
+	TP_printk("stream %u ctx @%pad: SCT %llu deq %llx cycle %llu", __entry->stream_id,
 		&__entry->ctx_array_dma, CTX_TO_SCT(__entry->stream_ring),
-		__entry->stream_ring
+		__entry->stream_ring & TR_DEQ_PTR_MASK,
+		__entry->stream_ring & EP_CTX_CYCLE_MASK
 	)
 );
 
-- 
2.50.1

[PATCH 4/7] usb: xhci: improve Endpoint Context register debugging

[Niklas Neronin]

Improve the debugging output for Endpoint Context registers in the xHCI
driver. The Endpoint Context registers consist of the following fields:
 bit 0 - Dequeue Cycle State.
 bits 3:1 - RsvdZ.
 bits 63:4 - TR Dequeue Pointer, is 16-byte aligned.

Instead of printing the entire 64-bit register as a single block, each
field is now printed separately. This approach enhances the readability.

xHCI specification, section 6.2.3.

Why not use 'dma_addr_t' for the address?
The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
system architecture or xHCI driver flags, whereas the 64-bit address field
size remains constant. Since hardware cannot be fully trusted, it's better
to print the entire 64-bit address to detect any non-zero values in the
upper 32 bits. This approach ensures that potential issues are easily
detected.

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

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 59ff84ba2d4a..2662356d048e 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2580,9 +2580,9 @@ static inline const char *xhci_decode_ep_context(char *str, u32 info,
 	ret += sprintf(str + ret, "interval %d us max ESIT payload %d CErr %d ",
 			(1 << interval) * 125, esit, cerr);
 
-	ret += sprintf(str + ret, "Type %s %sburst %d maxp %d deq %016llx ",
+	ret += sprintf(str + ret, "Type %s %sburst %d maxp %d deq %016llx cycle %llu",
 			xhci_ep_type_string(ep_type), hid ? "HID" : "",
-			burst, maxp, deq);
+			burst, maxp, deq & TR_DEQ_PTR_MASK, deq & EP_CTX_CYCLE_MASK);
 
 	ret += sprintf(str + ret, "avg trb len %d", avg);
 
-- 
2.50.1

[PATCH 5/7] usb: xhci: improve Command Ring Control register debugging

[Niklas Neronin]

Improve the debugging output for Command Ring Control registers in the xHCI
driver. The CRCR consist of the following fields:
 bit 0 - Ring Cycle State
 bit 1 - Command Stop
 bit 2 - Command Abort
 bit 3 - Command Ring Running
 bits 5:4 - RsvdP.
 bits 63:4 - TR Dequeue Pointer, is 16-byte aligned.

Instead of printing the entire 64-bit register as a single block, each
field is now printed separately using the appropriate printk() specifiers.
This approach enhances user interpretation and ensures the DMA address
format is displayed accurately.

Except for bit 3, reading the other bits will consistently return '0'.
Therefore, only modified bits and bit 3 are printed.

xHCI specification, section 5.4.5.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 5e5681c1eb4e..4526989169a3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -499,7 +499,8 @@ static void xhci_set_cmd_ring_deq(struct xhci_hcd *xhci)
 	crcr &= ~CMD_RING_CYCLE;
 	crcr |= xhci->cmd_ring->cycle_state;
 
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Setting command ring address to 0x%llx", crcr);
+	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Command ring deq %pad running %u cycle %u",
+		       &deq_dma, crcr & CMD_RING_RUNNING, xhci->cmd_ring->cycle_state);
 	xhci_write_64(xhci, crcr, &xhci->op_regs->cmd_ring);
 }
 
-- 
2.50.1

[PATCH 6/7] usb: xhci: improve Event Ring Dequeue Pointer Register debugging

[Niklas Neronin]

Improve debugging output for Event Ring Dequeue Pointer Register (ERDP).
The ERDP registers consist of the following fields:
 bits 2:0 - Dequeue ERST Segment Index
 bit  3 - Event Handler Busy
 bits 63:4 - Event Ring Dequeue Pointer, is 16-byte aligned.

Instead of printing the entire 64-bit register as a single block, each
field is now printed separately. This approach enhances the readability.

xHCI specification, section 5.5.2.3.3

Why not use 'dma_addr_t' for the address?
The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
system architecture or xHCI driver flags, whereas the 64-bit address field
size remains constant. Since hardware cannot be fully trusted, it's better
to print the entire 64-bit address to detect any non-zero values in the
upper 32 bits. This approach ensures that potential issues are easily
detected.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 4526989169a3..c7fa82f8b853 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -634,7 +634,7 @@ static int xhci_run_finished(struct xhci_hcd *xhci)
  */
 int xhci_run(struct usb_hcd *hcd)
 {
-	u64 temp_64;
+	u64 erdp;
 	int ret;
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	struct xhci_interrupter *ir = xhci->interrupters[0];
@@ -651,10 +651,9 @@ int xhci_run(struct usb_hcd *hcd)
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "xhci_run");
 
-	temp_64 = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
-	temp_64 &= ERST_PTR_MASK;
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-			"ERST deq = 64'h%0lx", (long unsigned int) temp_64);
+	erdp = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
+	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "ERST ptr %llx DESI %llu EHB %llu",
+		       erdp & ERST_PTR_MASK, erdp & ERST_DESI_MASK, erdp & ERST_EHB);
 
 	xhci_set_interrupter_moderation(ir, xhci->imod_interval);
 
-- 
2.50.1

[PATCH 7/7] usb: xhci: standardize address format

[Niklas Neronin]

In the xHCI driver, printed hexadecimal values can be ambiguous, making it
difficult to distinguish between values and addresses. To enhance clarity,
all DMA addresses are now prefixed with the '@' symbol, ensuring immediate
recognition as addresses.

The '@' prefix is removed from '%p' specifiers, as they represent unique
address IDs rather than actual addresses. The hashing process does not
preserve any relation between addresses; for instance, if two addresses
are X bytes apart, their hashed counterparts will not reflect this
distinction.

Exceptions to the '@' prefix rule are functions xhci_ring_enqueue_show()
and xhci_ring_dequeue_show(), which exclusively print to the enqueue and
dequeue debugfs files, containing only addresses.

Standardize printing of all 64-bit addresses read from registers, using
the "0x%llx" specifier. Adding padding is unnecessary and provides no
useful information. Prefix the value with "0x" to clearly indicate that its
a hexadecimal.

$ git grep -n '0x%' | wc -l
39796
$ git grep -n '%#' | wc -l
5204

Redundant "0x" string prefix is removed from DMA addresses printed using
the '%pad' specifier, since '%pad' automatically includes the "0x" prefix.

Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
---
 drivers/usb/host/xhci-dbgcap.c  |  2 +-
 drivers/usb/host/xhci-debugfs.c | 10 +++++-----
 drivers/usb/host/xhci-mem.c     | 10 +++++-----
 drivers/usb/host/xhci-ring.c    | 17 ++++++++---------
 drivers/usb/host/xhci-trace.h   | 10 +++++-----
 drivers/usb/host/xhci.c         | 12 ++++++------
 drivers/usb/host/xhci.h         |  2 +-
 7 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index 2f24a3a54439..5c8f87c24fe7 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -771,7 +771,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
 		break;
 	case COMP_STALL_ERROR:
 		deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
-		dev_warn(dbc->dev, "Stall error at bulk TRB %llx, remaining %zu, ep deq %llx\n",
+		dev_warn(dbc->dev, "Stall error at bulk TRB 0x%llx, remaining %zu, ep deq 0x%llx\n",
 			 ep_trb, remain_length, deq);
 		status = 0;
 		dep->halted = 1;
diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c
index 35398b95c5a2..698b9fb59630 100644
--- a/drivers/usb/host/xhci-debugfs.c
+++ b/drivers/usb/host/xhci-debugfs.c
@@ -204,7 +204,7 @@ static void xhci_ring_dump_segment(struct seq_file *s,
 	for (i = 0; i < TRBS_PER_SEGMENT; i++) {
 		trb = &seg->trbs[i];
 		dma = seg->dma + i * sizeof(*trb);
-		seq_printf(s, "%2u %pad: %s\n", seg->num, &dma,
+		seq_printf(s, "%2u @%pad: %s\n", seg->num, &dma,
 			   xhci_decode_trb(str, XHCI_MSG_MAX, le32_to_cpu(trb->generic.field[0]),
 					   le32_to_cpu(trb->generic.field[1]),
 					   le32_to_cpu(trb->generic.field[2]),
@@ -254,7 +254,7 @@ static int xhci_slot_context_show(struct seq_file *s, void *unused)
 
 	xhci = hcd_to_xhci(bus_to_hcd(dev->udev->bus));
 	slot_ctx = xhci_get_slot_ctx(xhci, dev->out_ctx);
-	seq_printf(s, "%pad: %s\n", &dev->out_ctx->dma,
+	seq_printf(s, "@%pad: %s\n", &dev->out_ctx->dma,
 		   xhci_decode_slot_context(str,
 					    le32_to_cpu(slot_ctx->dev_info),
 					    le32_to_cpu(slot_ctx->dev_info2),
@@ -279,7 +279,7 @@ static int xhci_endpoint_context_show(struct seq_file *s, void *unused)
 	for (ep_index = 0; ep_index < 31; ep_index++) {
 		ep_ctx = xhci_get_ep_ctx(xhci, dev->out_ctx, ep_index);
 		dma = dev->out_ctx->dma + (ep_index + 1) * CTX_SIZE(xhci->hcc_params);
-		seq_printf(s, "%pad: %s, virt_state:%#x\n", &dma,
+		seq_printf(s, "@%pad: %s, virt_state:%#x\n", &dma,
 			   xhci_decode_ep_context(str,
 						  le32_to_cpu(ep_ctx->ep_info),
 						  le32_to_cpu(ep_ctx->ep_info2),
@@ -537,11 +537,11 @@ static int xhci_stream_context_array_show(struct seq_file *s, void *unused)
 
 		if (id < epriv->stream_info->num_streams) {
 			ctx = le64_to_cpu(stream_ctx->stream_ring);
-			seq_printf(s, "%pad stream %d: deq %016llx SCT %llu cycle %llu\n",
+			seq_printf(s, "@%pad stream %d: deq 0x%llx SCT %llu cycle %llu\n",
 				   &dma, id, ctx & TR_DEQ_PTR_MASK, CTX_TO_SCT(ctx),
 				   ctx & EP_CTX_CYCLE_MASK);
 		} else {
-			seq_printf(s, "%pad stream %d: entry not used\n", &dma, id);
+			seq_printf(s, "@%pad stream %d: entry not used\n", &dma, id);
 		}
 	}
 
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 9520e7c6e774..7a4a878db1c9 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -987,14 +987,14 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 	if (!dev->out_ctx)
 		goto fail;
 
-	xhci_dbg(xhci, "Slot %d output ctx = 0x%pad (dma)\n", slot_id, &dev->out_ctx->dma);
+	xhci_dbg(xhci, "Slot %d output ctx = @%pad (dma)\n", slot_id, &dev->out_ctx->dma);
 
 	/* Allocate the (input) device context for address device command */
 	dev->in_ctx = xhci_alloc_container_ctx(xhci, XHCI_CTX_TYPE_INPUT, flags);
 	if (!dev->in_ctx)
 		goto fail;
 
-	xhci_dbg(xhci, "Slot %d input ctx = 0x%pad (dma)\n", slot_id, &dev->in_ctx->dma);
+	xhci_dbg(xhci, "Slot %d input ctx = @%pad (dma)\n", slot_id, &dev->in_ctx->dma);
 
 	/* Initialize the cancellation and bandwidth list for each ep */
 	for (i = 0; i < 31; i++) {
@@ -1014,7 +1014,7 @@ int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
 
 	/* Point to output device context in dcbaa. */
 	xhci->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(dev->out_ctx->dma);
-	xhci_dbg(xhci, "Set slot id %d dcbaa entry %p to %pad\n",
+	xhci_dbg(xhci, "Set slot id %d dcbaa entry %p to @%pad\n",
 		 slot_id,
 		 &xhci->dcbaa->dev_context_ptrs[slot_id],
 		 &dev->out_ctx->dma);
@@ -2422,7 +2422,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 
 	xhci->dcbaa->dma = dma;
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-		       "Device context base array address = 0x%pad (DMA), %p (virt)",
+		       "Device context base array address = @%pad (DMA), %p (virt)",
 		       &xhci->dcbaa->dma, xhci->dcbaa);
 
 	/*
@@ -2483,7 +2483,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 		goto fail;
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Allocated command ring at %p", xhci->cmd_ring);
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "First segment DMA is 0x%pad",
+	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "First segment DMA is @%pad",
 		       &xhci->cmd_ring->first_seg->dma);
 
 	/*
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7d57e2a3abf3..1e9c2024df30 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -754,15 +754,14 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
 	}
 
 	if ((ep->ep_state & SET_DEQ_PENDING)) {
-		xhci_warn(xhci, "Set TR Deq already pending, don't submit for 0x%pad\n",
-			  &addr);
+		xhci_warn(xhci, "Set TR Deq already pending, don't submit for @%pad\n", &addr);
 		return -EBUSY;
 	}
 
 	/* This function gets called from contexts where it cannot sleep */
 	cmd = xhci_alloc_command(xhci, false, GFP_ATOMIC);
 	if (!cmd) {
-		xhci_warn(xhci, "Can't alloc Set TR Deq cmd 0x%pad\n", &addr);
+		xhci_warn(xhci, "Can't alloc Set TR Deq cmd @%pad\n", &addr);
 		return -ENOMEM;
 	}
 
@@ -781,7 +780,7 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
 	ep->queued_deq_ptr = new_deq;
 
 	xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-		       "Set TR Deq ptr %pad, cycle %u\n", &addr, new_cycle);
+		       "Set TR Deq ptr @%pad, cycle %u\n", &addr, new_cycle);
 
 	/* Stop the TD queueing code from ringing the doorbell until
 	 * this command completes.  The HC won't set the dequeue pointer
@@ -1049,7 +1048,7 @@ static int xhci_invalidate_cancelled_tds(struct xhci_virt_ep *ep)
 	list_for_each_entry_safe(td, tmp_td, &ep->cancelled_td_list, cancelled_td_list) {
 		dma = xhci_trb_virt_to_dma(td->start_seg, td->start_trb);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-			       "Removing canceled TD starting at %pad (dma) in stream %u URB %p",
+			       "Removing canceled TD starting at @%pad (dma) in stream %u URB %p",
 			       &dma, td->urb->stream_id, td->urb);
 		list_del_init(&td->td_list);
 		ring = xhci_urb_to_transfer_ring(xhci, td->urb);
@@ -1500,7 +1499,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
 			deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
 		}
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-			"Successful Set TR Deq Ptr cmd, deq = @%08llx", deq);
+			"Successful Set TR Deq Ptr cmd, deq = 0x%llx", deq);
 		if (xhci_trb_virt_to_dma(ep->queued_deq_seg,
 					 ep->queued_deq_ptr) == deq) {
 			/* Update the ring's dequeue segment and dequeue pointer
@@ -2254,7 +2253,7 @@ static void finish_td(struct xhci_hcd *xhci, struct xhci_virt_ep *ep,
 			if ((ep->ep_state & EP_HALTED) &&
 			    !list_empty(&td->cancelled_td_list)) {
 				dma = xhci_trb_virt_to_dma(td->start_seg, td->start_trb);
-				xhci_dbg(xhci, "Already resolving halted ep for %pad\n", &dma);
+				xhci_dbg(xhci, "Already resolving halted ep for @%pad\n", &dma);
 				return;
 			}
 			/* endpoint not halted, don't reset it */
@@ -2914,7 +2913,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 			 * transfer or error on last TRB. Ignore it.
 			 */
 			if (xhci_spurious_success_tx_event(xhci, ep_ring)) {
-				xhci_dbg(xhci, "Spurious event dma %pad, comp_code %u after %u\n",
+				xhci_dbg(xhci, "Spurious event dma @%pad, comp_code %u after %u\n",
 					 &ep_trb_dma, trb_comp_code, ep_ring->old_trb_comp_code);
 				ep_ring->old_trb_comp_code = 0;
 				return 0;
@@ -2978,7 +2977,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 debug_finding_td:
 	td_start = xhci_trb_virt_to_dma(td->start_seg, td->start_trb);
 	td_end = xhci_trb_virt_to_dma(td->end_seg, td->end_trb);
-	xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %pad - %pad\n",
+	xhci_err(xhci, "Event dma @%pad for ep %d status %d not part of TD at @%pad - @%pad\n",
 		 &ep_trb_dma, ep_index, trb_comp_code, &td_start, &td_end);
 
 	return -ESHUTDOWN;
diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
index f6a2b4cedb8d..fd3ed105b5c4 100644
--- a/drivers/usb/host/xhci-trace.h
+++ b/drivers/usb/host/xhci-trace.h
@@ -95,7 +95,7 @@ DECLARE_EVENT_CLASS(xhci_log_ctx,
 			((HCC_64BYTE_CONTEXT(xhci->hcc_params) + 1) * 32) *
 			((ctx->type == XHCI_CTX_TYPE_INPUT) + ep_num + 1));
 	),
-	TP_printk("ctx_64=%d, ctx_type=%u, ctx_dma=@%pad, ctx_va=@%p",
+	TP_printk("ctx_64=%d, ctx_type=%u, ctx_dma=@%pad, ctx_va=%p",
 			__entry->ctx_64, __entry->ctx_type,
 			&__entry->ctx_dma, __entry->ctx_va
 	)
@@ -187,7 +187,7 @@ DECLARE_EVENT_CLASS(xhci_log_free_virt_dev,
 		__entry->slot_id = (int) vdev->slot_id;
 		__entry->current_mel = (u16) vdev->current_mel;
 		),
-	TP_printk("vdev %p slot %d ctx %pad | %pad current_mel %d",
+	TP_printk("vdev %p slot %d ctx @%pad | @%pad current_mel %d",
 		__entry->vdev, __entry->slot_id, &__entry->in_ctx,
 		&__entry->out_ctx, __entry->current_mel
 	)
@@ -223,7 +223,7 @@ DECLARE_EVENT_CLASS(xhci_log_virt_dev,
 		__entry->level = vdev->udev->level;
 		__entry->slot_id = vdev->udev->slot_id;
 	),
-	TP_printk("vdev %p ctx %pad | %pad num %d state %d speed %d port %d level %d slot %d",
+	TP_printk("vdev %p ctx @%pad | @%pad num %d state %d speed %d port %d level %d slot %d",
 		__entry->vdev, &__entry->in_ctx, &__entry->out_ctx,
 		__entry->devnum, __entry->state, __entry->speed,
 		__entry->portnum, __entry->level, __entry->slot_id
@@ -329,7 +329,7 @@ DECLARE_EVENT_CLASS(xhci_log_stream_ctx,
 		__entry->ctx_array_dma = info->ctx_array_dma + stream_id * 16;
 
 	),
-	TP_printk("stream %u ctx @%pad: SCT %llu deq %llx cycle %llu", __entry->stream_id,
+	TP_printk("stream %u ctx @%pad: SCT %llu deq 0x%llx cycle %llu", __entry->stream_id,
 		&__entry->ctx_array_dma, CTX_TO_SCT(__entry->stream_ring),
 		__entry->stream_ring & TR_DEQ_PTR_MASK,
 		__entry->stream_ring & EP_CTX_CYCLE_MASK
@@ -505,7 +505,7 @@ DECLARE_EVENT_CLASS(xhci_log_ring,
 		__entry->enq = xhci_trb_virt_to_dma(ring->enq_seg, ring->enqueue);
 		__entry->deq = xhci_trb_virt_to_dma(ring->deq_seg, ring->dequeue);
 	),
-	TP_printk("%s %p: enq %pad deq %pad segs %d stream %d bounce %d cycle %d",
+	TP_printk("%s %p: enq @%pad deq @%pad segs %d stream %d bounce %d cycle %d",
 			xhci_ring_type_string(__entry->type), __entry->ring,
 			&__entry->enq,
 			&__entry->deq,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c7fa82f8b853..17755a567c6c 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -499,7 +499,7 @@ static void xhci_set_cmd_ring_deq(struct xhci_hcd *xhci)
 	crcr &= ~CMD_RING_CYCLE;
 	crcr |= xhci->cmd_ring->cycle_state;
 
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Command ring deq %pad running %u cycle %u",
+	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Command ring deq @%pad running %u cycle %u",
 		       &deq_dma, crcr & CMD_RING_RUNNING, xhci->cmd_ring->cycle_state);
 	xhci_write_64(xhci, crcr, &xhci->op_regs->cmd_ring);
 }
@@ -652,7 +652,7 @@ int xhci_run(struct usb_hcd *hcd)
 	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "xhci_run");
 
 	erdp = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
-	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "ERST ptr %llx DESI %llu EHB %llu",
+	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "ERST ptr 0x%llx DESI %llu EHB %llu",
 		       erdp & ERST_PTR_MASK, erdp & ERST_DESI_MASK, erdp & ERST_EHB);
 
 	xhci_set_interrupter_moderation(ir, xhci->imod_interval);
@@ -1822,7 +1822,7 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		dma_addr_t dma = xhci_trb_virt_to_dma(urb_priv->td[i].start_seg,
 						      urb_priv->td[i].start_trb);
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
-				"Cancel URB %p, dev %s, ep 0x%x, starting at %pad",
+				"Cancel URB %p, dev %s, ep 0x%x, starting at @%pad",
 				urb, urb->dev->devpath,
 				urb->ep->desc.bEndpointAddress,
 				&dma);
@@ -4413,15 +4413,15 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
 		goto out;
 	temp_64 = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
-			"Op regs DCBAA ptr = %#016llx", temp_64);
+			"Op regs DCBAA ptr = 0x%llx", temp_64);
 	dma = le64_to_cpu(xhci->dcbaa->dev_context_ptrs[udev->slot_id]);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
-		"Slot ID %d dcbaa entry @%p = %pad",
+		"Slot ID %d dcbaa entry %p = @%pad",
 		udev->slot_id,
 		&xhci->dcbaa->dev_context_ptrs[udev->slot_id],
 		&dma);
 	xhci_dbg_trace(xhci, trace_xhci_dbg_address,
-			"Output Context DMA address = %pad",
+			"Output Context DMA address = @%pad",
 			&virt_dev->out_ctx->dma);
 	trace_xhci_address_ctx(xhci, virt_dev->in_ctx,
 				le32_to_cpu(slot_ctx->dev_info) >> 27);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 2662356d048e..f8d742695176 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2580,7 +2580,7 @@ static inline const char *xhci_decode_ep_context(char *str, u32 info,
 	ret += sprintf(str + ret, "interval %d us max ESIT payload %d CErr %d ",
 			(1 << interval) * 125, esit, cerr);
 
-	ret += sprintf(str + ret, "Type %s %sburst %d maxp %d deq %016llx cycle %llu",
+	ret += sprintf(str + ret, "Type %s %sburst %d maxp %d deq 0x%llx cycle %llu",
 			xhci_ep_type_string(ep_type), hid ? "HID" : "",
 			burst, maxp, deq & TR_DEQ_PTR_MASK, deq & EP_CTX_CYCLE_MASK);
 
-- 
2.50.1

Re: [PATCH 7/7] usb: xhci: standardize address format

[Michal Pecio]

On Wed,  3 Sep 2025 19:01:27 +0200, Niklas Neronin wrote:
> In the xHCI driver, printed hexadecimal values can be ambiguous, making it
> difficult to distinguish between values and addresses. To enhance clarity,
> all DMA addresses are now prefixed with the '@' symbol, ensuring immediate
> recognition as addresses.

Is it really a problem? Which values look like addresses?

Is it important enough to justify merge conflicts on multiple debug
messages when porting patches to stable? There are many inconsistencies
and typose in those messages, but I only clean them up if change is
necessary for other reasons.

> The '@' prefix is removed from '%p' specifiers, as they represent unique
> address IDs rather than actual addresses. The hashing process does not
> preserve any relation between addresses; for instance, if two addresses
> are X bytes apart, their hashed counterparts will not reflect this
> distinction.

Is it really a problem? BTW, you can reboot with 'no_hash_pointers'
to make %p work normally for debugging.

Personally, I do believe that there is a problem: those @ add no real
information and they need to be stripped by scripts or when copying
numbers from logs to calculators. But see comment above.

This problem applies to new cases added by this patch as well.

And what if the kernel starts hashing %pad by default? ;)

> Exceptions to the '@' prefix rule are functions xhci_ring_enqueue_show()
> and xhci_ring_dequeue_show(), which exclusively print to the enqueue and
> dequeue debugfs files, containing only addresses.

So debugfs will also get @ now, except for two files apparently?

Why are those files left out and inconsistent with the rest? If the
answer is "because the @ prefix is annoying and breaks tools" then the
same answer applies to every other debugfs file ;)

I have a script which parses event-ring/trbs and command-ring/trbs to
print commands and their completions together. Others may have other
scripts. They will stop working now. And for what gain?

debugfs is nothing but dumps od xHCI data structures, anyone going
there already knows what those fields are.

> Standardize printing of all 64-bit addresses read from registers, using
> the "0x%llx" specifier.

%#llx is easier to type and less eye sore than 0x%llx.

The 0x prefix is maybe rarely necessary for humans, but useful because
such format is ready to parse by hex-aware calculators or scripts.

Same argument works against @ prefix. Scripting languages, terminals
and text editors can select whole words and we don't need @ included.

> Adding padding is unnecessary and provides no useful information.
> Prefix the value with "0x" to clearly indicate that its a hexadecimal.

Sounds like an argument against converting to %pad in other places?

That being said, I'm not sure if %08llx is truly evil yet.

> $ git grep -n '0x%' | wc -l
> 39796
> $ git grep -n '%#' | wc -l
> 5204

Not sure what is this doing in a commit message?

> Redundant "0x" string prefix is removed from DMA addresses printed
> using the '%pad' specifier, since '%pad' automatically includes the
> "0x" prefix.

Not sure if urgent enough to bother, but makes sense of course.

Regards,
Michal

Re: [PATCH 5/7] usb: xhci: improve Command Ring Control register debugging

[Michal Pecio]

On Wed,  3 Sep 2025 19:01:25 +0200, Niklas Neronin wrote:
> Improve the debugging output for Command Ring Control registers in the xHCI
> driver. The CRCR consist of the following fields:
>  bit 0 - Ring Cycle State
>  bit 1 - Command Stop
>  bit 2 - Command Abort
>  bit 3 - Command Ring Running
>  bits 5:4 - RsvdP.
>  bits 63:4 - TR Dequeue Pointer, is 16-byte aligned.
> 
> Instead of printing the entire 64-bit register as a single block, each
> field is now printed separately using the appropriate printk() specifiers.
> This approach enhances user interpretation and ensures the DMA address
> format is displayed accurately.
> 
> Except for bit 3, reading the other bits will consistently return '0'.
> Therefore, only modified bits and bit 3 are printed.
> 
> xHCI specification, section 5.4.5.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 5e5681c1eb4e..4526989169a3 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -499,7 +499,8 @@ static void xhci_set_cmd_ring_deq(struct xhci_hcd *xhci)
>  	crcr &= ~CMD_RING_CYCLE;
>  	crcr |= xhci->cmd_ring->cycle_state;
>  
> -	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Setting command ring address to 0x%llx", crcr);
> +	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Command ring deq %pad running %u cycle %u",
> +		       &deq_dma, crcr & CMD_RING_RUNNING, xhci->cmd_ring->cycle_state);

I think the word "Setting" was useful here.

This function should probably not be called on a running ring (the
spec says: writes to most field are ignored in this state). If there
is concern that it may happen, but not enough concern to justify some
xhci_err(), then maybe

    "... (ring%s running)", ..., RUNNING ? "" : " not");

>  	xhci_write_64(xhci, crcr, &xhci->op_regs->cmd_ring);
>  }
>  
> -- 
> 2.50.1
> 

Re: [PATCH 4/7] usb: xhci: improve Endpoint Context register debugging

[Michal Pecio]

On Wed,  3 Sep 2025 19:01:24 +0200, Niklas Neronin wrote:
> Improve the debugging output for Endpoint Context registers in the xHCI
> driver. The Endpoint Context registers consist of the following fields:
>  bit 0 - Dequeue Cycle State.
>  bits 3:1 - RsvdZ.
>  bits 63:4 - TR Dequeue Pointer, is 16-byte aligned.
> 
> Instead of printing the entire 64-bit register as a single block, each
> field is now printed separately. This approach enhances the readability.
> 
> xHCI specification, section 6.2.3.
> 
> Why not use 'dma_addr_t' for the address?
> The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
> system architecture or xHCI driver flags, whereas the 64-bit address field
> size remains constant. Since hardware cannot be fully trusted, it's better
> to print the entire 64-bit address to detect any non-zero values in the
> upper 32 bits. This approach ensures that potential issues are easily
> detected.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 59ff84ba2d4a..2662356d048e 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -2580,9 +2580,9 @@ static inline const char *xhci_decode_ep_context(char *str, u32 info,
>  	ret += sprintf(str + ret, "interval %d us max ESIT payload %d CErr %d ",
>  			(1 << interval) * 125, esit, cerr);
>  
> -	ret += sprintf(str + ret, "Type %s %sburst %d maxp %d deq %016llx ",
> +	ret += sprintf(str + ret, "Type %s %sburst %d maxp %d deq %016llx cycle %llu",
>  			xhci_ep_type_string(ep_type), hid ? "HID" : "",
> -			burst, maxp, deq);
> +			burst, maxp, deq & TR_DEQ_PTR_MASK, deq & EP_CTX_CYCLE_MASK);

Does it really bother people who use debugfs that deq includes the
cycle bit, which is exactly what anyone who know xHCI will expect?

This line is quite damn long already.

Also, I am highly confident that you haven't even tested this patch.
Try it and see what happens ;)

>  
>  	ret += sprintf(str + ret, "avg trb len %d", avg);
>  
> -- 
> 2.50.1
> 

Re: [PATCH 3/7] usb: xhci: improve Stream Context register debugging

[Michal Pecio]

On Wed,  3 Sep 2025 19:01:23 +0200, Niklas Neronin wrote:
> Improve the debugging output for Stream Context registers in the xHCI
> driver. The Stream Context registers consist of the following fields:
>  bit 0 - Dequeue Cycle State.
>  bits 3:1 - Stream Context Type.
>  bits 63:4 - TR Dequeue Pointer, is 16-byte aligned.
> 
> Instead of printing the entire 64-bit register as a single block, each
> field is now printed separately. This approach enhances the readability.
> 
> Remove xhci_dbg() in xhci_alloc_stream_info(). A detailed trace message is
> printed after xhci_update_stream_mapping() call.
> 
> xHCI specification, section 6.2.4.1.
> 
> Why not use 'dma_addr_t' for the address?
> The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
> system architecture or xHCI driver flags, whereas the 64-bit address field
> size remains constant. Since hardware cannot be fully trusted, it's better
> to print the entire 64-bit address to detect any non-zero values in the
> upper 32 bits. This approach ensures that potential issues are easily
> detected.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-debugfs.c | 16 ++++++++++------
>  drivers/usb/host/xhci-mem.c     |  1 -
>  drivers/usb/host/xhci-trace.h   |  5 +++--
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c
> index c6d44977193f..35398b95c5a2 100644
> --- a/drivers/usb/host/xhci-debugfs.c
> +++ b/drivers/usb/host/xhci-debugfs.c
> @@ -521,6 +521,7 @@ static int xhci_stream_context_array_show(struct seq_file *s, void *unused)
>  	struct xhci_ep_priv	*epriv = s->private;
>  	struct xhci_stream_ctx	*stream_ctx;
>  	dma_addr_t		dma;
> +	u64			ctx;
>  	int			id;
>  
>  	if (!epriv->stream_info)
> @@ -533,12 +534,15 @@ static int xhci_stream_context_array_show(struct seq_file *s, void *unused)
>  	for (id = 0; id < epriv->stream_info->num_stream_ctxs; id++) {
>  		stream_ctx = epriv->stream_info->stream_ctx_array + id;
>  		dma = epriv->stream_info->ctx_array_dma + id * 16;
> -		if (id < epriv->stream_info->num_streams)
> -			seq_printf(s, "%pad stream id %d deq %016llx\n", &dma,
> -				   id, le64_to_cpu(stream_ctx->stream_ring));
> -		else
> -			seq_printf(s, "%pad stream context entry not used deq %016llx\n",
> -				   &dma, le64_to_cpu(stream_ctx->stream_ring));
> +
> +		if (id < epriv->stream_info->num_streams) {
> +			ctx = le64_to_cpu(stream_ctx->stream_ring);
> +			seq_printf(s, "%pad stream %d: deq %016llx SCT %llu cycle %llu\n",
> +				   &dma, id, ctx & TR_DEQ_PTR_MASK, CTX_TO_SCT(ctx),
> +				   ctx & EP_CTX_CYCLE_MASK);

That SCT could benefit from decoding to human readable form,
but AFAIK the driver currently isn't using it anyway, so it
doesn't matter very much.

> +		} else {
> +			seq_printf(s, "%pad stream %d: entry not used\n", &dma, id);
> +		}
>  	}
>  
>  	return 0;
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 2a414dee7233..9520e7c6e774 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -676,7 +676,6 @@ struct xhci_stream_info *xhci_alloc_stream_info(struct xhci_hcd *xhci,
>  			cur_ring->cycle_state;
>  		stream_info->stream_ctx_array[cur_stream].stream_ring =
>  			cpu_to_le64(addr);
> -		xhci_dbg(xhci, "Setting stream %d ring ptr to 0x%08llx\n", cur_stream, addr);
>  
>  		ret = xhci_update_stream_mapping(cur_ring, mem_flags);
>  
> diff --git a/drivers/usb/host/xhci-trace.h b/drivers/usb/host/xhci-trace.h
> index 8451e9386aa9..f6a2b4cedb8d 100644
> --- a/drivers/usb/host/xhci-trace.h
> +++ b/drivers/usb/host/xhci-trace.h
> @@ -329,9 +329,10 @@ DECLARE_EVENT_CLASS(xhci_log_stream_ctx,
>  		__entry->ctx_array_dma = info->ctx_array_dma + stream_id * 16;
>  
>  	),
> -	TP_printk("stream %u ctx @%pad: SCT %llu deq %llx", __entry->stream_id,
> +	TP_printk("stream %u ctx @%pad: SCT %llu deq %llx cycle %llu", __entry->stream_id,
>  		&__entry->ctx_array_dma, CTX_TO_SCT(__entry->stream_ring),
> -		__entry->stream_ring
> +		__entry->stream_ring & TR_DEQ_PTR_MASK,
> +		__entry->stream_ring & EP_CTX_CYCLE_MASK
>  	)
>  );
>  
> -- 
> 2.50.1
> 

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Wed,  3 Sep 2025 19:01:22 +0200, Niklas Neronin wrote:
> Switch all printing of DMA addresses to '%pad' specifier. This specifier
> ensures that the address is printed correctly, regardless of whether the
> kernel is running in a 32-bit or 64-bit environment.

Old %llx with (long long) cast also prints it corretly.

I had the same idea and even implemented it in some private debugging
patches, but I found %pad just annoying in practice.


%pad isn't guaranteed to be at least 64 bit long, so some DMAs from
64 bit hardware will always need to be printed with %llx or similar.

Secondly, padding is not optional with %pad. Maybe not a big deal, but
on 64 bit systems with comparatively little RAM it adds clutter.

Thirdly, %pad can't be passed by value. Hence pollution like:

> @@ -2654,7 +2654,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
>  	unsigned int slot_id;
>  	int ep_index;
>  	struct xhci_td *td = NULL;
> -	dma_addr_t ep_trb_dma;
> +	dma_addr_t ep_trb_dma, deq, td_start, td_end;
>  	struct xhci_segment *ep_seg;
>  	union xhci_trb *ep_trb;
>  	int status = -EINPROGRESS;

This function has plenty of variables already, not sure if it needs
three more. We could work around it by introducing {} scopes around
printing, or functions like print_scary_error_message(), but it ends
up being more hassle than type casting at some point.

Maybe a small helper if the verbose casts really bother people?
static inline unsigned long long dma2llx(dma_addr_t dma) {return dma;}

BTW, isn't unsigned unnecessary?

Re: [PATCH 1/7] usb: xhci-dbgcap: correct DMA address handling

[Michal Pecio]

On Wed,  3 Sep 2025 19:01:21 +0200, Niklas Neronin wrote:
> Address the handling of addresses within the dbc_handle_xfer_event(),
> ensuring accurate extraction and comparison.
> 
> Variable details:
> 'r->trb_dma'		    : A DMA address.
> 'ep_ctx->deq'		    : A le64, bits 63:4 contain the DMA address.
> 'event->trans_event.buffer' : A le64, bits 63:0 contain the DMA address.
> 
> 1. Convert 'ep_ctx->deq' and 'event->trans_event.buffer' addresses to
>    the CPU's native byte order.
> 
> 2. Use mask; TR_DEQ_PTR_MASK (bits 63:4) to extract the address from
>    'ep_ctx->deq', replacing the previous use of ~TRB_CYCLE (bits 63:1).
> 
> Why not use 'dma_addr_t' for the address?
> The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
> system architecture or xHCI driver flags, whereas the 64-bit address field
> size remains constant. Since hardware cannot be fully trusted, it's better
> to print the entire 64-bit address to detect any non-zero values in the
> upper 32 bits. This approach ensures that potential issues are easily
> detected.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-dbgcap.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> index 06a2edb9e86e..2f24a3a54439 100644
> --- a/drivers/usb/host/xhci-dbgcap.c
> +++ b/drivers/usb/host/xhci-dbgcap.c
> @@ -724,6 +724,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>  	u32			comp_code;
>  	size_t			remain_length;
>  	struct dbc_request	*req = NULL, *r;
> +	u64			ep_trb, deq;
>  
>  	comp_code	= GET_COMP_CODE(le32_to_cpu(event->generic.field[2]));
>  	remain_length	= EVENT_TRB_LEN(le32_to_cpu(event->generic.field[2]));
> @@ -733,10 +734,11 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>  	ep_ctx		= (ep_id == EPID_OUT) ?
>  				dbc_bulkout_ctx(dbc) : dbc_bulkin_ctx(dbc);
>  	ring		= dep->ring;
> +	ep_trb		= le64_to_cpu(event->trans_event.buffer);

In other places this variable would be called ep_trb_dma
and ep_trb would be xhci_trb*.

>  
>  	/* Match the pending request: */
>  	list_for_each_entry(r, &dep->list_pending, list_pending) {
> -		if (r->trb_dma == event->trans_event.buffer) {
> +		if (r->trb_dma == ep_trb) {

And here it is being compared with a something_dma variable.

>  			req = r;
>  			break;
>  		}
> @@ -768,8 +770,9 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>  		status = -comp_code;
>  		break;
>  	case COMP_STALL_ERROR:
> +		deq = le64_to_cpu(ep_ctx->deq) & TR_DEQ_PTR_MASK;
>  		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);
> +			 ep_trb, remain_length, deq);
>  		status = 0;
>  		dep->halted = 1;
>  
> @@ -788,7 +791,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>  		 * TRB again.
>  		 */
>  
> -		if ((ep_ctx->deq & ~TRB_CYCLE) == event->trans_event.buffer) {
> +		if (deq == ep_trb) {
>  			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");
> -- 
> 2.50.1
> 

Re: [PATCH 5/7] usb: xhci: improve Command Ring Control register debugging

[Michal Pecio]

On Tue, 9 Sep 2025 11:17:53 +0200, Michal Pecio wrote:
> This function should probably not be called on a running ring (the
> spec says: writes to most field are ignored in this state). If there
> is concern that it may happen, but not enough concern to justify some
> xhci_err(), then maybe
> 
>     "... (ring%s running)", ..., RUNNING ? "" : " not");

Or just "Setting command ring blah blah (ring running %d)". My point is
not about the exact format, but about adding a small remark to existing
message rather than rewriting it and making it unclear what's happening
and why is this message printed at all.

Re: [PATCH 4/7] usb: xhci: improve Endpoint Context register debugging

[Michal Pecio]

On Tue, 9 Sep 2025 11:20:17 +0200, Michal Pecio wrote:
> Also, I am highly confident that you haven't even tested this patch.
> Try it and see what happens ;)

And apologies for being rude, but this patch really is broken.

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Andy Shevchenko]

On Tue, Sep 09, 2025 at 11:59:49AM +0200, Michal Pecio wrote:
> On Wed,  3 Sep 2025 19:01:22 +0200, Niklas Neronin wrote:
> > Switch all printing of DMA addresses to '%pad' specifier. This specifier
> > ensures that the address is printed correctly, regardless of whether the
> > kernel is running in a 32-bit or 64-bit environment.
> 
> Old %llx with (long long) cast also prints it corretly.

Not really. It prints unnecessary long values on 32-bit machines making an
impression that something works somewhere in 64-bit address space.

> I had the same idea and even implemented it in some private debugging
> patches, but I found %pad just annoying in practice.
> 
> %pad isn't guaranteed to be at least 64 bit long, so some DMAs from
> 64 bit hardware will always need to be printed with %llx or similar.

When DMA address is fixed in the HW, it should refer to it as uXX.

> Secondly, padding is not optional with %pad. Maybe not a big deal, but
> on 64 bit systems with comparatively little RAM it adds clutter.

I don't get this, can you elaborate what's the problem in using _standard_
way of printing pointers / addresses?

> Thirdly, %pad can't be passed by value. Hence pollution like:

Yes, because of the C standard: we can't extend it with our wishes in
one-click. Hence, anything else can be a compile-time warning. The point
here is not about the form, but about the content. The content here is
when a Linux DMA address needs to be printed, we gotta use %pad to make
it standard on each of the platforms, including 32- and 64-bits.

> > @@ -2654,7 +2654,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
> >  	unsigned int slot_id;
> >  	int ep_index;
> >  	struct xhci_td *td = NULL;
> > -	dma_addr_t ep_trb_dma;
> > +	dma_addr_t ep_trb_dma, deq, td_start, td_end;
> >  	struct xhci_segment *ep_seg;
> >  	union xhci_trb *ep_trb;
> >  	int status = -EINPROGRESS;
> 
> This function has plenty of variables already, not sure if it needs
> three more. We could work around it by introducing {} scopes around
> printing, or functions like print_scary_error_message(), but it ends
> up being more hassle than type casting at some point.
> 
> Maybe a small helper if the verbose casts really bother people?
> static inline unsigned long long dma2llx(dma_addr_t dma) {return dma;}
> 
> BTW, isn't unsigned unnecessary?

Sign might be unnecessary in _this_ case, but it's very important to avoid
subtle mistakes with it (we have so many bugs for both cases, when unsigned
used as should signed be and vice versa). In this case we are talking about
something that can't be negative.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Tue, 9 Sep 2025 14:29:20 +0300, Andy Shevchenko wrote:
> On Tue, Sep 09, 2025 at 11:59:49AM +0200, Michal Pecio wrote:
> > Old %llx with (long long) cast also prints it corretly.  
> 
> Not really. It prints unnecessary long values on 32-bit machines
> making an impression that something works somewhere in 64-bit
> address space.

The %016llx format you are alluding to is used in two error messages
actually seen by users, that's an issue. My crazy personal preference
would be %08llx, but I concede it's unprofessional, so %pad it seems.

But it's the exact function I have shown where three otherwise useless
dma_addr_t are introduced to get around pass-by-reference limitation.

Maybe there would be a way to limit their scope at least?

In these messages, all values are indeed known-good DMA pointers or
truncated to dma_addr_t by the time they are printed (but ep_trb_dma
is truncated silently, not ideal).


Elsewhere, HW-originated DMA pointers are handled as u64 and dynamic-
debugged as %08llx. Call it messy or sloppy, but it's automagic and
convenient - with no leading zeros, significant digits stand out more.
A nonzero top DWORD on a 32 bit system sticks out like a sore thumb.

The exact same *value* may be handled as dma_addr_t before it passes
through HW and as u64 after it comes out. It would be nice if both
copies looked the same in the log.

> > Secondly, padding is not optional with %pad. Maybe not a big deal, but
> > on 64 bit systems with comparatively little RAM it adds clutter.  
> 
> I don't get this, can you elaborate what's the problem in using _standard_
> way of printing pointers / addresses?

I simply find that leading zeros are distracting and make it harder
to visually scan for equal or similar numbers in a wall of text,
which is what dynamic debug is about.

Regards,
Michal

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Tue, 9 Sep 2025 22:44:16 +0200, Michal Pecio wrote:
> On Tue, 9 Sep 2025 14:29:20 +0300, Andy Shevchenko wrote:
> > Not really. It prints unnecessary long values on 32-bit machines
> > making an impression that something works somewhere in 64-bit
> > address space.  
> 
> The %016llx format you are alluding to is used in two error messages
> actually seen by users, that's an issue. My crazy personal preference
> would be %08llx, but I concede it's unprofessional, so %pad it seems.

Actually, I take this back.

I think that leading zeros are evil and I agree this message is bad.
But I don't understand why 64 bit users should put up with this:

[  140.106751] xhci_hcd 0000:07:00.0: Event dma 0x00000000ffeec7f0 for ep 2 status 13 not part of TD at 00000000ffeec800 - 00000000ffeec800
[  140.476573] xhci_hcd 0000:07:00.0: Event dma 0x00000000ffeec1a0 for ep 2 status 13 not part of TD at 00000000ffeec1b0 - 00000000ffeec1b0
[  140.502855] xhci_hcd 0000:07:00.0: Event dma 0x00000000ffeecd60 for ep 2 status 13 not part of TD at 00000000ffeecd70 - 00000000ffeecd70
[  141.225300] xhci_hcd 0000:07:00.0: Event dma 0x00000000ffeeb970 for ep 2 status 13 not part of TD at 00000000ffeeb980 - 00000000ffeeb980

when we can have this:

[  419.967755] xhci_hcd 0000:07:00.0: Event dma 0xffc34760 for ep 2 status 13 not part of TD at 0xffc34770 - 0xffc34770
[  420.100611] xhci_hcd 0000:07:00.0: Event dma 0xffc34bc0 for ep 2 status 13 not part of TD at 0xffc34bd0 - 0xffc34bd0
[  420.360917] xhci_hcd 0000:07:00.0: Event dma 0xffc34e70 for ep 2 status 13 not part of TD at 0xffc34e80 - 0xffc34e80
[  421.719530] xhci_hcd 0000:07:00.0: Event dma 0xffc35770 for ep 2 status 13 not part of TD at 0xffc35780 - 0xffc35780

with a simple change (anything wrong with u64 cast here?):

-       xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n",
-                &ep_trb_dma, ep_index, trb_comp_code,
+       xhci_err(xhci, "Event dma %#08llx for ep %d status %d not part of TD at %#08llx - %#08llx\n",
+                (u64) ep_trb_dma, ep_index, trb_comp_code,

These zeros only add noise, and in many cases make difference between
line wrapping or not because this is longer than 99% of kernel messages
and some people want their terminal window not to take the whole screen.

The main thing we care about here are the last 3-4 digits and we could
have made it little more than (ep_trb_dma & 0xffff) long ago, but then
Niklas asked "what about correlation with tracing/debugfs/dyndbg?", so
it was left the way it is.

Regards,
Michal

Re: [PATCH 1/7] usb: xhci-dbgcap: correct DMA address handling

[Neronin, Niklas]

On 09/09/2025 13.13, Michal Pecio wrote:
> On Wed,  3 Sep 2025 19:01:21 +0200, Niklas Neronin wrote:
>> Address the handling of addresses within the dbc_handle_xfer_event(),
>> ensuring accurate extraction and comparison.
>>
>> Variable details:
>> 'r->trb_dma'		    : A DMA address.
>> 'ep_ctx->deq'		    : A le64, bits 63:4 contain the DMA address.
>> 'event->trans_event.buffer' : A le64, bits 63:0 contain the DMA address.
>>
>> 1. Convert 'ep_ctx->deq' and 'event->trans_event.buffer' addresses to
>>    the CPU's native byte order.
>>
>> 2. Use mask; TR_DEQ_PTR_MASK (bits 63:4) to extract the address from
>>    'ep_ctx->deq', replacing the previous use of ~TRB_CYCLE (bits 63:1).
>>
>> Why not use 'dma_addr_t' for the address?
>> The 'dma_addr_t' type can vary between 32 and 64 bits depending on the
>> system architecture or xHCI driver flags, whereas the 64-bit address field
>> size remains constant. Since hardware cannot be fully trusted, it's better
>> to print the entire 64-bit address to detect any non-zero values in the
>> upper 32 bits. This approach ensures that potential issues are easily
>> detected.
>>
>> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
>> ---
>>  drivers/usb/host/xhci-dbgcap.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
>> index 06a2edb9e86e..2f24a3a54439 100644
>> --- a/drivers/usb/host/xhci-dbgcap.c
>> +++ b/drivers/usb/host/xhci-dbgcap.c
>> @@ -724,6 +724,7 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>>  	u32			comp_code;
>>  	size_t			remain_length;
>>  	struct dbc_request	*req = NULL, *r;
>> +	u64			ep_trb, deq;
>>  
>>  	comp_code	= GET_COMP_CODE(le32_to_cpu(event->generic.field[2]));
>>  	remain_length	= EVENT_TRB_LEN(le32_to_cpu(event->generic.field[2]));
>> @@ -733,10 +734,11 @@ static void dbc_handle_xfer_event(struct xhci_dbc *dbc, union xhci_trb *event)
>>  	ep_ctx		= (ep_id == EPID_OUT) ?
>>  				dbc_bulkout_ctx(dbc) : dbc_bulkin_ctx(dbc);
>>  	ring		= dep->ring;
>> +	ep_trb		= le64_to_cpu(event->trans_event.buffer);
> 
> In other places this variable would be called ep_trb_dma
> and ep_trb would be xhci_trb*.

The variable is named 'ep_trb' because it represents a u64 value, not a DMA address.

The choice to avoid using a 'dma_addr_t' variable for the 64-bit value is intentional,
as it can prevent the loss of the upper 32 bits.

> 
>>  
>>  	/* Match the pending request: */
>>  	list_for_each_entry(r, &dep->list_pending, list_pending) {
>> -		if (r->trb_dma == event->trans_event.buffer) {
>> +		if (r->trb_dma == ep_trb) {
> 
> And here it is being compared with a something_dma variable.

dma_addr_t trb_dma	- is a DMA address
u64        ep_trb	- is a 64-bit value, which, if correct, represents a DMA address.

Best Regards,
Niklas

Re: [PATCH 1/7] usb: xhci-dbgcap: correct DMA address handling

[Michal Pecio]

On Wed, 10 Sep 2025 11:00:06 +0300, Neronin, Niklas wrote:
> dma_addr_t trb_dma	- is a DMA address
> u64        ep_trb	- is a 64-bit value, which, if correct, represents a DMA address.

Well, I guess that's true, but the inconsistency with the rest of the
driver looks odd.

And it is still a DMA space address, if it's any valid address at all.

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Wed,  3 Sep 2025 19:01:22 +0200, Niklas Neronin wrote:
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index d2ccf19292aa..7d57e2a3abf3 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -781,7 +781,7 @@ static int xhci_move_dequeue_past_td(struct xhci_hcd *xhci,
>  	ep->queued_deq_ptr = new_deq;
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> -		       "Set TR Deq ptr 0x%llx, cycle %u\n", addr, new_cycle);
> +		       "Set TR Deq ptr %pad, cycle %u\n", &addr, new_cycle);

This terminating newline shouldn't be here:

[13035.022589] xhci_hcd 0000:00:10.0: Stopped on Transfer TRB for slot 1 ep 2
[13035.022648] xhci_hcd 0000:00:10.0: Removing canceled TD starting at @0x00000000fffae050 (dma) in stream 0 URB ffff88813358a3c0
[13035.022657] xhci_hcd 0000:00:10.0: Set TR Deq ptr @0x00000000fffae060, cycle 1

[13035.022662] xhci_hcd 0000:00:10.0: // Ding dong!
[13035.022666] xhci_hcd 0000:00:10.0: xhci_giveback_invalidated_tds: Keep cancelled URB ffff88813358a3c0 TD as cancel_status is 2
[13035.022835] xhci_hcd 0000:00:10.0: Successful Set TR Deq Ptr cmd, deq = 0xfffae060

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Wed, 10 Sep 2025 11:04:48 +0200, Michal Pecio wrote:
> [13035.022589] xhci_hcd 0000:00:10.0: Stopped on Transfer TRB for slot 1 ep 2
> [13035.022648] xhci_hcd 0000:00:10.0: Removing canceled TD starting at @0x00000000fffae050 (dma) in stream 0 URB ffff88813358a3c0
> [13035.022657] xhci_hcd 0000:00:10.0: Set TR Deq ptr @0x00000000fffae060, cycle 1
> 
> [13035.022662] xhci_hcd 0000:00:10.0: // Ding dong!
> [13035.022666] xhci_hcd 0000:00:10.0: xhci_giveback_invalidated_tds: Keep cancelled URB ffff88813358a3c0 TD as cancel_status is 2
> [13035.022835] xhci_hcd 0000:00:10.0: Successful Set TR Deq Ptr cmd, deq = 0xfffae060

This also illustrates how this series degrades usability by printing
the same DMA twice in different manner:

@0x00000000fffae060	<-- zero padding and @ (?)
         0xfffae060	<-- bare number

It's the exact same number. And this is what everyone looking at this
log wants to know - is the number the same, or is it different?

It doesn't matter what type of variable is storing this number in given
moment. Source code provides this information much more reliably than
type annotations in debug messages which people will break over time.

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Andy Shevchenko]

On Wed, Sep 10, 2025 at 07:56:30AM +0200, Michal Pecio wrote:
> On Tue, 9 Sep 2025 22:44:16 +0200, Michal Pecio wrote:
> > On Tue, 9 Sep 2025 14:29:20 +0300, Andy Shevchenko wrote:
> > > Not really. It prints unnecessary long values on 32-bit machines
> > > making an impression that something works somewhere in 64-bit
> > > address space.  
> > 
> > The %016llx format you are alluding to is used in two error messages
> > actually seen by users, that's an issue. My crazy personal preference
> > would be %08llx, but I concede it's unprofessional, so %pad it seems.
> 
> Actually, I take this back.
> 
> I think that leading zeros are evil and I agree this message is bad.
> But I don't understand why 64 bit users should put up with this:
> 
> [  140.106751] xhci_hcd 0000:07:00.0: Event dma 0x00000000ffeec7f0 for ep 2 status 13 not part of TD at 00000000ffeec800 - 00000000ffeec800
> [  140.476573] xhci_hcd 0000:07:00.0: Event dma 0x00000000ffeec1a0 for ep 2 status 13 not part of TD at 00000000ffeec1b0 - 00000000ffeec1b0
> [  140.502855] xhci_hcd 0000:07:00.0: Event dma 0x00000000ffeecd60 for ep 2 status 13 not part of TD at 00000000ffeecd70 - 00000000ffeecd70
> [  141.225300] xhci_hcd 0000:07:00.0: Event dma 0x00000000ffeeb970 for ep 2 status 13 not part of TD at 00000000ffeeb980 - 00000000ffeeb980
> 
> when we can have this:
> 
> [  419.967755] xhci_hcd 0000:07:00.0: Event dma 0xffc34760 for ep 2 status 13 not part of TD at 0xffc34770 - 0xffc34770
> [  420.100611] xhci_hcd 0000:07:00.0: Event dma 0xffc34bc0 for ep 2 status 13 not part of TD at 0xffc34bd0 - 0xffc34bd0
> [  420.360917] xhci_hcd 0000:07:00.0: Event dma 0xffc34e70 for ep 2 status 13 not part of TD at 0xffc34e80 - 0xffc34e80
> [  421.719530] xhci_hcd 0000:07:00.0: Event dma 0xffc35770 for ep 2 status 13 not part of TD at 0xffc35780 - 0xffc35780
> 
> with a simple change (anything wrong with u64 cast here?):
> 
> -       xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n",
> -                &ep_trb_dma, ep_index, trb_comp_code,
> +       xhci_err(xhci, "Event dma %#08llx for ep %d status %d not part of TD at %#08llx - %#08llx\n",

How is 0 will be printed with %#08x?

> +                (u64) ep_trb_dma, ep_index, trb_comp_code,
> 
> These zeros only add noise, and in many cases make difference between
> line wrapping or not because this is longer than 99% of kernel messages
> and some people want their terminal window not to take the whole screen.

I disagree on this. The 64-bit platforms are 64-bit. If the address in use is
_capable_ of 64-bit, it should be printed as 64-bit. Otherwise make it u32 in
the code and then I will agree with you.

> The main thing we care about here are the last 3-4 digits and we could
> have made it little more than (ep_trb_dma & 0xffff) long ago, but then
> Niklas asked "what about correlation with tracing/debugfs/dyndbg?", so
> it was left the way it is.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Thu, 11 Sep 2025 10:41:49 +0300, Andy Shevchenko wrote:
> > -       xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n",
> > -                &ep_trb_dma, ep_index, trb_comp_code,
> > +       xhci_err(xhci, "Event dma %#08llx for ep %d status %d not part of TD at %#08llx - %#08llx\n",  
> 
> How is 0 will be printed with %#08x?

Oops, thanks, this won't work indeed.

> > +                (u64) ep_trb_dma, ep_index, trb_comp_code,
> > 
> > These zeros only add noise, and in many cases make difference between
> > line wrapping or not because this is longer than 99% of kernel messages
> > and some people want their terminal window not to take the whole screen.  
> 
> I disagree on this. The 64-bit platforms are 64-bit. If the address in use is
> _capable_ of 64-bit, it should be printed as 64-bit. Otherwise make it u32 in
> the code and then I will agree with you.

Maybe some people unfamiliar with this driver would want to know the
width of those fields for some reason without needing to grep the code
(thuogh off the top of my head I don't know who and why).

But when I see this line, I mainly want to know if the 1st pointer is
less than the 2nd or more than the 3rd. Padding only spreads them apart.

I can see how padding beyond actual variable size (as in example above)
can be dangereous, because people might see it and not even think about
verifying if the code isn't truncating something. The opposite should
be less problematic.


As for the %08llx format widespread in dynamic debug, I think it was
used in the past because it does approximately the right thing on both
types of systems and it's the only format capable of giving consistent
result on both dma_addr_t and u64, used for some DMA pointers too.

If dma_addr_t really *must* be padded, then I guess it would only make
sense to also convert those u64 %08llx to %016llx. But I see later in
this series some reductions to %llx, which decision I find puzzling.

Regards,
Michal

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Andy Shevchenko]

On Thu, Sep 11, 2025 at 11:34:51AM +0200, Michal Pecio wrote:
> On Thu, 11 Sep 2025 10:41:49 +0300, Andy Shevchenko wrote:

...

> > > -       xhci_err(xhci, "Event dma %pad for ep %d status %d not part of TD at %016llx - %016llx\n",
> > > -                &ep_trb_dma, ep_index, trb_comp_code,
> > > +       xhci_err(xhci, "Event dma %#08llx for ep %d status %d not part of TD at %#08llx - %#08llx\n",  
> > 
> > How is 0 will be printed with %#08x?
> 
> Oops, thanks, this won't work indeed.
> 
> > > +                (u64) ep_trb_dma, ep_index, trb_comp_code,
> > > 
> > > These zeros only add noise, and in many cases make difference between
> > > line wrapping or not because this is longer than 99% of kernel messages
> > > and some people want their terminal window not to take the whole screen.  
> > 
> > I disagree on this. The 64-bit platforms are 64-bit. If the address in use is
> > _capable_ of 64-bit, it should be printed as 64-bit. Otherwise make it u32 in
> > the code and then I will agree with you.
> 
> Maybe some people unfamiliar with this driver would want to know the
> width of those fields for some reason without needing to grep the code
> (thuogh off the top of my head I don't know who and why).
> 
> But when I see this line, I mainly want to know if the 1st pointer is
> less than the 2nd or more than the 3rd. Padding only spreads them apart.
> 
> I can see how padding beyond actual variable size (as in example above)
> can be dangereous, because people might see it and not even think about
> verifying if the code isn't truncating something. The opposite should
> be less problematic.
> 
> As for the %08llx format widespread in dynamic debug, I think it was
> used in the past because it does approximately the right thing on both
> types of systems and it's the only format capable of giving consistent
> result on both dma_addr_t and u64, used for some DMA pointers too.

The problem with it is that it can't give the proper result for the ranges that
span over the 4G. Which I consider a bad thing. So, the correct use is to stick
with HW register size and do appropriate specifier as it was a pointer.

> If dma_addr_t really *must* be padded, then I guess it would only make
> sense to also convert those u64 %08llx to %016llx.

Yes. And this is the case on 64-bit platforms with device and/or
main memory being resided above 4G independently if we use bounce buffers
(DMA mask < address space where device or memory to transfer data is)
or not.

> But I see later in this series some reductions to %llx, which decision I find
> puzzling.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Thu, 11 Sep 2025 23:13:54 +0300, Andy Shevchenko wrote:
> On Thu, Sep 11, 2025 at 11:34:51AM +0200, Michal Pecio wrote:
> > As for the %08llx format widespread in dynamic debug, I think it was
> > used in the past because it does approximately the right thing on both
> > types of systems and it's the only format capable of giving consistent
> > result on both dma_addr_t and u64, used for some DMA pointers too.  
> 
> The problem with it is that it can't give the proper result for the ranges that
> span over the 4G. Which I consider a bad thing. So, the correct use is to stick
> with HW register size and do appropriate specifier as it was a pointer.

I see no reason to bother padding pointers to full variable width and
when I run 'dmesg' on my 64 bit machine I see that most of the kernel
doesn't really bother either, so xhci isn't any outlier.

(Plus: why should we stop at pointers? Integers too have a width.)

It amounts to embedding static type information in logs. Maybe there
are cases where it could be helpful for people reading the log, maybe
there aren't, but this patch doesn't even attempt to make such case,
it just talks vaguely about "correctness".

I only see one truly incorrect case fixed here, a missing (u64) cast
for %llx format, which I presume will print garbage on 32 bits.


This brings up another problem with %pad: it is unknown to compilers
so they don't type check it. I can make the above bug a build error:

drivers/usb/host/xhci-ring.c: In function ‘xhci_move_dequeue_past_td’:
drivers/usb/host/xhci-ring.c:784:45: error: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘dma_addr_t’ {aka ‘unsigned int’} [-Werror=format=]
  784 |                        "Set TR Deq ptr 0x%llx, cycle %u", addr, new_cycle);
      |                                          ~~~^             ~~~~
      |                                             |             |
      |                                             |             dma_addr_t {aka unsigned int}
      |                                             long long unsigned int
      |                                          %x


but if I switch to %pad and later change my mind and extend 'addr' to
u64 without updating this format, the compiler will eat it up and once
again garbage will be printed on some systems.

Regards,
Michal

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Andy Shevchenko]

On Fri, Sep 12, 2025 at 11:46:44AM +0200, Michal Pecio wrote:
> On Thu, 11 Sep 2025 23:13:54 +0300, Andy Shevchenko wrote:
> > On Thu, Sep 11, 2025 at 11:34:51AM +0200, Michal Pecio wrote:
> > > As for the %08llx format widespread in dynamic debug, I think it was
> > > used in the past because it does approximately the right thing on both
> > > types of systems and it's the only format capable of giving consistent
> > > result on both dma_addr_t and u64, used for some DMA pointers too.  
> > 
> > The problem with it is that it can't give the proper result for the ranges that
> > span over the 4G. Which I consider a bad thing. So, the correct use is to stick
> > with HW register size and do appropriate specifier as it was a pointer.
> 
> I see no reason to bother padding pointers to full variable width and
> when I run 'dmesg' on my 64 bit machine I see that most of the kernel
> doesn't really bother either, so xhci isn't any outlier.

Can you point out to some examples? I see that pointers printed in many cases
normally as 64-bit values.

> (Plus: why should we stop at pointers? Integers too have a width.)

This is not an argument at all. Pointers are special, they are not _just_
integers. Coincidentally I read an article about pointer in C
https://stefansf.de/post/pointers-are-more-abstract-than-you-might-expect/

> It amounts to embedding static type information in logs. Maybe there
> are cases where it could be helpful for people reading the log, maybe
> there aren't, but this patch doesn't even attempt to make such case,
> it just talks vaguely about "correctness".

The correctness here is in information that is printed. Again, imagine
the loop that goes above 4G on a 64-bit machine. Out of a sudden %08llx will be
expanded to cover the 64-bit addresses and becomes one digit at a time creating
a ladder (ugly looking) output. This is incorrect.

> I only see one truly incorrect case fixed here, a missing (u64) cast
> for %llx format, which I presume will print garbage on 32 bits.

No, it's other way around, we should not put explicit casts in printf() in C
as there are plenty of the format specifiers that allows us to be sure that
the printed value is correct independently on architecture, endianess, etc.

> This brings up another problem with %pad: it is unknown to compilers
> so they don't type check it. 
> 
> but if I switch to %pad and later change my mind and extend 'addr' to
> u64 without updating this format, the compiler will eat it up and once
> again garbage will be printed on some systems.

This topic was risen a few times in the past. Somebody proposed to have a GCC
plugin with that, somebody else proposed to completely rewrite the %p
extensions to be more like Pyhon or C++ ones (when you just specify argument
and handler for it). None so far is implemented AFAIK. But this is not
particular problem of %pad, it's for all %p extensions. And the extensions
exist for a purpose. What you are proposing here behind the lines is to kill
that completely. I believe this is not the right direction to go. So, TL;DR:
one should be careful about %p extensions, but at the same time open coding
them is not a good idea either.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Fri, 12 Sep 2025 21:02:23 +0300, Andy Shevchenko wrote:
> Again, imagine the loop that goes above 4G on a 64-bit machine. Out
> of a sudden %08llx will be expanded to cover the 64-bit addresses and
> becomes one digit at a time creating a ladder (ugly looking) output.
> This is incorrect.

If I equated correctness with ugliness I would equally confidently
state that unnecessary zero-padding is incorrect. But I don't.
This is exactly the absurd argument I previously made for padding all
%x and %d formats to full width. Pad tables, not lone log messages.

> No, it's other way around, we should not put explicit casts in printf() in C
> as there are plenty of the format specifiers that allows us to be sure that
> the printed value is correct independently on architecture, endianess, etc.

At least if you do it, the compiler will also do the right thing:
- if the cast doesn't match the format, warn (xhci needs a patch here)
- if it matches, widen the provided value as necessary

And it works consistently regardless of whether the variable is a
dma_addr_t or u64, on all architectures, with or without PAE.

Reminder: this drivers handles DMAs as u64 too, so it will *never*
print all DMAs as %pad. And if it tries, it will be a silent bug.

> > This brings up another problem with %pad: it is unknown to compilers
> > so they don't type check it. 
> > 
> > but if I switch to %pad and later change my mind and extend 'addr' to
> > u64 without updating this format, the compiler will eat it up and once
> > again garbage will be printed on some systems.  
> 
> This topic was risen a few times in the past. Somebody proposed to have a GCC
> plugin with that, somebody else proposed to completely rewrite the %p
> extensions to be more like Pyhon or C++ ones (when you just specify argument
> and handler for it). None so far is implemented AFAIK.

Indeed, not holding my breath for the %pad situation improving.
This includes impossibility of passing it by value - it's forced
by compilers being unaware of %pad and following usual %p rules.

> But this is not particular problem of %pad, it's for all %p
> extensions. And the extensions exist for a purpose. What you are
> proposing here behind the lines is to kill that completely.

Surely people would laugh me off if I actually suggested that.
If those formats work for their cases, use them.

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Andy Shevchenko]

On Sat, Sep 13, 2025 at 10:12:46AM +0200, Michal Pecio wrote:
> On Fri, 12 Sep 2025 21:02:23 +0300, Andy Shevchenko wrote:
> > Again, imagine the loop that goes above 4G on a 64-bit machine. Out
> > of a sudden %08llx will be expanded to cover the 64-bit addresses and
> > becomes one digit at a time creating a ladder (ugly looking) output.
> > This is incorrect.
> 
> If I equated correctness with ugliness I would equally confidently

Ugliness?! No, the full information printed is way too better than
whatever pieces of it are (even if it's correct in some cases).
We are doing 64-bit hardware, we have to deal with 64-bit pointers.
Period. Shortcutting and premature optimizing is always a slippery slope
when we enter to the debugging field.

> state that unnecessary zero-padding is incorrect. But I don't.

Right, because it won't be incorrect, there is no such thing as unnecessary
zero-padding for the pointers.

> This is exactly the absurd argument I previously made for padding all
> %x and %d formats to full width. Pad tables, not lone log messages.

> > No, it's other way around, we should not put explicit casts in printf() in C
> > as there are plenty of the format specifiers that allows us to be sure that
> > the printed value is correct independently on architecture, endianess, etc.
> 
> At least if you do it, the compiler will also do the right thing:
> - if the cast doesn't match the format, warn (xhci needs a patch here)

Of course this doesn't work properly on the types that are less than int. So,
this is fragile argument to support explicit castings.

> - if it matches, widen the provided value as necessary
> 
> And it works consistently regardless of whether the variable is a
> dma_addr_t or u64, on all architectures, with or without PAE.

Yes, with "unnecessary" zero-paddings (that case when they are not needed).

> Reminder: this drivers handles DMAs as u64 too, so it will *never*
> print all DMAs as %pad. And if it tries, it will be a silent bug.

Yes, and the problem here is not in the printf() specifiers, the problem is
in the (used) data types. For the printf() it's crystal clear, if the type of
the variable is dma_addr_t, we have %pad. No need to reinvent the wheel.

> > > This brings up another problem with %pad: it is unknown to compilers
> > > so they don't type check it. 
> > > 
> > > but if I switch to %pad and later change my mind and extend 'addr' to
> > > u64 without updating this format, the compiler will eat it up and once
> > > again garbage will be printed on some systems.  
> > 
> > This topic was risen a few times in the past. Somebody proposed to have a GCC
> > plugin with that, somebody else proposed to completely rewrite the %p
> > extensions to be more like Pyhon or C++ ones (when you just specify argument
> > and handler for it). None so far is implemented AFAIK.
> 
> Indeed, not holding my breath for the %pad situation improving.
> This includes impossibility of passing it by value - it's forced
> by compilers being unaware of %pad and following usual %p rules.
> 
> > But this is not particular problem of %pad, it's for all %p
> > extensions. And the extensions exist for a purpose. What you are
> > proposing here behind the lines is to kill that completely.
> 
> Surely people would laugh me off if I actually suggested that.
> If those formats work for their cases, use them.

Sure, there are thousands of the cases in the kernel when we print our custom
data types in a better format and this works and will continue working
independently on the legacy of the compilers or C standard modifications.

Removing them is like suggesting Americans to drop automatic gear in favour of
the manual to the promoting eco-transportation (horses + carts). I don't believe
this is valuable proposal.

P.S.
I'm sorry, but I lost the objective of this discussion. Can you summarize,
please, what's wrong with the patch?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Mon, 15 Sep 2025 10:20:33 +0300, Andy Shevchenko wrote:
> > > No, it's other way around, we should not put explicit casts in
> > > printf() in C as there are plenty of the format specifiers that
> > > allows us to be sure that the printed value is correct
> > > independently on architecture, endianess, etc.  
> > 
> > At least if you do it, the compiler will also do the right thing:
> > - if the cast doesn't match the format, warn (xhci needs a patch)
> 
> Of course this doesn't work properly on the types that are less than
> int. So, this is fragile argument to support explicit castings.

The issue is how to print u64 and dma_addr_t, and the suggestion is
to stay with ("%08llx", (u64)addr) for both. What should go wrong?

1. 'addr' is transparently widened if necessary
2. if 'addr' type changes later, nothing happens
3. missing cast is a build error on common platforms (needs patch)
4. wrong format (%lx, %d, %pad, %p) is a build error

With %pad used for dma_addr_t:

1. different formats must be written manually
2a. u64 to dma_addr_t: manual edit
2b. dma_addr_t to u64: manual edit or it's a silent bug, invisible
    to compilers, invisible on 64 bit platforms used by developers

That's for type safety. And further:

5. rvalues work without proliferation of temp variables
6. same number looks same, whether stored as u64 or dma_addr_t
7. consistency with the rest of the kernel

Seriously, *lots* of drivers and even the PCI subsystem itself print
addresses unpadded, using %llx or similar formats. The numbers have
8 digits on a PC (even 64 bit) and grow to 12 or more elsewhere.

It's first time I see somebody who appears really bothered by this.

> > Reminder: this drivers handles DMAs as u64 too, so it will *never*
> > print all DMAs as %pad. And if it tries, it will be a silent bug.  
> 
> Yes, and the problem here is not in the printf() specifiers, the
> problem is in the (used) data types.

And what else can be done? The driver uses dma_addr_t where applicable
for efficiency on 32 bits, the HW uses 64 bits like 'buffer' below:

struct xhci_transfer_event {
        /* 64-bit buffer address, or immediate data */
        __le64  buffer;
        __le32  transfer_len;
        /* This field is interpreted differently based on the type of TRB */
        __le32  flags;     
};

Same address may be logged at various stages of the flow where it
exists in variabes of different type. The number matters, not type.

Regards,
Michal

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Neronin, Niklas]

On 15/09/2025 13.22, Michal Pecio wrote:
> 
> The issue is how to print u64 and dma_addr_t, and the suggestion is
> to stay with ("%08llx", (u64)addr) for both. What should go wrong?

I agree that printing long 64-bit hex values is annoying. However,
"%08llx" is not the common format for printing DMA addresses,
and having inconsistencies is much more annoying.

Additionally, you made a good point in another review:
 "And what if the kernel starts hashing %pad by default?"
Now, all non-pad DMA addresses are not hashed. :/

I would agree on another format if it is consistently used
throughout the xHCI driver. I chose this format because it's the
most common format.
> 1. 'addr' is transparently widened if necessary
> 2. if 'addr' type changes later, nothing happens
> 3. missing cast is a build error on common platforms (needs patch)
> 4. wrong format (%lx, %d, %pad, %p) is a build error
> 
> With %pad used for dma_addr_t:
> 
> 1. different formats must be written manually
> 2a. u64 to dma_addr_t: manual edit
> 2b. dma_addr_t to u64: manual edit or it's a silent bug, invisible
>     to compilers, invisible on 64 bit platforms used by developers

Why is this conversion necessary when %pad is used for 'dma_addr_t'?
AFAIK, aside from printing, the xHCI driver does not convert
'dma_addr_t' to 'u64'.
> That's for type safety. And further:
> 5. rvalues work without proliferation of temp variables
> 6. same number looks same, whether stored as u64 or dma_addr_t
> 7. consistency with the rest of the kernel

How did you verify this?

> git grep -e '0x%08llx' -e '%#08llx' -e '%08llx' | wc -l
310

> git grep '%pad' | wc -l
446

> 
> Seriously, *lots* of drivers and even the PCI subsystem itself print
> addresses unpadded, using %llx or similar formats. The numbers have
> 8 digits on a PC (even 64 bit) and grow to 12 or more elsewhere.
> 
> It's first time I see somebody who appears really bothered by this.

Personally, when I see "0x7b271bb9ec," I think "hex value," but when I
see "0x0000007b271bb9ec," I think "address." This is because that is
how I have usually seen addresses represented. Otherwise I do prefer
the shorter format.

> 
>>> Reminder: this drivers handles DMAs as u64 too, so it will *never*
>>> print all DMAs as %pad. And if it tries, it will be a silent bug.  
>>
>> Yes, and the problem here is not in the printf() specifiers, the
>> problem is in the (used) data types.
> 
> And what else can be done? The driver uses dma_addr_t where applicable
> for efficiency on 32 bits, the HW uses 64 bits like 'buffer' below:
> 
> struct xhci_transfer_event {
>         /* 64-bit buffer address, or immediate data */
>         __le64  buffer;
>         __le32  transfer_len;
>         /* This field is interpreted differently based on the type of TRB */
>         __le32  flags;     
> };
> 
> Same address may be logged at various stages of the flow where it
> exists in variabes of different type. The number matters, not type.

I was working on implementing a helper function that would extract the DMA
address and validate it so that it can be returned as a 'dma_addr_t'.
This was supposed to be step 2, following this patch series.

Best Regard,
Niklas

Re: [PATCH 4/7] usb: xhci: improve Endpoint Context register debugging

[Neronin, Niklas]

On 09/09/2025 12.20, Michal Pecio wrote:
>>  drivers/usb/host/xhci.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 59ff84ba2d4a..2662356d048e 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -2580,9 +2580,9 @@ static inline const char *xhci_decode_ep_context(char *str, u32 info,
>>  	ret += sprintf(str + ret, "interval %d us max ESIT payload %d CErr %d ",
>>  			(1 << interval) * 125, esit, cerr);
>>  
>> -	ret += sprintf(str + ret, "Type %s %sburst %d maxp %d deq %016llx ",
>> +	ret += sprintf(str + ret, "Type %s %sburst %d maxp %d deq %016llx cycle %llu",
>>  			xhci_ep_type_string(ep_type), hid ? "HID" : "",
>> -			burst, maxp, deq);
>> +			burst, maxp, deq & TR_DEQ_PTR_MASK, deq & EP_CTX_CYCLE_MASK);
> 
> Does it really bother people who use debugfs that deq includes the
> cycle bit, which is exactly what anyone who know xHCI will expect?

Not only because of the cycle bit, but also for the other bits. All 64-bit
register are not exactly the same and clearly communicating what each value
represents is always preferable IMO. As demonstrated in the examples below,
bits 3:1 differ from one another.

Stream Context register, section 6.2.4.1.
 bit 0 - Dequeue Cycle State.
 bits 3:1 - Stream Context Type.
 bits 63:4 - TR Dequeue Pointer, is 16-byte aligned.

Endpoint Context register, section 6.2.3.
 bit 0 - Dequeue Cycle State.
 bits 3:1 - RsvdZ.
 bits 63:4 - TR Dequeue Pointer, is 16-byte aligned.

> This line is quite damn long already.

Is this really a problem for debugfs?
Should debugfs not be more verbose than debug messages? 
> Also, I am highly confident that you haven't even tested this patch.
> Try it and see what happens ;)

Nice catch, I'll add the missing space in v2. Thanks!

Best Regards,
Niklas 

Re: [PATCH 7/7] usb: xhci: standardize address format

[Neronin, Niklas]

On 09/09/2025 12.06, Michal Pecio wrote:
> On Wed,  3 Sep 2025 19:01:27 +0200, Niklas Neronin wrote:
>> In the xHCI driver, printed hexadecimal values can be ambiguous, making it
>> difficult to distinguish between values and addresses. To enhance clarity,
>> all DMA addresses are now prefixed with the '@' symbol, ensuring immediate
>> recognition as addresses.
> 
> Is it really a problem? Which values look like addresses?

Examples from before my patch series:
"Setting command ring address to 0x%llx" - Not an address.
"Already resolving halted ep for 0x%llx" - DMA address
"stream %u ctx @%pad: SCT %llu deq %llx" - deq is not and address
"Slot ID %d dcbaa entry @%p = %#016llx - Both addresses

I'll drop the "@" prefix from DMA and virtual addresses, as it's no longer
necessary now that addresses read from hardware are printed separately.
Originally, the prefix was used to distinguish between register hex dumps
and pure addresses.

> And what if the kernel starts hashing %pad by default? ;)

Good point. But this also means that 'dma_addr_t' should be printed using %pad :)
>> Exceptions to the '@' prefix rule are functions xhci_ring_enqueue_show()
>> and xhci_ring_dequeue_show(), which exclusively print to the enqueue and
>> dequeue debugfs files, containing only addresses.
> 
> So debugfs will also get @ now, except for two files apparently?
> 
> Why are those files left out and inconsistent with the rest? If the
> answer is "because the @ prefix is annoying and breaks tools" then the
> same answer applies to every other debugfs file ;)

The prefix was originally intended to indicate that a hex value represents an
address. However, if a user opens a file containing only a address, the prefix
becomes redundant, as you said "anyone going there already knows what those
fields are".
>> Adding padding is unnecessary and provides no useful information.
> 
> Sounds like an argument against converting to %pad in other places?

The value is not an actual address but a 'u64' that represents an address,
so it doesn't require padding. All 'dma_addr_t' values, which are actual
addresses, include padding.

>> $ git grep -n '0x%' | wc -l
>> 39796
>> $ git grep -n '%#' | wc -l
>> 5204
> 
> Not sure what is this doing in a commit message?

Justifying "0x" prefix instead of '#' flag. The "0x" prefix is more commonly
used in the Linux kernel compared to the '#' flag.

Even with "git grep -e '%#' -e '%08#' -e '%016#'" it is only 5207.

Best Regards,
Niklas

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Andy Shevchenko]

On Mon, Sep 15, 2025 at 12:22:51PM +0200, Michal Pecio wrote:
> On Mon, 15 Sep 2025 10:20:33 +0300, Andy Shevchenko wrote:
> > > > No, it's other way around, we should not put explicit casts in
> > > > printf() in C as there are plenty of the format specifiers that
> > > > allows us to be sure that the printed value is correct
> > > > independently on architecture, endianess, etc.  
> > > 
> > > At least if you do it, the compiler will also do the right thing:
> > > - if the cast doesn't match the format, warn (xhci needs a patch)
> > 
> > Of course this doesn't work properly on the types that are less than
> > int. So, this is fragile argument to support explicit castings.
> 
> The issue is how to print u64 and dma_addr_t, and the suggestion is
> to stay with ("%08llx", (u64)addr) for both. What should go wrong?

I already pointed out and we are going circles here.
The %08llx is wrong when printing addresses in the range that
starts < 4G and goes > 4G. The printing *will* be incosistent.
This what is wrong.

> 1. 'addr' is transparently widened if necessary
> 2. if 'addr' type changes later, nothing happens
> 3. missing cast is a build error on common platforms (needs patch)
> 4. wrong format (%lx, %d, %pad, %p) is a build error
> 
> With %pad used for dma_addr_t:
> 
> 1. different formats must be written manually
> 2a. u64 to dma_addr_t: manual edit
> 2b. dma_addr_t to u64: manual edit or it's a silent bug, invisible
>     to compilers, invisible on 64 bit platforms used by developers

Again, if you need a u64 address to be passed to the HW, mark it there as u64.
But, if the DMA address is masked to smaller amount of bits, the way it's
printed or used kernel is the same as use unsigned int value for unsigned long
container, i.e. we don't do manual castings to and from as long.

> That's for type safety. And further:
> 
> 5. rvalues work without proliferation of temp variables
> 6. same number looks same, whether stored as u64 or dma_addr_t
> 7. consistency with the rest of the kernel
> 
> Seriously, *lots* of drivers and even the PCI subsystem itself print
> addresses unpadded, using %llx or similar formats. The numbers have
> 8 digits on a PC (even 64 bit) and grow to 12 or more elsewhere.

Why do you think that that subsystem is a good example? Maybe it needs to be
fixed for the sake of consistency?

> It's first time I see somebody who appears really bothered by this.

Really?! I think it's opponents of this patch started to be bothered with
the consistency fix :-)

> > > Reminder: this drivers handles DMAs as u64 too, so it will *never*
> > > print all DMAs as %pad. And if it tries, it will be a silent bug.  
> > 
> > Yes, and the problem here is not in the printf() specifiers, the
> > problem is in the (used) data types.
> 
> And what else can be done? The driver uses dma_addr_t where applicable
> for efficiency on 32 bits, the HW uses 64 bits like 'buffer' below:
> 
> struct xhci_transfer_event {
>         /* 64-bit buffer address, or immediate data */
>         __le64  buffer;
>         __le32  transfer_len;
>         /* This field is interpreted differently based on the type of TRB */
>         __le32  flags;     
> };
> 
> Same address may be logged at various stages of the flow where it
> exists in variabes of different type. The number matters, not type.

So, what's the problem to use dma_addr_t in kernel as an abstraction of DMA
addresses?

I'm sorry, but I feel this discussion goes nowhere. I'm going to stop answering
here as I see no good counterargument against this patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Mon, 15 Sep 2025 15:32:32 +0300, Neronin, Niklas wrote:
> On 15/09/2025 13.22, Michal Pecio wrote:
> > 
> > The issue is how to print u64 and dma_addr_t, and the suggestion is
> > to stay with ("%08llx", (u64)addr) for both. What should go wrong?  
> 
> I agree that printing long 64-bit hex values is annoying. However,
> "%08llx" is not the common format for printing DMA addresses,

I'm not sure if there is anything special about DMA addresses which
makes them different from any other addresses. And if I run 'dmesg'
on x86-64, the only addresses (of any sort) that I see printed with
padding are the memory map table in early boot and GPU drivers.

Both are understandable, because the map has entries from a few KB to
a few GB and it is more readable when they are aligned, and GPUs have
lots of memory and addresses. 

Other drivers don't bother. The %pr 'struct resource' format used in
various places doesn't bother: [mem 0xcff80290-0xcff80383].

IOMMU drivers from Intel and AMD don't bother:
https://bugzilla.kernel.org/show_bug.cgi?id=220069#c63
https://bugzilla.kernel.org/show_bug.cgi?id=215906#c0

These are DMA addresses being stored as u64 and printed with %llx.
And they can be the same or related to addresses logged by xhci_hcd,
e.g. if transfer ring is accessed out of bounds (commit bb0ba4cb1065):

[ 1.041954] xhci_hcd: Miss service interval error for slot 1 ep 2 expected TD DMA ffa08fe0
[ 1.042120] xhci_hcd: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffa09000 flags=0x0000]
[ 1.042146] xhci_hcd: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0005 address=0xffa09040 flags=0x0000]

> I would agree on another format if it is consistently used
> throughout the xHCI driver. I chose this format because it's the
> most common format.

I guess it will ultimately be for Mathias to decide.

My arguments for staying with %llx/%08llx are same as before:

1. We can control padding to suit practical needs, such as:
2. Same rendering of same numbers in u64 or dma_addr_t types.
3. Consistency with *real world* dmesg output.
4. Can be passed by value, no temp variables for each trb_virt_to_dma()
5. Can be type checked by compilers (see below).

Disadvantages:

1. Needs casting, IMO it's still better readability than problem 4.
2. In some cases (64 bit, no IOMMU or passthrough) address length will
   vary a little. IMO not a dealbreaker in logs, but obviously tables
   like debugfs/event-ring/trbs should stay padded and aligned.
3. Pedants will compplain that dma_addr_t should go with %pad

In my opinion it's a trap - %pad may look like "just the right format
for this type", but in reality dma_addr_t isn't a standard C type, but
a hack to use different types depending on CONFIG options.

To make this work, %pad defeats type checking by pretending to be just
a pointer. Tools assume that its value will be printed, but in  reality
it will be dereferenced. No type checking is done on the target type.

This compiles, works on x86, and is wrong:

	u64 addr;
	xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
                       "Set TR Deq ptr 0x%llx, cycle %u\n", &addr, new_cycle);

Existing %llx can be fixed by adding (u64), and the missing cast
could have been detected years ago with this trivial change:

--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1785,6 +1785,7 @@ static inline bool xhci_link_chain_quirk(struct xhci_hcd *xhci, enum xhci_ring_t
 /* xHCI debugging */
 char *xhci_get_slot_state(struct xhci_hcd *xhci,
                struct xhci_container_ctx *ctx);
+__printf(3, 4)
 void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct va_format *),
                        const char *fmt, ...);


On a more humorous note, I really became a hater of %pad after writing
a patch which logs [sw_dequeue/hw_dequeue/sw_enqueue] for each command:

25/0 (042/3) [fff47870/fff47871/fff47890] queue_set_tr_deq stream 0 addr 0x0x00000000fff47890
25/0 (041/3) [fff47870/fff47891/fff47890] handle_cmd_completion cmd_type 16 comp_code 1

You wouldn't want to see this with %pad. It's bad enough that there
is a %pad at the end of the first line, I'll need to fix it ;)

Regards,
Michal

Re: [PATCH 2/7] usb: xhci: use '%pad' specifier for DMA address printing

[Michal Pecio]

On Tue, 16 Sep 2025 11:32:10 +0200, Michal Pecio wrote:
> This compiles, works on x86, and is wrong:
> 
> 	u64 addr;
> 	xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
>                        "Set TR Deq ptr 0x%llx, cycle %u\n", &addr, new_cycle);

Sorry, I tried to do one thing and I did it wrong.

Of course I meant that ("%pad", &addr) is wrong. Basically, as soon as
formats are converted to %p-anything and variables are passed by ref,
it's game over for type checking.