| Message ID | 20221113010823.6436-6-guoqing.jiang@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Misc changes for rtrs | expand |
On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > We should check with nr_sgt, also the only successful case is that > all sg elements are mapped, so make it explict. > > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > --- > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > index 88eae0dcf87f..f3bf5bbb4377 100644 > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > @@ -622,8 +622,8 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path) > } > nr = ib_map_mr_sg(mr, sgt->sgl, nr_sgt, > NULL, max_chunk_size); > - if (nr < 0 || nr < sgt->nents) { > - err = nr < 0 ? nr : -EINVAL; > + if (nr != nr_sgt) { > + err = -EINVAL; but with this, the initial errno are lost, we only return EINVAL > goto dereg_mr; > } > > -- > 2.31.1 >
On 11/15/22 7:46 PM, Jinpu Wang wrote: > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: >> We should check with nr_sgt, also the only successful case is that >> all sg elements are mapped, so make it explict. >> >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> >> --- >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c >> index 88eae0dcf87f..f3bf5bbb4377 100644 >> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c >> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c >> @@ -622,8 +622,8 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path) >> } >> nr = ib_map_mr_sg(mr, sgt->sgl, nr_sgt, >> NULL, max_chunk_size); >> - if (nr < 0 || nr < sgt->nents) { >> - err = nr < 0 ? nr : -EINVAL; >> + if (nr != nr_sgt) { >> + err = -EINVAL; > but with this, the initial errno are lost, we only return EINVAL OK, assume you mean 'nr' here, I can change it back as iser_memory did. But seems the only negative value returned from ib_map_mr_sg is also "-EINVAL" from ib_sg_to_pages after go through hw drivers. BTW, I looked all call sites of ib_map_mr_sg, seems they have different kind of checking. 1. if (ret < 0 || ret < nents) 2. if (ret < nents) or if (ret < 0) 3. if (ret != nents) Thanks, Guoqing
On Wed, Nov 16, 2022 at 2:13 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > > > > On 11/15/22 7:46 PM, Jinpu Wang wrote: > > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote: > >> We should check with nr_sgt, also the only successful case is that > >> all sg elements are mapped, so make it explict. > >> > >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> > >> --- > >> drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > >> index 88eae0dcf87f..f3bf5bbb4377 100644 > >> --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c > >> +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c > >> @@ -622,8 +622,8 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path) > >> } > >> nr = ib_map_mr_sg(mr, sgt->sgl, nr_sgt, > >> NULL, max_chunk_size); > >> - if (nr < 0 || nr < sgt->nents) { > >> - err = nr < 0 ? nr : -EINVAL; > >> + if (nr != nr_sgt) { > >> + err = -EINVAL; > > but with this, the initial errno are lost, we only return EINVAL > > OK, assume you mean 'nr' here, I can change it back as iser_memory did. > But seems the only negative value returned from ib_map_mr_sg is also > "-EINVAL" from ib_sg_to_pages after go through hw drivers. Yes, I meant nr. there is also cases " if (unlikely(!mr->device->map_mr_sg)) return -ENOSYS; " > > BTW, I looked all call sites of ib_map_mr_sg, seems they have different > kind of checking. > > 1. if (ret < 0 || ret < nents) > > 2. if (ret < nents) or if (ret < 0) > > 3. if (ret != nents) Yes, I also checked them. :) > > Thanks, > Guoqing
diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c index 88eae0dcf87f..f3bf5bbb4377 100644 --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c @@ -622,8 +622,8 @@ static int map_cont_bufs(struct rtrs_srv_path *srv_path) } nr = ib_map_mr_sg(mr, sgt->sgl, nr_sgt, NULL, max_chunk_size); - if (nr < 0 || nr < sgt->nents) { - err = nr < 0 ? nr : -EINVAL; + if (nr != nr_sgt) { + err = -EINVAL; goto dereg_mr; }
We should check with nr_sgt, also the only successful case is that all sg elements are mapped, so make it explict. Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev> --- drivers/infiniband/ulp/rtrs/rtrs-srv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)