On Thu, Jan 03, 2019 at 03:18:19PM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 24, 2018 at 04:32:22PM -0600, Shiraz Saleem wrote:
> > - for_each_sg(umem->sg_head.sgl, sg, umem->nmap, entry) {
> > - pages = sg_dma_len(sg) >> PAGE_SHIFT;
> > - for (i = 0; i < pages; i++) {
> > - paddr = sg_dma_address(sg) + (i << PAGE_SHIFT);
> > - if (pbl_tbl == pbl_tbl_orig)
> > - *pbl_tbl++ = paddr & ~page_mask;
> > - else if ((paddr & page_mask) == 0)
> > - *pbl_tbl++ = paddr;
> > - }
> > + struct sg_page_iter sg_iter;
> > +
> > + for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->nmap, 0) {
> > + paddr = sg_page_iter_dma_address(&sg_iter);
>
> Actually, I'm looking at this again, and thinking this looks
> problematic..
>
> for_each_sg_page iterates over the original SGL, it does not iterate
> over the DMA mapped SGL (ie it does not use sg_dma_len()) - so this is
> not an entirely equivalent transformation.
>
> I think you need to add a for_each_sg_dma_page() to make this work
> correctly? The only difference would be to use sg_dma_len instead of
> length?
>
> And I wonder how many of these places in drivers work properly if
> PAGE_SIZE != 4k :(
>
> CH: Does sg_page_iter_dma_address() even make any sense??
No, it doesn't and should never have been edited. It's one of these
typical scatterlist abuses in the media code :(
