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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> On Tue, 11 Mar 2008 16:09:26 -0400 (EDT)
> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Tue, 11 Mar 2008, FUJITA Tomonori wrote:
> > 
> > > On Mon, 10 Mar 2008 16:10:36 +0200
> > > Boaz Harrosh <bharrosh@xxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux