linux-next: [PATCH] infiniband/hw/ipath/ipath_sdma.c , fix compiler warnings

On linux-next from today , allmodconfig, I see the following warnings on 64bit:

...

CC [M] drivers/infiniband/hw/ipath/ipath_sdma.o
drivers/infiniband/hw/ipath/ipath_sdma.c: In function 'sdma_abort_task':
drivers/infiniband/hw/ipath/ipath_sdma.c:267: warning: passing argument 2 of 'constant_test_bit' from incompatible pointer type drivers/infiniband/hw/ipath/ipath_sdma.c:267: warning: passing argument 2 of 'variable_test_bit' from incompatible pointer type drivers/infiniband/hw/ipath/ipath_sdma.c:269: warning: passing argument 2 of 'constant_test_bit' from incompatible pointer type drivers/infiniband/hw/ipath/ipath_sdma.c:269: warning: passing argument 2 of 'variable_test_bit' from incompatible pointer type drivers/infiniband/hw/ipath/ipath_sdma.c:271: warning: passing argument 2 of 'constant_test_bit' from incompatible pointer type drivers/infiniband/hw/ipath/ipath_sdma.c:271: warning: passing argument 2 of 'variable_test_bit' from incompatible pointer type drivers/infiniband/hw/ipath/ipath_sdma.c:273: warning: passing argument 2 of 'constant_test_bit' from incompatible pointer type drivers/infiniband/hw/ipath/ipath_sdma.c:273: warning: passing argument 2 of 'variable_test_bit' from incompatible pointer type drivers/infiniband/hw/ipath/ipath_sdma.c:348: warning: format '%016llx' expects type 'long long unsigned int', but argument 3 has type 'long unsigned int' drivers/infiniband/hw/ipath/ipath_sdma.c: In function 'ipath_restart_sdma':
drivers/infiniband/hw/ipath/ipath_sdma.c:618: warning: format '%016llx' expects type 'long long unsigned int', but argument 3 has type 'long unsigned int'

...

---

I see the 'format' warnings in mainline also.

 drivers/infiniband/hw/ipath/ipath_sdma.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

index 3697449..5f80151 100644
@@ -257,7 +257,7 @@ static void sdma_abort_task(unsigned long opaque)
 	/* everything is stopped, time to clean up and restart */
 	if (status == IPATH_SDMA_ABORT_ABORTED) {
 		struct ipath_sdma_txreq *txp, *txpnext;
-		u64 hwstatus;
+		unsigned long hwstatus;
 		int notify = 0;
 
 		hwstatus = ipath_read_kreg64(dd,

@@ -346,7 +346,7 @@ resched:
 	 */
 	if (jiffies > dd->ipath_sdma_abort_jiffies) {
 		ipath_dbg("looping with status 0x%016llx\n",
-			  dd->ipath_sdma_status);
+			  (unsigned long long)dd->ipath_sdma_status);
 		dd->ipath_sdma_abort_jiffies = jiffies + 5 * HZ;
 	}
 resched_noprint:

@@ -616,7 +616,7 @@ void ipath_restart_sdma(struct ipath_devdata *dd)
 	spin_unlock_irqrestore(&dd->ipath_sdma_lock, flags);
 	if (!needed) {
 		ipath_dbg("invalid attempt to restart SDMA, status 0x%016llx\n",
-			dd->ipath_sdma_status);
+			(unsigned long long)dd->ipath_sdma_status);
 		goto bail;
 	}
 	spin_lock_irqsave(&dd->ipath_sendctrl_lock, flags);

Hi Gabriel,

We appreciate the reports, thanks.

What architecture are you building on?

Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/

2008/5/22 Stephen Rothwell <[email protected]>:

It is x86_64

Gabriel

x86_64 right?

<snip>

    test_bit(63, &hwstatus) ||
    /* AbortInProg */
    test_bit(62, &hwstatus) ||
    /* InternalSDmaEnable */
    test_bit(61, &hwstatus) ||

2008/5/22 Tony Breeds <[email protected]>:

Hmm , right it need be 64-bits.

I should drink my coffee first and read code more carefully before sending out wrong patches , sorry.

