Re: [bug report] RDMA/bnxt_re: Add SRQ support for Broadcom adapters

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:

quoted

On Wed, Jan 31, 2018 at 11:16:55AM +0530, Devesh Sharma wrote:

quoted

On Tue, Jan 30, 2018 at 6:15 PM, Dan Carpenter [off-list ref] wrote:

quoted

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 [off-list ref]
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD