[PATCH 0/7] Fix UBI Block wrt. highmem

[PATCH 0/7] Fix UBI Block wrt. highmem

[Richard Weinberger]

Patch 1 changes UBIblock to use a copy of scsi_kmap_atomic_sg()
for sg list processing. This patch is meant for backporting to stable.
It makes use of kmap_atomic() and a bounce buffer because MTD/UBI IO
can sleep.

Patches 2 to 7 move scsi_kmap_atomic_sg() into lib/scatterlist.c,
convert it to kmap_local(), convert all users to it and remove the
bounce buffer from UBIblock again.

Richard Weinberger (7):
  ubi: block: Refactor sg list processing for highmem
  scatterlist: Add kmap helpers
  scsi: dc395x: Switch to kmap_sg
  scsi: esp_scsi: Switch to kmap_sg
  scsi: fdomain: Switch to kmap_sg
  ubi: block: Switch to kmap_sg
  scsi: core: Remove scsi_kmap_atomic_sg()

 drivers/mtd/ubi/block.c     | 11 +++----
 drivers/mtd/ubi/eba.c       | 50 +++++++++++++------------------
 drivers/scsi/dc395x.c       | 12 ++++----
 drivers/scsi/esp_scsi.c     |  4 +--
 drivers/scsi/fdomain.c      | 10 +++----
 drivers/scsi/scsi_lib.c     | 60 -------------------------------------
 include/linux/mtd/ubi.h     | 12 ++++----
 include/linux/scatterlist.h |  3 ++
 include/scsi/scsi_cmnd.h    |  4 ---
 lib/scatterlist.c           | 55 ++++++++++++++++++++++++++++++++++
 10 files changed, 100 insertions(+), 121 deletions(-)

-- 
2.35.3

[PATCH 1/7] ubi: block: Refactor sg list processing for highmem

[Richard Weinberger]

Currently sg_virt() is used while filling the sg list from LEB data.
This approach cannot work with highmem.

Refactor ubi_eba_read_leb_sg() to use kmap_atomic() for sg list
access.
Since kmap_atomic() disables preempt a bounce buffer is needed.
kmap_local_page() is not used to allow easy backporting of this patch
to older kernels.

The followup patches in this series will switch to kmap_sg()
and we can remove our own helper and the bounce buffer.

Cc: stable@vger.kernel.org
Fixes: 9ff08979e1742 ("UBI: Add initial support for scatter gather")
Reported-by: Stephan Wurm <stephan.wurm@a-eberle.de>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/block.c | 11 ++---
 drivers/mtd/ubi/eba.c   | 95 ++++++++++++++++++++++++++++-------------
 include/linux/mtd/ubi.h | 12 +++---
 3 files changed, 76 insertions(+), 42 deletions(-)

diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index 437c5b83ffe51..5b2e6c74ac5a8 100644
--- a/drivers/mtd/ubi/block.c
+++ b/drivers/mtd/ubi/block.c
@@ -193,13 +193,10 @@ static blk_status_t ubiblock_read(struct request *req)
 
 	blk_mq_start_request(req);
 
-	/*
-	 * It is safe to ignore the return value of blk_rq_map_sg() because
-	 * the number of sg entries is limited to UBI_MAX_SG_COUNT
-	 * and ubi_read_sg() will check that limit.
-	 */
 	ubi_sgl_init(&pdu->usgl);
-	blk_rq_map_sg(req->q, req, pdu->usgl.sg);
+	ret = blk_rq_map_sg(req->q, req, pdu->usgl.sg);
+	ubi_assert(ret > 0 && ret < UBI_MAX_SG_COUNT);
+	pdu->usgl.len = ret;
 
 	while (bytes_left) {
 		/*
@@ -212,7 +209,7 @@ static blk_status_t ubiblock_read(struct request *req)
 		ret = ubi_read_sg(dev->desc, leb, &pdu->usgl, offset, to_read);
 		if (ret < 0)
 			break;
-
+		pdu->usgl.tot_offset += to_read;
 		bytes_left -= to_read;
 		to_read = bytes_left;
 		leb += 1;
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 655ff41863e2b..82c54bf7c2067 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -31,6 +31,7 @@
 #include <linux/slab.h>
 #include <linux/crc32.h>
 #include <linux/err.h>
+#include <linux/highmem.h>
 #include "ubi.h"
 
 /* Number of physical eraseblocks reserved for atomic LEB change operation */
@@ -730,6 +731,44 @@ int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
 	return err;
 }
 
