| Message ID | 20150511121315.21044.78464.stgit@phlsvslse11.ph.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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@intel.com> Cc: Luis R. Rodriguez <mcgrof@do-not-panic.com> Cc: Luis R. Rodriguez <mcgrof@suse.com> > --- > 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.
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@intel.com> > > Cc: Luis R. Rodriguez <mcgrof@do-not-panic.com> > Cc: Luis R. Rodriguez <mcgrof@suse.com> > > > --- > > 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.
> 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
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;
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@intel.com> --- drivers/infiniband/hw/qib/qib_wc_x86_64.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html