[PATCH 00/10] sg buffer copy helper functions

[PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

This patchset adds new two helper functions to copy data between an SG
table and liner buffer, and converts severl LLDs to use them.

The new APIs are used mainly in the code to spoof SCSI commands
(INQUIRY, READ CAPACITY, MODE SENSE etc), that is, LLDs build a fake
reposense and copy it to the sg list in scsi_cmnd struct. Several LLDs
have similar functions for such code. This patchset removes such
duplication.

Another reason to do this work is that because we relaxed the default
alignment requirements (from 511 to 3) post 2.6.24, the above commands
might come with multiple scatter gather entries but several LLDs make
the assumption that such commands come with only one sg entries and
can't handle multiple sg entries. The new APIs can handle multiple sg
entries so LLDs don't need to care about anything.

The first patch adds the new APIs to lib/scatterlist.c and the rest
are for SCSI. I like to push the whole patchset via scsi-misc (since
it's easier).

This is against scsi-misc (a6680f71ca27ea78c4c4e577076aecb9ace476f1).

 arch/ia64/hp/sim/simscsi.c    |   23 ++--------
 drivers/scsi/3w-9xxx.c        |   21 ++++-----
 drivers/scsi/3w-xxxx.c        |   12 +----
 drivers/scsi/aacraid/aachba.c |   49 ++++++++--------------
 drivers/scsi/ips.c            |   50 ++++-------------------
 drivers/scsi/ps3rom.c         |   92 ++++------------------------------------
 drivers/scsi/scsi_debug.c     |   79 ++++++-----------------------------
 drivers/scsi/stex.c           |   66 ++++-------------------------
 include/linux/scatterlist.h   |    5 ++
 include/scsi/scsi_cmnd.h      |   14 ++++++
 lib/scatterlist.c             |   90 ++++++++++++++++++++++++++++++++++++++++
 11 files changed, 182 insertions(+), 319 deletions(-)

[PATCH 01/10] block: add sg buffer copy helper functions

[FUJITA Tomonori]

This patch adds new two helper functions to copy data between an SG
table and liner buffer.

- sg_copy_from_buffer copies data from liner buffer to an SG table
- sg_copy_to_buffer copies data from an SG table to liner buffer

sg_copy_from_buffer always call flush_kernel_dcache_page. It's not
necessary for everyone but it's a no-op on most architectures and in
general the API is not used in performance critical path.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Jens Axboe <jens.axboe@oracle.com>
---
 include/linux/scatterlist.h |    5 ++
 lib/scatterlist.c           |   90 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 0 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index a3d567a..ab6c672 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -213,6 +213,11 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
 		     sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
 
+int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+			void *buf, int buflen);
+int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+		      void *buf, int buflen);
+
 /*
  * Maximum number of entries that will be allocated in one piece, if
  * a list larger than this is required then chaining will be utilized.
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index acca490..63e9c35 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -8,6 +8,7 @@
  */
 #include <linux/module.h>
 #include <linux/scatterlist.h>
+#include <linux/highmem.h>
 
 /**
  * sg_next - return the next scatterlist entry in a list
@@ -292,3 +293,92 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
 	return ret;
 }
 EXPORT_SYMBOL(sg_alloc_table);
+
+static int sg_copy_buffer(struct scatterlist *sgl, unsigned int nents,
+			  void *buf, int buflen, int to_buf)
+{
+	struct scatterlist *sg;
+	unsigned long buf_off = 0;
+	int i;
+
+	WARN_ON(!irqs_disabled());
+
+	for_each_sg(sgl, sg, nents, i) {
+		struct page *page;
+		int n = 0;
+		unsigned int sg_off = sg->offset;
+		unsigned int sg_copy = sg->length;
+
+		if (sg_copy > buflen)
+			sg_copy = buflen;
+		buflen -= sg_copy;
+
+		while (sg_copy > 0) {
+			unsigned int page_copy;
+			void *p;
+
+			page_copy = PAGE_SIZE - sg_off;
+			if (page_copy > sg_copy)
+				page_copy = sg_copy;
+
+			page = nth_page(sg_page(sg), n);
+			p = kmap_atomic(page, KM_BIO_SRC_IRQ);
+
+			if (to_buf)
+				memcpy(buf + buf_off, p + sg_off, page_copy);
+			else {
+				memcpy(p + sg_off, buf + buf_off, page_copy);
+				flush_kernel_dcache_page(page);
+			}
+
+			kunmap_atomic(p, KM_BIO_SRC_IRQ);
+
+			buf_off += page_copy;
+			sg_off += page_copy;
+			if (sg_off == PAGE_SIZE) {
+				sg_off = 0;
+				n++;
+			}
+			sg_copy -= page_copy;
+		}
+
+		if (!buflen)
+			break;
+	}
+
+	return buf_off;
+}
+
+/**
+ * sg_copy_from_buffer - Copy from liner buffer to an SG table
+ * @sgl:		 The SG table
+ * @nents:		 Number of SG entries
+ * @buf:		 Where to copy from
+ * @buflen:		 The number of bytes to copy
+ *
+ * Returns the number of copied byte.
+ *
+ **/
+int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+			void *buf, int buflen)
+{
+	return sg_copy_buffer(sgl, nents, buf, buflen, 0);
+}
+EXPORT_SYMBOL(sg_copy_from_buffer);
+
+/**
+ * sg_copy_to_buffer - Copy from an SG table to liner buffer
+ * @sgl:		 The SG table
+ * @nents:		 Number of SG entries
+ * @buf:		 Where to copy to
+ * @buflen:		 The number of bytes to copy
+ *
+ * Returns the number of copied byte.
+ *
+ **/
+int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+		      void *buf, int buflen)
+{
+	return sg_copy_buffer(sgl, nents, buf, buflen, 1);
+}
+EXPORT_SYMBOL(sg_copy_to_buffer);
-- 
1.5.3.7

[PATCH 02/10] scsi: add wrapper functions for sg buffer copy helper functions

[FUJITA Tomonori]

LLDs need to copies data between the SG table in struct scsi_cmnd and
liner buffer. So they use the helper functions like

sg_copy_from_buffer(scsi_sglist(sc), scsi_sg_count(sc), buf, buflen)
sg_copy_to_buffer(scsi_sglist(sc), scsi_sg_count(sc), buf, buflen)

This patch just adds wrapper functions:

scsi_sg_copy_from_buffer(sc, buf, buflen)
scsi_sg_copy_to_buffer(sc, buf, buflen)

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 include/scsi/scsi_cmnd.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index de28aab..b260be6 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -175,4 +175,18 @@ static inline struct scsi_data_buffer *scsi_out(struct scsi_cmnd *cmd)
 	return &cmd->sdb;
 }
 
+static inline int scsi_sg_copy_from_buffer(struct scsi_cmnd *cmd,
+					   void *buf, int buflen)
+{
+	return sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
+				   buf, buflen);
+}
+
+static inline int scsi_sg_copy_to_buffer(struct scsi_cmnd *cmd,
+					 void *buf, int buflen)
+{
+	return sg_copy_to_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
+				 buf, buflen);
+}
+
 #endif /* _SCSI_SCSI_CMND_H */
-- 
1.5.3.7

[PATCH 03/10] scsi_debug: use sg buffer copy helper functions

[FUJITA Tomonori]

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Douglas Gilbert <dougg@torque.net>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/scsi_debug.c |   79 +++++++-------------------------------------
 1 files changed, 13 insertions(+), 66 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 3abd286..7a2a3ed 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -591,81 +591,37 @@ static int check_readiness(struct scsi_cmnd * SCpnt, int reset_only,
 }
 
 /* Returns 0 if ok else (DID_ERROR << 16). Sets scp->resid . */
-static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
+static int fill_from_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr,
 				int arr_len)
 {
-	int k, req_len, act_len, len, active;
-	void * kaddr;
-	void * kaddr_off;
-	struct scatterlist *sg;
+	int act_len;
 	struct scsi_data_buffer *sdb = scsi_in(scp);
 
 	if (!sdb->length)
 		return 0;
-	if (!sdb->table.sgl)
-		return (DID_ERROR << 16);
 	if (!(scsi_bidi_cmnd(scp) || scp->sc_data_direction == DMA_FROM_DEVICE))
 		return (DID_ERROR << 16);
-	active = 1;
-	req_len = act_len = 0;
-	for_each_sg(sdb->table.sgl, sg, sdb->table.nents, k) {
-		if (active) {
-			kaddr = (unsigned char *)
-				kmap_atomic(sg_page(sg), KM_USER0);
-			if (NULL == kaddr)
-				return (DID_ERROR << 16);
-			kaddr_off = (unsigned char *)kaddr + sg->offset;
-			len = sg->length;
-			if ((req_len + len) > arr_len) {
-				active = 0;
-				len = arr_len - req_len;
-			}
-			memcpy(kaddr_off, arr + req_len, len);
-			kunmap_atomic(kaddr, KM_USER0);
-			act_len += len;
-		}
-		req_len += sg->length;
-	}
+
+	act_len = sg_copy_from_buffer(sdb->table.sgl, sdb->table.nents,
+				      arr, arr_len);
 	if (sdb->resid)
 		sdb->resid -= act_len;
 	else
-		sdb->resid = req_len - act_len;
+		sdb->resid = scsi_bufflen(scp) - act_len;
+
 	return 0;
 }
 
 /* Returns number of bytes fetched into 'arr' or -1 if error. */
-static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
-			       int max_arr_len)
+static int fetch_to_dev_buffer(struct scsi_cmnd *scp, unsigned char *arr,
+			       int arr_len)
 {
-	int k, req_len, len, fin;
-	void * kaddr;
-	void * kaddr_off;
-	struct scatterlist * sg;
-
-	if (0 == scsi_bufflen(scp))
+	if (!scsi_bufflen(scp))
 		return 0;
-	if (NULL == scsi_sglist(scp))
-		return -1;
 	if (!(scsi_bidi_cmnd(scp) || scp->sc_data_direction == DMA_TO_DEVICE))
 		return -1;
-	req_len = fin = 0;
-	scsi_for_each_sg(scp, sg, scsi_sg_count(scp), k) {
-		kaddr = (unsigned char *)kmap_atomic(sg_page(sg), KM_USER0);
-		if (NULL == kaddr)
-			return -1;
-		kaddr_off = (unsigned char *)kaddr + sg->offset;
-		len = sg->length;
-		if ((req_len + len) > max_arr_len) {
-			len = max_arr_len - req_len;
-			fin = 1;
-		}
-		memcpy(arr + req_len, kaddr_off, len);
-		kunmap_atomic(kaddr, KM_USER0);
-		if (fin)
-			return req_len + len;
-		req_len += sg->length;
-	}
-	return req_len;
+
+	return scsi_sg_copy_to_buffer(scp, arr, arr_len);
 }
 
 
@@ -1965,16 +1921,7 @@ static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,
 	if (!buf)
 		return ret;
 
-	offset = 0;
-	scsi_for_each_sg(scp, sg, scsi_sg_count(scp), i) {
-		kaddr = (unsigned char *)kmap_atomic(sg_page(sg), KM_USER0);
-		if (!kaddr)
-			goto out;
-
-		memcpy(buf + offset, kaddr + sg->offset, sg->length);
-		offset += sg->length;
-		kunmap_atomic(kaddr, KM_USER0);
-	}
+	scsi_sg_copy_to_buffer(scp, buf, scsi_bufflen(scp));
 
 	offset = 0;
 	for_each_sg(sdb->table.sgl, sg, sdb->table.nents, i) {
-- 
1.5.3.7

[PATCH 04/10] ps3rom: use sg buffer copy helper funcitons

[FUJITA Tomonori]

Note that if scsi_bufflen(cmd) is not zero, the command always has an
sg list. So this patch doesn't do the error checking in
fill_from_dev_buffer and fetch_to_dev_buffer did.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/ps3rom.c |   92 +++++-------------------------------------------
 1 files changed, 10 insertions(+), 82 deletions(-)

diff --git a/drivers/scsi/ps3rom.c b/drivers/scsi/ps3rom.c
index 0eed5ca..d1e7845 100644
--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -90,76 +90,6 @@ static int ps3rom_slave_configure(struct scsi_device *scsi_dev)
 	return 0;
 }
 
