[PATCH] IB/qib: fix test of unsigned variable

[PATCH] IB/qib: fix test of unsigned variable

[Mike Marciniszyn]

Commit d4988623cc60 ("IB/qib: use arch_phys_wc_add()")
adjusted mtrr inititialization to use the new interface.

Unfortunately, the new interface returns a signed
value and the patch tested the unsigned wc_cookie.

Fix the issue by using 'ret' and only assigning
wc_cookie from ret when ret >= 0.   Additionally, insure
ret is 0 on success.

Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/hw/qib/qib_wc_x86_64.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_wc_x86_64.c b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
index 6d61ef9..85525be 100644
--- a/drivers/infiniband/hw/qib/qib_wc_x86_64.c
+++ b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
@@ -116,9 +116,11 @@ int qib_enable_wc(struct qib_devdata *dd)
 	}
 
 	if (!ret) {
-		dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
-		if (dd->wc_cookie < 0)
-			ret = -EINVAL;
+		ret = arch_phys_wc_add(pioaddr, piolen);
+		if (ret >= 0) {
+			dd->wc_cookie = ret;
+			ret = 0;
+		}
 	}
 
 	return ret;

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/qib: fix test of unsigned variable

[Yann Droneaud]

Hi,

Le lundi 11 mai 2015 à 08:13 -0400, Mike Marciniszyn a écrit :
> Commit d4988623cc60 ("IB/qib: use arch_phys_wc_add()")
> adjusted mtrr inititialization to use the new interface.
> 
> Unfortunately, the new interface returns a signed
> value and the patch tested the unsigned wc_cookie.
> 
> Fix the issue by using 'ret' and only assigning
> wc_cookie from ret when ret >= 0.   Additionally, insure
> ret is 0 on success.
> 
> Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Cc: Luis R. Rodriguez <mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
Cc: Luis R. Rodriguez <mcgrof-IBi9RG/b67k@public.gmane.org>

> ---
>  drivers/infiniband/hw/qib/qib_wc_x86_64.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/qib/qib_wc_x86_64.c b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> index 6d61ef9..85525be 100644
> --- a/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> +++ b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> @@ -116,9 +116,11 @@ int qib_enable_wc(struct qib_devdata *dd)
>  	}
>  
>  	if (!ret) {
> -		dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> -		if (dd->wc_cookie < 0)
> -			ret = -EINVAL;
> +		ret = arch_phys_wc_add(pioaddr, piolen);
> +		if (ret >= 0) {
> +			dd->wc_cookie = ret;
> +			ret = 0;
> +		}
>  	}
>  
>  	return ret;
> 

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/qib: fix test of unsigned variable

[Doug Ledford]

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

On Mon, 2015-05-11 at 14:46 +0200, Yann Droneaud wrote:
> Hi,
> 
> Le lundi 11 mai 2015 à 08:13 -0400, Mike Marciniszyn a écrit :
> > Commit d4988623cc60 ("IB/qib: use arch_phys_wc_add()")
> > adjusted mtrr inititialization to use the new interface.
> > 
> > Unfortunately, the new interface returns a signed
> > value and the patch tested the unsigned wc_cookie.
> > 
> > Fix the issue by using 'ret' and only assigning
> > wc_cookie from ret when ret >= 0.   Additionally, insure
> > ret is 0 on success.
> > 
> > Signed-off-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Cc: Luis R. Rodriguez <mcgrof-3uybbJdB1yH774rrrx3eTA@public.gmane.org>
> Cc: Luis R. Rodriguez <mcgrof-IBi9RG/b67k@public.gmane.org>
> 
> > ---
> >  drivers/infiniband/hw/qib/qib_wc_x86_64.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/infiniband/hw/qib/qib_wc_x86_64.c b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> > index 6d61ef9..85525be 100644
> > --- a/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> > +++ b/drivers/infiniband/hw/qib/qib_wc_x86_64.c
> > @@ -116,9 +116,11 @@ int qib_enable_wc(struct qib_devdata *dd)
> >  	}
> >  
> >  	if (!ret) {
> > -		dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > -		if (dd->wc_cookie < 0)
> > -			ret = -EINVAL;
> > +		ret = arch_phys_wc_add(pioaddr, piolen);
> > +		if (ret >= 0) {
> > +			dd->wc_cookie = ret;
> > +			ret = 0;
> > +		}

This fix is broken.  Luis and I discussed this at some length with his
original patch.  You're right that the code is broken because wc_cookie
is unsigned and that needs fixed, but once qib_enable_wc returns an
error, we don't fail the adapter init, we print out a warning about poor
performance and move on.  Later, we test wc_cookie != 0 in order to
determine if we used PAT.  It doesn't matter if our return was positive
or negative, we only try to enable wc via arch_phys_wc_add() when we
didn't use PAT, and so we need to capture the return value whether it's
a valid cookie or an error return in order to make sure we don't attempt
to use PAT later on.

So, the matrix of what the driver does now looks like this:

dd->wc_cookie == 0: PAT was used
dd->wc_cookie > 0: wc was used successfully
dd->wc_cookie < 0: neither PAT nor wc was used

I've fixed up the change to simply switch wc_cookie from unsigned long
to plain long and left the original code as it was, and it is now staged
for 4.1-rc.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] IB/qib: fix test of unsigned variable

[Marciniszyn, Mike]

> So, the matrix of what the driver does now looks like this:
> 
> dd->wc_cookie == 0: PAT was used
> dd->wc_cookie > 0: wc was used successfully wc_cookie < 0: neither PAT
> dd->nor wc was used
> 
> I've fixed up the change to simply switch wc_cookie from unsigned long to plain
> long and left the original code as it was, and it is now staged for 4.1-rc.
> 

I have a v2 of this patch that changes the type of the wc_cookie.

It also uses any negative cookie as the ret.   It seems better to use in actual error arch_phys_wc_add() from  vs. a canned one.

Mike 

Re: [PATCH] IB/qib: fix test of unsigned variable

[Jason Gunthorpe]

On Mon, May 11, 2015 at 08:13:15AM -0400, Mike Marciniszyn wrote:
> Commit d4988623cc60 ("IB/qib: use arch_phys_wc_add()")
> adjusted mtrr inititialization to use the new interface.
> 
> Unfortunately, the new interface returns a signed
> value and the patch tested the unsigned wc_cookie.

Looks like wc_cookie is only used with arch_phys_wc_add/del, so it
should become an int, not casted..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html