RDMA/bnxt_re: Add SRQ support for Broadcom adapters

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

Quoted 15 lines · Dan Carpenter

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))

Quoted 5 lines

                                  ^^^^^^^^^^^^^^^^

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.

Quoted 10 lines

  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.

Quoted 23 lines · Devesh Sharma

Quoted 15 lines · Dan Carpenter

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))

Quoted 5 lines

                                  ^^^^^^^^^^^^^^^^

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,

Quoted 4 lines

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

Quoted 25 lines · Dan Carpenter

Quoted 24 lines · Devesh Sharma

Quoted 15 lines · Dan Carpenter

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))

Quoted 5 lines

                                  ^^^^^^^^^^^^^^^^

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,

Quoted 8 lines

Quoted 4 lines

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

Quoted 39 lines · Leon Romanovsky

Quoted 25 lines · Dan Carpenter

Quoted 24 lines · Devesh Sharma

Quoted 15 lines · Dan Carpenter

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))

Quoted 5 lines

                                  ^^^^^^^^^^^^^^^^

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,

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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

drivers/infiniband/hw/bnxt_re/ib_verbs.c +2 -5

index 9b8fa77b8831..ae9e9ff54826 100644
@@ -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 *)]$

Quoted 68 lines · Doug Ledford

Quoted 39 lines · Leon Romanovsky

Quoted 25 lines · Dan Carpenter

Quoted 24 lines · Devesh Sharma

Quoted 15 lines · Dan Carpenter

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))

Quoted 5 lines

                                  ^^^^^^^^^^^^^^^^

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,

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

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

drivers/infiniband/hw/bnxt_re/ib_verbs.c +1 -1

index 9b8fa77b8831..ae9e9ff54826 100644
@@ -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;
 

Quoted 10 lines

drivers/infiniband/hw/bnxt_re/ib_verbs.c +1 -4

 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);

Quoted 2 lines

[dledford@haswell-e linus (k.o/wip/dl-for-next *)]$

This was committed in my tree for the next merge pull request.

--
Doug Ledford <[email protected]>

    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

Quoted 24 lines · Dan Carpenter

Quoted 23 lines · Devesh Sharma

Quoted 15 lines · Dan Carpenter

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))

Quoted 5 lines

                                  ^^^^^^^^^^^^^^^^

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,

Quoted 8 lines

Quoted 4 lines

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 :)