-/*
- * copy data from device into scatter/gather buffer
- */
-static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf)
-{
-	int k, req_len, len, fin;
-	void *kaddr;
-	struct scatterlist *sgpnt;
-	unsigned int buflen;
-
-	buflen = scsi_bufflen(cmd);
-	if (!buflen)
-		return 0;
-
-	if (!scsi_sglist(cmd))
-		return -1;
-
-	req_len = fin = 0;
-	scsi_for_each_sg(cmd, sgpnt, scsi_sg_count(cmd), k) {
-		kaddr = kmap_atomic(sg_page(sgpnt), KM_IRQ0);
-		len = sgpnt->length;
-		if ((req_len + len) > buflen) {
-			len = buflen - req_len;
-			fin = 1;
-		}
-		memcpy(kaddr + sgpnt->offset, buf + req_len, len);
-		flush_kernel_dcache_page(sg_page(sgpnt));
-		kunmap_atomic(kaddr, KM_IRQ0);
-		req_len += len;
-		if (fin)
-			break;
-	}
-	scsi_set_resid(cmd, buflen - req_len);
-	return 0;
-}
-
-/*
- * copy data from scatter/gather into device's buffer
- */
-static int fetch_to_dev_buffer(struct scsi_cmnd *cmd, void *buf)
-{
-	int k, req_len, len, fin;
-	void *kaddr;
-	struct scatterlist *sgpnt;
-	unsigned int buflen;
-
-	buflen = scsi_bufflen(cmd);
-	if (!buflen)
-		return 0;
-
-	if (!scsi_sglist(cmd))
-		return -1;
-
-	req_len = fin = 0;
-	scsi_for_each_sg(cmd, sgpnt, scsi_sg_count(cmd), k) {
-		kaddr = kmap_atomic(sg_page(sgpnt), KM_IRQ0);
-		len = sgpnt->length;
-		if ((req_len + len) > buflen) {
-			len = buflen - req_len;
-			fin = 1;
-		}
-		memcpy(buf + req_len, kaddr + sgpnt->offset, len);
-		kunmap_atomic(kaddr, KM_IRQ0);
-		if (fin)
-			return req_len + len;
-		req_len += sgpnt->length;
-	}
-	return req_len;
-}
-
 static int ps3rom_atapi_request(struct ps3_storage_device *dev,
 				struct scsi_cmnd *cmd)
 {
@@ -193,9 +123,7 @@ static int ps3rom_atapi_request(struct ps3_storage_device *dev,
 		else
 			atapi_cmnd.proto = PIO_DATA_OUT_PROTO;
 		atapi_cmnd.in_out = DIR_WRITE;
-		res = fetch_to_dev_buffer(cmd, dev->bounce_buf);
-		if (res < 0)
-			return DID_ERROR << 16;
+		scsi_sg_copy_to_buffer(cmd, dev->bounce_buf, dev->bounce_size);
 		break;
 
 	default:
@@ -267,9 +195,7 @@ static int ps3rom_write_request(struct ps3_storage_device *dev,
 	dev_dbg(&dev->sbd.core, "%s:%u: write %u sectors starting at %u\n",
 		__func__, __LINE__, sectors, start_sector);
 
-	res = fetch_to_dev_buffer(cmd, dev->bounce_buf);
-	if (res < 0)
-		return DID_ERROR << 16;
+	scsi_sg_copy_to_buffer(cmd, dev->bounce_buf, dev->bounce_size);
 
 	res = lv1_storage_write(dev->sbd.dev_id,
 				dev->regions[dev->region_idx].id, start_sector,
@@ -379,11 +305,13 @@ static irqreturn_t ps3rom_interrupt(int irq, void *data)
 	if (!status) {
 		/* OK, completed */
 		if (cmd->sc_data_direction == DMA_FROM_DEVICE) {
-			res = fill_from_dev_buffer(cmd, dev->bounce_buf);
-			if (res) {
-				cmd->result = DID_ERROR << 16;
-				goto done;
-			}
+			int len;
+
+			len = scsi_sg_copy_from_buffer(cmd,
+						       dev->bounce_buf,
+						       dev->bounce_size);
+
+			scsi_set_resid(cmd, scsi_bufflen(cmd) - len);
 		}
 		cmd->result = DID_OK << 16;
 		goto done;
@@ -425,7 +353,7 @@ static struct scsi_host_template ps3rom_host_template = {
 	.cmd_per_lun =		1,
 	.emulated =             1,		/* only sg driver uses this */
 	.max_sectors =		PS3ROM_MAX_SECTORS,
-	.use_clustering =	DISABLE_CLUSTERING,
+	.use_clustering =	ENABLE_CLUSTERING,
 	.module =		THIS_MODULE,
 };
 
-- 
1.5.3.7

[PATCH 05/10] simscsi: use sg buffer copy helper funcitons

[FUJITA Tomonori]

This replaces simscsi_fillresult with scsi_sg_copy_from_buffer.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Tony Luck <tony.luck@intel.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 arch/ia64/hp/sim/simscsi.c |   23 ++++-------------------
 1 files changed, 4 insertions(+), 19 deletions(-)

diff --git a/arch/ia64/hp/sim/simscsi.c b/arch/ia64/hp/sim/simscsi.c
index 7661bb0..3a078ad 100644
--- a/arch/ia64/hp/sim/simscsi.c
+++ b/arch/ia64/hp/sim/simscsi.c
@@ -201,22 +201,6 @@ simscsi_readwrite10 (struct scsi_cmnd *sc, int mode)
 	simscsi_sg_readwrite(sc, mode, offset);
 }
 
-static void simscsi_fillresult(struct scsi_cmnd *sc, char *buf, unsigned len)
-{
-
-	int i;
-	unsigned thislen;
-	struct scatterlist *slp;
-
-	scsi_for_each_sg(sc, slp, scsi_sg_count(sc), i) {
-		if (!len)
-			break;
-		thislen = min(len, slp->length);
-		memcpy(sg_virt(slp), buf, thislen);
-		len -= thislen;
-	}
-}
-
 static int
 simscsi_queuecommand (struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
 {
@@ -258,7 +242,7 @@ simscsi_queuecommand (struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
 			buf[6] = 0;	/* reserved */
 			buf[7] = 0;	/* various flags */
 			memcpy(buf + 8, "HP      SIMULATED DISK  0.00",  28);
-			simscsi_fillresult(sc, buf, 36);
+			scsi_sg_copy_from_buffer(sc, buf, 36);
 			sc->result = GOOD;
 			break;
 
@@ -306,14 +290,15 @@ simscsi_queuecommand (struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
 			buf[5] = 0;
 			buf[6] = 2;
 			buf[7] = 0;
-			simscsi_fillresult(sc, buf, 8);
+			scsi_sg_copy_from_buffer(sc, buf, 8);
 			sc->result = GOOD;
 			break;
 
 		      case MODE_SENSE:
 		      case MODE_SENSE_10:
 			/* sd.c uses this to determine whether disk does write-caching. */
-			simscsi_fillresult(sc, (char *)empty_zero_page, scsi_bufflen(sc));
+			scsi_sg_copy_from_buffer(sc, (char *)empty_zero_page,
+						 PAGE_SIZE);
 			sc->result = GOOD;
 			break;
 
-- 
1.5.3.7

[PATCH 06/10] ips: use sg buffer copy helper funcitons

[FUJITA Tomonori]

This rewrites ips_scmd_buf_write/read with scsi_sg_copy_from/to_buffer
respectively.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Salyzyn, Mark <Mark_Salyzyn@adaptec.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/ips.c |   50 ++++++++------------------------------------------
 1 files changed, 8 insertions(+), 42 deletions(-)

diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
index 6264036..cc4f44f 100644
--- a/drivers/scsi/ips.c
+++ b/drivers/scsi/ips.c
@@ -3502,28 +3502,11 @@ ips_send_wait(ips_ha_t * ha, ips_scb_t * scb, int timeout, int intr)
 static void
 ips_scmd_buf_write(struct scsi_cmnd *scmd, void *data, unsigned int count)
 {
-        int i;
-        unsigned int min_cnt, xfer_cnt;
-        char *cdata = (char *) data;
-        unsigned char *buffer;
-        unsigned long flags;
-        struct scatterlist *sg = scsi_sglist(scmd);
-
-        for (i = 0, xfer_cnt = 0;
-	(i < scsi_sg_count(scmd)) && (xfer_cnt < count);
-             i++, sg = sg_next(sg)) {
-                min_cnt = min(count - xfer_cnt, sg->length);
-
-                /* kmap_atomic() ensures addressability of the data buffer.*/
-                /* local_irq_save() protects the KM_IRQ0 address slot.     */
-                local_irq_save(flags);
-		buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
-                memcpy(buffer, &cdata[xfer_cnt], min_cnt);
-		kunmap_atomic(buffer - sg->offset, KM_IRQ0);
-                local_irq_restore(flags);
+	unsigned long flags;
 
-                xfer_cnt += min_cnt;
-        }
+	local_irq_save(flags);
+	scsi_sg_copy_from_buffer(scmd, data, count);
+	local_irq_restore(flags);
 }
 
 /****************************************************************************/
@@ -3536,28 +3519,11 @@ ips_scmd_buf_write(struct scsi_cmnd *scmd, void *data, unsigned int count)
 static void
 ips_scmd_buf_read(struct scsi_cmnd *scmd, void *data, unsigned int count)
 {
-        int i;
-        unsigned int min_cnt, xfer_cnt;
-        char *cdata = (char *) data;
-        unsigned char *buffer;
-        unsigned long flags;
-        struct scatterlist *sg = scsi_sglist(scmd);
-
-        for (i = 0, xfer_cnt = 0;
-             (i < scsi_sg_count(scmd)) && (xfer_cnt < count);
-             i++, sg = sg_next(sg)) {
-		min_cnt = min(count - xfer_cnt, sg->length);
-
-                /* kmap_atomic() ensures addressability of the data buffer.*/
-                /* local_irq_save() protects the KM_IRQ0 address slot.     */
-                local_irq_save(flags);
-		buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
-                memcpy(&cdata[xfer_cnt], buffer, min_cnt);
-		kunmap_atomic(buffer - sg->offset, KM_IRQ0);
-                local_irq_restore(flags);
+	unsigned long flags;
 
-                xfer_cnt += min_cnt;
-        }
+	local_irq_save(flags);
+	scsi_sg_copy_to_buffer(scmd, data, count);
+	local_irq_restore(flags);
 }
 
 /****************************************************************************/
-- 
1.5.3.7

[PATCH 07/10] aacraid: use sg buffer copy helper functions

[FUJITA Tomonori]

This replaces aac_internal_transfer with scsi_sg_copy_to/from_buffer.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Mark Salyzyn <Mark_Salyzyn@adaptec.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/aacraid/aachba.c |   49 ++++++++++++++--------------------------
 1 files changed, 17 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index b9fc9b1..c947e72 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -379,24 +379,6 @@ int aac_get_containers(struct aac_dev *dev)
 	return status;
 }
 
-static void aac_internal_transfer(struct scsi_cmnd *scsicmd, void *data, unsigned int offset, unsigned int len)
-{
-	void *buf;
-	int transfer_len;
-	struct scatterlist *sg = scsi_sglist(scsicmd);
-
-	buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
-	transfer_len = min(sg->length, len + offset);
-
-	transfer_len -= offset;
-	if (buf && transfer_len > 0)
-		memcpy(buf + offset, data, transfer_len);
-
-	flush_kernel_dcache_page(kmap_atomic_to_page(buf - sg->offset));
-	kunmap_atomic(buf - sg->offset, KM_IRQ0);
-
-}
-
 static void get_container_name_callback(void *context, struct fib * fibptr)
 {
 	struct aac_get_name_resp * get_name_reply;
@@ -419,14 +401,17 @@ static void get_container_name_callback(void *context, struct fib * fibptr)
 		while (*sp == ' ')
 			++sp;
 		if (*sp) {
+			struct inquiry_data inq;
 			char d[sizeof(((struct inquiry_data *)NULL)->inqd_pid)];
 			int count = sizeof(d);
 			char *dp = d;
 			do {
 				*dp++ = (*sp) ? *sp++ : ' ';
 			} while (--count > 0);
-			aac_internal_transfer(scsicmd, d,
-			  offsetof(struct inquiry_data, inqd_pid), sizeof(d));
+
+			scsi_sg_copy_to_buffer(scsicmd, &inq, sizeof(inq));
+			memcpy(inq.inqd_pid, d, sizeof(d));
+			scsi_sg_copy_from_buffer(scsicmd, &inq, sizeof(inq));
 		}
 	}
 
@@ -811,7 +796,7 @@ static void get_container_serial_callback(void *context, struct fib * fibptr)
 		sp[2] = 0;
 		sp[3] = snprintf(sp+4, sizeof(sp)-4, "%08X",
 		  le32_to_cpu(get_serial_reply->uid));
-		aac_internal_transfer(scsicmd, sp, 0, sizeof(sp));
+		scsi_sg_copy_from_buffer(scsicmd, sp, sizeof(sp));
 	}
 
 	scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
@@ -1986,8 +1971,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				arr[4] = 0x0;
 				arr[5] = 0x80;
 				arr[1] = scsicmd->cmnd[2];
-				aac_internal_transfer(scsicmd, &inq_data, 0,
-				  sizeof(inq_data));
+				scsi_sg_copy_from_buffer(scsicmd, &inq_data,
+							 sizeof(inq_data));
 				scsicmd->result = DID_OK << 16 |
 				  COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
 			} else if (scsicmd->cmnd[2] == 0x80) {
@@ -1995,8 +1980,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				arr[3] = setinqserial(dev, &arr[4],
 				  scmd_id(scsicmd));
 				arr[1] = scsicmd->cmnd[2];
-				aac_internal_transfer(scsicmd, &inq_data, 0,
-				  sizeof(inq_data));
+				scsi_sg_copy_from_buffer(scsicmd, &inq_data,
+							 sizeof(inq_data));
 				return aac_get_container_serial(scsicmd);
 			} else {
 				/* vpd page not implemented */
@@ -2027,7 +2012,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 		if (cid == host->this_id) {
 			setinqstr(dev, (void *) (inq_data.inqd_vid), ARRAY_SIZE(container_types));
 			inq_data.inqd_pdt = INQD_PDT_PROC;	/* Processor device */
-			aac_internal_transfer(scsicmd, &inq_data, 0, sizeof(inq_data));
+			scsi_sg_copy_from_buffer(scsicmd, &inq_data,
+						 sizeof(inq_data));
 			scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
 			scsicmd->scsi_done(scsicmd);
 			return 0;
@@ -2036,7 +2022,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 			return -1;
 		setinqstr(dev, (void *) (inq_data.inqd_vid), fsa_dev_ptr[cid].type);
 		inq_data.inqd_pdt = INQD_PDT_DA;	/* Direct/random access device */
-		aac_internal_transfer(scsicmd, &inq_data, 0, sizeof(inq_data));
+		scsi_sg_copy_from_buffer(scsicmd, &inq_data, sizeof(inq_data));
 		return aac_get_container_name(scsicmd);
 	}
 	case SERVICE_ACTION_IN:
@@ -2070,8 +2056,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 			     + (scsicmd->cmnd[12] << 8) + scsicmd->cmnd[13]);
 
 		alloc_len = min_t(size_t, alloc_len, sizeof(cp));
-		aac_internal_transfer(scsicmd, cp, 0, alloc_len);
-
+		scsi_sg_copy_from_buffer(scsicmd, cp, alloc_len);
 		if (alloc_len < scsi_bufflen(scsicmd))
 			scsi_set_resid(scsicmd,
 				       scsi_bufflen(scsicmd) - alloc_len);
@@ -2104,7 +2089,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 		cp[5] = 0;
 		cp[6] = 2;
 		cp[7] = 0;
-		aac_internal_transfer(scsicmd, cp, 0, sizeof(cp));
+		scsi_sg_copy_from_buffer(scsicmd, cp, sizeof(cp));
 		/* Do not cache partition table for arrays */
 		scsicmd->device->removable = 1;
 
@@ -2139,7 +2124,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 			if (mode_buf_length > scsicmd->cmnd[4])
 				mode_buf_length = scsicmd->cmnd[4];
 		}
-		aac_internal_transfer(scsicmd, mode_buf, 0, mode_buf_length);
+		scsi_sg_copy_from_buffer(scsicmd, mode_buf, mode_buf_length);
 		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
 		scsicmd->scsi_done(scsicmd);
 
@@ -2174,7 +2159,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 			if (mode_buf_length > scsicmd->cmnd[8])
 				mode_buf_length = scsicmd->cmnd[8];
 		}
-		aac_internal_transfer(scsicmd, mode_buf, 0, mode_buf_length);
+		scsi_sg_copy_from_buffer(scsicmd, mode_buf, mode_buf_length);
 
 		scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
 		scsicmd->scsi_done(scsicmd);
-- 
1.5.3.7

[PATCH 08/10] stex: use sg buffer copy helper functions

[FUJITA Tomonori]

This replaces stex_internal_copy with scsi_sg_copy_to/from_buffer.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Ed Lin <ed.lin@promise.com>
Cc: James Bottomley <James.Bottomley@SteelEye.com>
---
 drivers/scsi/stex.c |   66 +++++++-------------------------------------------
 1 files changed, 10 insertions(+), 56 deletions(-)

diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index 654430e..8c7b183 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -426,49 +426,13 @@ static int stex_map_sg(struct st_hba *hba,
 	return 0;
 }
 
-static void stex_internal_copy(struct scsi_cmnd *cmd,
-	const void *src, size_t *count, int sg_count, int direction)
-{
-	size_t lcount;
-	size_t len;
-	void *s, *d, *base = NULL;
-	size_t offset;
-
-	if (*count > scsi_bufflen(cmd))
-		*count = scsi_bufflen(cmd);
-	lcount = *count;
-	while (lcount) {
-		len = lcount;
-		s = (void *)src;
-
-		offset = *count - lcount;
-		s += offset;
-		base = scsi_kmap_atomic_sg(scsi_sglist(cmd),
-					   sg_count, &offset, &len);
-		if (!base) {
-			*count -= lcount;
-			return;
-		}
-		d = base + offset;
-
-		if (direction == ST_TO_CMD)
-			memcpy(d, s, len);
-		else
-			memcpy(s, d, len);
-
-		lcount -= len;
-		scsi_kunmap_atomic_sg(base);
-	}
-}
-
 static void stex_controller_info(struct st_hba *hba, struct st_ccb *ccb)
 {
 	struct st_frame *p;
 	size_t count = sizeof(struct st_frame);
 
 	p = hba->copy_buffer;
-	stex_internal_copy(ccb->cmd, p, &count, scsi_sg_count(ccb->cmd),
-			   ST_FROM_CMD);
+	count = scsi_sg_copy_to_buffer(ccb->cmd, p, count);
 	memset(p->base, 0, sizeof(u32)*6);
 	*(unsigned long *)(p->base) = pci_resource_start(hba->pdev, 0);
 	p->rom_addr = 0;
@@ -486,8 +450,7 @@ static void stex_controller_info(struct st_hba *hba, struct st_ccb *ccb)
 	p->subid =
 		hba->pdev->subsystem_vendor << 16 | hba->pdev->subsystem_device;
 
-	stex_internal_copy(ccb->cmd, p, &count, scsi_sg_count(ccb->cmd),
-			   ST_TO_CMD);
+	count = scsi_sg_copy_from_buffer(ccb->cmd, p, count);
 }
 
 static void
@@ -554,10 +517,8 @@ stex_queuecommand(struct scsi_cmnd *cmd, void (* done)(struct scsi_cmnd *))
 		unsigned char page;
 		page = cmd->cmnd[2] & 0x3f;
 		if (page == 0x8 || page == 0x3f) {
-			size_t cp_len = sizeof(ms10_caching_page);
-			stex_internal_copy(cmd, ms10_caching_page,
-					   &cp_len, scsi_sg_count(cmd),
-					   ST_TO_CMD);
+			scsi_sg_copy_from_buffer(cmd, ms10_caching_page,
+						 sizeof(ms10_caching_page));
 			cmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
 			done(cmd);
 		} else
@@ -586,10 +547,8 @@ stex_queuecommand(struct scsi_cmnd *cmd, void (* done)(struct scsi_cmnd *))
 		if (id != host->max_id - 1)
 			break;
 		if (lun == 0 && (cmd->cmnd[1] & INQUIRY_EVPD) == 0) {
-			size_t cp_len = sizeof(console_inq_page);
-			stex_internal_copy(cmd, console_inq_page,
-					   &cp_len, scsi_sg_count(cmd),
-					   ST_TO_CMD);
+			scsi_sg_copy_from_buffer(cmd, (void *)console_inq_page,
+						 sizeof(console_inq_page));
 			cmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8;
 			done(cmd);
 		} else
@@ -606,8 +565,7 @@ stex_queuecommand(struct scsi_cmnd *cmd, void (* done)(struct scsi_cmnd *))
 			ver.signature[0] = PASSTHRU_SIGNATURE;
 			ver.console_id = host->max_id - 1;
 			ver.host_no = hba->host->host_no;
-			stex_internal_copy(cmd, &ver, &cp_len,
-					   scsi_sg_count(cmd), ST_TO_CMD);
+			cp_len = scsi_sg_copy_from_buffer(cmd, &ver, cp_len);
 			cmd->result = sizeof(ver) == cp_len ?
 				DID_OK << 16 | COMMAND_COMPLETE << 8 :
 				DID_ERROR << 16 | COMMAND_COMPLETE << 8;
@@ -700,15 +658,12 @@ static void stex_copy_data(struct st_ccb *ccb,
 
 	if (ccb->cmd == NULL)
 		return;
-	stex_internal_copy(ccb->cmd,
-		resp->variable, &count, scsi_sg_count(ccb->cmd), ST_TO_CMD);
+	count = scsi_sg_copy_from_buffer(ccb->cmd, resp->variable, count);
 }
 
 static void stex_ys_commands(struct st_hba *hba,
 	struct st_ccb *ccb, struct status_msg *resp)
 {
-	size_t count;
-
 	if (ccb->cmd->cmnd[0] == MGT_CMD &&
 		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
 		scsi_set_resid(ccb->cmd, scsi_bufflen(ccb->cmd) -
@@ -724,9 +679,8 @@ static void stex_ys_commands(struct st_hba *hba,
 		resp->scsi_status == SAM_STAT_GOOD) {
 		ST_INQ *inq_data;
 
-		count = STEX_EXTRA_SIZE;
-		stex_internal_copy(ccb->cmd, hba->copy_buffer,
-			&count, scsi_sg_count(ccb->cmd), ST_FROM_CMD);
+		scsi_sg_copy_to_buffer(ccb->cmd, hba->copy_buffer,
+				       STEX_EXTRA_SIZE);
 		inq_data = (ST_INQ *)hba->copy_buffer;
 		if (inq_data->DeviceTypeQualifier != 0)
 			ccb->srb_status = SRB_STATUS_SELECTION_TIMEOUT;
-- 
1.5.3.7

[PATCH 09/10] 3w-xxxx: use sg buffer copy helper functions

[FUJITA Tomonori]

This rewrites tw_transfer_internal with scsi_sg_copy_from/to_buffer.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Adam Radford <linuxraid@amcc.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/3w-xxxx.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/3w-xxxx.c b/drivers/scsi/3w-xxxx.c
index d095321..a2fc060 100644
--- a/drivers/scsi/3w-xxxx.c
+++ b/drivers/scsi/3w-xxxx.c
@@ -1463,18 +1463,10 @@ static void tw_transfer_internal(TW_Device_Extension *tw_dev, int request_id,
 				 void *data, unsigned int len)
 {
 	struct scsi_cmnd *cmd = tw_dev->srb[request_id];
-	void *buf;
-	unsigned int transfer_len;
-	unsigned long flags = 0;
-	struct scatterlist *sg = scsi_sglist(cmd);
+	unsigned long flags;
 
 	local_irq_save(flags);
-	buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
-	transfer_len = min(sg->length, len);
-
-	memcpy(buf, data, transfer_len);
-
-	kunmap_atomic(buf - sg->offset, KM_IRQ0);
+	scsi_sg_copy_from_buffer(cmd, data, len);
 	local_irq_restore(flags);
 }
 
-- 
1.5.3.7

[PATCH 10/10] 3w-9xxx: use sg buffer copy helper functions

[FUJITA Tomonori]

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Adam Radford <linuxraid@amcc.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/3w-9xxx.c |   21 +++++++++------------
 1 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/3w-9xxx.c b/drivers/scsi/3w-9xxx.c
index b4912d1..fa922f8 100644
--- a/drivers/scsi/3w-9xxx.c
+++ b/drivers/scsi/3w-9xxx.c
@@ -1838,12 +1838,11 @@ static int twa_scsiop_execute_scsi(TW_Device_Extension *tw_dev, int request_id,
 		if (scsi_sg_count(srb)) {
 			if ((scsi_sg_count(srb) == 1) &&
 			    (scsi_bufflen(srb) < TW_MIN_SGL_LENGTH)) {
-				if (srb->sc_data_direction == DMA_TO_DEVICE || srb->sc_data_direction == DMA_BIDIRECTIONAL) {
-					struct scatterlist *sg = scsi_sglist(srb);
-					char *buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
-					memcpy(tw_dev->generic_buffer_virt[request_id], buf, sg->length);
-					kunmap_atomic(buf - sg->offset, KM_IRQ0);
-				}
+				if (srb->sc_data_direction == DMA_TO_DEVICE ||
+				    srb->sc_data_direction == DMA_BIDIRECTIONAL)
+					scsi_sg_copy_to_buffer(srb,
+							       tw_dev->generic_buffer_virt[request_id],
+							       TW_SECTOR_SIZE);
 				command_packet->sg_list[0].address = TW_CPU_TO_SGL(tw_dev->generic_buffer_phys[request_id]);
 				command_packet->sg_list[0].length = cpu_to_le32(TW_MIN_SGL_LENGTH);
 			} else {
@@ -1915,13 +1914,11 @@ static void twa_scsiop_execute_scsi_complete(TW_Device_Extension *tw_dev, int re
 	    (cmd->sc_data_direction == DMA_FROM_DEVICE ||
 	     cmd->sc_data_direction == DMA_BIDIRECTIONAL)) {
 		if (scsi_sg_count(cmd) == 1) {
-			struct scatterlist *sg = scsi_sglist(tw_dev->srb[request_id]);
-			char *buf;
-			unsigned long flags = 0;
+			unsigned long flags;
+			void *buf = tw_dev->generic_buffer_virt[request_id];
+
 			local_irq_save(flags);
-			buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
-			memcpy(buf, tw_dev->generic_buffer_virt[request_id], sg->length);
-			kunmap_atomic(buf - sg->offset, KM_IRQ0);
+			scsi_sg_copy_from_buffer(cmd, buf, TW_SECTOR_SIZE);
 			local_irq_restore(flags);
 		}
 	}
-- 
1.5.3.7

Re: [PATCH 07/10] aacraid: use sg buffer copy helper functions

[Mark Salyzyn]

ACK

Especially like that flush_kernel_dcache_page is performed in the  
block layer helper function, one less architectural piece of code in  
an LLD.

Sincerely -- Mark Salyzyn

On Mar 8, 2008, at 11:44 PM, FUJITA Tomonori wrote:

> This replaces aac_internal_transfer with scsi_sg_copy_to/from_buffer.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Mark Salyzyn <Mark_Salyzyn@adaptec.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
> drivers/scsi/aacraid/aachba.c |   49 +++++++++++++ 
> +--------------------------
> 1 files changed, 17 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/ 
> aachba.c
> index b9fc9b1..c947e72 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -379,24 +379,6 @@ int aac_get_containers(struct aac_dev *dev)
>        return status;
> }
>
> -static void aac_internal_transfer(struct scsi_cmnd *scsicmd, void  
> *data, unsigned int offset, unsigned int len)
> -{
> -       void *buf;
> -       int transfer_len;
> -       struct scatterlist *sg = scsi_sglist(scsicmd);
> -
> -       buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
> -       transfer_len = min(sg->length, len + offset);
> -
> -       transfer_len -= offset;
> -       if (buf && transfer_len > 0)
> -               memcpy(buf + offset, data, transfer_len);
> -
> -       flush_kernel_dcache_page(kmap_atomic_to_page(buf - sg- 
> >offset));
> -       kunmap_atomic(buf - sg->offset, KM_IRQ0);
> -
> -}
> -
> static void get_container_name_callback(void *context, struct fib *  
> fibptr)
> {
>        struct aac_get_name_resp * get_name_reply;
> @@ -419,14 +401,17 @@ static void get_container_name_callback(void  
> *context, struct fib * fibptr)
>                while (*sp == ' ')
>                        ++sp;
>                if (*sp) {
> +                       struct inquiry_data inq;
>                        char d[sizeof(((struct inquiry_data *)NULL)- 
> >inqd_pid)];
>                        int count = sizeof(d);
>                        char *dp = d;
>                        do {
>                                *dp++ = (*sp) ? *sp++ : ' ';
>                        } while (--count > 0);
> -                       aac_internal_transfer(scsicmd, d,
> -                         offsetof(struct inquiry_data, inqd_pid),  
> sizeof(d));
> +
> +                       scsi_sg_copy_to_buffer(scsicmd, &inq,  
> sizeof(inq));
> +                       memcpy(inq.inqd_pid, d, sizeof(d));
> +                       scsi_sg_copy_from_buffer(scsicmd, &inq,  
> sizeof(inq));
>                }
>        }
>
> @@ -811,7 +796,7 @@ static void get_container_serial_callback(void  
> *context, struct fib * fibptr)
>                sp[2] = 0;
>                sp[3] = snprintf(sp+4, sizeof(sp)-4, "%08X",
>                  le32_to_cpu(get_serial_reply->uid));
> -               aac_internal_transfer(scsicmd, sp, 0, sizeof(sp));
> +               scsi_sg_copy_from_buffer(scsicmd, sp, sizeof(sp));
>        }
>
>        scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE << 8 |  
> SAM_STAT_GOOD;
> @@ -1986,8 +1971,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
>                                arr[4] = 0x0;
>                                arr[5] = 0x80;
>                                arr[1] = scsicmd->cmnd[2];
> -                               aac_internal_transfer(scsicmd,  
> &inq_data, 0,
> -                                 sizeof(inq_data));
> +                               scsi_sg_copy_from_buffer(scsicmd,  
> &inq_data,
> +                                                         
> sizeof(inq_data));
>                                scsicmd->result = DID_OK << 16 |
>                                  COMMAND_COMPLETE << 8 |  
> SAM_STAT_GOOD;
>                        } else if (scsicmd->cmnd[2] == 0x80) {
> @@ -1995,8 +1980,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
>                                arr[3] = setinqserial(dev, &arr[4],
>                                  scmd_id(scsicmd));
>                                arr[1] = scsicmd->cmnd[2];
> -                               aac_internal_transfer(scsicmd,  
> &inq_data, 0,
> -                                 sizeof(inq_data));
> +                               scsi_sg_copy_from_buffer(scsicmd,  
> &inq_data,
> +                                                         
> sizeof(inq_data));
>                                return  
> aac_get_container_serial(scsicmd);
>                        } else {
>                                /* vpd page not implemented */
> @@ -2027,7 +2012,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
>                if (cid == host->this_id) {
>                        setinqstr(dev, (void *) (inq_data.inqd_vid),  
> ARRAY_SIZE(container_types));
>                        inq_data.inqd_pdt = INQD_PDT_PROC;      /*  
> Processor device */
> -                       aac_internal_transfer(scsicmd, &inq_data, 0,  
> sizeof(inq_data));
> +                       scsi_sg_copy_from_buffer(scsicmd, &inq_data,
> +                                                sizeof(inq_data));
>                        scsicmd->result = DID_OK << 16 |  
> COMMAND_COMPLETE << 8 | SAM_STAT_GOOD;
>                        scsicmd->scsi_done(scsicmd);
>                        return 0;
> @@ -2036,7 +2022,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
>                        return -1;
>                setinqstr(dev, (void *) (inq_data.inqd_vid),  
> fsa_dev_ptr[cid].type);
>                inq_data.inqd_pdt = INQD_PDT_DA;        /* Direct/ 
> random access device */
> -               aac_internal_transfer(scsicmd, &inq_data, 0,  
> sizeof(inq_data));
> +               scsi_sg_copy_from_buffer(scsicmd, &inq_data,  
> sizeof(inq_data));
>                return aac_get_container_name(scsicmd);
>        }
>        case SERVICE_ACTION_IN:
> @@ -2070,8 +2056,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
>                             + (scsicmd->cmnd[12] << 8) + scsicmd- 
> >cmnd[13]);
>
>                alloc_len = min_t(size_t, alloc_len, sizeof(cp));
> -               aac_internal_transfer(scsicmd, cp, 0, alloc_len);
> -
> +               scsi_sg_copy_from_buffer(scsicmd, cp, alloc_len);
>                if (alloc_len < scsi_bufflen(scsicmd))
>                        scsi_set_resid(scsicmd,
>                                       scsi_bufflen(scsicmd) -  
> alloc_len);
> @@ -2104,7 +2089,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
>                cp[5] = 0;
>                cp[6] = 2;
>                cp[7] = 0;
> -               aac_internal_transfer(scsicmd, cp, 0, sizeof(cp));
> +               scsi_sg_copy_from_buffer(scsicmd, cp, sizeof(cp));
>                /* Do not cache partition table for arrays */
>                scsicmd->device->removable = 1;
>
> @@ -2139,7 +2124,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
>                        if (mode_buf_length > scsicmd->cmnd[4])
>                                mode_buf_length = scsicmd->cmnd[4];
>                }
> -               aac_internal_transfer(scsicmd, mode_buf, 0,  
> mode_buf_length);
> +               scsi_sg_copy_from_buffer(scsicmd, mode_buf,  
> mode_buf_length);
>                scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE <<  
> 8 | SAM_STAT_GOOD;
>                scsicmd->scsi_done(scsicmd);
>
> @@ -2174,7 +2159,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
>                        if (mode_buf_length > scsicmd->cmnd[8])
>                                mode_buf_length = scsicmd->cmnd[8];
>                }
> -               aac_internal_transfer(scsicmd, mode_buf, 0,  
> mode_buf_length);
> +               scsi_sg_copy_from_buffer(scsicmd, mode_buf,  
> mode_buf_length);
>
>                scsicmd->result = DID_OK << 16 | COMMAND_COMPLETE <<  
> 8 | SAM_STAT_GOOD;
>                scsicmd->scsi_done(scsicmd);
> --
> 1.5.3.7
>

Re: [PATCH 06/10] ips: use sg buffer copy helper funcitons

[Mark Salyzyn]

ACK

Sincerely -- Mark Salyzyn

On Mar 8, 2008, at 11:44 PM, FUJITA Tomonori wrote:

> This rewrites ips_scmd_buf_write/read with scsi_sg_copy_from/to_buffer
> respectively.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Salyzyn, Mark <Mark_Salyzyn@adaptec.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
> drivers/scsi/ips.c |   50 +++++++ 
> +------------------------------------------
> 1 files changed, 8 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/scsi/ips.c b/drivers/scsi/ips.c
> index 6264036..cc4f44f 100644
> --- a/drivers/scsi/ips.c
> +++ b/drivers/scsi/ips.c
> @@ -3502,28 +3502,11 @@ ips_send_wait(ips_ha_t * ha, ips_scb_t *  
> scb, int timeout, int intr)
> static void
> ips_scmd_buf_write(struct scsi_cmnd *scmd, void *data, unsigned int  
> count)
> {
> -        int i;
> -        unsigned int min_cnt, xfer_cnt;
> -        char *cdata = (char *) data;
> -        unsigned char *buffer;
> -        unsigned long flags;
> -        struct scatterlist *sg = scsi_sglist(scmd);
> -
> -        for (i = 0, xfer_cnt = 0;
> -       (i < scsi_sg_count(scmd)) && (xfer_cnt < count);
> -             i++, sg = sg_next(sg)) {
> -                min_cnt = min(count - xfer_cnt, sg->length);
> -
> -                /* kmap_atomic() ensures addressability of the data  
> buffer.*/
> -                /* local_irq_save() protects the KM_IRQ0 address  
> slot.     */
> -                local_irq_save(flags);
> -               buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg- 
> >offset;
> -                memcpy(buffer, &cdata[xfer_cnt], min_cnt);
> -               kunmap_atomic(buffer - sg->offset, KM_IRQ0);
> -                local_irq_restore(flags);
> +       unsigned long flags;
>
> -                xfer_cnt += min_cnt;
> -        }
> +       local_irq_save(flags);
> +       scsi_sg_copy_from_buffer(scmd, data, count);
> +       local_irq_restore(flags);
> }
>
> / 
> ****************************************************************************/
> @@ -3536,28 +3519,11 @@ ips_scmd_buf_write(struct scsi_cmnd *scmd,  
> void *data, unsigned int count)
> static void
> ips_scmd_buf_read(struct scsi_cmnd *scmd, void *data, unsigned int  
> count)
> {
> -        int i;
> -        unsigned int min_cnt, xfer_cnt;
> -        char *cdata = (char *) data;
> -        unsigned char *buffer;
> -        unsigned long flags;
> -        struct scatterlist *sg = scsi_sglist(scmd);
> -
> -        for (i = 0, xfer_cnt = 0;
> -             (i < scsi_sg_count(scmd)) && (xfer_cnt < count);
> -             i++, sg = sg_next(sg)) {
> -               min_cnt = min(count - xfer_cnt, sg->length);
> -
> -                /* kmap_atomic() ensures addressability of the data  
> buffer.*/
> -                /* local_irq_save() protects the KM_IRQ0 address  
> slot.     */
> -                local_irq_save(flags);
> -               buffer = kmap_atomic(sg_page(sg), KM_IRQ0) + sg- 
> >offset;
> -                memcpy(&cdata[xfer_cnt], buffer, min_cnt);
> -               kunmap_atomic(buffer - sg->offset, KM_IRQ0);
> -                local_irq_restore(flags);
> +       unsigned long flags;
>
> -                xfer_cnt += min_cnt;
> -        }
> +       local_irq_save(flags);
> +       scsi_sg_copy_to_buffer(scmd, data, count);
> +       local_irq_restore(flags);
> }
>
> / 
> ****************************************************************************/
> --
> 1.5.3.7
>

Re: [PATCH 04/10] ps3rom: use sg buffer copy helper funcitons

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1048 bytes --]

On Sun, 9 Mar 2008, FUJITA Tomonori wrote:
> Note that if scsi_bufflen(cmd) is not zero, the command always has an
> sg list. So this patch doesn't do the error checking in
> fill_from_dev_buffer and fetch_to_dev_buffer did.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>

Look OK (but not really tested yet by me).

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [PATCH 01/10] block: add sg buffer copy helper functions

[Jens Axboe]

On Sun, Mar 09 2008, FUJITA Tomonori wrote:
> This patch adds new two helper functions to copy data between an SG
> table and liner buffer.
> 
> - sg_copy_from_buffer copies data from liner buffer to an SG table
> - sg_copy_to_buffer copies data from an SG table to liner buffer
> 
> sg_copy_from_buffer always call flush_kernel_dcache_page. It's not
> necessary for everyone but it's a no-op on most architectures and in
> general the API is not used in performance critical path.

Looks good!

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

-- 
Jens Axboe

Re: [PATCH 00/10] sg buffer copy helper functions

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 835 bytes --]

On Sun, 9 Mar 2008, FUJITA Tomonori wrote:
> This patchset adds new two helper functions to copy data between an SG
> table and liner buffer, and converts severl LLDs to use them.
            ^^^^^^^^^^^^
a linear buffer (also in the patches).

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Mon, 10 Mar 2008 11:14:49 +0100 (CET)
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> On Sun, 9 Mar 2008, FUJITA Tomonori wrote:
> > This patchset adds new two helper functions to copy data between an SG
> > table and liner buffer, and converts severl LLDs to use them.
>             ^^^^^^^^^^^^
> a linear buffer (also in the patches).

Ah, a stupid typo. I'll fix it on the next round.

Thanks,

Re: [PATCH 00/10] sg buffer copy helper functions

[Boaz Harrosh]

On Sun, Mar 09 2008 at 6:44 +0200, FUJITA Tomonori <fujita.tomonori-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org> wrote:
> This patchset adds new two helper functions to copy data between an SG
> table and liner buffer, and converts severl LLDs to use them.
> 
> The new APIs are used mainly in the code to spoof SCSI commands
> (INQUIRY, READ CAPACITY, MODE SENSE etc), that is, LLDs build a fake
> reposense and copy it to the sg list in scsi_cmnd struct. Several LLDs
> have similar functions for such code. This patchset removes such
> duplication.
> 
> Another reason to do this work is that because we relaxed the default
> alignment requirements (from 511 to 3) post 2.6.24, the above commands
> might come with multiple scatter gather entries but several LLDs make
> the assumption that such commands come with only one sg entries and
> can't handle multiple sg entries. The new APIs can handle multiple sg
> entries so LLDs don't need to care about anything.
> 
> The first patch adds the new APIs to lib/scatterlist.c and the rest
> are for SCSI. I like to push the whole patchset via scsi-misc (since
> it's easier).
> 
> This is against scsi-misc (a6680f71ca27ea78c4c4e577076aecb9ace476f1).
> 
>  arch/ia64/hp/sim/simscsi.c    |   23 ++--------
>  drivers/scsi/3w-9xxx.c        |   21 ++++-----
>  drivers/scsi/3w-xxxx.c        |   12 +----
>  drivers/scsi/aacraid/aachba.c |   49 ++++++++--------------
>  drivers/scsi/ips.c            |   50 ++++-------------------
>  drivers/scsi/ps3rom.c         |   92 ++++------------------------------------
>  drivers/scsi/scsi_debug.c     |   79 ++++++-----------------------------
>  drivers/scsi/stex.c           |   66 ++++-------------------------
>  include/linux/scatterlist.h   |    5 ++
>  include/scsi/scsi_cmnd.h      |   14 ++++++
>  lib/scatterlist.c             |   90 ++++++++++++++++++++++++++++++++++++++++
>  11 files changed, 182 insertions(+), 319 deletions(-)
> 
>

Hi Tomo. Nice cleanup, Cheers.

Have you had a look at drivers/usb/storage/protocol.c usb_stor_access_xfer_buf() ?
It looks like it could also use these, but there they have a twist where they want
to do it in parts. Do you think that the code there could also use the helpers 
presented here somehow? (I know that one of the USB guys was asking about it)

Boaz

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

CC'ed linux-ide,

On Mon, 10 Mar 2008 16:10:36 +0200
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On Sun, Mar 09 2008 at 6:44 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > This patchset adds new two helper functions to copy data between an SG
> > table and liner buffer, and converts severl LLDs to use them.
> > 
> > The new APIs are used mainly in the code to spoof SCSI commands
> > (INQUIRY, READ CAPACITY, MODE SENSE etc), that is, LLDs build a fake
> > reposense and copy it to the sg list in scsi_cmnd struct. Several LLDs
> > have similar functions for such code. This patchset removes such
> > duplication.
> > 
> > Another reason to do this work is that because we relaxed the default
> > alignment requirements (from 511 to 3) post 2.6.24, the above commands
> > might come with multiple scatter gather entries but several LLDs make
> > the assumption that such commands come with only one sg entries and
> > can't handle multiple sg entries. The new APIs can handle multiple sg
> > entries so LLDs don't need to care about anything.
> > 
> > The first patch adds the new APIs to lib/scatterlist.c and the rest
> > are for SCSI. I like to push the whole patchset via scsi-misc (since
> > it's easier).
> > 
> > This is against scsi-misc (a6680f71ca27ea78c4c4e577076aecb9ace476f1).
> > 
> >  arch/ia64/hp/sim/simscsi.c    |   23 ++--------
> >  drivers/scsi/3w-9xxx.c        |   21 ++++-----
> >  drivers/scsi/3w-xxxx.c        |   12 +----
> >  drivers/scsi/aacraid/aachba.c |   49 ++++++++--------------
> >  drivers/scsi/ips.c            |   50 ++++-------------------
> >  drivers/scsi/ps3rom.c         |   92 ++++------------------------------------
> >  drivers/scsi/scsi_debug.c     |   79 ++++++-----------------------------
> >  drivers/scsi/stex.c           |   66 ++++-------------------------
> >  include/linux/scatterlist.h   |    5 ++
> >  include/scsi/scsi_cmnd.h      |   14 ++++++
> >  lib/scatterlist.c             |   90 ++++++++++++++++++++++++++++++++++++++++
> >  11 files changed, 182 insertions(+), 319 deletions(-)
> > 
> >
> 
> Hi Tomo. Nice cleanup, Cheers.
> 
> Have you had a look at drivers/usb/storage/protocol.c usb_stor_access_xfer_buf() ?
> It looks like it could also use these, but there they have a twist where they want
> to do it in parts. Do you think that the code there could also use the helpers 
> presented here somehow? (I know that one of the USB guys was asking about it)

I've not. If the USB people are eager to use the new APIs, I'll try
though I prefer to keep them simple.

There are other potential users of the new APIs, gdth and libata.

I think that gdth can use the APIs without any changes to the APIs but
I leave it to a gdth expert.

ata_scsi_simulate could use the new APIs but it directly builds a
response in the buffer of a sg list instead of building a response in
temporary buffer and copying it to a sg list. So the new APIs are not
fit for it. If libata people are fine with switching to the latter,
I'll send a patch.

Re: [PATCH 00/10] sg buffer copy helper functions

[Boaz Harrosh]

On Tue, Mar 11 2008 at 0:39 +0200, FUJITA Tomonori <tomof@acm.org> wrote:
> CC'ed linux-ide,
> 
> On Mon, 10 Mar 2008 16:10:36 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> On Sun, Mar 09 2008 at 6:44 +0200, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>> This patchset adds new two helper functions to copy data between an SG
>>> table and liner buffer, and converts severl LLDs to use them.
>>>
>>> The new APIs are used mainly in the code to spoof SCSI commands
>>> (INQUIRY, READ CAPACITY, MODE SENSE etc), that is, LLDs build a fake
>>> reposense and copy it to the sg list in scsi_cmnd struct. Several LLDs
>>> have similar functions for such code. This patchset removes such
>>> duplication.
>>>
>>> Another reason to do this work is that because we relaxed the default
>>> alignment requirements (from 511 to 3) post 2.6.24, the above commands
>>> might come with multiple scatter gather entries but several LLDs make
>>> the assumption that such commands come with only one sg entries and
>>> can't handle multiple sg entries. The new APIs can handle multiple sg
>>> entries so LLDs don't need to care about anything.
>>>
>>> The first patch adds the new APIs to lib/scatterlist.c and the rest
>>> are for SCSI. I like to push the whole patchset via scsi-misc (since
>>> it's easier).
>>>
>>> This is against scsi-misc (a6680f71ca27ea78c4c4e577076aecb9ace476f1).
>>>
>>>  arch/ia64/hp/sim/simscsi.c    |   23 ++--------
>>>  drivers/scsi/3w-9xxx.c        |   21 ++++-----
>>>  drivers/scsi/3w-xxxx.c        |   12 +----
>>>  drivers/scsi/aacraid/aachba.c |   49 ++++++++--------------
>>>  drivers/scsi/ips.c            |   50 ++++-------------------
>>>  drivers/scsi/ps3rom.c         |   92 ++++------------------------------------
>>>  drivers/scsi/scsi_debug.c     |   79 ++++++-----------------------------
>>>  drivers/scsi/stex.c           |   66 ++++-------------------------
>>>  include/linux/scatterlist.h   |    5 ++
>>>  include/scsi/scsi_cmnd.h      |   14 ++++++
>>>  lib/scatterlist.c             |   90 ++++++++++++++++++++++++++++++++++++++++
>>>  11 files changed, 182 insertions(+), 319 deletions(-)
>>>
>>>
>> Hi Tomo. Nice cleanup, Cheers.
>>
>> Have you had a look at drivers/usb/storage/protocol.c usb_stor_access_xfer_buf() ?
>> It looks like it could also use these, but there they have a twist where they want
>> to do it in parts. Do you think that the code there could also use the helpers 
>> presented here somehow? (I know that one of the USB guys was asking about it)
> 
> I've not. If the USB people are eager to use the new APIs, I'll try
> though I prefer to keep them simple.
> 

CC: Alan Stern <stern@rowland.harvard.edu>
CC: Matthew Dharm <mdharm-scsi@one-eyed-alien.net>

> There are other potential users of the new APIs, gdth and libata.
> 
> I think that gdth can use the APIs without any changes to the APIs but
> I leave it to a gdth expert.

Sure I can do that. Thanks

> 
> ata_scsi_simulate could use the new APIs but it directly builds a
> response in the buffer of a sg list instead of building a response in
> temporary buffer and copying it to a sg list. So the new APIs are not
> fit for it. If libata people are fine with switching to the latter,
> I'll send a patch.

Boaz

Re: [PATCH 00/10] sg buffer copy helper functions

[Alan Stern]

On Tue, 11 Mar 2008, FUJITA Tomonori wrote:

> On Mon, 10 Mar 2008 16:10:36 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:

> > Have you had a look at drivers/usb/storage/protocol.c usb_stor_access_xfer_buf() ?
> > It looks like it could also use these, but there they have a twist where they want
> > to do it in parts. Do you think that the code there could also use the helpers 
> > presented here somehow? (I know that one of the USB guys was asking about it)
> 
> I've not. If the USB people are eager to use the new APIs, I'll try
> though I prefer to keep them simple.

It would be great if the code could be removed from usb-storage and put
in a central library.  The problem is, as Boaz mentioned, that some of
the subdrivers need to transfer their data in pieces.

It may be that the device is able to send or receive only a few blocks 
at a time, or it may be that the blocks aren't stored in continguous 
locations on the device.  Either way, the drivers need to do multiple 
transfers, each starting from where the previous one left off.

It shouldn't be too hard to adjust your code to make this work.  
Instead of passing sgl and nents directly to sg_copy_buffer(), pass a
pointer to a structure containing fields for sgl, nents, n, sg_off, and
sg_copy.  Then the caller could retain the ending values for use in a
later call.

Does this sound reasonable?

Alan Stern

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Tue, 11 Mar 2008 16:09:26 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Tue, 11 Mar 2008, FUJITA Tomonori wrote:
> 
> > On Mon, 10 Mar 2008 16:10:36 +0200
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> > > Have you had a look at drivers/usb/storage/protocol.c usb_stor_access_xfer_buf() ?
> > > It looks like it could also use these, but there they have a twist where they want
> > > to do it in parts. Do you think that the code there could also use the helpers 
> > > presented here somehow? (I know that one of the USB guys was asking about it)
> > 
> > I've not. If the USB people are eager to use the new APIs, I'll try
> > though I prefer to keep them simple.
> 
> It would be great if the code could be removed from usb-storage and put
> in a central library.  The problem is, as Boaz mentioned, that some of
> the subdrivers need to transfer their data in pieces.

I'm fine with add a trick for USB to the APIs as long as USB people
inspect and test the trick ;)


> It may be that the device is able to send or receive only a few blocks 
> at a time, or it may be that the blocks aren't stored in continguous 
> locations on the device.  Either way, the drivers need to do multiple 
> transfers, each starting from where the previous one left off.
> 
> It shouldn't be too hard to adjust your code to make this work.  
> Instead of passing sgl and nents directly to sg_copy_buffer(), pass a
> pointer to a structure containing fields for sgl, nents, n, sg_off, and
> sg_copy.  Then the caller could retain the ending values for use in a
> later call.
> 
> Does this sound reasonable?

How about this?

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index a3d567a..8951e3c 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -213,6 +213,13 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
 		     sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
 
+int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
+		   unsigned long *offset, void *buf, int buflen, int to_buffer);
+int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+			void *buf, int buflen);
+int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+		      void *buf, int buflen);
+
 /*
  * Maximum number of entries that will be allocated in one piece, if
  * a list larger than this is required then chaining will be utilized.
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index acca490..587188c 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -8,6 +8,7 @@
  */
 #include <linux/module.h>
 #include <linux/scatterlist.h>
+#include <linux/highmem.h>
 
 /**
  * sg_next - return the next scatterlist entry in a list
@@ -292,3 +293,111 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
 	return ret;
 }
 EXPORT_SYMBOL(sg_alloc_table);
+
+int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
+		   unsigned long *offset, void *buf, int buflen, int to_buffer)
+{
+	struct scatterlist *sg;
+	unsigned long buf_off = 0;
+	int i;
+
+	WARN_ON(!irqs_disabled());
+
+	for_each_sg(*sgl, sg, nents, i) {
+		struct page *page;
+		int n = 0;
+		unsigned int sg_off = sg->offset;
+		unsigned int sg_copy = sg->length;
+
+		BUG_ON(*offset > sg_copy);
+
+		if (!buflen)
+			break;
+
+		sg_off += *offset;
+		n = sg_off >> PAGE_SHIFT;
+		sg_off &= ~PAGE_MASK;
+		sg_copy -= *offset;
+
+		if (sg_copy > buflen) {
+			sg_copy = buflen;
+			*offset += sg_copy;
+		} else
+			*offset = 0;
+
+		buflen -= sg_copy;
+
+		while (sg_copy > 0) {
+			unsigned int page_copy;
+			void *p;
+
+			page_copy = PAGE_SIZE - sg_off;
+			if (page_copy > sg_copy)
+				page_copy = sg_copy;
+
+			page = nth_page(sg_page(sg), n);
+			p = kmap_atomic(page, KM_BIO_SRC_IRQ);
+
+			if (to_buffer)
+				memcpy(buf + buf_off, p + sg_off, page_copy);
+			else {
+				memcpy(p + sg_off, buf + buf_off, page_copy);
+				flush_kernel_dcache_page(page);
+			}
+
+			kunmap_atomic(p, KM_BIO_SRC_IRQ);
+
+			buf_off += page_copy;
+			sg_off += page_copy;
+			if (sg_off == PAGE_SIZE) {
+				sg_off = 0;
+				n++;
+			}
+			sg_copy -= page_copy;
+		}
+	}
+
+	*sgl = sg;
+
+	return buf_off;
+}
+
+/**
+ * sg_copy_from_buffer - Copy from liner buffer to an SG table
+ * @sgl:		 The SG table
+ * @nents:		 Number of SG entries
+ * @buf:		 Where to copy from
+ * @buflen:		 The number of bytes to copy
+ *
+ * Returns the number of copied byte.
+ *
+ **/
+int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+			void *buf, int buflen)
+{
+	struct scatterlist *s = sgl;
+	unsigned long offset = 0;
+
+	return sg_copy_buffer(&s, nents, &offset, buf, buflen, 0);
+}
+EXPORT_SYMBOL(sg_copy_from_buffer);
+
+/**
+ * sg_copy_to_buffer - Copy from an SG table to liner buffer
+ * @sgl:		 The SG table
+ * @nents:		 Number of SG entries
+ * @buf:		 Where to copy to
+ * @buflen:		 The number of bytes to copy
+ *
+ * Returns the number of copied byte.
+ *
+ **/
+int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+		      void *buf, int buflen)
+{
+	struct scatterlist *s = sgl;
+	unsigned long offset = 0;
+
+	return sg_copy_buffer(&s, nents, &offset, buf, buflen, 1);
+}
+EXPORT_SYMBOL(sg_copy_to_buffer);

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Wed, 12 Mar 2008 09:14:00 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Tue, 11 Mar 2008 16:09:26 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Tue, 11 Mar 2008, FUJITA Tomonori wrote:
> > 
> > > On Mon, 10 Mar 2008 16:10:36 +0200
> > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> > > > Have you had a look at drivers/usb/storage/protocol.c usb_stor_access_xfer_buf() ?
> > > > It looks like it could also use these, but there they have a twist where they want
> > > > to do it in parts. Do you think that the code there could also use the helpers 
> > > > presented here somehow? (I know that one of the USB guys was asking about it)
> > > 
> > > I've not. If the USB people are eager to use the new APIs, I'll try
> > > though I prefer to keep them simple.
> > 
> > It would be great if the code could be removed from usb-storage and put
> > in a central library.  The problem is, as Boaz mentioned, that some of
> > the subdrivers need to transfer their data in pieces.
> 
> I'm fine with add a trick for USB to the APIs as long as USB people
> inspect and test the trick ;)
> 
> 
> > It may be that the device is able to send or receive only a few blocks 
> > at a time, or it may be that the blocks aren't stored in continguous 
> > locations on the device.  Either way, the drivers need to do multiple 
> > transfers, each starting from where the previous one left off.
> > 
> > It shouldn't be too hard to adjust your code to make this work.  
> > Instead of passing sgl and nents directly to sg_copy_buffer(), pass a
> > pointer to a structure containing fields for sgl, nents, n, sg_off, and
> > sg_copy.  Then the caller could retain the ending values for use in a
> > later call.
> > 
> > Does this sound reasonable?
> 
> How about this?
> 
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index a3d567a..8951e3c 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -213,6 +213,13 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
>  		     sg_alloc_fn *);
>  int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
>  
> +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
> +		   unsigned long *offset, void *buf, int buflen, int to_buffer);
> +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> +			void *buf, int buflen);
> +int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> +		      void *buf, int buflen);
> +

