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

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

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