On Wed, Feb 13, 2019 at 10:36 PM Saleem, Shiraz <shiraz.saleem@xxxxxxxxx> wrote:
>
> >Subject: Re: [rdma-next 01/12] RDMA/bnxt_re: Use for_each_sg_dma_page
> >iterator on umem SGL
> >
> >On Wed, Feb 13, 2019 at 01:12:37PM +0200, Gal Pressman wrote:
> >> On 11-Feb-19 17:24, Shiraz Saleem wrote:
> >> > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_res.c
> >> > b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> >> > index c8502c2..c8478c1 100644
> >> > +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> >> > @@ -85,7 +85,7 @@ static void __free_pbl(struct pci_dev *pdev,
> >> > struct bnxt_qplib_pbl *pbl, static int __alloc_pbl(struct pci_dev *pdev, struct
> >bnxt_qplib_pbl *pbl,
> >> > struct scatterlist *sghead, u32 pages, u32 pg_size) {
> >> > - struct scatterlist *sg;
> >> > + struct sg_dma_page_iter sg_iter;
> >> > bool is_umem = false;
> >> > int i;
> >> >
> >> > @@ -116,12 +116,13 @@ static int __alloc_pbl(struct pci_dev *pdev, struct
> >bnxt_qplib_pbl *pbl,
> >> > } else {
> >> > i = 0;
> >> > is_umem = true;
> >> > - for_each_sg(sghead, sg, pages, i) {
> >> > - pbl->pg_map_arr[i] = sg_dma_address(sg);
> >> > - pbl->pg_arr[i] = sg_virt(sg);
> >> > + for_each_sg_dma_page(sghead, &sg_iter, pages, 0) {
> >> > + pbl->pg_map_arr[i] =
> >sg_page_iter_dma_address(&sg_iter);
> >> > + pbl->pg_arr[i] = NULL;
> >> if (!pbl->pg_arr[i])
> >>
> >> Surely something went wrong here?
> >
> >That if does look wrong.
> >
> >I recall Shiraz removed the pg_arr as it wasn't being used, so maybe the if should
> >go to?
>
> Thanks Gal for catching this. Yes the reason pg_arr is set to NULL here is because it
> appears used only in !umem case. But then the following if check is a bug. Duh!
>
> bnxt_re folks - is this acceptable?
>
> --- a/drivers/infiniband/hw/bnxt_re/qplib_res.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_res.c
> @@ -85,7 +85,7 @@ static void __free_pbl(struct pci_dev *pdev, struct bnxt_qplib_pbl *pbl,
> static int __alloc_pbl(struct pci_dev *pdev, struct bnxt_qplib_pbl *pbl,
> struct scatterlist *sghead, u32 pages, u32 pg_size)
> {
> - struct scatterlist *sg;
> + struct sg_dma_page_iter sg_iter;
> bool is_umem = false;
> int i;
>
> @@ -116,12 +116,9 @@ static int __alloc_pbl(struct pci_dev *pdev, struct bnxt_qplib_pbl *pbl,
> } else {
> i = 0;
> is_umem = true;
> - for_each_sg(sghead, sg, pages, i) {
> - pbl->pg_map_arr[i] = sg_dma_address(sg);
> - pbl->pg_arr[i] = sg_virt(sg);
> - if (!pbl->pg_arr[i])
> - goto fail;
> -
> + for_each_sg_dma_page(sghead, &sg_iter, pages, 0) {
> + pbl->pg_map_arr[i] = sg_page_iter_dma_address(&sg_iter);
> + i++;
> pbl->pg_count++;
The thing is we need the virtual address for future reference in this
path. If we do not have virt-addr populated it breaks!
> }
> }
>
> Shiraz
>
