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.
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:
if (pfault->type & MLX5_PFAULT_REQUESTOR)
struct mlx5_ib_qp *qp = qp = res_to_qp(res);
[..]
else if (res->res == MLX5_RES_QP)
struct mlx5_ib_qp *qp = qp = res_to_qp(res);
[..]
else if (res->res == MLX5_RES_SRQ || res->res == MLx5_RES_XSQ)
struct mlx5_ib_srq *srq =res_to_srq(res);
[..]
else
ml5_ib_err(...)
Jason
