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

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- 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 --]