+/*
+ * Basically a copy of scsi_kmap_atomic_sg().
+ * As long scsi_kmap_atomic_sg() is not part of lib/scatterlist.c have
+ * our own version to avoid a dependency on CONFIG_SCSI.
+ */
+static void *ubi_kmap_atomic_sg(struct scatterlist *sgl, int sg_count,
+			  size_t *offset, size_t *len)
+{
+	int i;
+	size_t sg_len = 0, len_complete = 0;
+	struct scatterlist *sg;
+	struct page *page;
+
+	for_each_sg(sgl, sg, sg_count, i) {
+		len_complete = sg_len; /* Complete sg-entries */
+		sg_len += sg->length;
+		if (sg_len > *offset)
+			break;
+	}
+
+	if (WARN_ON_ONCE(i == sg_count))
+		return NULL;
+
+	/* Offset starting from the beginning of first page in this sg-entry */
+	*offset = *offset - len_complete + sg->offset;
+
+	/* Assumption: contiguous pages can be accessed as "page + i" */
+	page = nth_page(sg_page(sg), (*offset >> PAGE_SHIFT));
+	*offset &= ~PAGE_MASK;
+
+	/* Bytes in this sg-entry from *offset to the end of the page */
+	sg_len = PAGE_SIZE - *offset;
+	if (*len > sg_len)
+		*len = sg_len;
+
+	return kmap_atomic(page);
+}
+
 /**
  * ubi_eba_read_leb_sg - read data into a scatter gather list.
  * @ubi: UBI device description object
@@ -748,40 +787,38 @@ int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
 			struct ubi_sgl *sgl, int lnum, int offset, int len,
 			int check)
 {
-	int to_read;
-	int ret;
-	struct scatterlist *sg;
+	size_t map_len, map_offset, cur_offset;
+	int ret, to_read = len;
+	char *bounce_buf;
 
-	for (;;) {
-		ubi_assert(sgl->list_pos < UBI_MAX_SG_COUNT);
-		sg = &sgl->sg[sgl->list_pos];
-		if (len < sg->length - sgl->page_pos)
-			to_read = len;
-		else
-			to_read = sg->length - sgl->page_pos;
-
-		ret = ubi_eba_read_leb(ubi, vol, lnum,
-				       sg_virt(sg) + sgl->page_pos, offset,
-				       to_read, check);
-		if (ret < 0)
-			return ret;
-
-		offset += to_read;
-		len -= to_read;
-		if (!len) {
-			sgl->page_pos += to_read;
-			if (sgl->page_pos == sg->length) {
-				sgl->list_pos++;
-				sgl->page_pos = 0;
-			}
+	bounce_buf = kvmalloc(to_read, GFP_KERNEL);
+	if (!bounce_buf) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
-			break;
-		}
+	ret = ubi_eba_read_leb(ubi, vol, lnum, bounce_buf, offset, to_read, check);
+	if (ret < 0)
+		goto out;
+
+	cur_offset = 0;
+	while (to_read > 0) {
+		char *dst;
 
-		sgl->list_pos++;
-		sgl->page_pos = 0;
+		map_len = to_read;
+		map_offset = cur_offset + sgl->tot_offset;
+
+		dst = ubi_kmap_atomic_sg(sgl->sg, sgl->len, &map_offset, &map_len);
+		memcpy(dst + map_offset, bounce_buf + cur_offset, map_len);
+		kunmap_atomic(dst);
+
+		cur_offset += map_len;
+		to_read -= map_len;
 	}
 
+	ret = 0;
+out:
+	kvfree(bounce_buf);
 	return ret;
 }
 
diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
index a529347fd75b2..521e0e8b3ede3 100644
--- a/include/linux/mtd/ubi.h
+++ b/include/linux/mtd/ubi.h
@@ -115,8 +115,8 @@ struct ubi_volume_info {
 
 /**
  * struct ubi_sgl - UBI scatter gather list data structure.
- * @list_pos: current position in @sg[]
- * @page_pos: current position in @sg[@list_pos]
+ * @list_len: number of elemtns in @sg[]
+ * @tot_offset: current position the scatter gather list
  * @sg: the scatter gather list itself
  *
  * ubi_sgl is a wrapper around a scatter list which keeps track of the
@@ -124,8 +124,8 @@ struct ubi_volume_info {
  * it can be used across multiple ubi_leb_read_sg() calls.
  */
 struct ubi_sgl {
-	int list_pos;
-	int page_pos;
+	int len;
+	int tot_offset;
 	struct scatterlist sg[UBI_MAX_SG_COUNT];
 };
 
@@ -138,8 +138,8 @@ struct ubi_sgl {
  */
 static inline void ubi_sgl_init(struct ubi_sgl *usgl)
 {
-	usgl->list_pos = 0;
-	usgl->page_pos = 0;
+	usgl->len = 0;
+	usgl->tot_offset = 0;
 }
 
 /**
-- 
2.35.3

Re: [PATCH 1/7] ubi: block: Refactor sg list processing for highmem

[Christoph Hellwig]

On Thu, Aug 10, 2023 at 06:00:12PM +0200, Richard Weinberger wrote:
> Currently sg_virt() is used while filling the sg list from LEB data.
> This approach cannot work with highmem.
> 
> Refactor ubi_eba_read_leb_sg() to use kmap_atomic() for sg list
> access.
> Since kmap_atomic() disables preempt a bounce buffer is needed.
> kmap_local_page() is not used to allow easy backporting of this patch
> to older kernels.
> 
> The followup patches in this series will switch to kmap_sg()
> and we can remove our own helper and the bounce buffer.

Please just use kmap_local and avoid the bounce buffering.

Re: [PATCH 1/7] ubi: block: Refactor sg list processing for highmem

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Christoph Hellwig" <hch@infradead.org>
>> Refactor ubi_eba_read_leb_sg() to use kmap_atomic() for sg list
>> access.
>> Since kmap_atomic() disables preempt a bounce buffer is needed.
>> kmap_local_page() is not used to allow easy backporting of this patch
>> to older kernels.
>> 
>> The followup patches in this series will switch to kmap_sg()
>> and we can remove our own helper and the bounce buffer.
> 
> Please just use kmap_local and avoid the bounce buffering.

Patch 6 does this.

Thanks,
//richard

Re: [PATCH 1/7] ubi: block: Refactor sg list processing for highmem

[Christoph Hellwig]

On Thu, Aug 10, 2023 at 06:15:36PM +0200, Richard Weinberger wrote:
> >> The followup patches in this series will switch to kmap_sg()
> >> and we can remove our own helper and the bounce buffer.
> > 
> > Please just use kmap_local and avoid the bounce buffering.
> 
> Patch 6 does this.

But why add the bounce buffering first if you can avoid it from the
very beginning by just using kmap_local instead of adding a new
caller for the deprecate kmap_atomic?

Re: [PATCH 1/7] ubi: block: Refactor sg list processing for highmem

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Christoph Hellwig" <hch@infradead.org>
> An: "richard" <richard@nod.at>
> On Thu, Aug 10, 2023 at 06:15:36PM +0200, Richard Weinberger wrote:
>> >> The followup patches in this series will switch to kmap_sg()
>> >> and we can remove our own helper and the bounce buffer.
>> > 
>> > Please just use kmap_local and avoid the bounce buffering.
>> 
>> Patch 6 does this.
> 
> But why add the bounce buffering first if you can avoid it from the
> very beginning by just using kmap_local instead of adding a new
> caller for the deprecate kmap_atomic?

Because I want this fix also in all stable trees. kmap_local() is rather
new. When back porting patch 1/7, bounce buffers and kmap_atomic()
are needed anyway.
By doing this in patch 1/7 I avoid backport troubles and keep the
delta between upstream and stable trees minimal.

Thanks,
//richard

Re: [PATCH 1/7] ubi: block: Refactor sg list processing for highmem

[Christoph Hellwig]

On Thu, Aug 10, 2023 at 09:54:46PM +0200, Richard Weinberger wrote:
> > But why add the bounce buffering first if you can avoid it from the
> > very beginning by just using kmap_local instead of adding a new
> > caller for the deprecate kmap_atomic?
> 
> Because I want this fix also in all stable trees. kmap_local() is rather
> new. When back porting patch 1/7, bounce buffers and kmap_atomic()
> are needed anyway.
> By doing this in patch 1/7 I avoid backport troubles and keep the
> delta between upstream and stable trees minimal.

Just use plain kmap for the historic backports.

Re: [PATCH 1/7] ubi: block: Refactor sg list processing for highmem

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Christoph Hellwig" <hch@infradead.org>
> On Thu, Aug 10, 2023 at 09:54:46PM +0200, Richard Weinberger wrote:
>> > But why add the bounce buffering first if you can avoid it from the
>> > very beginning by just using kmap_local instead of adding a new
>> > caller for the deprecate kmap_atomic?
>> 
>> Because I want this fix also in all stable trees. kmap_local() is rather
>> new. When back porting patch 1/7, bounce buffers and kmap_atomic()
>> are needed anyway.
>> By doing this in patch 1/7 I avoid backport troubles and keep the
>> delta between upstream and stable trees minimal.
> 
> Just use plain kmap for the historic backports.

Hm, yes. For UBIblock kmap should be too expensive.

Thanks,
//richard

Re: [PATCH 1/7] ubi: block: Refactor sg list processing for highmem

[Christoph Hellwig]

On Fri, Aug 11, 2023 at 10:34:52AM +0200, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Christoph Hellwig" <hch@infradead.org>
> > On Thu, Aug 10, 2023 at 09:54:46PM +0200, Richard Weinberger wrote:
> >> > But why add the bounce buffering first if you can avoid it from the
> >> > very beginning by just using kmap_local instead of adding a new
> >> > caller for the deprecate kmap_atomic?
> >> 
> >> Because I want this fix also in all stable trees. kmap_local() is rather
> >> new. When back porting patch 1/7, bounce buffers and kmap_atomic()
> >> are needed anyway.
> >> By doing this in patch 1/7 I avoid backport troubles and keep the
> >> delta between upstream and stable trees minimal.
> > 
> > Just use plain kmap for the historic backports.
> 
> Hm, yes. For UBIblock kmap should be too expensive.

?  kmap is a no-op for !highmem.  And it will always be way cheaper
than bounce buffering for the case where you are actually fed highmem
pages.

[PATCH 2/7] scatterlist: Add kmap helpers

[Richard Weinberger]

kmap_sg() is basically scsi_kmap_atomic_sg() but uses kmap_local()
and does not enforce disabled interrupts.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 include/linux/scatterlist.h |  3 ++
 lib/scatterlist.c           | 55 +++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 77df3d7b18a61..dd25a87609491 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -692,4 +692,7 @@ bool sg_miter_skip(struct sg_mapping_iter *miter, off_t offset);
 bool sg_miter_next(struct sg_mapping_iter *miter);
 void sg_miter_stop(struct sg_mapping_iter *miter);
 
+void *kmap_sg(struct scatterlist *sgl, int sg_count, size_t *offset, size_t *len);
+void kunmap_sg(void *virt);
+
 #endif /* _LINUX_SCATTERLIST_H */
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index e86231a44c3de..7428d9461711d 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -1364,3 +1364,58 @@ ssize_t extract_iter_to_sg(struct iov_iter *iter, size_t maxsize,
 	}
 }
 EXPORT_SYMBOL_GPL(extract_iter_to_sg);
