* [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters
@ 2018-01-30 12:45 Dan Carpenter
2018-01-31 5:46 ` Devesh Sharma
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2018-01-30 12:45 UTC (permalink / raw)
To: devesh.sharma-dY08KVG/lbpWk0Htik3J/w; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA
Hello Devesh Sharma,
The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
adapters" from Jan 11, 2018, leads to the following static checker
warning:
drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
warn: 'srq->umem' isn't an ERR_PTR
drivers/infiniband/hw/bnxt_re/ib_verbs.c
1313 dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
1314 return rc;
1315 }
1316
1317 if (srq->umem && !IS_ERR(srq->umem))
^^^^^^^^^^^^^^^^
We never store error pointers to srq->umem. It's pretty consistently
checked for error pointers though so maybe that's fine. It causes a
static checker warning because error pointer confusion is a pretty
common source of bugs. Anyway, feel free to ignore if you want...
1318 ib_umem_release(srq->umem);
1319 kfree(srq);
1320 atomic_dec(&rdev->srq_count);
1321 if (nq)
1322 nq->budget--;
1323 return 0;
1324 }
1325
1326 static int bnxt_re_init_user_srq(struct bnxt_re_dev *rdev,
1327 struct bnxt_re_pd *pd,
1328 struct bnxt_re_srq *srq,
1329 struct ib_udata *udata)
1330 {
1331 struct bnxt_re_srq_req ureq;
1332 struct bnxt_qplib_srq *qplib_srq = &srq->qplib_srq;
1333 struct ib_umem *umem;
1334 int bytes = 0;
1335 struct ib_ucontext *context = pd->ib_pd.uobject->context;
1336 struct bnxt_re_ucontext *cntx = container_of(context,
1337 struct bnxt_re_ucontext,
1338 ib_uctx);
1339 if (ib_copy_from_udata(&ureq, udata, sizeof(ureq)))
1340 return -EFAULT;
1341
1342 bytes = (qplib_srq->max_wqe * BNXT_QPLIB_MAX_RQE_ENTRY_SIZE);
1343 bytes = PAGE_ALIGN(bytes);
1344 umem = ib_umem_get(context, ureq.srqva, bytes,
1345 IB_ACCESS_LOCAL_WRITE, 1);
1346 if (IS_ERR(umem))
1347 return PTR_ERR(umem);
1348
1349 srq->umem = umem;
^^^^^^^^^^^^^^^^
Set here, I guess.
1350 qplib_srq->nmap = umem->nmap;
regards,
dan carpenter
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters
2018-01-30 12:45 [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters Dan Carpenter
@ 2018-01-31 5:46 ` Devesh Sharma
[not found] ` <CANjDDBiiL1TtH5OZ9gYuiZquKu2=AV+u9nJDQ--qibOP5CXgYg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Devesh Sharma @ 2018-01-31 5:46 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-rdma
On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> Hello Devesh Sharma,
>
> The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> adapters" from Jan 11, 2018, leads to the following static checker
> warning:
>
> drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> warn: 'srq->umem' isn't an ERR_PTR
>
> drivers/infiniband/hw/bnxt_re/ib_verbs.c
> 1313 dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> 1314 return rc;
> 1315 }
> 1316
> 1317 if (srq->umem && !IS_ERR(srq->umem))
> ^^^^^^^^^^^^^^^^
> We never store error pointers to srq->umem. It's pretty consistently
> checked for error pointers though so maybe that's fine. It causes a
> static checker warning because error pointer confusion is a pretty
> common source of bugs. Anyway, feel free to ignore if you want...
Thanks for reporting Dan,
Is there a way out, I want to call ib_umem_release only if it was valid.
I think if ib_umem_release checks for the validity of pointer then I
can get rid of this?
There are other places also in bnxt_re driver where such checks are present.
>
> 1318 ib_umem_release(srq->umem);
> 1319 kfree(srq);
> 1320 atomic_dec(&rdev->srq_count);
> 1321 if (nq)
> 1322 nq->budget--;
> 1323 return 0;
> 1324 }
> 1325
> 1326 static int bnxt_re_init_user_srq(struct bnxt_re_dev *rdev,
> 1327 struct bnxt_re_pd *pd,
> 1328 struct bnxt_re_srq *srq,
> 1329 struct ib_udata *udata)
> 1330 {
> 1331 struct bnxt_re_srq_req ureq;
> 1332 struct bnxt_qplib_srq *qplib_srq = &srq->qplib_srq;
> 1333 struct ib_umem *umem;
> 1334 int bytes = 0;
> 1335 struct ib_ucontext *context = pd->ib_pd.uobject->context;
> 1336 struct bnxt_re_ucontext *cntx = container_of(context,
> 1337 struct bnxt_re_ucontext,
> 1338 ib_uctx);
> 1339 if (ib_copy_from_udata(&ureq, udata, sizeof(ureq)))
> 1340 return -EFAULT;
> 1341
> 1342 bytes = (qplib_srq->max_wqe * BNXT_QPLIB_MAX_RQE_ENTRY_SIZE);
> 1343 bytes = PAGE_ALIGN(bytes);
> 1344 umem = ib_umem_get(context, ureq.srqva, bytes,
> 1345 IB_ACCESS_LOCAL_WRITE, 1);
> 1346 if (IS_ERR(umem))
> 1347 return PTR_ERR(umem);
> 1348
> 1349 srq->umem = umem;
> ^^^^^^^^^^^^^^^^
> Set here, I guess.
Yeah, the checker is confused due to this.
>
> 1350 qplib_srq->nmap = umem->nmap;
>
> regards,
> dan carpenter
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters
[not found] ` <CANjDDBiiL1TtH5OZ9gYuiZquKu2=AV+u9nJDQ--qibOP5CXgYg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2018-01-31 6:32 ` Dan Carpenter
2018-01-31 6:48 ` Leon Romanovsky
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2018-01-31 6:32 UTC (permalink / raw)
To: Devesh Sharma; +Cc: linux-rdma
On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> > Hello Devesh Sharma,
> >
> > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > adapters" from Jan 11, 2018, leads to the following static checker
> > warning:
> >
> > drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > warn: 'srq->umem' isn't an ERR_PTR
> >
> > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > 1313 dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > 1314 return rc;
> > 1315 }
> > 1316
> > 1317 if (srq->umem && !IS_ERR(srq->umem))
> > ^^^^^^^^^^^^^^^^
> > We never store error pointers to srq->umem. It's pretty consistently
> > checked for error pointers though so maybe that's fine. It causes a
> > static checker warning because error pointer confusion is a pretty
> > common source of bugs. Anyway, feel free to ignore if you want...
> Thanks for reporting Dan,
>
> Is there a way out, I want to call ib_umem_release only if it was valid.
> I think if ib_umem_release checks for the validity of pointer then I
> can get rid of this?
> There are other places also in bnxt_re driver where such checks are present.
Yeah. Those places generate warnings as well, but I thought one was
enough. It's fine if you want to ignore the warning, no one will be
upset. :P
[ snip ]
> > 1342 bytes = (qplib_srq->max_wqe * BNXT_QPLIB_MAX_RQE_ENTRY_SIZE);
> > 1343 bytes = PAGE_ALIGN(bytes);
> > 1344 umem = ib_umem_get(context, ureq.srqva, bytes,
> > 1345 IB_ACCESS_LOCAL_WRITE, 1);
> > 1346 if (IS_ERR(umem))
> > 1347 return PTR_ERR(umem);
> > 1348
> > 1349 srq->umem = umem;
> > ^^^^^^^^^^^^^^^^
> > Set here, I guess.
> Yeah, the checker is confused due to this.
It does bother me that you're saying the "checker is confused". The
checker is printing a 100% accurate, factual warning... :/ We have an
IS_ERR() check when the pointer can not possibly be an error pointer.
regards,
dan carpenter
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters
2018-01-31 6:32 ` Dan Carpenter
@ 2018-01-31 6:48 ` Leon Romanovsky
[not found] ` <20180131064829.GR2055-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Leon Romanovsky @ 2018-01-31 6:48 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Devesh Sharma, linux-rdma
[-- Attachment #1: Type: text/plain, Size: 1988 bytes --]
On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
> On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> > > Hello Devesh Sharma,
> > >
> > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > > adapters" from Jan 11, 2018, leads to the following static checker
> > > warning:
> > >
> > > drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > > warn: 'srq->umem' isn't an ERR_PTR
> > >
> > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > 1313 dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > > 1314 return rc;
> > > 1315 }
> > > 1316
> > > 1317 if (srq->umem && !IS_ERR(srq->umem))
> > > ^^^^^^^^^^^^^^^^
> > > We never store error pointers to srq->umem. It's pretty consistently
> > > checked for error pointers though so maybe that's fine. It causes a
> > > static checker warning because error pointer confusion is a pretty
> > > common source of bugs. Anyway, feel free to ignore if you want...
> > Thanks for reporting Dan,
> >
> > Is there a way out, I want to call ib_umem_release only if it was valid.
> > I think if ib_umem_release checks for the validity of pointer then I
> > can get rid of this?
> > There are other places also in bnxt_re driver where such checks are present.
>
> Yeah. Those places generate warnings as well, but I thought one was
> enough. It's fine if you want to ignore the warning, no one will be
> upset. :P
Not really, we are trying to clean the subsystem from the warnings
and driver authors who ignore such warnings simply and very effective
sabotage it.
Currently my checks print ~400 warnings for the drivers/infiniband/* +
drivers/net/ethernet/mellanox/*
So please don't increase this number, or fix the driver or fix the tool :)
Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters
[not found] ` <20180131064829.GR2055-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
@ 2018-01-31 16:04 ` Doug Ledford
[not found] ` <1517414670.19117.16.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2018-02-01 17:44 ` Jason Gunthorpe
1 sibling, 1 reply; 8+ messages in thread
From: Doug Ledford @ 2018-01-31 16:04 UTC (permalink / raw)
To: Leon Romanovsky, Dan Carpenter; +Cc: Devesh Sharma, linux-rdma
[-- Attachment #1: Type: text/plain, Size: 3387 bytes --]
On Wed, 2018-01-31 at 08:48 +0200, Leon Romanovsky wrote:
> On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
> > On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> > > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > Hello Devesh Sharma,
> > > >
> > > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > > > adapters" from Jan 11, 2018, leads to the following static checker
> > > > warning:
> > > >
> > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > > > warn: 'srq->umem' isn't an ERR_PTR
> > > >
> > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > > 1313 dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > > > 1314 return rc;
> > > > 1315 }
> > > > 1316
> > > > 1317 if (srq->umem && !IS_ERR(srq->umem))
> > > > ^^^^^^^^^^^^^^^^
> > > > We never store error pointers to srq->umem. It's pretty consistently
> > > > checked for error pointers though so maybe that's fine. It causes a
> > > > static checker warning because error pointer confusion is a pretty
> > > > common source of bugs. Anyway, feel free to ignore if you want...
> > >
> > > Thanks for reporting Dan,
> > >
> > > Is there a way out, I want to call ib_umem_release only if it was valid.
> > > I think if ib_umem_release checks for the validity of pointer then I
> > > can get rid of this?
> > > There are other places also in bnxt_re driver where such checks are present.
> >
> > Yeah. Those places generate warnings as well, but I thought one was
> > enough. It's fine if you want to ignore the warning, no one will be
> > upset. :P
>
> Not really, we are trying to clean the subsystem from the warnings
> and driver authors who ignore such warnings simply and very effective
> sabotage it.
>
> Currently my checks print ~400 warnings for the drivers/infiniband/* +
> drivers/net/ethernet/mellanox/*
>
> So please don't increase this number, or fix the driver or fix the tool :)
>
> Thanks
Looking at the code, the proper fix for this is:
[dledford@haswell-e linus (k.o/wip/dl-for-next *)]$ git diff
diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 9b8fa77b8831..ae9e9ff54826 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -1314,7 +1314,7 @@ int bnxt_re_destroy_srq(struct ib_srq *ib_srq)
return rc;
}
- if (srq->umem && !IS_ERR(srq->umem))
+ if (srq->umem)
ib_umem_release(srq->umem);
kfree(srq);
atomic_dec(&rdev->srq_count);
@@ -1430,11 +1430,8 @@ struct ib_srq *bnxt_re_create_srq(struct ib_pd
*ib_pd,
return &srq->ib_srq;
fail:
- if (udata && srq->umem && !IS_ERR(srq->umem)) {
+ if (srq->umem)
ib_umem_release(srq->umem);
- srq->umem = NULL;
- }
-
kfree(srq);
exit:
return ERR_PTR(rc);
[dledford@haswell-e linus (k.o/wip/dl-for-next *)]$
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters
[not found] ` <1517414670.19117.16.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2018-01-31 21:07 ` Doug Ledford
[not found] ` <1517432861.19117.42.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 8+ messages in thread
From: Doug Ledford @ 2018-01-31 21:07 UTC (permalink / raw)
To: Leon Romanovsky, Dan Carpenter; +Cc: Devesh Sharma, linux-rdma
[-- Attachment #1: Type: text/plain, Size: 3671 bytes --]
On Wed, 2018-01-31 at 11:04 -0500, Doug Ledford wrote:
> On Wed, 2018-01-31 at 08:48 +0200, Leon Romanovsky wrote:
> > On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
> > > On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> > > > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > > Hello Devesh Sharma,
> > > > >
> > > > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > > > > adapters" from Jan 11, 2018, leads to the following static checker
> > > > > warning:
> > > > >
> > > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > > > > warn: 'srq->umem' isn't an ERR_PTR
> > > > >
> > > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > > > 1313 dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > > > > 1314 return rc;
> > > > > 1315 }
> > > > > 1316
> > > > > 1317 if (srq->umem && !IS_ERR(srq->umem))
> > > > > ^^^^^^^^^^^^^^^^
> > > > > We never store error pointers to srq->umem. It's pretty consistently
> > > > > checked for error pointers though so maybe that's fine. It causes a
> > > > > static checker warning because error pointer confusion is a pretty
> > > > > common source of bugs. Anyway, feel free to ignore if you want...
> > > >
> > > > Thanks for reporting Dan,
> > > >
> > > > Is there a way out, I want to call ib_umem_release only if it was valid.
> > > > I think if ib_umem_release checks for the validity of pointer then I
> > > > can get rid of this?
> > > > There are other places also in bnxt_re driver where such checks are present.
> > >
> > > Yeah. Those places generate warnings as well, but I thought one was
> > > enough. It's fine if you want to ignore the warning, no one will be
> > > upset. :P
> >
> > Not really, we are trying to clean the subsystem from the warnings
> > and driver authors who ignore such warnings simply and very effective
> > sabotage it.
> >
> > Currently my checks print ~400 warnings for the drivers/infiniband/* +
> > drivers/net/ethernet/mellanox/*
> >
> > So please don't increase this number, or fix the driver or fix the tool :)
> >
> > Thanks
>
> Looking at the code, the proper fix for this is:
>
> [dledford@haswell-e linus (k.o/wip/dl-for-next *)]$ git diff
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 9b8fa77b8831..ae9e9ff54826 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -1314,7 +1314,7 @@ int bnxt_re_destroy_srq(struct ib_srq *ib_srq)
> return rc;
> }
>
> - if (srq->umem && !IS_ERR(srq->umem))
> + if (srq->umem)
> ib_umem_release(srq->umem);
> kfree(srq);
> atomic_dec(&rdev->srq_count);
> @@ -1430,11 +1430,8 @@ struct ib_srq *bnxt_re_create_srq(struct ib_pd
> *ib_pd,
> return &srq->ib_srq;
>
> fail:
> - if (udata && srq->umem && !IS_ERR(srq->umem)) {
> + if (srq->umem)
> ib_umem_release(srq->umem);
> - srq->umem = NULL;
> - }
> -
> kfree(srq);
> exit:
> return ERR_PTR(rc);
> [dledford@haswell-e linus (k.o/wip/dl-for-next *)]$
>
This was committed in my tree for the next merge pull request.
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters
[not found] ` <1517432861.19117.42.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
@ 2018-02-01 3:34 ` Devesh Sharma
0 siblings, 0 replies; 8+ messages in thread
From: Devesh Sharma @ 2018-02-01 3:34 UTC (permalink / raw)
To: Doug Ledford; +Cc: Leon Romanovsky, Dan Carpenter, linux-rdma
On Thu, Feb 1, 2018 at 2:37 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 2018-01-31 at 11:04 -0500, Doug Ledford wrote:
>> On Wed, 2018-01-31 at 08:48 +0200, Leon Romanovsky wrote:
>> > On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
>> > > On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
>> > > > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
>> > > > > Hello Devesh Sharma,
>> > > > >
>> > > > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
>> > > > > adapters" from Jan 11, 2018, leads to the following static checker
>> > > > > warning:
>> > > > >
>> > > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
>> > > > > warn: 'srq->umem' isn't an ERR_PTR
>> > > > >
>> > > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> > > > > 1313 dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
>> > > > > 1314 return rc;
>> > > > > 1315 }
>> > > > > 1316
>> > > > > 1317 if (srq->umem && !IS_ERR(srq->umem))
>> > > > > ^^^^^^^^^^^^^^^^
>> > > > > We never store error pointers to srq->umem. It's pretty consistently
>> > > > > checked for error pointers though so maybe that's fine. It causes a
>> > > > > static checker warning because error pointer confusion is a pretty
>> > > > > common source of bugs. Anyway, feel free to ignore if you want...
>> > > >
>> > > > Thanks for reporting Dan,
>> > > >
>> > > > Is there a way out, I want to call ib_umem_release only if it was valid.
>> > > > I think if ib_umem_release checks for the validity of pointer then I
>> > > > can get rid of this?
>> > > > There are other places also in bnxt_re driver where such checks are present.
>> > >
>> > > Yeah. Those places generate warnings as well, but I thought one was
>> > > enough. It's fine if you want to ignore the warning, no one will be
>> > > upset. :P
>> >
>> > Not really, we are trying to clean the subsystem from the warnings
>> > and driver authors who ignore such warnings simply and very effective
>> > sabotage it.
>> >
>> > Currently my checks print ~400 warnings for the drivers/infiniband/* +
>> > drivers/net/ethernet/mellanox/*
>> >
>> > So please don't increase this number, or fix the driver or fix the tool :)
>> >
>> > Thanks
>>
>> Looking at the code, the proper fix for this is:
>>
>> [dledford@haswell-e linus (k.o/wip/dl-for-next *)]$ git diff
>> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> index 9b8fa77b8831..ae9e9ff54826 100644
>> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
>> @@ -1314,7 +1314,7 @@ int bnxt_re_destroy_srq(struct ib_srq *ib_srq)
>> return rc;
>> }
>>
>> - if (srq->umem && !IS_ERR(srq->umem))
>> + if (srq->umem)
>> ib_umem_release(srq->umem);
>> kfree(srq);
>> atomic_dec(&rdev->srq_count);
>> @@ -1430,11 +1430,8 @@ struct ib_srq *bnxt_re_create_srq(struct ib_pd
>> *ib_pd,
>> return &srq->ib_srq;
>>
>> fail:
>> - if (udata && srq->umem && !IS_ERR(srq->umem)) {
>> + if (srq->umem)
>> ib_umem_release(srq->umem);
>> - srq->umem = NULL;
>> - }
>> -
>> kfree(srq);
>> exit:
>> return ERR_PTR(rc);
>> [dledford@haswell-e linus (k.o/wip/dl-for-next *)]$
>>
>
> This was committed in my tree for the next merge pull request.
Thanks Doug, I will review the driver code once and see if I can
supply the fix for rest of the occurrences.
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> GPG KeyID: B826A3330E572FDD
> Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters
[not found] ` <20180131064829.GR2055-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
2018-01-31 16:04 ` Doug Ledford
@ 2018-02-01 17:44 ` Jason Gunthorpe
1 sibling, 0 replies; 8+ messages in thread
From: Jason Gunthorpe @ 2018-02-01 17:44 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Dan Carpenter, Devesh Sharma, linux-rdma
On Wed, Jan 31, 2018 at 08:48:29AM +0200, Leon Romanovsky wrote:
> On Wed, Jan 31, 2018 at 09:32:38AM +0300, Dan Carpenter wrote:
> > On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:
> > > On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> > > > Hello Devesh Sharma,
> > > >
> > > > The patch 37cb11acf1f7: "RDMA/bnxt_re: Add SRQ support for Broadcom
> > > > adapters" from Jan 11, 2018, leads to the following static checker
> > > > warning:
> > > >
> > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c:1317 bnxt_re_destroy_srq()
> > > > warn: 'srq->umem' isn't an ERR_PTR
> > > >
> > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > > 1313 dev_err(rdev_to_dev(rdev), "Destroy HW SRQ failed!");
> > > > 1314 return rc;
> > > > 1315 }
> > > > 1316
> > > > 1317 if (srq->umem && !IS_ERR(srq->umem))
> > > > ^^^^^^^^^^^^^^^^
> > > > We never store error pointers to srq->umem. It's pretty consistently
> > > > checked for error pointers though so maybe that's fine. It causes a
> > > > static checker warning because error pointer confusion is a pretty
> > > > common source of bugs. Anyway, feel free to ignore if you want...
> > > Thanks for reporting Dan,
> > >
> > > Is there a way out, I want to call ib_umem_release only if it was valid.
> > > I think if ib_umem_release checks for the validity of pointer then I
> > > can get rid of this?
> > > There are other places also in bnxt_re driver where such checks are present.
> >
> > Yeah. Those places generate warnings as well, but I thought one was
> > enough. It's fine if you want to ignore the warning, no one will be
> > upset. :P
>
> Not really, we are trying to clean the subsystem from the warnings
> and driver authors who ignore such warnings simply and very effective
> sabotage it.
>
> Currently my checks print ~400 warnings for the drivers/infiniband/* +
> drivers/net/ethernet/mellanox/*
>
> So please don't increase this number, or fix the driver or fix the tool :)
Yes..
Generally speaking, in rdma, I discourage storing ERR_PTR in kalloc
memory, structures, etc. Should be 0 or a valid value, or need a
really good reason why ERR_PTR should be stored.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-02-01 17:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-30 12:45 [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters Dan Carpenter
2018-01-31 5:46 ` Devesh Sharma
[not found] ` <CANjDDBiiL1TtH5OZ9gYuiZquKu2=AV+u9nJDQ--qibOP5CXgYg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-01-31 6:32 ` Dan Carpenter
2018-01-31 6:48 ` Leon Romanovsky
[not found] ` <20180131064829.GR2055-U/DQcQFIOTAAJjI8aNfphQ@public.gmane.org>
2018-01-31 16:04 ` Doug Ledford
[not found] ` <1517414670.19117.16.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2018-01-31 21:07 ` Doug Ledford
[not found] ` <1517432861.19117.42.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2018-02-01 3:34 ` Devesh Sharma
2018-02-01 17:44 ` Jason Gunthorpe
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.