It enables us to simplify usb_stor_access_xfer_buf like this, I think
(it's not tested).

diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index b9b8ede..0992809 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -156,70 +156,11 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
 	unsigned int buflen, struct scsi_cmnd *srb, struct scatterlist **sgptr,
 	unsigned int *offset, enum xfer_buf_dir dir)
 {
-	unsigned int cnt;
-	struct scatterlist *sg = *sgptr;
-
-	/* We have to go through the list one entry
-	 * at a time.  Each s-g entry contains some number of pages, and
-	 * each page has to be kmap()'ed separately.  If the page is already
-	 * in kernel-addressable memory then kmap() will return its address.
-	 * If the page is not directly accessible -- such as a user buffer
-	 * located in high memory -- then kmap() will map it to a temporary
-	 * position in the kernel's virtual address space.
-	 */
-
-	if (!sg)
-		sg = scsi_sglist(srb);
+	if (!*sgptr)
+		*sgptr = scsi_sglist(srb);
 
-	/* This loop handles a single s-g list entry, which may
-	 * include multiple pages.  Find the initial page structure
-	 * and the starting offset within the page, and update
-	 * the *offset and **sgptr values for the next loop.
-	 */
-	cnt = 0;
-	while (cnt < buflen && sg) {
-		struct page *page = sg_page(sg) +
-				((sg->offset + *offset) >> PAGE_SHIFT);
-		unsigned int poff = (sg->offset + *offset) & (PAGE_SIZE-1);
-		unsigned int sglen = sg->length - *offset;
-
-		if (sglen > buflen - cnt) {
-
-			/* Transfer ends within this s-g entry */
-			sglen = buflen - cnt;
-			*offset += sglen;
-		} else {
-
-			/* Transfer continues to next s-g entry */
-			*offset = 0;
-			sg = sg_next(sg);
-		}
-
-		/* Transfer the data for all the pages in this
-			* s-g entry.  For each page: call kmap(), do the
-			* transfer, and call kunmap() immediately after. */
-		while (sglen > 0) {
-			unsigned int plen = min(sglen, (unsigned int)
-					PAGE_SIZE - poff);
-			unsigned char *ptr = kmap(page);
-
-			if (dir == TO_XFER_BUF)
-				memcpy(ptr + poff, buffer + cnt, plen);
-			else
-				memcpy(buffer + cnt, ptr + poff, plen);
-			kunmap(page);
-
-			/* Start at the beginning of the next page */
-			poff = 0;
-			++page;
-			cnt += plen;
-			sglen -= plen;
-		}
-	}
-	*sgptr = sg;
-
-	/* Return the amount actually transferred */
-	return cnt;
+	return sg_copy_buffer(sgptr, scsi_sg_count(srb),
+			      offset, buffer, buflen, dir != TO_XFER_BUF);
 }
 
 /* Store the contents of buffer into srb's transfer buffer and set the

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Wed, 12 Mar 2008 09:28:56 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Wed, 12 Mar 2008 09:14:00 +0900
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > On Tue, 11 Mar 2008 16:09:26 -0400 (EDT)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > On Tue, 11 Mar 2008, FUJITA Tomonori wrote:
> > > 
> > > > On Mon, 10 Mar 2008 16:10:36 +0200
> > > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > 
> > > > > Have you had a look at drivers/usb/storage/protocol.c usb_stor_access_xfer_buf() ?
> > > > > It looks like it could also use these, but there they have a twist where they want
> > > > > to do it in parts. Do you think that the code there could also use the helpers 
> > > > > presented here somehow? (I know that one of the USB guys was asking about it)
> > > > 
> > > > I've not. If the USB people are eager to use the new APIs, I'll try
> > > > though I prefer to keep them simple.
> > > 
> > > It would be great if the code could be removed from usb-storage and put
> > > in a central library.  The problem is, as Boaz mentioned, that some of
> > > the subdrivers need to transfer their data in pieces.
> > 
> > I'm fine with add a trick for USB to the APIs as long as USB people
> > inspect and test the trick ;)
> > 
> > 
> > > It may be that the device is able to send or receive only a few blocks 
> > > at a time, or it may be that the blocks aren't stored in continguous 
> > > locations on the device.  Either way, the drivers need to do multiple 
> > > transfers, each starting from where the previous one left off.
> > > 
> > > It shouldn't be too hard to adjust your code to make this work.  
> > > Instead of passing sgl and nents directly to sg_copy_buffer(), pass a
> > > pointer to a structure containing fields for sgl, nents, n, sg_off, and
> > > sg_copy.  Then the caller could retain the ending values for use in a
> > > later call.
> > > 
> > > Does this sound reasonable?
> > 
> > How about this?
> > 
> > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> > index a3d567a..8951e3c 100644
> > --- a/include/linux/scatterlist.h
> > +++ b/include/linux/scatterlist.h
> > @@ -213,6 +213,13 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
> >  		     sg_alloc_fn *);
> >  int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
> >  
> > +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
> > +		   unsigned long *offset, void *buf, int buflen, int to_buffer);
> > +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> > +			void *buf, int buflen);
> > +int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> > +		      void *buf, int buflen);
> > +
> 
> It enables us to simplify usb_stor_access_xfer_buf like this, I think
> (it's not tested).

I fixed some minor issues (comments, warning, etc) and I've uploaded
the patchset:

git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git sg-copy

Re: [PATCH 00/10] sg buffer copy helper functions

[Alan Stern]

On Wed, 12 Mar 2008, FUJITA Tomonori wrote:

> It enables us to simplify usb_stor_access_xfer_buf like this, I think
> (it's not tested).
> 
> diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
> index b9b8ede..0992809 100644
> --- a/drivers/usb/storage/protocol.c
> +++ b/drivers/usb/storage/protocol.c
> @@ -156,70 +156,11 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
>  	unsigned int buflen, struct scsi_cmnd *srb, struct scatterlist **sgptr,
>  	unsigned int *offset, enum xfer_buf_dir dir)
>  {
> -	unsigned int cnt;
> -	struct scatterlist *sg = *sgptr;
> -
> -	/* We have to go through the list one entry
> -	 * at a time.  Each s-g entry contains some number of pages, and
> -	 * each page has to be kmap()'ed separately.  If the page is already
> -	 * in kernel-addressable memory then kmap() will return its address.
> -	 * If the page is not directly accessible -- such as a user buffer
> -	 * located in high memory -- then kmap() will map it to a temporary
> -	 * position in the kernel's virtual address space.
> -	 */
> -
> -	if (!sg)
> -		sg = scsi_sglist(srb);
> +	if (!*sgptr)
> +		*sgptr = scsi_sglist(srb);
>  
> -	/* This loop handles a single s-g list entry, which may
> -	 * include multiple pages.  Find the initial page structure
> -	 * and the starting offset within the page, and update
> -	 * the *offset and **sgptr values for the next loop.
> -	 */
> -	cnt = 0;
> -	while (cnt < buflen && sg) {
> -		struct page *page = sg_page(sg) +
> -				((sg->offset + *offset) >> PAGE_SHIFT);
> -		unsigned int poff = (sg->offset + *offset) & (PAGE_SIZE-1);
> -		unsigned int sglen = sg->length - *offset;
> -
> -		if (sglen > buflen - cnt) {
> -
> -			/* Transfer ends within this s-g entry */
> -			sglen = buflen - cnt;
> -			*offset += sglen;
> -		} else {
> -
> -			/* Transfer continues to next s-g entry */
> -			*offset = 0;
> -			sg = sg_next(sg);
> -		}
> -
> -		/* Transfer the data for all the pages in this
> -			* s-g entry.  For each page: call kmap(), do the
> -			* transfer, and call kunmap() immediately after. */
> -		while (sglen > 0) {
> -			unsigned int plen = min(sglen, (unsigned int)
> -					PAGE_SIZE - poff);
> -			unsigned char *ptr = kmap(page);
> -
> -			if (dir == TO_XFER_BUF)
> -				memcpy(ptr + poff, buffer + cnt, plen);
> -			else
> -				memcpy(buffer + cnt, ptr + poff, plen);
> -			kunmap(page);
> -
> -			/* Start at the beginning of the next page */
> -			poff = 0;
> -			++page;
> -			cnt += plen;
> -			sglen -= plen;
> -		}
> -	}
> -	*sgptr = sg;
> -
> -	/* Return the amount actually transferred */
> -	return cnt;
> +	return sg_copy_buffer(sgptr, scsi_sg_count(srb),
> +			      offset, buffer, buflen, dir != TO_XFER_BUF);
>  }

It's a big simplification!

There are two problems.  One is the types of the arguments and return 
value.  The other is that local interrupts need to be disabled.

Alan Stern

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Wed, 12 Mar 2008 12:04:26 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, 12 Mar 2008, FUJITA Tomonori wrote:
> 
> > It enables us to simplify usb_stor_access_xfer_buf like this, I think
> > (it's not tested).
> > 
> > diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
> > index b9b8ede..0992809 100644
> > --- a/drivers/usb/storage/protocol.c
> > +++ b/drivers/usb/storage/protocol.c
> > @@ -156,70 +156,11 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
> >  	unsigned int buflen, struct scsi_cmnd *srb, struct scatterlist **sgptr,
> >  	unsigned int *offset, enum xfer_buf_dir dir)
> >  {
> > -	unsigned int cnt;
> > -	struct scatterlist *sg = *sgptr;
> > -
> > -	/* We have to go through the list one entry
> > -	 * at a time.  Each s-g entry contains some number of pages, and
> > -	 * each page has to be kmap()'ed separately.  If the page is already
> > -	 * in kernel-addressable memory then kmap() will return its address.
> > -	 * If the page is not directly accessible -- such as a user buffer
> > -	 * located in high memory -- then kmap() will map it to a temporary
> > -	 * position in the kernel's virtual address space.
> > -	 */
> > -
> > -	if (!sg)
> > -		sg = scsi_sglist(srb);
> > +	if (!*sgptr)
> > +		*sgptr = scsi_sglist(srb);
> >  
> > -	/* This loop handles a single s-g list entry, which may
> > -	 * include multiple pages.  Find the initial page structure
> > -	 * and the starting offset within the page, and update
> > -	 * the *offset and **sgptr values for the next loop.
> > -	 */
> > -	cnt = 0;
> > -	while (cnt < buflen && sg) {
> > -		struct page *page = sg_page(sg) +
> > -				((sg->offset + *offset) >> PAGE_SHIFT);
> > -		unsigned int poff = (sg->offset + *offset) & (PAGE_SIZE-1);
> > -		unsigned int sglen = sg->length - *offset;
> > -
> > -		if (sglen > buflen - cnt) {
> > -
> > -			/* Transfer ends within this s-g entry */
> > -			sglen = buflen - cnt;
> > -			*offset += sglen;
> > -		} else {
> > -
> > -			/* Transfer continues to next s-g entry */
> > -			*offset = 0;
> > -			sg = sg_next(sg);
> > -		}
> > -
> > -		/* Transfer the data for all the pages in this
> > -			* s-g entry.  For each page: call kmap(), do the
> > -			* transfer, and call kunmap() immediately after. */
> > -		while (sglen > 0) {
> > -			unsigned int plen = min(sglen, (unsigned int)
> > -					PAGE_SIZE - poff);
> > -			unsigned char *ptr = kmap(page);
> > -
> > -			if (dir == TO_XFER_BUF)
> > -				memcpy(ptr + poff, buffer + cnt, plen);
> > -			else
> > -				memcpy(buffer + cnt, ptr + poff, plen);
> > -			kunmap(page);
> > -
> > -			/* Start at the beginning of the next page */
> > -			poff = 0;
> > -			++page;
> > -			cnt += plen;
> > -			sglen -= plen;
> > -		}
> > -	}
> > -	*sgptr = sg;
> > -
> > -	/* Return the amount actually transferred */
> > -	return cnt;
> > +	return sg_copy_buffer(sgptr, scsi_sg_count(srb),
> > +			      offset, buffer, buflen, dir != TO_XFER_BUF);
> >  }
> 
> It's a big simplification!
> 
> There are two problems.  One is the types of the arguments and return 
> value.

They should be ok with the updated patch.


> The other is that local interrupts need to be disabled.

Can you disable local interrupts here?

Basically, the APIs are used in queuecommand.

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Thu, 13 Mar 2008 09:03:26 +0900
FUJITA Tomonori <tomof@acm.org> wrote:

> > There are two problems.  One is the types of the arguments and return 
> > value.
> 
> They should be ok with the updated patch.
> 
> 
> > The other is that local interrupts need to be disabled.
> 
> Can you disable local interrupts here?

I meant, are you fine with disabling local interrupts here?

If you don't like that, we need to add another argument and seveal
'if' to sg_copy_buffer. But I prefer to keep it simple.


> Basically, the APIs are used in queuecommand.

Re: [PATCH 00/10] sg buffer copy helper functions

[Alan Stern]

On Thu, 13 Mar 2008, FUJITA Tomonori wrote:

> On Thu, 13 Mar 2008 09:03:26 +0900
> FUJITA Tomonori <tomof@acm.org> wrote:
> 
> > > There are two problems.  One is the types of the arguments and return 
> > > value.
> > 
> > They should be ok with the updated patch.
> > 
> > 
> > > The other is that local interrupts need to be disabled.
> > 
> > Can you disable local interrupts here?
> 
> I meant, are you fine with disabling local interrupts here?
> 
> If you don't like that, we need to add another argument and seveal
> 'if' to sg_copy_buffer. But I prefer to keep it simple.

