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

[Michal Pecio <michal.pecio@gmail.com>]

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