printk and dma_addr_t

PATCH In review Andrew Morton 7 participants 12 messages Feb 13, 2002 – Feb 15, 2002

Subsystems (5)

Network drivers Library code ATM LSI Logic MPT fusion drivers (FC/SAS/SPI) +1 more subsystems

lore

Share & tools

lore

Andrew Morton

author

2002-02-13 09:21 UTC #0 lore raw

PATCH 1/1 printk and dma_addr_t

A number of drivers like to print out the values of variables
which have type dma_addr_t. But there's no sane safe way
of doing this, because the size of the dma_addr_t type depends upon platform and config.

This code:

	dma_addr_t a;
	char *s;
	printk("stuff: %lx %s", a, s);

will crash the kernel if dma_addr_t is 64-bit, because printk will get the string's address wrong.

The patch introduces a DMA_ADDR_T_FMT macro which is the appropriate printf conversion string for the selected
dma_addr_t type. So the above usage will become

	printk("stuff: " DMA_ADDR_T_FMT " %s", a, s);

A patch which fixes all the drivers which I could find follows.

Ralf, could you please double-check the mips implementation?

include/asm-i386/types.h +2 -0

@@ -47,8 +47,10 @@ typedef unsigned long long u64;
 
 #ifdef CONFIG_HIGHMEM
 typedef u64 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%Lx"
 #else
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 #endif
 typedef u64 dma64_addr_t;

include/asm-alpha/types.h +2 -0

@@ -50,5 +50,7 @@ typedef unsigned long u64;
 typedef u64 dma_addr_t;
 typedef u64 dma64_addr_t;
 
+#define DMA_ADDR_T_FMT		"%Lx"
+
 #endif /* __KERNEL__ */
 #endif /* _ALPHA_TYPES_H */

include/asm-arm/types.h +1 -0

@@ -44,6 +44,7 @@ typedef unsigned long long u64;
 /* Dma addresses are 32-bits wide.  */
 
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 #endif /* __KERNEL__ */

include/asm-cris/types.h +1 -0

@@ -44,6 +44,7 @@ typedef unsigned long long u64;
 /* Dma addresses are 32-bits wide, just like our other addresses.  */
  
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 #endif /* __KERNEL__ */

include/asm-ia64/types.h +1 -0

@@ -63,6 +63,7 @@ typedef __u64 u64;
 /* DMA addresses are 64-bits wide, in general.  */
 
 typedef u64 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%Lx"
 
 # endif /* __KERNEL__ */
 #endif /* !__ASSEMBLY__ */

include/asm-m68k/types.h +1 -0

@@ -52,6 +52,7 @@ typedef unsigned long long u64;
 /* DMA addresses are 32-bits wide */
 
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 #endif /* __KERNEL__ */

include/asm-mips64/types.h +1 -0

@@ -70,6 +70,7 @@ typedef unsigned long long u64;
 #define BITS_PER_LONG _MIPS_SZLONG
 
 typedef unsigned long dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 #endif /* __KERNEL__ */

include/asm-mips/types.h +1 -0

@@ -71,6 +71,7 @@ typedef unsigned long long u64;
 #define BITS_PER_LONG _MIPS_SZLONG
 
 typedef unsigned long dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 #endif /* __KERNEL__ */

include/asm-parisc/types.h +1 -0

@@ -48,6 +48,7 @@ typedef unsigned long long u64;
 /* Dma addresses are 32-bits wide.  */
 
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 #endif /* __KERNEL__ */

include/asm-ppc/types.h +2 -0

@@ -46,6 +46,8 @@ typedef __vector128 vector128;
 
 /* DMA addresses are 32-bits wide */
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
+
 typedef u64 dma64_addr_t;
 
 #endif /* __KERNEL__ */

include/asm-s390/types.h +1 -0