Disabling local interrupts in usb_stor_access_xfer_buf() is okay.

Alan Stern

Re: [PATCH 00/10] sg buffer copy helper functions

[Alan Stern]

On Wed, 12 Mar 2008, FUJITA Tomonori wrote:

> I fixed some minor issues (comments, warning, etc) and I've uploaded
> the patchset:

> git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git 
> sg-copy

Sorry, I don't see it there yet in the gitweb interface.  Here are my
comments on the parts you posted earlier.

> How about this?
> 
> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> index a3d567a..8951e3c 100644
> --- a/include/linux/scatterlist.h
> +++ b/include/linux/scatterlist.h
> @@ -213,6 +213,13 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
>  		     sg_alloc_fn *);
>  int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
>  
> +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
> +		   unsigned long *offset, void *buf, int buflen, int to_buffer);
> +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> +			void *buf, int buflen);
> +int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> +		      void *buf, int buflen);

These routines probably should not return int.  Maybe unsigned int or 
unsigned long.  If you really want to be picky, it should be size_t.

Same goes for the type of the buflen parameter.

> +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
> +		   unsigned long *offset, void *buf, int buflen, int to_buffer)
> +{
> +	struct scatterlist *sg;
> +	unsigned long buf_off = 0;

The type of buf_off should be the same as the function's return type.

> +	int i;
> +
> +	WARN_ON(!irqs_disabled());
> +
> +	for_each_sg(*sgl, sg, nents, i) {

Will there be a problem in subsequent calls if *sgl has been
incremented but nents hasn't been changed?  Maybe nents needs to be a 
pointer also.

> +		struct page *page;
> +		int n = 0;
> +		unsigned int sg_off = sg->offset;
> +		unsigned int sg_copy = sg->length;
> +
> +		BUG_ON(*offset > sg_copy);
> +
> +		if (!buflen)
> +			break;
> +
> +		sg_off += *offset;
> +		n = sg_off >> PAGE_SHIFT;
> +		sg_off &= ~PAGE_MASK;
> +		sg_copy -= *offset;
> +
> +		if (sg_copy > buflen) {
> +			sg_copy = buflen;
> +			*offset += sg_copy;
> +		} else
> +			*offset = 0;
> +
> +		buflen -= sg_copy;
> +
> +		while (sg_copy > 0) {
> +			unsigned int page_copy;
> +			void *p;
> +
> +			page_copy = PAGE_SIZE - sg_off;
> +			if (page_copy > sg_copy)
> +				page_copy = sg_copy;
> +
> +			page = nth_page(sg_page(sg), n);
> +			p = kmap_atomic(page, KM_BIO_SRC_IRQ);
> +
> +			if (to_buffer)
> +				memcpy(buf + buf_off, p + sg_off, page_copy);
> +			else {
> +				memcpy(p + sg_off, buf + buf_off, page_copy);
> +				flush_kernel_dcache_page(page);
> +			}
> +
> +			kunmap_atomic(p, KM_BIO_SRC_IRQ);
> +
> +			buf_off += page_copy;
> +			sg_off += page_copy;
> +			if (sg_off == PAGE_SIZE) {
> +				sg_off = 0;
> +				n++;
> +			}
> +			sg_copy -= page_copy;
> +		}

Here you need to add:

		if (*offset)
			break;

Otherwise *sgl will be incremented wrongly if the transfer ends in the
middle of an sg entry.

> +	}
> +
> +	*sgl = sg;
> +
> +	return buf_off;
> +}

> +
> +/**
> + * sg_copy_from_buffer - Copy from liner buffer to an SG table

s/liner/linear/

> + * @sgl:		 The SG table
> + * @nents:		 Number of SG entries
> + * @buf:		 Where to copy from
> + * @buflen:		 The number of bytes to copy
> + *
> + * Returns the number of copied byte.

s/byte/bytes/

> + *
> + **/
> +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> +			void *buf, int buflen)
> +{
> +	struct scatterlist *s = sgl;
> +	unsigned long offset = 0;
> +
> +	return sg_copy_buffer(&s, nents, &offset, buf, buflen, 0);

You don't have to copy sgl into s.  Just pass &sgl directly.

> +}
> +EXPORT_SYMBOL(sg_copy_from_buffer);
> +
> +/**
> + * sg_copy_to_buffer - Copy from an SG table to liner buffer

s/liner/linear/

> + * @sgl:		 The SG table
> + * @nents:		 Number of SG entries
> + * @buf:		 Where to copy to
> + * @buflen:		 The number of bytes to copy
> + *
> + * Returns the number of copied byte.

s/byte/bytes/

> + *
> + **/
> +int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> +		      void *buf, int buflen)
> +{
> +	struct scatterlist *s = sgl;
> +	unsigned long offset = 0;
> +
> +	return sg_copy_buffer(&s, nents, &offset, buf, buflen, 1);

Same thing here.

> +}
> +EXPORT_SYMBOL(sg_copy_to_buffer);

