Re: [PATCH 2/2] USB: storage: use sg_miter_* APIs to access scsi buffer
On Wed, 30 Oct 2013, Ming Lei wrote:
We have sg_miter_* APIs for accessing scsi sg buffer, so
use them to make code clean and bug free.
Hmmm. You could simply call sg_copy_buffer, if you didn't mind the
quadratic penalty for the sg_miter_skip operations.
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -135,69 +135,43 @@ 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;
+ unsigned int cnt = 0;
struct scatterlist *sg = *sgptr;
+ struct sg_mapping_iter miter;
+ unsigned int nents = scsi_sg_count(srb);
- /* 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)
+ if (sg)
+ nents -= sg - scsi_sglist(srb);
This is definitely wrong. Scatterlist entries are not always stored in
a linear array. To do this properly, you would have to make the caller
keep track of the current value of nents.
Or better yet, have the caller store and pass the sg_mapping_iter
structure rather than sgptr and offset.
+ else
sg = 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 {
+ if (dir == FROM_XFER_BUF)
+ sg_miter_start(miter, sg, nents, SG_MITER_FROM_SG);
+ else
+ sg_miter_start(miter, sg, nents, SG_MITER_TO_SG);
I find this style somewhat annoying. Maybe the compiler is smart
enough to optimize it, maybe not. In any case, I would prefer to see
if (dir == FROM_XFER_BUF)
sgdir = SG_MITER_FROM_SG;
else
sgdir = SG_MITER_TO_SG;
sg_miter_start(miter, nents, sgdir);
Or even:
sg_miter_start(miter, nents, (dir == FROM_XFER_BUF ?
SG_MITER_FROM_SG : SG_MITER_TO_SG));
- /* Transfer continues to next s-g entry */
- *offset = 0;
- sg = sg_next(sg);
- }
+ if (!sg_miter_skip(miter, *offset))
+ return cnt;
+
+ while (sg_miter_next(miter) cnt buflen) {
+ unsigned int len = min(miter.length, buflen - cnt);
+
+ if (dir == FROM_XFER_BUF)
+ memcpy(buffer + cnt, miter.addr, len);
+ else
+ memcpy(miter.addr, buffer + cnt, len);
- /* 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;
+ if (*offset + len miter.piter.sg-length) {
+ *offset += len;
+ *sgptr = miter.piter.sg;
+ } else {
+ *offset = 0;
+ *sgptr = sg_next(miter.piter.sg);
}
+ cnt += len;
}
- *sgptr = sg;
+ sg_miter_stop(miter);
- /* Return the amount actually transferred */
Why remove this comment?
return cnt;
}
Re: [PATCH 2/2] USB: storage: use sg_miter_* APIs to access scsi buffer
On Fri, Nov 1, 2013 at 10:54 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Wed, 30 Oct 2013, Ming Lei wrote:
We have sg_miter_* APIs for accessing scsi sg buffer, so
use them to make code clean and bug free.
Hmmm. You could simply call sg_copy_buffer, if you didn't mind the
quadratic penalty for the sg_miter_skip operations.
I don't want to change all current callers now.
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -135,69 +135,43 @@ 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;
+ unsigned int cnt = 0;
struct scatterlist *sg = *sgptr;
+ struct sg_mapping_iter miter;
+ unsigned int nents = scsi_sg_count(srb);
- /* 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)
+ if (sg)
+ nents -= sg - scsi_sglist(srb);
This is definitely wrong. Scatterlist entries are not always stored in
a linear array. To do this properly, you would have to make the caller
keep track of the current value of nents.
Or better yet, have the caller store and pass the sg_mapping_iter
structure rather than sgptr and offset.
You are right, but looks we can use sg_nents(), which is easier, :-)
+ else
sg = 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 {
+ if (dir == FROM_XFER_BUF)
+ sg_miter_start(miter, sg, nents, SG_MITER_FROM_SG);
+ else
+ sg_miter_start(miter, sg, nents, SG_MITER_TO_SG);
I find this style somewhat annoying. Maybe the compiler is smart
enough to optimize it, maybe not. In any case, I would prefer to see
if (dir == FROM_XFER_BUF)
sgdir = SG_MITER_FROM_SG;
else
sgdir = SG_MITER_TO_SG;
sg_miter_start(miter, nents, sgdir);
Or even:
sg_miter_start(miter, nents, (dir == FROM_XFER_BUF ?
SG_MITER_FROM_SG : SG_MITER_TO_SG));
Looks the above is fine.
- /* Transfer continues to next s-g entry */
- *offset = 0;
- sg = sg_next(sg);
- }
+ if (!sg_miter_skip(miter, *offset))
+ return cnt;
+
+ while (sg_miter_next(miter) cnt buflen) {
+ unsigned int len = min(miter.length, buflen - cnt);
+
+ if (dir == FROM_XFER_BUF)
+ memcpy(buffer + cnt, miter.addr, len);
+ else
+ memcpy(miter.addr, buffer + cnt, len);
- /* 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;
+ if (*offset + len miter.piter.sg-length) {
+ *offset += len;
+ *sgptr = miter.piter.sg;
+ } else {
+ *offset = 0;
+ *sgptr =
[PATCH 2/2] USB: storage: use sg_miter_* APIs to access scsi buffer
We have sg_miter_* APIs for accessing scsi sg buffer, so
use them to make code clean and bug free.
Cc: Matthew Dharm mdharm-...@one-eyed-alien.net
Cc: Alan Stern st...@rowland.harvard.edu
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/storage/protocol.c | 82 ++--
1 file changed, 28 insertions(+), 54 deletions(-)
diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
index 5dfb4c3..01697e5 100644
--- a/drivers/usb/storage/protocol.c
+++ b/drivers/usb/storage/protocol.c
@@ -135,69 +135,43 @@ 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;
+ unsigned int cnt = 0;
struct scatterlist *sg = *sgptr;
+ struct sg_mapping_iter miter;
+ unsigned int nents = scsi_sg_count(srb);
- /* 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)
+ if (sg)
+ nents -= sg - scsi_sglist(srb);
+ else
sg = 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 {
+ if (dir == FROM_XFER_BUF)
+ sg_miter_start(miter, sg, nents, SG_MITER_FROM_SG);
+ else
+ sg_miter_start(miter, sg, nents, SG_MITER_TO_SG);
- /* Transfer continues to next s-g entry */
- *offset = 0;
- sg = sg_next(sg);
- }
+ if (!sg_miter_skip(miter, *offset))
+ return cnt;
+
+ while (sg_miter_next(miter) cnt buflen) {
+ unsigned int len = min(miter.length, buflen - cnt);
+
+ if (dir == FROM_XFER_BUF)
+ memcpy(buffer + cnt, miter.addr, len);
+ else
+ memcpy(miter.addr, buffer + cnt, len);
- /* 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;
+ if (*offset + len miter.piter.sg-length) {
+ *offset += len;
+ *sgptr = miter.piter.sg;
+ } else {
+ *offset = 0;
+ *sgptr = sg_next(miter.piter.sg);
}
+ cnt += len;
}
- *sgptr = sg;
+ sg_miter_stop(miter);
- /* Return the amount actually transferred */
return cnt;
}
EXPORT_SYMBOL_GPL(usb_stor_access_xfer_buf);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
