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 <[email protected]>
>
> Cc: Luis R. Rodriguez <[email protected]>
> Cc: Luis R. Rodriguez <[email protected]>
>
> > ---
> > 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 <[email protected]> GPG KeyID: 0E572FDD
signature.asc
Description: This is a digitally signed message part
