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

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- 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 --]