@@ -54,6 +54,7 @@ typedef unsigned long long u64;
 #define BITS_PER_LONG 32
 
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 typedef union {
 	unsigned long long pair;

include/asm-s390x/types.h +1 -0

@@ -56,6 +56,7 @@ typedef unsigned  long u64;
 #define BITS_PER_LONG 64
 
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 #endif                                 /* __KERNEL__                       */
 #endif

include/asm-sh/types.h +1 -0

@@ -44,6 +44,7 @@ typedef unsigned long long u64;
 /* Dma addresses are 32-bits wide.  */
 
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
 
 #endif /* __KERNEL__ */

include/asm-sparc64/types.h +2 -0

@@ -48,6 +48,8 @@ typedef unsigned long u64;
 /* Dma addresses come in generic and 64-bit flavours.  */
 
 typedef u32 dma_addr_t;
+#define DMA_ADDR_T_FMT		"%lx"
+
 typedef u64 dma64_addr_t;
 
 #endif /* __KERNEL__ */

Alan Cox

2002-02-13 09:27 UTC #1 lore raw

Quoted 3 lines

dma_addr_t type. So the above usage will become
	printk("stuff: " DMA_ADDR_T_FMT " %s", a, s);

Vomit. How about adding a dma_addr_t %code to the printk function ?

Jeff Garzik

2002-02-13 09:34 UTC #2 lore raw

Quoted 6 lines · Alan Cox

Quoted 4 lines
dma_addr_t type. So the above usage will become
      printk("stuff: " DMA_ADDR_T_FMT " %s", a, s);
Vomit. How about adding a dma_addr_t %code to the printk function ?

heh, my comment on the patch was, "about as good as you can do without teaching printk and gcc about the type"

Can we hack gcc warnings, too?

Signature · 3 lines

Jeff Garzik      | "I went through my candy like hot oatmeal
Building 1024    |  through an internally-buttered weasel."
MandrakeSoft     |             - goats.com

David S. Miller

2002-02-13 09:38 UTC #3 lore raw

In reply to Alan Cox

From: Alan Cox <[email protected]>
Date: Wed, 13 Feb 2002 09:40:44 +0000 (GMT)

Vomit. How about adding a dma_addr_t %code to the printk function ?

And gcc will discover this via what? Osmosis perhaps? :-)

Alan Cox

2002-02-13 10:11 UTC #4 lore raw

Quoted 6 lines

From: Alan Cox <[email protected]>
Date: Wed, 13 Feb 2002 09:40:44 +0000 (GMT)
Vomit. How about adding a dma_addr_t %code to the printk function ?
And gcc will discover this via what? Osmosis perhaps? :-)

Yeah, seems gcc is a bit lacking in its overclever printf handlers. It lacks the ability to declare additional non standard % code - despite the fact glibc itself expands on the standard

glibc 2.0 adds conversion characters C and S.

glibc 2.1 adds length modifiers hh,j,t,z and conversion characters a,A.

glibc 2.2 adds the conversion character F with C99 seman- tics, and the flag character I.

So how do they modify the printf format rules in gcc ?

David S. Miller

2002-02-13 11:38 UTC #5 lore raw

From: Alan Cox <[email protected]>
Date: Wed, 13 Feb 2002 10:23:50 +0000 (GMT)

So how do they modify the printf format rules in gcc ?

Because they can claim that they are part of the C environment, and for the most part they are right so their extensions go into gcc's magic list.

In fact I'd claim their case to be plugging holes in the standards specified set of printf format strings. :-)

Hey... we could "borrow" one of these printf format strings we don't have any need for in the kernel and pretend that is for "dma_addr_t". :-)

Richard Gooch

2002-02-13 17:39 UTC #6 lore raw

Quoted 9 lines · David S. Miller

From: Alan Cox <[email protected]>
Date: Wed, 13 Feb 2002 10:23:50 +0000 (GMT)
So how do they modify the printf format rules in gcc ?
Because they can claim that they are part of the C environment, and for the most part they are right so their extensions go into gcc's magic list.

Quoted 6 lines

In fact I'd claim their case to be plugging holes in the standards specified set of printf format strings. :-)
Hey... we could "borrow" one of these printf format strings we don't have any need for in the kernel and pretend that is for "dma_addr_t". :-)

Eeeep! I know you put a smiley there, but don't even think that!

				Regards,

					Richard....
Permanent: [email protected]
Current:   [email protected]

Andrew Morton

author

2002-02-13 10:28 UTC #7 lore raw

In reply to David S. Miller

Quoted 2 lines · Alan Cox

So how do they modify the printf format rules in gcc ?

Good question. It'd be nice for NIPQUAD and such.

Here's an alternative fix. Less vomitous?

Sorry about sticking a prototype in types.h. It needs to be somewhere where all dma_addr_t users will see it, and where dma_addr_t is already in scope. Maybe there's a better place?

include/linux/types.h +4 -0

@@ -127,4 +127,8 @@ struct ustat {
 	char			f_fpack[6];
 };
 
+#ifdef __KERNEL__
+char *form_dma_addr_t(char *buf, dma_addr_t a);
+#endif
+
 #endif /* _LINUX_TYPES_H */