On the whole it looks good.

Alan Stern

Re: [PATCH 00/10] sg buffer copy helper functions

[Boaz Harrosh]

On Wed, Mar 12 2008 at 18:01 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 12 Mar 2008, FUJITA Tomonori wrote:
> 
>> I fixed some minor issues (comments, warning, etc) and I've uploaded
>> the patchset:
> 
>> git://git.kernel.org/pub/scm/linux/kernel/git/tomo/linux-2.6-misc.git 
>> sg-copy
> 
> Sorry, I don't see it there yet in the gitweb interface.  Here are my
> comments on the parts you posted earlier.
> 
>> How about this?
>>
>> diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
>> index a3d567a..8951e3c 100644
>> --- a/include/linux/scatterlist.h
>> +++ b/include/linux/scatterlist.h
>> @@ -213,6 +213,13 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
>>  		     sg_alloc_fn *);
>>  int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
>>  
>> +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
>> +		   unsigned long *offset, void *buf, int buflen, int to_buffer);
>> +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
>> +			void *buf, int buflen);
>> +int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
>> +		      void *buf, int buflen);
> 
> These routines probably should not return int.  Maybe unsigned int or 
> unsigned long.  If you really want to be picky, it should be size_t.
> 
> Same goes for the type of the buflen parameter.
> 
>> +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
>> +		   unsigned long *offset, void *buf, int buflen, int to_buffer)
>> +{
>> +	struct scatterlist *sg;
>> +	unsigned long buf_off = 0;
> 
> The type of buf_off should be the same as the function's return type.
> 
>> +	int i;
>> +
>> +	WARN_ON(!irqs_disabled());
>> +
>> +	for_each_sg(*sgl, sg, nents, i) {
> 
> Will there be a problem in subsequent calls if *sgl has been
> incremented but nents hasn't been changed?  Maybe nents needs to be a 
> pointer also.
> 
>> +		struct page *page;
>> +		int n = 0;
>> +		unsigned int sg_off = sg->offset;
>> +		unsigned int sg_copy = sg->length;
>> +
>> +		BUG_ON(*offset > sg_copy);
>> +
>> +		if (!buflen)
>> +			break;
>> +
>> +		sg_off += *offset;
>> +		n = sg_off >> PAGE_SHIFT;
>> +		sg_off &= ~PAGE_MASK;
>> +		sg_copy -= *offset;
>> +
>> +		if (sg_copy > buflen) {
>> +			sg_copy = buflen;
>> +			*offset += sg_copy;
>> +		} else
>> +			*offset = 0;
>> +
>> +		buflen -= sg_copy;
>> +
>> +		while (sg_copy > 0) {
>> +			unsigned int page_copy;
>> +			void *p;
>> +
>> +			page_copy = PAGE_SIZE - sg_off;
>> +			if (page_copy > sg_copy)
>> +				page_copy = sg_copy;
>> +
>> +			page = nth_page(sg_page(sg), n);
>> +			p = kmap_atomic(page, KM_BIO_SRC_IRQ);
>> +
>> +			if (to_buffer)
>> +				memcpy(buf + buf_off, p + sg_off, page_copy);
>> +			else {
>> +				memcpy(p + sg_off, buf + buf_off, page_copy);
>> +				flush_kernel_dcache_page(page);
>> +			}
>> +
>> +			kunmap_atomic(p, KM_BIO_SRC_IRQ);
>> +
>> +			buf_off += page_copy;
>> +			sg_off += page_copy;
>> +			if (sg_off == PAGE_SIZE) {
>> +				sg_off = 0;
>> +				n++;
>> +			}
>> +			sg_copy -= page_copy;
>> +		}
> 
> Here you need to add:
> 
> 		if (*offset)
> 			break;
> 
> Otherwise *sgl will be incremented wrongly if the transfer ends in the
> middle of an sg entry.
> 
>> +	}
>> +
>> +	*sgl = sg;
>> +
>> +	return buf_off;
>> +}
> 
>> +
>> +/**
>> + * sg_copy_from_buffer - Copy from liner buffer to an SG table
> 
> s/liner/linear/
> 
>> + * @sgl:		 The SG table
>> + * @nents:		 Number of SG entries
>> + * @buf:		 Where to copy from
>> + * @buflen:		 The number of bytes to copy
>> + *
>> + * Returns the number of copied byte.
> 
> s/byte/bytes/
> 
>> + *
>> + **/
>> +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
>> +			void *buf, int buflen)
>> +{
>> +	struct scatterlist *s = sgl;
>> +	unsigned long offset = 0;
>> +
>> +	return sg_copy_buffer(&s, nents, &offset, buf, buflen, 0);
> 
> You don't have to copy sgl into s.  Just pass &sgl directly.
> 
>> +}
>> +EXPORT_SYMBOL(sg_copy_from_buffer);
>> +
>> +/**
>> + * sg_copy_to_buffer - Copy from an SG table to liner buffer
> 
> s/liner/linear/
> 
>> + * @sgl:		 The SG table
>> + * @nents:		 Number of SG entries
>> + * @buf:		 Where to copy to
>> + * @buflen:		 The number of bytes to copy
>> + *
>> + * Returns the number of copied byte.
> 
> s/byte/bytes/
> 
>> + *
>> + **/
>> +int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
>> +		      void *buf, int buflen)
>> +{
>> +	struct scatterlist *s = sgl;
>> +	unsigned long offset = 0;
>> +
>> +	return sg_copy_buffer(&s, nents, &offset, buf, buflen, 1);
> 
> Same thing here.
> 
>> +}
>> +EXPORT_SYMBOL(sg_copy_to_buffer);
> 
> On the whole it looks good.
> 
> Alan Stern
> 
> 

I agree with all your comments. I think I liked your original Idea of
putting all the sg_copy_buffer() parameters in a structure that is then
is easier for resubmission and future changes. It will not change TOMO's
patchset because the two wrappers stay the same and only USB is affected.

TOMO
could we not do better then that *disable interrupts* thing? I think it is
fair to say that we disallow the memory pointed-to by the sglist to share
cache lines with other concurrent IO. Is this not true already?

I mean, come on, a pure memory access like that, we must be able to do better?
No? 

Drivers that use this API can make sure to set their masks HW_CACHE_ALIGNED
and the block will bounce the buffers for them to make sure concurrent cache-line 
IO will not happen. No?

Thanks
Boaz

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Wed, 12 Mar 2008 12:01:57 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
> > index a3d567a..8951e3c 100644
> > --- a/include/linux/scatterlist.h
> > +++ b/include/linux/scatterlist.h
> > @@ -213,6 +213,13 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
> >  		     sg_alloc_fn *);
> >  int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
> >  
> > +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
> > +		   unsigned long *offset, void *buf, int buflen, int to_buffer);
> > +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> > +			void *buf, int buflen);
> > +int sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
> > +		      void *buf, int buflen);
> 
> These routines probably should not return int.  Maybe unsigned int or 
> unsigned long.  If you really want to be picky, it should be size_t.
> 
> Same goes for the type of the buflen parameter.

OK, let's use size_t.


> > +int sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
> > +		   unsigned long *offset, void *buf, int buflen, int to_buffer)
> > +{
> > +	struct scatterlist *sg;
> > +	unsigned long buf_off = 0;
> 
> The type of buf_off should be the same as the function's return type.

Fixed.


> > +	int i;
> > +
> > +	WARN_ON(!irqs_disabled());
> > +
> > +	for_each_sg(*sgl, sg, nents, i) {
> 
> Will there be a problem in subsequent calls if *sgl has been
> incremented but nents hasn't been changed?  Maybe nents needs to be a 
> pointer also.

usb_stor_access_xfer_buf doesn't check scsi_sg_count (the number of sg
entries). It assumes that callers take care about the issue.

If you want nents to be a pointer, I'm fine with it.


> > +		struct page *page;
> > +		int n = 0;
> > +		unsigned int sg_off = sg->offset;
> > +		unsigned int sg_copy = sg->length;
> > +
> > +		BUG_ON(*offset > sg_copy);
> > +
> > +		if (!buflen)
> > +			break;
> > +
> > +		sg_off += *offset;
> > +		n = sg_off >> PAGE_SHIFT;
> > +		sg_off &= ~PAGE_MASK;
> > +		sg_copy -= *offset;
> > +
> > +		if (sg_copy > buflen) {
> > +			sg_copy = buflen;
> > +			*offset += sg_copy;
> > +		} else
> > +			*offset = 0;
> > +
> > +		buflen -= sg_copy;
> > +
> > +		while (sg_copy > 0) {
> > +			unsigned int page_copy;
> > +			void *p;
> > +
> > +			page_copy = PAGE_SIZE - sg_off;
> > +			if (page_copy > sg_copy)
> > +				page_copy = sg_copy;
> > +
> > +			page = nth_page(sg_page(sg), n);
> > +			p = kmap_atomic(page, KM_BIO_SRC_IRQ);
> > +
> > +			if (to_buffer)
> > +				memcpy(buf + buf_off, p + sg_off, page_copy);
> > +			else {
> > +				memcpy(p + sg_off, buf + buf_off, page_copy);
> > +				flush_kernel_dcache_page(page);
> > +			}
> > +
> > +			kunmap_atomic(p, KM_BIO_SRC_IRQ);
> > +
> > +			buf_off += page_copy;
> > +			sg_off += page_copy;
> > +			if (sg_off == PAGE_SIZE) {
> > +				sg_off = 0;
> > +				n++;
> > +			}
> > +			sg_copy -= page_copy;
> > +		}
> 
> Here you need to add:
> 
> 		if (*offset)
> 			break;
> 
> Otherwise *sgl will be incremented wrongly if the transfer ends in the
> middle of an sg entry.

Thanks, fixed.


> > +	}
> > +
> > +	*sgl = sg;
> > +
> > +	return buf_off;
> > +}
> 
> > +
> > +/**
> > + * sg_copy_from_buffer - Copy from liner buffer to an SG table
> 
> s/liner/linear/

It was fixed in the git tree.


> > + * @sgl:		 The SG table
> > + * @nents:		 Number of SG entries
> > + * @buf:		 Where to copy from
> > + * @buflen:		 The number of bytes to copy
> > + *
> > + * Returns the number of copied byte.
> 
> s/byte/bytes/

Fixed, thanks.


> > + *
> > + **/
> > +int sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
> > +			void *buf, int buflen)
> > +{
> > +	struct scatterlist *s = sgl;
> > +	unsigned long offset = 0;
> > +
> > +	return sg_copy_buffer(&s, nents, &offset, buf, buflen, 0);
> 
> You don't have to copy sgl into s.  Just pass &sgl directly.

Duh, fixed.

Here's an updated version.

=
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index a3d567a..2f863f3 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -213,6 +213,14 @@ int __sg_alloc_table(struct sg_table *, unsigned int, unsigned int, gfp_t,
 		     sg_alloc_fn *);
 int sg_alloc_table(struct sg_table *, unsigned int, gfp_t);
 
+size_t sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
+		      unsigned int *offset, void *buf, size_t buflen,
+		      int to_buffer);
+size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+			   void *buf, size_t buflen);
+size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+			 void *buf, size_t buflen);
+
 /*
  * Maximum number of entries that will be allocated in one piece, if
  * a list larger than this is required then chaining will be utilized.
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index acca490..4f0813c 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -8,6 +8,7 @@
  */
 #include <linux/module.h>
 #include <linux/scatterlist.h>
+#include <linux/highmem.h>
 
 /**
  * sg_next - return the next scatterlist entry in a list
@@ -292,3 +293,129 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
 	return ret;
 }
 EXPORT_SYMBOL(sg_alloc_table);
+
+/**
+ * sg_copy_buffer - Copy data between a linear buffer and an SG list
+ * @sgl:		 The SG list
+ * @nents:		 Number of SG entries
+ * @offset:		 start data transfer in the first SG entry at
+ * @buf:		 Where to copy from
+ * @buflen:		 The number of bytes to copy
+ * @to_buffer: 		 transfer direction (non zero == from an sg list to a
+ * 			 buffer, 0 == from a buffer to an sg list
+ *
+ * Returns the number of copied bytes.
+ *
+ * *sgl is updated to point a SG list that next data transfer should
+ * start with. *offset is updated to a value that next data transfer
+ * should use.
+ *
+ **/
+size_t sg_copy_buffer(struct scatterlist **sgl, unsigned int nents,
+		      unsigned int *offset, void *buf, size_t buflen,
+		      int to_buffer)
+{
+	struct scatterlist *sg;
+	size_t buf_off = 0;
+	int i;
+
+	WARN_ON(!irqs_disabled());
+
+	for_each_sg(*sgl, sg, nents, i) {
+		struct page *page;
+		int n = 0;
+		unsigned int sg_off = sg->offset;
+		unsigned int sg_copy = sg->length;
+
+		BUG_ON(*offset > sg_copy);
+
+		if (!buflen)
+			break;
+
+		sg_off += *offset;
+		n = sg_off >> PAGE_SHIFT;
+		sg_off &= ~PAGE_MASK;
+		sg_copy -= *offset;
+
+		if (sg_copy > buflen) {
+			sg_copy = buflen;
+			*offset += sg_copy;
+		} else
+			*offset = 0;
+
+		buflen -= sg_copy;
+
+		while (sg_copy > 0) {
+			unsigned int page_copy;
+			void *p;
+
+			page_copy = PAGE_SIZE - sg_off;
+			if (page_copy > sg_copy)
+				page_copy = sg_copy;
+
+			page = nth_page(sg_page(sg), n);
+			p = kmap_atomic(page, KM_BIO_SRC_IRQ);
+
+			if (to_buffer)
+				memcpy(buf + buf_off, p + sg_off, page_copy);
+			else {
+				memcpy(p + sg_off, buf + buf_off, page_copy);
+				flush_kernel_dcache_page(page);
+			}
+
+			kunmap_atomic(p, KM_BIO_SRC_IRQ);
+
+			buf_off += page_copy;
+			sg_off += page_copy;
+			if (sg_off == PAGE_SIZE) {
+				sg_off = 0;
+				n++;
+			}
+			sg_copy -= page_copy;
+		}
+
+		if (*offset)
+			break;
+	}
+
+	*sgl = sg;
+
+	return buf_off;
+}
+EXPORT_SYMBOL(sg_copy_buffer);
+
+/**
+ * sg_copy_from_buffer - Copy from a linear buffer to an SG list
+ * @sgl:		 The SG list
+ * @nents:		 Number of SG entries
+ * @buf:		 Where to copy from
+ * @buflen:		 The number of bytes to copy
+ *
+ * Returns the number of copied bytes.
+ *
+ **/
+size_t sg_copy_from_buffer(struct scatterlist *sgl, unsigned int nents,
+			   void *buf, size_t buflen)
+{
+	unsigned int offset = 0;
+	return sg_copy_buffer(&sgl, nents, &offset, buf, buflen, 0);
+}
+EXPORT_SYMBOL(sg_copy_from_buffer);
+
+/**
+ * sg_copy_to_buffer - Copy from an SG list to a linear buffer
+ * @sgl:		 The SG list
+ * @nents:		 Number of SG entries
+ * @buf:		 Where to copy to
+ * @buflen:		 The number of bytes to copy
+ *
+ * Returns the number of copied bytes.
+ *
+ **/
+size_t sg_copy_to_buffer(struct scatterlist *sgl, unsigned int nents,
+			 void *buf, size_t buflen)
+{
+	unsigned int offset = 0;
+	return sg_copy_buffer(&sgl, nents, &offset, buf, buflen, 1);
+}
+EXPORT_SYMBOL(sg_copy_to_buffer);

