On Tue, Feb 05, 2019 at 08:33:44AM -0800, Bart Van Assche wrote:
> On Mon, 2019-02-04 at 19:50 +0200, Max Gurtovoy wrote:
> > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> > index 31d91538bbf4..fa56343aeb98 100644
> > +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> > @@ -1511,15 +1511,15 @@ static void srp_reg_mr_err_done(struct ib_cq *cq, struct ib_wc *wc)
> > }
> >
> > /*
> > - * Map up to sg_nents elements of state->sg where *sg_offset_p is the offset
> > - * where to start in the first element. If sg_offset_p != NULL then
> > - * *sg_offset_p is updated to the offset in state->sg[retval] of the first
> > + * Map up to ib_sg->dma_nents elements of state->sg where ib_sg->offset
> > + * is the offset where to start in the first element. If ib_sg->offset != 0 then
> > + * ib_sg->offset is updated to the offset in state->sg[retval] of the first
> > * byte that has not yet been mapped.
> > */
>
> The above change modifies a correct comment into an incorrect comment. Since
> this patch guarantees that &ib_sg->offset != NULL I think the references to
> &ib_sg->offset == NULL should be left out.
>
> > if (unlikely(n < 0)) {
> > srp_fr_pool_put(ch->fr_pool, &desc, 1);
> > pr_debug("%s: ib_map_mr_sg(%d, %d) returned %d.\n",
> > - dev_name(&req->scmnd->device->sdev_gendev), sg_nents,
> > - sg_offset_p ? *sg_offset_p : -1, n);
> > + dev_name(&req->scmnd->device->sdev_gendev),
> > + ib_sg->dma_nents, ib_sg->offset ? : -1, n);
>
> Same mistake here: you are changing sg_offset_p ? *sg_offset_p : -1 into the
> following: *sg_offset_p ? *sg_offset_p : -1. These expressions are not
> equivalent!
>
> >
> > diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > index 9044356e3bb7..f5d0df8d9fcb 100644
> > +++ b/include/rdma/ib_verbs.h
> > @@ -2157,6 +2157,18 @@ struct ib_counters_read_attr {
> > u32 flags; /* use enum ib_read_counters_flags */
> > };
> >
> > +/*
> > + * struct ib_scatterlist - Mapped scatterlist for RDMA operations
> > + * @sg: dma mapped sg list
> > + * @dma_nents: returned by dma_map_sg
> > + * @offset: start offset in bytes into the first sg element
> > + */
> > +struct ib_scatterlist {
> > + struct scatterlist *sg;
> > + int dma_nents;
> > + unsigned int offset;
> > +};
>
> There is nothing in this data structure that is RDMA specific. Are you sure
> the name "ib_scatterlist" is a good choice?
Why do we need an offset anyhow? What is that?
Is that actually the MR virtual base address?
Jason
