On Wed, Feb 20, 2019 at 03:59:14PM +0000, Jason Gunthorpe wrote:
> On Wed, Feb 20, 2019 at 05:13:21PM +0200, Leon Romanovsky wrote:
> > On Tue, Feb 19, 2019 at 06:42:10PM +0000, Jason Gunthorpe wrote:
> > > On Tue, Feb 19, 2019 at 07:02:25AM -0800, Bart Van Assche wrote:
> > > > On 2/19/19 5:05 AM, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > >
> > > > > The error reported below is not possible in real life because
> > > > > "requestor != NULL" means that "qp != NULL" too. However smatch
> > > > > can't know it without extra help.
> > > > >
> > > > > drivers/infiniband/hw/mlx5/odp.c:1254 mlx5_ib_mr_wqe_pfault_handler()
> > > > > error: we previously assumed 'qp' could be null (see line 1230)
> > > > >
> > > > > Fixes: 08100fad5cac ("IB/mlx5: Add ODP SRQ support")
> > > > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > drivers/infiniband/hw/mlx5/odp.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
> > > > > index d828c20af38c..5e585cf5ee98 100644
> > > > > +++ b/drivers/infiniband/hw/mlx5/odp.c
> > > > > @@ -1259,7 +1259,7 @@ static void mlx5_ib_mr_wqe_pfault_handler(struct mlx5_ib_dev *dev,
> > > > > }
> > > > > wqe = buffer;
> > > > > - if (requestor)
> > > > > + if (requestor && qp)
> > > > > ret = mlx5_ib_mr_initiator_pfault_handler(dev, pfault, qp,
> > > > > &wqe, &wqe_end,
> > > > > bytes_copied);
> > > >
> > > > This kind of change makes the code confusing to human readers. Have you
> > > > considered to add a BUG_ON(!qp) or WARN_ON(!qp) with a comment that refers
> > > > to sparse instead?
> > >
> > > Just don't be so unnecessarily clever with the logic flow (and maybe
> > > put the if block in a function):
> >
> > It doesn't solve complain that tools believe that QP can be NULL, you
> > are just delaying the error. In our case, you should tell to the tool
> > that "requester != NULL and qp != NULL", but in your solution only
> > first part is expressed.
>
> Are you sure? That seems like an overly agressive static check.
No, I'm not sure
>
> if (bar)
> foo(qp)
>
> if (qp)
> blah(qp)
>
> Should not produce an error.. Dan?
>
> In any event, my remark still holds - but you need to fold the
> redundent switch statement in too:
ok
Thanks
Attachment:
signature.asc
Description: PGP signature