+
+/**
+ * kmap_sg - find and kmap an sg-elemnt
+ * @sgl:	scatter-gather list
+ * @sg_count:	number of segments in sg
+ * @offset:	offset in bytes into sg, on return offset into the mapped area
+ * @len:	bytes to map, on return number of bytes mapped
+ *
+ * Returns virtual address of the start of the mapped page
+ */
+void *kmap_sg(struct scatterlist *sgl, int sg_count, size_t *offset, size_t *len)
+{
+	int i;
+	size_t sg_len = 0, len_complete = 0;
+	struct scatterlist *sg;
+	struct page *page;
+
+	for_each_sg(sgl, sg, sg_count, i) {
+		len_complete = sg_len; /* Complete sg-entries */
+		sg_len += sg->length;
+		if (sg_len > *offset)
+			break;
+	}
+
+	if (WARN_ON_ONCE(i == sg_count)) {
+		pr_err("%s: Bytes in sg: %zu, requested offset %zu, elements %d\n",
+		       __func__, sg_len, *offset, sg_count);
+		return NULL;
+	}
+
+	/* Offset starting from the beginning of first page in this sg-entry */
+	*offset = *offset - len_complete + sg->offset;
+
+	/* Assumption: contiguous pages can be accessed as "page + i" */
+	page = nth_page(sg_page(sg), (*offset >> PAGE_SHIFT));
+	*offset &= ~PAGE_MASK;
+
+	/* Bytes in this sg-entry from *offset to the end of the page */
+	sg_len = PAGE_SIZE - *offset;
+	if (*len > sg_len)
+		*len = sg_len;
+
+	return kmap_local_page(page);
+}
+EXPORT_SYMBOL(kmap_sg);
+
+/**
+ * kunmap_sg - atomically unmap a virtual address, previously mapped with kmap_sg
+ * @virt:	virtual address to be unmapped
+ */
+void kunmap_sg(void *virt)
+{
+	kunmap_local(virt);
+}
+EXPORT_SYMBOL(kunmap_sg);
-- 
2.35.3

