On 2/4/21 8:00 PM, Jason Gunthorpe wrote:

On Wed, Feb 03, 2021 at 04:15:53PM -0800, John Hubbard wrote:

quoted

quoted

diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
index 2dde99a9ba07..ea4ebb3261d9 100644
+++ b/drivers/infiniband/core/umem.c

@@ -47,17 +47,17 @@
  static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
  {
-	struct sg_page_iter sg_iter;
-	struct page *page;
+	bool make_dirty = umem->writable && dirty;
+	struct scatterlist *sg;
+	int i;

Maybe unsigned int is better, so as to perfectly match the scatterlist.length.

Yes please

Fixed in v2.

quoted

quoted

  	if (umem->nmap > 0)
  		ib_dma_unmap_sg(dev, umem->sg_head.sgl, umem->sg_nents,
  				DMA_BIDIRECTIONAL);
-	for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
-		page = sg_page_iter_page(&sg_iter);
-		unpin_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
-	}
+	for_each_sg(umem->sg_head.sgl, sg, umem->nmap, i)

The change from umem->sg_nents to umem->nmap looks OK, although we should get
IB people to verify that there is not some odd bug or reason to leave it as is.

No, nmap wouldn't be right here. nmap is the number of dma mapped SGLs
in the list and should only be used by things doing sg_dma* stuff.

umem->sg_nents is the number of CPU SGL entries and is the correct
thing here.

And this was fixed in v2 as well.