Re: [PATCH 00/10] sg buffer copy helper functions

[Alan Stern]

On Thu, 13 Mar 2008, FUJITA Tomonori wrote:

> > > +	for_each_sg(*sgl, sg, nents, i) {
> > 
> > Will there be a problem in subsequent calls if *sgl has been
> > incremented but nents hasn't been changed?  Maybe nents needs to be a 
> > pointer also.
> 
> usb_stor_access_xfer_buf doesn't check scsi_sg_count (the number of sg
> entries). It assumes that callers take care about the issue.
> 
> If you want nents to be a pointer, I'm fine with it.

If nents doesn't change then for_each_sg() won't work right.  There 
could be an alternative macro:

/*
 * Loop over each sg element, stopping at the end of the chain
 */
#define for_each_sg_all(sglist, sg, __i)	\
	for (__i = 0, sg = (sglist); sg; __i++, sg = sg_next(sg))

If you added this macro to include/linux/scatterlist.h and used it
instead of for_each_sg() then you can get rid of nents entirely.  
However I'm not sure whether this would be safe.  Do people sometimes 
use a subset of the entries in a scatterlist?

If it isn't safe then nents would have to be passed as a pointer too.  
At this stage I think it would be better to encapsulate sgl, offset, 
and nents in a single structure than to pass multiple pointers.

Alan Stern

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Thu, 13 Mar 2008 14:32:59 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Thu, 13 Mar 2008, FUJITA Tomonori wrote:
> 
> > > > +	for_each_sg(*sgl, sg, nents, i) {
> > > 
> > > Will there be a problem in subsequent calls if *sgl has been
> > > incremented but nents hasn't been changed?  Maybe nents needs to be a 
> > > pointer also.
> > 
> > usb_stor_access_xfer_buf doesn't check scsi_sg_count (the number of sg
> > entries). It assumes that callers take care about the issue.
> > 
> > If you want nents to be a pointer, I'm fine with it.
> 
> If nents doesn't change then for_each_sg() won't work right.  There 
> could be an alternative macro:

Oops, I thought that for_each_sg is defined like:

#define for_each_sg(sglist, sg, nr, __i)	\
	for (__i = 0, sg = (sglist); __i < (nr) && sg; __i++, sg = sg_next(sg))


> /*
>  * Loop over each sg element, stopping at the end of the chain
>  */
> #define for_each_sg_all(sglist, sg, __i)	\
> 	for (__i = 0, sg = (sglist); sg; __i++, sg = sg_next(sg))
> 
> If you added this macro to include/linux/scatterlist.h and used it
> instead of for_each_sg() then you can get rid of nents entirely.  
> However I'm not sure whether this would be safe.  Do people sometimes 
> use a subset of the entries in a scatterlist?

IIRC, some drivers do that (though they might use sg_next).


I don't think that we add a new macro just for this function. We could
change for_each_sg in the above way or we could just do in
usb_stor_access_xfer_buf

for (i = 0, sg = *sgl; i < nents && sg; i++, sg = sg_next(sg))


> If it isn't safe then nents would have to be passed as a pointer too.  
> At this stage I think it would be better to encapsulate sgl, offset, 
> and nents in a single structure than to pass multiple pointers.

Re: [PATCH 00/10] sg buffer copy helper functions

[Alan Stern]

On Fri, 14 Mar 2008, FUJITA Tomonori wrote:

> > If nents doesn't change then for_each_sg() won't work right.  There 
> > could be an alternative macro:
> 
> Oops, I thought that for_each_sg is defined like:
> 
> #define for_each_sg(sglist, sg, nr, __i)	\
> 	for (__i = 0, sg = (sglist); __i < (nr) && sg; __i++, sg = sg_next(sg))
> 
> 
> > /*
> >  * Loop over each sg element, stopping at the end of the chain
> >  */
> > #define for_each_sg_all(sglist, sg, __i)	\
> > 	for (__i = 0, sg = (sglist); sg; __i++, sg = sg_next(sg))
> > 
> > If you added this macro to include/linux/scatterlist.h and used it
> > instead of for_each_sg() then you can get rid of nents entirely.  
> > However I'm not sure whether this would be safe.  Do people sometimes 
> > use a subset of the entries in a scatterlist?
> 
> IIRC, some drivers do that (though they might use sg_next).

But will usb-storage ever receive a scatterlist like that?  For 
example, if there are three 4096-byte entries in the list, but the 
transfer length is 8192 and nents is 2, then there could be a problem.

(This could happen if some software layer preallocated an sg chain and
used it over and over again, each time setting nents to whatever value
was needed for a particular transfer.)

> I don't think that we add a new macro just for this function. We could
> change for_each_sg in the above way or we could just do in
> usb_stor_access_xfer_buf
> 
> for (i = 0, sg = *sgl; i < nents && sg; i++, sg = sg_next(sg))

This wouldn't be safe in the example I just mentioned if the
usb-storage driver tried to do three transfers, each of 4096 bytes.  
All three would succeed, but in fact the third call shouldn't transfer
any data.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Fri, 14 Mar 2008 10:46:52 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Fri, 14 Mar 2008, FUJITA Tomonori wrote:
> 
> > > If nents doesn't change then for_each_sg() won't work right.  There 
> > > could be an alternative macro:
> > 
> > Oops, I thought that for_each_sg is defined like:
> > 
> > #define for_each_sg(sglist, sg, nr, __i)	\
> > 	for (__i = 0, sg = (sglist); __i < (nr) && sg; __i++, sg = sg_next(sg))
> > 
> > 
> > > /*
> > >  * Loop over each sg element, stopping at the end of the chain
> > >  */
> > > #define for_each_sg_all(sglist, sg, __i)	\
> > > 	for (__i = 0, sg = (sglist); sg; __i++, sg = sg_next(sg))
> > > 
> > > If you added this macro to include/linux/scatterlist.h and used it
> > > instead of for_each_sg() then you can get rid of nents entirely.  
> > > However I'm not sure whether this would be safe.  Do people sometimes 
> > > use a subset of the entries in a scatterlist?
> > 
> > IIRC, some drivers do that (though they might use sg_next).
> 
> But will usb-storage ever receive a scatterlist like that?  For 
> example, if there are three 4096-byte entries in the list, but the 
> transfer length is 8192 and nents is 2, then there could be a problem.

If LLDs don't use the padding or drain buffer feature (USB uses
neither), scsi midlayer doesn't send such (that is, the block layer
doesn't create such).


> (This could happen if some software layer preallocated an sg chain and
> used it over and over again, each time setting nents to whatever value
> was needed for a particular transfer.)
> 
> > I don't think that we add a new macro just for this function. We could
> > change for_each_sg in the above way or we could just do in
> > usb_stor_access_xfer_buf
> > 
> > for (i = 0, sg = *sgl; i < nents && sg; i++, sg = sg_next(sg))
> 
> This wouldn't be safe in the example I just mentioned if the
> usb-storage driver tried to do three transfers, each of 4096 bytes.  
> All three would succeed, but in fact the third call shouldn't transfer
> any data.

As I explained above, it should be safe with USB. But in general, LLDs
should not rely on a scatterlist about how much data they transfer.

Re: [PATCH 00/10] sg buffer copy helper functions

[Alan Stern]

On Sun, 16 Mar 2008, FUJITA Tomonori wrote:

> On Fri, 14 Mar 2008 10:46:52 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Fri, 14 Mar 2008, FUJITA Tomonori wrote:
> > 
> > > > If nents doesn't change then for_each_sg() won't work right.  There 
> > > > could be an alternative macro:
> > > 
> > > Oops, I thought that for_each_sg is defined like:
> > > 
> > > #define for_each_sg(sglist, sg, nr, __i)	\
> > > 	for (__i = 0, sg = (sglist); __i < (nr) && sg; __i++, sg = sg_next(sg))
> > > 
> > > 
> > > > /*
> > > >  * Loop over each sg element, stopping at the end of the chain
> > > >  */
> > > > #define for_each_sg_all(sglist, sg, __i)	\
> > > > 	for (__i = 0, sg = (sglist); sg; __i++, sg = sg_next(sg))
> > > > 
> > > > If you added this macro to include/linux/scatterlist.h and used it
> > > > instead of for_each_sg() then you can get rid of nents entirely.  
> > > > However I'm not sure whether this would be safe.  Do people sometimes 
> > > > use a subset of the entries in a scatterlist?
> > > 
> > > IIRC, some drivers do that (though they might use sg_next).
> > 
> > But will usb-storage ever receive a scatterlist like that?  For 
> > example, if there are three 4096-byte entries in the list, but the 
> > transfer length is 8192 and nents is 2, then there could be a problem.
> 
> If LLDs don't use the padding or drain buffer feature (USB uses
> neither), scsi midlayer doesn't send such (that is, the block layer
> doesn't create such).

Then it should be okay to open-code that loop and test whether sg is 
NULL.

> As I explained above, it should be safe with USB. But in general, LLDs
> should not rely on a scatterlist about how much data they transfer.

True.  Let's add a comment explaining the situation.  For example:

	/* If this routine is called multiple times for a single
	 * scatterlist, the nents value will not be updated properly.
	 * Transfers will stop when the end of the list is reached,
	 * which might not be correct in cases where nents is less than
	 * the actual number of entries in the list.  However, if 
	 * drivers are careful not to use multiple calls to transfer
	 * more data than was requested then this will be safe.
	 */
	for (sg = *sgl; nents > 0 && sg; --nents, sg = sg_next(sg)) {

When this change is made, we will be able to convert usb-storage over 
to using your library routine.

Alan Stern

Re: [PATCH 00/10] sg buffer copy helper functions

[FUJITA Tomonori]

On Sun, 16 Mar 2008 13:18:07 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Sun, 16 Mar 2008, FUJITA Tomonori wrote:
> 
> > On Fri, 14 Mar 2008 10:46:52 -0400 (EDT)
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > On Fri, 14 Mar 2008, FUJITA Tomonori wrote:
> > > 
> > > > > If nents doesn't change then for_each_sg() won't work right.  There 
> > > > > could be an alternative macro:
> > > > 
> > > > Oops, I thought that for_each_sg is defined like:
> > > > 
> > > > #define for_each_sg(sglist, sg, nr, __i)	\
> > > > 	for (__i = 0, sg = (sglist); __i < (nr) && sg; __i++, sg = sg_next(sg))
> > > > 
> > > > 
> > > > > /*
> > > > >  * Loop over each sg element, stopping at the end of the chain
> > > > >  */
> > > > > #define for_each_sg_all(sglist, sg, __i)	\
> > > > > 	for (__i = 0, sg = (sglist); sg; __i++, sg = sg_next(sg))
> > > > > 
> > > > > If you added this macro to include/linux/scatterlist.h and used it
> > > > > instead of for_each_sg() then you can get rid of nents entirely.  
> > > > > However I'm not sure whether this would be safe.  Do people sometimes 
> > > > > use a subset of the entries in a scatterlist?
> > > > 
> > > > IIRC, some drivers do that (though they might use sg_next).
> > > 
> > > But will usb-storage ever receive a scatterlist like that?  For 
> > > example, if there are three 4096-byte entries in the list, but the 
> > > transfer length is 8192 and nents is 2, then there could be a problem.
> > 
> > If LLDs don't use the padding or drain buffer feature (USB uses
> > neither), scsi midlayer doesn't send such (that is, the block layer
> > doesn't create such).
> 
> Then it should be okay to open-code that loop and test whether sg is 
> NULL.
> 
> > As I explained above, it should be safe with USB. But in general, LLDs
> > should not rely on a scatterlist about how much data they transfer.
> 
> True.  Let's add a comment explaining the situation.  For example:
> 
> 	/* If this routine is called multiple times for a single
> 	 * scatterlist, the nents value will not be updated properly.
> 	 * Transfers will stop when the end of the list is reached,
> 	 * which might not be correct in cases where nents is less than
> 	 * the actual number of entries in the list.  However, if 
> 	 * drivers are careful not to use multiple calls to transfer
> 	 * more data than was requested then this will be safe.
> 	 */
> 	for (sg = *sgl; nents > 0 && sg; --nents, sg = sg_next(sg)) {
> 
> When this change is made, we will be able to convert usb-storage over 
> to using your library routine.

For me, it would be much better to fix USB drivers since LLDs should
not rely on a scatterlist about how much data they transfer, as I
said. If LLDs do that, they are broken. It's not good to add such a
workaround to the common API for broken LLDs.

USB drivers should do something like this if they definitely need to
call this API multiple times:

     done = sg_copy_buffer(buffer, scsi_bufflen(sc) - us->done_length,
     us->done_length += done;

Re: [PATCH 00/10] sg buffer copy helper functions

[Alan Stern]

On Mon, 17 Mar 2008, FUJITA Tomonori wrote:

> For me, it would be much better to fix USB drivers since LLDs should
> not rely on a scatterlist about how much data they transfer, as I
> said. If LLDs do that, they are broken. It's not good to add such a
> workaround to the common API for broken LLDs.
> 
> USB drivers should do something like this if they definitely need to
> call this API multiple times:
> 
>      done = sg_copy_buffer(buffer, scsi_bufflen(sc) - us->done_length,
>      us->done_length += done;

The USB drivers don't need to be fixed; they are already correct.

But if you are going to export a library routine for general use then
it should be written to fail gracefully -- it should not be able to
cause an invalid memory access.

Alan Stern