lib/vsprintf.c +17 -0

@@ -709,3 +709,20 @@ int sscanf(const char * buf, const char 
 	va_end(args);
 	return i;
 }
+
+/*
+ * dma_addr_t's have different sizes on different platforms,
+ * so we use this helper when we want to pass one to printk.
+ */
+char *form_dma_addr_t(char *buf, dma_addr_t a)
+{
+	char *fmt;	/* Funny code to prevent a printf warning */
+
+	if (sizeof(dma_addr_t) == sizeof(long))
+		fmt = "%lx";
+	else
+		fmt = "%Lx";
+
+	sprintf(buf, fmt, a);
+	return buf;
+}

kernel/ksyms.c +1 -0

@@ -458,6 +458,7 @@ EXPORT_SYMBOL(sscanf);
 EXPORT_SYMBOL(vsprintf);
 EXPORT_SYMBOL(vsnprintf);
 EXPORT_SYMBOL(vsscanf);
+EXPORT_SYMBOL(form_dma_addr_t);
 EXPORT_SYMBOL(kdevname);
 EXPORT_SYMBOL(bdevname);
 EXPORT_SYMBOL(cdevname);

drivers/atm/idt77252.c +4 -2

@@ -641,6 +641,7 @@ static struct scq_info *
 alloc_scq(struct idt77252_dev *card, int class)
 {
 	struct scq_info *scq;
+	char da_buf[33];
 
 	scq = (struct scq_info *) kmalloc(sizeof(struct scq_info), GFP_KERNEL);
 	if (!scq)

@@ -665,8 +666,9 @@ alloc_scq(struct idt77252_dev *card, int
 	skb_queue_head_init(&scq->transmit);
 	skb_queue_head_init(&scq->pending);
 
-	TXPRINTK("idt77252: SCQ: base 0x%p, next 0x%p, last 0x%p, paddr %08x\n",
-		 scq->base, scq->next, scq->last, scq->paddr);
+	TXPRINTK("idt77252: SCQ: base 0x%p, next 0x%p, last 0x%p, paddr %s\n",
+		 scq->base, scq->next, scq->last,
+		form_dma_addr_t(da_buf, scq->paddr));
 
 	return scq;
 }

drivers/message/fusion/mptbase.c +7 -4

@@ -3186,6 +3186,7 @@ mpt_print_ioc_facts(MPT_ADAPTER *ioc, ch
 {
 	char expVer[32];
 	char iocName[16];
+	char da_buf[33];
 	int sz;
 	int y;
 	int p;

@@ -3217,8 +3218,9 @@ mpt_print_ioc_facts(MPT_ADAPTER *ioc, ch
 	y += sprintf(buffer+len+y, "  MaxChainDepth = 0x%02x frames\n", ioc->facts.MaxChainDepth);
 	y += sprintf(buffer+len+y, "  MinBlockSize = 0x%02x bytes\n", 4*ioc->facts.BlockSize);
 
-	y += sprintf(buffer+len+y, "  RequestFrames @ 0x%p (Dma @ 0x%08x)\n",
-					ioc->req_alloc, ioc->req_alloc_dma);
+	y += sprintf(buffer+len+y, "  RequestFrames @ 0x%p (Dma @ 0x%s)\n",
+					ioc->req_alloc,
+					form_dma_addr_t(da_buf, ioc->req_alloc_dma));
 	/*
 	 *  Rounding UP to nearest 4-kB boundary here...
 	 */

@@ -3230,8 +3232,9 @@ mpt_print_ioc_facts(MPT_ADAPTER *ioc, ch
 					4*ioc->facts.RequestFrameSize,
 					ioc->facts.GlobalCredits);
 
-	y += sprintf(buffer+len+y, "  ReplyFrames   @ 0x%p (Dma @ 0x%08x)\n",
-					ioc->reply_alloc, ioc->reply_alloc_dma);
+	y += sprintf(buffer+len+y, "  ReplyFrames   @ 0x%p (Dma @ 0x%s)\n",
+					ioc->reply_alloc,
+					form_dma_addr_t(da_buf, ioc->reply_alloc_dma));
 	sz = (ioc->reply_sz * ioc->reply_depth) + 128;
 	y += sprintf(buffer+len+y, "    {CurRepSz=%d} x {CurRepDepth=%d} = %d bytes ^= 0x%x\n",
 					ioc->reply_sz, ioc->reply_depth, ioc->reply_sz*ioc->reply_depth, sz);

drivers/net/tokenring/tms380tr.c +3 -1

@@ -1428,6 +1428,7 @@ static int tms380tr_bringup_diags(struct
 static int tms380tr_init_adapter(struct net_device *dev)
 {
 	struct net_local *tp = (struct net_local *)dev->priv;
+	char da_buf[33];
 
 	const unsigned char SCB_Test[6] = {0x00, 0x00, 0xC1, 0xE2, 0xD4, 0x8B};
 	const unsigned char SSB_Test[8] = {0xFF, 0xFF, 0xD1, 0xD7,

@@ -1446,7 +1447,8 @@ static int tms380tr_init_adapter(struct 
 	if(tms380tr_debug > 3)
 	{
 		printk(KERN_INFO "%s: buffer (real): %lx\n", dev->name, (long) &tp->scb);
-		printk(KERN_INFO "%s: buffer (virt): %lx\n", dev->name, (long) ((char *)&tp->scb - (char *)tp) + tp->dmabuffer);
+		printk(KERN_INFO "%s: buffer (virt): %s\n", dev->name,
+			form_dma_addr_t(da_buf, (long) ((char *)&tp->scb - (char *)tp) + tp->dmabuffer));
 		printk(KERN_INFO "%s: buffer (DMA) : %lx\n", dev->name, (long) tp->dmabuffer);
 		printk(KERN_INFO "%s: buffer (tp)  : %lx\n", dev->name, (long) tp);
 	}

drivers/net/pcnet32.c +3 -1

@@ -529,6 +529,7 @@ pcnet32_probe1(unsigned long ioaddr, uns
     char *chipname;
     struct net_device *dev;
     struct pcnet32_access *a = NULL;
+    char da_buf[33];
 
     /* reset the chip */
     pcnet32_dwio_reset(ioaddr);

@@ -711,7 +712,8 @@ pcnet32_probe1(unsigned long ioaddr, uns
     memset(lp, 0, sizeof(*lp));
     lp->dma_addr = lp_dma_addr;
     lp->pci_dev = pdev;
-    printk("\n" KERN_INFO "pcnet32: pcnet32_private lp=%p lp_dma_addr=%#08x", lp, lp_dma_addr);
+    printk("\n" KERN_INFO "pcnet32: pcnet32_private lp=%p lp_dma_addr=%s",
+		lp, form_dma_addr_t(da_buf, lp_dma_addr));
 
     spin_lock_init(&lp->lock);

drivers/net/yellowfin.c +6 -2

@@ -1273,7 +1273,10 @@ static int yellowfin_close(struct net_de
 
 #if defined(__i386__)
 	if (yellowfin_debug > 2) {
-		printk("\n"KERN_DEBUG"  Tx ring at %8.8x:\n", yp->tx_ring_dma);
+		char da_buf[33];
+
+		printk("\n"KERN_DEBUG"  Tx ring at %s:\n",
+				form_dma_addr_t(da_buf, yp->tx_ring_dma));
 		for (i = 0; i < TX_RING_SIZE*2; i++)
 			printk(" %c #%d desc. %8.8x %8.8x %8.8x %8.8x.\n",
 				   inl(ioaddr + TxPtr) == (long)&yp->tx_ring[i] ? '>' : ' ',

@@ -1285,7 +1288,8 @@ static int yellowfin_close(struct net_de
 				   i, yp->tx_status[i].tx_cnt, yp->tx_status[i].tx_errs,
 				   yp->tx_status[i].total_tx_cnt, yp->tx_status[i].paused);
 
-		printk("\n"KERN_DEBUG "  Rx ring %8.8x:\n", yp->rx_ring_dma);
+		printk("\n"KERN_DEBUG "  Rx ring %s:\n",
+				form_dma_addr_t(da_buf, yp->rx_ring_dma));
 		for (i = 0; i < RX_RING_SIZE; i++) {
 			printk(KERN_DEBUG " %c #%d desc. %8.8x %8.8x %8.8x\n",
 				   inl(ioaddr + RxPtr) == (long)&yp->rx_ring[i] ? '>' : ' ',

drivers/net/starfire.c +6 -4

@@ -1911,15 +1911,17 @@ static int netdev_close(struct net_devic
 
 #ifdef __i386__
 	if (debug > 2) {
-		printk("\n"KERN_DEBUG"  Tx ring at %8.8x:\n",
-			   np->tx_ring_dma);
+		char da_buf[33];
+
+		printk("\n"KERN_DEBUG"  Tx ring at %s:\n",
+			   form_dma_addr_t(da_buf, np->tx_ring_dma));
 		for (i = 0; i < 8 /* TX_RING_SIZE is huge! */; i++)
 			printk(KERN_DEBUG " #%d desc. %8.8x %8.8x -> %8.8x.\n",
 			       i, le32_to_cpu(np->tx_ring[i].status),
 			       le32_to_cpu(np->tx_ring[i].first_addr),
 			       le32_to_cpu(np->tx_done_q[i].status));
-		printk(KERN_DEBUG "  Rx ring at %8.8x -> %p:\n",
-		       np->rx_ring_dma, np->rx_done_q);
+		printk(KERN_DEBUG "  Rx ring at %s -> %p:\n",
+		       form_dma_addr_t(da_buf, np->rx_ring_dma), np->rx_done_q);
 		if (np->rx_done_q)
 			for (i = 0; i < 8 /* RX_RING_SIZE */; i++) {
 				printk(KERN_DEBUG " #%d desc. %8.8x -> %8.8x\n",

Troy Benjegerdes

2002-02-15 21:38 UTC #8 lore raw

In reply to David S. Miller

Quoted 11 lines · Andrew Morton

Quoted 2 lines · Alan Cox
So how do they modify the printf format rules in gcc ?
Good question. It'd be nice for NIPQUAD and such.
Here's an alternative fix. Less vomitous?
Sorry about sticking a prototype in types.h. It needs to be somewhere where all dma_addr_t users will see it, and where dma_addr_t is already in scope. Maybe there's a better place?

Personally, I like this less.. I don't think the previous #define DMA_ADDR_T_FMT (or whatever it was) is that bad, considering the options.

Once we set the precedent of having little 'char
* form_some_random_type()' functions, they will show up all over the place.

I need something like this for the MTD patch I've got that supports 64 bit buswidths on 32 bit machines and needs to printk the data on error.

I found that #define CFI_FMT '%lx' or whatever to be more straightforward, and easier to understand.

Quoted 13 lines

+ */

(quoted diff) +12 -0

+char *form_dma_addr_t(char *buf, dma_addr_t a)
+{
+	char *fmt;	/* Funny code to prevent a printf warning */
+
+	if (sizeof(dma_addr_t) == sizeof(long))
+		fmt = "%lx";
+	else
+		fmt = "%Lx";
+
+	sprintf(buf, fmt, a);
+	return buf;
+}

Signature · 5 lines

Troy Benjegerdes | master of mispeeling | 'da hozer' |  [email protected]
-----"If this message isn't misspelled, I didn't write it" -- Me -----
"Why do musicians compose symphonies and poets write poems? They do it
because life wouldn't have any meaning for them if they didn't. That's 
why I draw cartoons. It's my life." -- Charles Schulz

David Woodhouse

2002-02-15 21:52 UTC #9 lore raw

Quoted 6 lines · [email protected] said:

I need something like this for the MTD patch I've got that supports
64 bit buswidths on 32 bit machines and needs to printk the data on error.
I found that #define CFI_FMT '%lx' or whatever to be more straightforward, and easier to understand.

I got annoyed with the original implementation that turned up in CVS, with a completely separate printk line #ifdef CFI_WORD_64, and I changed it to always cast to (long long) before printing. Although I half suspect I'll end up redoing it again before actually sending it to Linus.

--
dwmw2

Andrew Morton

author

2002-02-13 09:40 UTC #10 lore raw

In reply to Alan Cox

Quoted 11 lines · Jeff Garzik

Quoted 6 lines · Alan Cox
Quoted 4 lines
dma_addr_t type. So the above usage will become
      printk("stuff: " DMA_ADDR_T_FMT " %s", a, s);
Vomit. How about adding a dma_addr_t %code to the printk function ?
heh, my comment on the patch was, "about as good as you can do without teaching printk and gcc about the type"

we could do:

	char *format_dma_addr_t(char *buf, dma_addr_t a);

Andrew Morton

author

2002-02-13 09:34 UTC #11 lore raw

In reply to Andrew Morton

Quoted 6 lines · Alan Cox

Quoted 4 lines
dma_addr_t type. So the above usage will become
      printk("stuff: " DMA_ADDR_T_FMT " %s", a, s);
Vomit.

See, I said I should have used [email protected] for this one.

Quoted 1 line

How about adding a dma_addr_t %code to the printk function ?

The compiler's printf arg checking will choke on that.