I'm not sure about this one.

Gabriel

2008/5/22 Tony Breeds <[email protected]>:

Yes it is x86_64

Perhaps the best way to fix these is to change code like

		if (/* ScoreBoardDrainInProg */
		    test_bit(63, &hwstatus) ||
		    /* AbortInProg */
		    test_bit(62, &hwstatus) ||
		    /* InternalSDmaEnable */
		    test_bit(61, &hwstatus) ||
		    /* ScbEmpty */
		    !test_bit(30, &hwstatus)) {

to something like

		if ((hwstatus & (IPATH_SDMA_STATUS_SCORE_BOARD_DRAIN_IN_PROG |
				 IPATH_SDMA_STATUS_ABORT_IN_PROG	     |
				 IPATH_SDMA_STATUS_INTERNAL_SDMA_ENABLE)) ||
		    !(hwstatus & IPATH_SDMA_STATUS_SCB_EMPTY)) {

with appropriate defines for the constants 1ull << 63 etc.

(I think I got the logic correct but someone should check)

I have a fix for this pending; will ask Linus to pull today.

This looks good to me.

OK, I added the following to my tree:

commit e8ffef73c8dd2c2d00287829db87cdaf229d3859

Author: Roland Dreier <[email protected]>
Date:   Mon May 26 15:20:34 2008 -0700

IB/ipath: Avoid test_bit() on u64 SDMA status value

Gabriel C <[email protected]> pointed out that when the x86 bitops are updated to operate on unsigned long, the code in sdma_abort_task() will produce warnings:

drivers/infiniband/hw/ipath/ipath_sdma.c: In function 'sdma_abort_task':
drivers/infiniband/hw/ipath/ipath_sdma.c:267: warning: passing argument 2 of 'constant_test_bit' from incompatible pointer type

and so on, because it uses test_bit() to operation on a u64 value (returned by ipath_read_kref64() for a hardware register).

Fix up these warnings by converting the test_bit() operations to &ing with appropriate symbolic defines of the bits within the hardware register. This has the benign side-effect of making the code more self-documenting as well.

index 59a8b25..0bd8bcb 100644
@@ -232,6 +232,11 @@ struct ipath_sdma_desc {
 #define IPATH_SDMA_TXREQ_S_ABORTED   2
 #define IPATH_SDMA_TXREQ_S_SHUTDOWN  3
 
+#define IPATH_SDMA_STATUS_SCORE_BOARD_DRAIN_IN_PROG	(1ull << 63)
+#define IPATH_SDMA_STATUS_ABORT_IN_PROG			(1ull << 62)
+#define IPATH_SDMA_STATUS_INTERNAL_SDMA_ENABLE		(1ull << 61)
+#define IPATH_SDMA_STATUS_SCB_EMPTY			(1ull << 30)
+
 /* max dwords in small buffer packet */
 #define IPATH_SMALLBUF_DWORDS (dd->ipath_piosize2k >> 2)
 

index 0a8c1b8..eaba032 100644
@@ -263,14 +263,10 @@ static void sdma_abort_task(unsigned long opaque)
 		hwstatus = ipath_read_kreg64(dd,
 				dd->ipath_kregs->kr_senddmastatus);
 
-		if (/* ScoreBoardDrainInProg */
-		    test_bit(63, &hwstatus) ||
-		    /* AbortInProg */
-		    test_bit(62, &hwstatus) ||
-		    /* InternalSDmaEnable */
-		    test_bit(61, &hwstatus) ||
-		    /* ScbEmpty */
-		    !test_bit(30, &hwstatus)) {
+		if ((hwstatus & (IPATH_SDMA_STATUS_SCORE_BOARD_DRAIN_IN_PROG |
+				 IPATH_SDMA_STATUS_ABORT_IN_PROG	     |
+				 IPATH_SDMA_STATUS_INTERNAL_SDMA_ENABLE)) ||
+		    !(hwstatus & IPATH_SDMA_STATUS_SCB_EMPTY)) {
 			if (dd->ipath_sdma_reset_wait > 0) {
 				/* not done shutting down sdma */
 				--dd->ipath_sdma_reset_wait;