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

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