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):
diff --git a/drivers/infiniband/hw/mlx5/odp.c b/drivers/infiniband/hw/mlx5/odp.c
index 335fd0c6ea2a24..9ce32370cf8e32 100644
--- a/drivers/infiniband/hw/mlx5/odp.c
+++ b/drivers/infiniband/hw/mlx5/odp.c
@@ -1227,42 +1227,30 @@ static void mlx5_ib_mr_wqe_pfault_handler(struct mlx5_ib_dev *dev,
goto resolve_page_fault;
}
- if (qp) {
- if (requestor) {
- ret = mlx5_ib_read_user_wqe_sq(qp, wqe_index,
- buffer, PAGE_SIZE,
- &bytes_copied);
- } else {
- ret = mlx5_ib_read_user_wqe_rq(qp, wqe_index,
- buffer, PAGE_SIZE,
- &bytes_copied);
- }
+ wqe = buffer;
+ if (requestor) {
+ ret = mlx5_ib_read_user_wqe_sq(qp, wqe_index, buffer, PAGE_SIZE,
+ &bytes_copied);
+ if (ret)
+ goto err_wqe;
+ ret = mlx5_ib_mr_initiator_pfault_handler(
+ dev, pfault, qp, &wqe, &wqe_end, bytes_copied);
+ } else if (qp) {
+ ret = mlx5_ib_read_user_wqe_rq(qp, wqe_index, buffer, PAGE_SIZE,
+ &bytes_copied);
+ if (ret)
+ goto err_wqe;
+ ret = mlx5_ib_mr_responder_pfault_handler_rq(
+ dev, qp, wqe, &wqe_end, bytes_copied);
} else {
- ret = mlx5_ib_read_user_wqe_srq(srq, wqe_index,
- buffer, PAGE_SIZE,
- &bytes_copied);
- }
-
- if (ret) {
- mlx5_ib_err(dev, "Failed reading a WQE following page fault, error=%d, wqe_index=%x, qpn=%x\n",
- ret, wqe_index, pfault->token);
- goto resolve_page_fault;
+ ret = mlx5_ib_read_user_wqe_srq(srq, wqe_index, buffer,
+ PAGE_SIZE, &bytes_copied);
+ if (ret)
+ goto err_wqe;
+ ret = mlx5_ib_mr_responder_pfault_handler_srq(
+ dev, srq, &wqe, &wqe_end, bytes_copied);
}
- wqe = buffer;
- if (requestor)
- ret = mlx5_ib_mr_initiator_pfault_handler(dev, pfault, qp,
- &wqe, &wqe_end,
- bytes_copied);
- else if (qp)
- ret = mlx5_ib_mr_responder_pfault_handler_rq(dev, qp,
- wqe, &wqe_end,
- bytes_copied);
- else
- ret = mlx5_ib_mr_responder_pfault_handler_srq(dev, srq,
- &wqe, &wqe_end,
- bytes_copied);
-
if (ret < 0)
goto resolve_page_fault;
@@ -1282,6 +1270,11 @@ static void mlx5_ib_mr_wqe_pfault_handler(struct mlx5_ib_dev *dev,
}
resume_with_error = 0;
+err_wqe:
+ mlx5_ib_err(
+ dev,
+ "Failed reading a WQE following page fault, error=%d, wqe_index=%x, qpn=%x\n",
+ ret, wqe_index, pfault->token);
resolve_page_fault:
mlx5_ib_page_fault_resume(dev, pfault, resume_with_error);
mlx5_ib_dbg(dev, "PAGE FAULT completed. QP 0x%x resume_with_error=%d, type: 0x%x\n",