[PATCH 3/7] scsi: dc395x: Switch to kmap_sg

[Richard Weinberger]

Switch to our new helper from scatterlist lib.
No functional change, the mapped region is still used in atomic
context.
Maybe local_irq_save() can be dropped, but I don't know this driver
well enough.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/scsi/dc395x.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/dc395x.c b/drivers/scsi/dc395x.c
index c8e86f8a631eb..4a4e7a35328b9 100644
--- a/drivers/scsi/dc395x.c
+++ b/drivers/scsi/dc395x.c
@@ -2122,7 +2122,7 @@ static void data_in_phase0(struct AdapterCtlBlk *acb, struct ScsiReqBlk *srb,
 				local_irq_save(flags);
 				/* Assumption: it's inside one page as it's at most 4 bytes and
 				   I just assume it's on a 4-byte boundary */
-				base = scsi_kmap_atomic_sg(scsi_sglist(srb->cmd),
+				base = kmap_sg(scsi_sglist(srb->cmd),
 							   srb->sg_count, &offset, &len);
 				virt = base + offset;
 
@@ -2165,7 +2165,7 @@ static void data_in_phase0(struct AdapterCtlBlk *acb, struct ScsiReqBlk *srb,
 					DC395x_write8(acb, TRM_S1040_SCSI_CONFIG2, 0);
 				}
 
-				scsi_kunmap_atomic_sg(base);
+				kunmap_sg(base);
 				local_irq_restore(flags);
 			}
 			/*printk(" %08x", *(u32*)(bus_to_virt (addr))); */
@@ -2339,7 +2339,7 @@ static void data_io_transfer(struct AdapterCtlBlk *acb,
 
 				local_irq_save(flags);
 				/* Again, max 4 bytes */
-				base = scsi_kmap_atomic_sg(scsi_sglist(srb->cmd),
+				base = kmap_sg(scsi_sglist(srb->cmd),
 							   srb->sg_count, &offset, &len);
 				virt = base + offset;
 
@@ -2354,7 +2354,7 @@ static void data_io_transfer(struct AdapterCtlBlk *acb,
 					sg_subtract_one(srb);
 				}
 
-				scsi_kunmap_atomic_sg(base);
+				kunmap_sg(base);
 				local_irq_restore(flags);
 			}
 			if (srb->dcb->sync_period & WIDE_SYNC) {
@@ -3290,7 +3290,7 @@ static void srb_done(struct AdapterCtlBlk *acb, struct DeviceCtlBlk *dcb,
 		size_t offset = 0, len = sizeof(struct ScsiInqData);
 
 		local_irq_save(flags);
-		base = scsi_kmap_atomic_sg(sg, scsi_sg_count(cmd), &offset, &len);
+		base = kmap_sg(sg, scsi_sg_count(cmd), &offset, &len);
 		ptr = (struct ScsiInqData *)(base + offset);
 
 		if (!ckc_only && get_host_byte(cmd) == DID_OK
@@ -3308,7 +3308,7 @@ static void srb_done(struct AdapterCtlBlk *acb, struct DeviceCtlBlk *dcb,
 			}
 		}
 
-		scsi_kunmap_atomic_sg(base);
+		kunmap_sg(base);
 		local_irq_restore(flags);
 	}
 
-- 
2.35.3

[PATCH 4/7] scsi: esp_scsi: Switch to kmap_sg

[Richard Weinberger]

Switch to our new helper from scatterlist lib.
No functional change, the mapped region is still used in atomic
context.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/scsi/esp_scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index 97816a0e6240a..10d2fcf65f28a 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -1355,11 +1355,11 @@ static int esp_data_bytes_sent(struct esp *esp, struct esp_cmd_entry *ent,
 			struct esp_cmd_priv *p = ESP_CMD_PRIV(cmd);
 			u8 *ptr;
 
-			ptr = scsi_kmap_atomic_sg(p->cur_sg, p->num_sg,
+			ptr = kmap_sg(p->cur_sg, p->num_sg,
 						  &offset, &count);
 			if (likely(ptr)) {
 				*(ptr + offset) = bval;
-				scsi_kunmap_atomic_sg(ptr);
+				kunmap_sg(ptr);
 			}
 		}
 		bytes_sent += fifo_cnt;
-- 
2.35.3

[PATCH 5/7] scsi: fdomain: Switch to kmap_sg

[Richard Weinberger]

Switch to our new helper from scatterlist lib.
No functional change, the mapped region is still used in atomic
context.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/scsi/fdomain.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/fdomain.c b/drivers/scsi/fdomain.c
index 504c4e0c5d17a..5d58a9ec1c66a 100644
--- a/drivers/scsi/fdomain.c
+++ b/drivers/scsi/fdomain.c
@@ -223,15 +223,14 @@ static void fdomain_read_data(struct scsi_cmnd *cmd)
 
 	while ((len = inw(fd->base + REG_FIFO_COUNT)) > 0) {
 		offset = scsi_bufflen(cmd) - scsi_get_resid(cmd);
-		virt = scsi_kmap_atomic_sg(scsi_sglist(cmd), scsi_sg_count(cmd),
-					   &offset, &len);
+		virt = kmap_sg(scsi_sglist(cmd), scsi_sg_count(cmd), &offset, &len);
 		ptr = virt + offset;
 		if (len & 1)
 			*ptr++ = inb(fd->base + REG_FIFO);
 		if (len > 1)
 			insw(fd->base + REG_FIFO, ptr, len >> 1);
 		scsi_set_resid(cmd, scsi_get_resid(cmd) - len);
-		scsi_kunmap_atomic_sg(virt);
+		kunmap_sg(virt);
 	}
 }
 
@@ -250,15 +249,14 @@ static void fdomain_write_data(struct scsi_cmnd *cmd)
 			if (len == 0)
 				break;
 		}
-		virt = scsi_kmap_atomic_sg(scsi_sglist(cmd), scsi_sg_count(cmd),
-					   &offset, &len);
+		virt = kmap_sg(scsi_sglist(cmd), scsi_sg_count(cmd), &offset, &len);
 		ptr = virt + offset;
 		if (len & 1)
 			outb(*ptr++, fd->base + REG_FIFO);
 		if (len > 1)
 			outsw(fd->base + REG_FIFO, ptr, len >> 1);
 		scsi_set_resid(cmd, scsi_get_resid(cmd) - len);
-		scsi_kunmap_atomic_sg(virt);
+		kunmap_sg(virt);
 	}
 }
 
