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
