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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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