-- 
2.35.3

[PATCH 6/7] ubi: block: Switch to kmap_sg

[Richard Weinberger]

Remove our copy of scsi_kmap_atomic_sg() and use the new helper
kmap_sg(). Since it is based on kmap_local() we can remove the
bounce buffer and perform IO while we hold the mapping.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/eba.c | 59 +++++--------------------------------------
 1 file changed, 6 insertions(+), 53 deletions(-)

diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 82c54bf7c2067..650eacab27f03 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -731,44 +731,6 @@ int ubi_eba_read_leb(struct ubi_device *ubi, struct ubi_volume *vol, int lnum,
 	return err;
 }
 
-/*
- * Basically a copy of scsi_kmap_atomic_sg().
- * As long scsi_kmap_atomic_sg() is not part of lib/scatterlist.c have
- * our own version to avoid a dependency on CONFIG_SCSI.
- */
-static void *ubi_kmap_atomic_sg(struct scatterlist *sgl, int sg_count,
-			  size_t *offset, size_t *len)
-{
-	int i;
-	size_t sg_len = 0, len_complete = 0;
-	struct scatterlist *sg;
-	struct page *page;
-
-	for_each_sg(sgl, sg, sg_count, i) {
-		len_complete = sg_len; /* Complete sg-entries */
-		sg_len += sg->length;
-		if (sg_len > *offset)
-			break;
-	}
-
-	if (WARN_ON_ONCE(i == sg_count))
-		return NULL;
-
-	/* Offset starting from the beginning of first page in this sg-entry */
-	*offset = *offset - len_complete + sg->offset;
-
-	/* Assumption: contiguous pages can be accessed as "page + i" */
-	page = nth_page(sg_page(sg), (*offset >> PAGE_SHIFT));
-	*offset &= ~PAGE_MASK;
-
-	/* Bytes in this sg-entry from *offset to the end of the page */
-	sg_len = PAGE_SIZE - *offset;
-	if (*len > sg_len)
-		*len = sg_len;
-
-	return kmap_atomic(page);
-}
-
 /**
  * ubi_eba_read_leb_sg - read data into a scatter gather list.
  * @ubi: UBI device description object
@@ -789,17 +751,6 @@ int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
 {
 	size_t map_len, map_offset, cur_offset;
 	int ret, to_read = len;
-	char *bounce_buf;
-
-	bounce_buf = kvmalloc(to_read, GFP_KERNEL);
-	if (!bounce_buf) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	ret = ubi_eba_read_leb(ubi, vol, lnum, bounce_buf, offset, to_read, check);
-	if (ret < 0)
-		goto out;
 
 	cur_offset = 0;
 	while (to_read > 0) {
@@ -808,9 +759,12 @@ int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
 		map_len = to_read;
 		map_offset = cur_offset + sgl->tot_offset;
 
-		dst = ubi_kmap_atomic_sg(sgl->sg, sgl->len, &map_offset, &map_len);
-		memcpy(dst + map_offset, bounce_buf + cur_offset, map_len);
-		kunmap_atomic(dst);
+		dst = kmap_sg(sgl->sg, sgl->len, &map_offset, &map_len);
+		ret = ubi_eba_read_leb(ubi, vol, lnum, dst + map_offset, offset + cur_offset,
+				       map_len, check);
+		if (ret < 0)
+			goto out;
+		kunmap_sg(dst);
 
 		cur_offset += map_len;
 		to_read -= map_len;
@@ -818,7 +772,6 @@ int ubi_eba_read_leb_sg(struct ubi_device *ubi, struct ubi_volume *vol,
 
 	ret = 0;
 out:
-	kvfree(bounce_buf);
 	return ret;
 }
 
-- 
2.35.3

[PATCH 7/7] scsi: core: Remove scsi_kmap_atomic_sg()

[Richard Weinberger]

All users have been migrated to use kmap_sg(), let's kill
this helper.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/scsi/scsi_lib.c  | 60 ----------------------------------------
 include/scsi/scsi_cmnd.h |  4 ---
 2 files changed, 64 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ad9afae49544a..70469502aeb76 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3001,66 +3001,6 @@ scsi_host_unblock(struct Scsi_Host *shost, int new_state)
 }
 EXPORT_SYMBOL_GPL(scsi_host_unblock);
 
-/**
- * scsi_kmap_atomic_sg - find and atomically map an sg-elemnt
- * @sgl:	scatter-gather list
- * @sg_count:	number of segments in sg
- * @offset:	offset in bytes into sg, on return offset into the mapped area
- * @len:	bytes to map, on return number of bytes mapped
- *
- * Returns virtual address of the start of the mapped page
- */
-void *scsi_kmap_atomic_sg(struct scatterlist *sgl, int sg_count,
-			  size_t *offset, size_t *len)
-{
-	int i;
-	size_t sg_len = 0, len_complete = 0;
-	struct scatterlist *sg;
-	struct page *page;
-
-	WARN_ON(!irqs_disabled());
-
-	for_each_sg(sgl, sg, sg_count, i) {
-		len_complete = sg_len; /* Complete sg-entries */
-		sg_len += sg->length;
-		if (sg_len > *offset)
-			break;
-	}
-
-	if (unlikely(i == sg_count)) {
-		printk(KERN_ERR "%s: Bytes in sg: %zu, requested offset %zu, "
-			"elements %d\n",
-		       __func__, sg_len, *offset, sg_count);
-		WARN_ON(1);
-		return NULL;
-	}
-
-	/* Offset starting from the beginning of first page in this sg-entry */
-	*offset = *offset - len_complete + sg->offset;
-
-	/* Assumption: contiguous pages can be accessed as "page + i" */
-	page = nth_page(sg_page(sg), (*offset >> PAGE_SHIFT));
-	*offset &= ~PAGE_MASK;
-
-	/* Bytes in this sg-entry from *offset to the end of the page */
-	sg_len = PAGE_SIZE - *offset;
-	if (*len > sg_len)
-		*len = sg_len;
-
-	return kmap_atomic(page);
-}
-EXPORT_SYMBOL(scsi_kmap_atomic_sg);
-
-/**
- * scsi_kunmap_atomic_sg - atomically unmap a virtual address, previously mapped with scsi_kmap_atomic_sg
- * @virt:	virtual address to be unmapped
- */
-void scsi_kunmap_atomic_sg(void *virt)
-{
-	kunmap_atomic(virt);
-}
-EXPORT_SYMBOL(scsi_kunmap_atomic_sg);
-
 void sdev_disable_disk_events(struct scsi_device *sdev)
 {
 	atomic_inc(&sdev->disk_events_disable_depth);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 526def14e7fb7..919ac97229481 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -163,10 +163,6 @@ void scsi_done_direct(struct scsi_cmnd *cmd);
 
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 
-extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
-				 size_t *offset, size_t *len);
-extern void scsi_kunmap_atomic_sg(void *virt);
-
 blk_status_t scsi_alloc_sgtables(struct scsi_cmnd *cmd);
 void scsi_free_sgtables(struct scsi_cmnd *cmd);
 
-- 
2.35.3

Re: [PATCH 0/7] Fix UBI Block wrt. highmem

[Miquel Raynal]

Hi Richard,

richard@nod.at wrote on Thu, 10 Aug 2023 18:00:11 +0200:

> Patch 1 changes UBIblock to use a copy of scsi_kmap_atomic_sg()
> for sg list processing. This patch is meant for backporting to stable.
> It makes use of kmap_atomic() and a bounce buffer because MTD/UBI IO
> can sleep.
> 
> Patches 2 to 7 move scsi_kmap_atomic_sg() into lib/scatterlist.c,
> convert it to kmap_local(), convert all users to it and remove the
> bounce buffer from UBIblock again.

Both the idea and the implementation look nice, I don't feel skilled
enough for sending Reviewed-by here, but it seems okay at a first
glance.

Well done Richard :-)

Miquèl