On Wed, Feb 13, 2019 at 9:42 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> On Wed, Feb 13, 2019 at 09:31:39PM +0530, Selvin Xavier wrote:
> > On Wed, Feb 13, 2019 at 4:42 PM Gal Pressman <galpress@xxxxxxxxxx> 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?
> > Thanks for the review. Yes, its breaks the functionality. pg_arr needs
> > to hold the virtual address, which would be used by the caller of
> > __alloc_pbl. Also, the current code always goes to the fail statement
> > because of the !pbl->pg_arr[i] check .
> >
> > I feel the following change is required.
> > pbl->pg_arr[i] = page_address(sg_page_iter_page(&sg_iter.base));
>
> Drivers cannot iterate over DMA and struct pages like this, it is not
> allowed by the DMA API.
And what Shiraz proposing is the way to go?
>
> Jason
