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

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

[Gabriel C]

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'

...

Signed-off-by: Gabriel C <nix.or.die@googlemail.com>

---

I see the 'format' warnings in mainline also.

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

diff --git a/drivers/infiniband/hw/ipath/ipath_sdma.c b/drivers/infiniband/hw/ipath/ipath_sdma.c
index 3697449..5f80151 100644
--- a/drivers/infiniband/hw/ipath/ipath_sdma.c
+++ b/drivers/infiniband/hw/ipath/ipath_sdma.c
@@ -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);

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

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

Hi Gabriel,

We appreciate the reports, thanks.

On Wed, 21 May 2008 16:06:36 +0200 Gabriel C <nix.or.die@googlemail.com> wrote:
>
> On linux-next from today, allmodconfig, I see the following warnings on 64bit:

What architecture are you building on?

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Tony Breeds]

On Wed, May 21, 2008 at 04:06:36PM +0200, Gabriel C wrote:
> On linux-next from today , allmodconfig, I see the following warnings on 64bit:

x86_64 right?

<snip>

> diff --git a/drivers/infiniband/hw/ipath/ipath_sdma.c b/drivers/infiniband/hw/ipath/ipath_sdma.c
> index 3697449..5f80151 100644
> --- a/drivers/infiniband/hw/ipath/ipath_sdma.c
> +++ b/drivers/infiniband/hw/ipath/ipath_sdma.c
> @@ -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,

This can't be right.  hwstatus needs to be u64, as that's what ipath_read_kreg64() retuns.
and a little bit further down we see:

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

so hwstatus, clearly needs to be 64-bits.  This brings up an interesting
point.  test_bit() and co are essntally expecting to be passed the address
of an unsigned long[], so is it correct to pass &u64?

Yours Tony

  linux.conf.au    http://www.marchsouth.org/
  Jan 19 - 24 2009 The Australian Linux Technical Conference!

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

[Gabriel C]

[-- Attachment #1.1: Type: text/plain, Size: 388 bytes --]

2008/5/22 Stephen Rothwell <sfr@canb.auug.org.au>:

> Hi Gabriel,
>
> We appreciate the reports, thanks.
>
> On Wed, 21 May 2008 16:06:36 +0200 Gabriel C <nix.or.die@googlemail.com>
> wrote:
> >
> > On linux-next from today, allmodconfig, I see the following warnings on
> 64bit:
>
> What architecture are you building on?


It is x86_64

Gabriel

> <http://www.canb.auug.org.au/%7Esfr/>

[-- Attachment #1.2: Type: text/html, Size: 937 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Gabriel C]

2008/5/22 Tony Breeds <tony@bakeyournoodle.com>:
>
> On Wed, May 21, 2008 at 04:06:36PM +0200, Gabriel C wrote:
> > On linux-next from today , allmodconfig, I see the following warnings on 64bit:
>
> x86_64 right?
>
> <snip>
>
> > diff --git a/drivers/infiniband/hw/ipath/ipath_sdma.c b/drivers/infiniband/hw/ipath/ipath_sdma.c
> > index 3697449..5f80151 100644
> > --- a/drivers/infiniband/hw/ipath/ipath_sdma.c
> > +++ b/drivers/infiniband/hw/ipath/ipath_sdma.c
> > @@ -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,
>
> This can't be right.  hwstatus needs to be u64, as that's what ipath_read_kreg64() retuns.
> and a little bit further down we see:
>
> ---
> if (/* ScoreBoardDrainInProg */
>    test_bit(63, &hwstatus) ||
>    /* AbortInProg */
>    test_bit(62, &hwstatus) ||
>    /* InternalSDmaEnable */
>    test_bit(61, &hwstatus) ||
> ---
>
> so hwstatus, clearly needs to be 64-bits.

Hmm , right it need be 64-bits.

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

> This brings up an interesting point.  test_bit() and co are
> essntally expecting to be passed the address
> of an unsigned long[], so is it correct to pass &u64?

I'm not sure about this one.

>
> Yours Tony
>

Gabriel

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

[Gabriel C]

2008/5/22 Tony Breeds <tony@bakeyournoodle.com>:
> On Wed, May 21, 2008 at 04:06:36PM +0200, Gabriel C wrote:
>> On linux-next from today , allmodconfig, I see the following warnings on 64bit:
>
> x86_64 right?
>

Yes it is x86_64

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

[Roland Dreier]

 > 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

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)

 > 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 have a fix for this pending; will ask Linus to pull today.

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

[Ralph Campbell]

This looks good to me.

On Fri, 2008-05-23 at 10:42 -0700, Roland Dreier wrote:
>  > 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
> 
> 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)
> 
>  > 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 have a fix for this pending; will ask Linus to pull today.
> _______________________________________________
> general mailing list
> general@lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

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

[Roland Dreier]

OK, I added the following to my tree:

commit e8ffef73c8dd2c2d00287829db87cdaf229d3859
Author: Roland Dreier <rolandd@cisco.com>
Date:   Mon May 26 15:20:34 2008 -0700

    IB/ipath: Avoid test_bit() on u64 SDMA status value
    
    Gabriel C <nix.or.die@googlemail.com> 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.
    
    Signed-off-by: Roland Dreier <rolandd@cisco.com>

diff --git a/drivers/infiniband/hw/ipath/ipath_kernel.h b/drivers/infiniband/hw/ipath/ipath_kernel.h
index 59a8b25..0bd8bcb 100644
--- a/drivers/infiniband/hw/ipath/ipath_kernel.h
+++ b/drivers/infiniband/hw/ipath/ipath_kernel.h
@@ -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)
 
diff --git a/drivers/infiniband/hw/ipath/ipath_sdma.c b/drivers/infiniband/hw/ipath/ipath_sdma.c
index 0a8c1b8..eaba032 100644
--- a/drivers/infiniband/hw/ipath/ipath_sdma.c
+++ b/drivers/infiniband/hw/ipath/ipath_sdma.c
@@ -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;