This patch adds some comments to clarify why we have to jump through all of these hoops for scatter-gather...
Greg, please apply.
Matt
----- Forwarded message from Alan Stern <[EMAIL PROTECTED]> -----
Date: Tue, 2 Dec 2003 17:12:31 -0500 (EST)
From: Alan Stern <[EMAIL PROTECTED]>
Subject: PATCH: (as150) Add comments explaining new s-g usage
To: Matthew Dharm <[EMAIL PROTECTED]>
cc: USB Storage List <[EMAIL PROTECTED]>
X-Spam-Status: No, hits=-0.6 required=5.0
tests=AWL,EMAIL_ATTRIBUTION,IN_REP_TO,PATCH_UNIFIED_DIFF,
QUOTED_EMAIL_TEXT,RCVD_IN_OSIRUSOFT_COM,SPAM_PHRASE_00_01,
USER_AGENT_PINE,X_OSIRU_OPEN_RELAY
version=2.44
On Sun, 30 Nov 2003, Matthew Dharm wrote:
> I'm going to pass this one along to Greg, but I think some places in this
> could really use some better comments. Especially the way you use a single
> buffer inside the loop -- it took me a few minutes to figure out how your
> logic to refresh the buffer with new data worked.
>
> I'm also wondering if the access_xfer_buf() function could use some more
> header comments, stating why this is needed (i.e. spelling out the
> kmap()-isms).
Okay, here it is. This patch basically just adds comments. Each routine
that uses the new scatter-gather function gets a brief explanation of
what's going on, and access_xfer_buf() itself gets detailed comments
saying what it's doing and why it's necessary. You may even want to cut
some of it back; I was pretty verbose.
Alan Stern
diff -u 2.6/drivers/usb/storage2/datafab.c 2.6/drivers/usb/storage/datafab.c
--- 2.6/drivers/usb/storage2/datafab.c Tue Dec 2 16:15:06 2003
+++ 2.6/drivers/usb/storage/datafab.c Tue Dec 2 16:31:08 2003
@@ -116,7 +116,11 @@
totallen = sectors * info->ssize;
// Since we don't read more than 64 KB at a time, we have to create
- // a bounce buffer if the transfer uses scatter-gather.
+ // a bounce buffer if the transfer uses scatter-gather. We will
+ // move the data a piece at a time between the bounce buffer and
+ // the actual transfer buffer. If we're not using scatter-gather,
+ // we can simply update the transfer buffer pointer to get the
+ // same effect.
alloclen = min(totallen, 65536u);
if (use_sg) {
@@ -153,6 +157,7 @@
if (result != USB_STOR_XFER_GOOD)
goto leave;
+ // Store the data (s-g) or update the pointer (!s-g)
if (use_sg)
usb_stor_access_xfer_buf(buffer, len, us->srb,
&sg_idx, &sg_offset, TO_XFER_BUF);
@@ -205,7 +210,11 @@
totallen = sectors * info->ssize;
// Since we don't write more than 64 KB at a time, we have to create
- // a bounce buffer if the transfer uses scatter-gather.
+ // a bounce buffer if the transfer uses scatter-gather. We will
+ // move the data a piece at a time between the bounce buffer and
+ // the actual transfer buffer. If we're not using scatter-gather,
+ // we can simply update the transfer buffer pointer to get the
+ // same effect.
alloclen = min(totallen, 65536u);
if (use_sg) {
@@ -221,6 +230,7 @@
len = min(totallen, alloclen);
thistime = (len / info->ssize) & 0xff;
+ // Get the data from the transfer buffer (s-g)
if (use_sg)
usb_stor_access_xfer_buf(buffer, len, us->srb,
&sg_idx, &sg_offset, FROM_XFER_BUF);
@@ -259,6 +269,7 @@
goto leave;
}
+ // Update the transfer buffer pointer (!s-g)
if (!use_sg)
buffer += len;
diff -u 2.6/drivers/usb/storage2/jumpshot.c 2.6/drivers/usb/storage/jumpshot.c
--- 2.6/drivers/usb/storage2/jumpshot.c Tue Dec 2 16:15:06 2003
+++ 2.6/drivers/usb/storage/jumpshot.c Tue Dec 2 16:32:09 2003
@@ -130,7 +130,11 @@
totallen = sectors * info->ssize;
// Since we don't read more than 64 KB at a time, we have to create
- // a bounce buffer if the transfer uses scatter-gather.
+ // a bounce buffer if the transfer uses scatter-gather. We will
+ // move the data a piece at a time between the bounce buffer and
+ // the actual transfer buffer. If we're not using scatter-gather,
+ // we can simply update the transfer buffer pointer to get the
+ // same effect.
alloclen = min(totallen, 65536u);
if (use_sg) {
@@ -167,6 +171,7 @@
US_DEBUGP("jumpshot_read_data: %d bytes\n", len);
+ // Store the data (s-g) or update the pointer (!s-g)
if (use_sg)
usb_stor_access_xfer_buf(buffer, len, us->srb,
&sg_idx, &sg_offset, TO_XFER_BUF);
@@ -212,7 +217,11 @@
totallen = sectors * info->ssize;
// Since we don't write more than 64 KB at a time, we have to create
- // a bounce buffer if the transfer uses scatter-gather.
+ // a bounce buffer if the transfer uses scatter-gather. We will
+ // move the data a piece at a time between the bounce buffer and
+ // the actual transfer buffer. If we're not using scatter-gather,
+ // we can simply update the transfer buffer pointer to get the
+ // same effect.
alloclen = min(totallen, 65536u);
if (use_sg) {
@@ -228,6 +237,7 @@
len = min(totallen, alloclen);
thistime = (len / info->ssize) & 0xff;
+ // Get the data from the transfer buffer (s-g)
if (use_sg)
usb_stor_access_xfer_buf(buffer, len, us->srb,
&sg_idx, &sg_offset, FROM_XFER_BUF);
@@ -268,6 +278,7 @@
if (result != USB_STOR_TRANSPORT_GOOD)
US_DEBUGP("jumpshot_write_data: Gah! Waitcount = 10. Bad
write!?\n");
+ // Update the transfer buffer pointer (!s-g)
if (!use_sg)
buffer += len;
diff -u 2.6/drivers/usb/storage2/protocol.c 2.6/drivers/usb/storage/protocol.c
--- 2.6/drivers/usb/storage2/protocol.c Tue Dec 2 16:15:06 2003
+++ 2.6/drivers/usb/storage/protocol.c Tue Dec 2 17:02:16 2003
@@ -233,7 +233,11 @@
***********************************************************************/
/* Copy a buffer of length buflen to/from the srb's transfer buffer.
- * Update the index and offset variables so that the next copy will
+ * (Note: for scatter-gather transfers (srb->use_sg > 0), srb->request_buffer
+ * points to a list of s-g entries and we ignore srb->request_bufflen.
+ * For non-scatter-gather transfers, srb->request_buffer points to the
+ * transfer buffer itself and srb->request_bufflen is the buffer's length.)
+ * Update the *index and *offset variables so that the next copy will
* pick up from where this one left off. */
unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
@@ -242,6 +246,8 @@
{
unsigned int cnt;
+ /* If not using scatter-gather, just transfer the data directly.
+ * Make certain it will fit in the available buffer space. */
if (srb->use_sg == 0) {
if (*offset >= srb->request_bufflen)
return 0;
@@ -254,11 +260,22 @@
*offset, cnt);
*offset += cnt;
+ /* Using scatter-gather. 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. */
} else {
struct scatterlist *sg =
(struct scatterlist *) srb->request_buffer
+ *index;
+ /* 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 *index values for the next loop. */
cnt = 0;
while (cnt < buflen && *index < srb->use_sg) {
struct page *page = sg->page +
@@ -268,14 +285,21 @@
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;
++*index;
+ ++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);
@@ -286,6 +310,8 @@
else
memcpy(buffer + cnt, ptr + poff, plen);
kunmap(page);
+
+ /* Start at the beginning of the next page */
poff = 0;
++page;
cnt += plen;
@@ -293,11 +319,13 @@
}
}
}
+
+ /* Return the amount actually transferred */
return cnt;
}
/* Store the contents of buffer into srb's transfer buffer and set the
- * residue. */
+ * SCSI residue. */
void usb_stor_set_xfer_buf(unsigned char *buffer,
unsigned int buflen, Scsi_Cmnd *srb)
{
diff -u 2.6/drivers/usb/storage2/sddr09.c 2.6/drivers/usb/storage/sddr09.c
--- 2.6/drivers/usb/storage2/sddr09.c Tue Dec 2 16:15:06 2003
+++ 2.6/drivers/usb/storage/sddr09.c Tue Dec 2 16:28:49 2003
@@ -673,7 +673,11 @@
int result;
// Since we only read in one block at a time, we have to create
- // a bounce buffer if the transfer uses scatter-gather.
+ // a bounce buffer if the transfer uses scatter-gather. We will
+ // move the data a piece at a time between the bounce buffer and
+ // the actual transfer buffer. If we're not using scatter-gather,
+ // we can simply update the transfer buffer pointer to get the
+ // same effect.
if (use_sg) {
len = min(sectors, (unsigned int) info->blocksize) *
@@ -738,6 +742,8 @@
if (result != USB_STOR_TRANSPORT_GOOD)
break;
}
+
+ // Store the data (s-g) or update the pointer (!s-g)
if (use_sg)
usb_stor_access_xfer_buf(buffer, len, us->srb,
&index, &offset, TO_XFER_BUF);
@@ -918,7 +924,10 @@
// Since we don't write the user data directly to the device,
// we have to create a bounce buffer if the transfer uses
- // scatter-gather.
+ // scatter-gather. We will move the data a piece at a time
+ // between the bounce buffer and the actual transfer buffer.
+ // If we're not using scatter-gather, we can simply update
+ // the transfer buffer pointer to get the same effect.
if (use_sg) {
len = min(sectors, (unsigned int) info->blocksize) *
@@ -944,6 +953,8 @@
pages = min(sectors, info->blocksize - page);
len = (pages << info->pageshift);
+
+ // Get the data from the transfer buffer (s-g)
if (use_sg)
usb_stor_access_xfer_buf(buffer, len, us->srb,
&index, &offset, FROM_XFER_BUF);
@@ -952,6 +963,8 @@
buffer, blockbuffer);
if (result != USB_STOR_TRANSPORT_GOOD)
break;
+
+ // Update the transfer buffer pointer (!s-g)
if (!use_sg)
buffer += len;
diff -u 2.6/drivers/usb/storage2/sddr55.c 2.6/drivers/usb/storage/sddr55.c
--- 2.6/drivers/usb/storage2/sddr55.c Tue Dec 2 16:15:06 2003
+++ 2.6/drivers/usb/storage/sddr55.c Tue Dec 2 16:29:43 2003
@@ -169,7 +169,11 @@
unsigned int len, index, offset;
// Since we only read in one block at a time, we have to create
- // a bounce buffer if the transfer uses scatter-gather.
+ // a bounce buffer if the transfer uses scatter-gather. We will
+ // move the data a piece at a time between the bounce buffer and
+ // the actual transfer buffer. If we're not using scatter-gather,
+ // we can simply update the transfer buffer pointer to get the
+ // same effect.
if (use_sg) {
len = min((unsigned int) sectors,
@@ -253,6 +257,8 @@
goto leave;
}
}
+
+ // Store the data (s-g) or update the pointer (!s-g)
if (use_sg)
usb_stor_access_xfer_buf(buffer, len, us->srb,
&index, &offset, TO_XFER_BUF);
@@ -300,7 +306,11 @@
}
// Since we only write one block at a time, we have to create
- // a bounce buffer if the transfer uses scatter-gather.
+ // a bounce buffer if the transfer uses scatter-gather. We will
+ // move the data a piece at a time between the bounce buffer and
+ // the actual transfer buffer. If we're not using scatter-gather,
+ // we can simply update the transfer buffer pointer to get the
+ // same effect.
if (use_sg) {
len = min((unsigned int) sectors,
@@ -325,6 +335,8 @@
pages = min((unsigned int) sectors << info->smallpageshift,
info->blocksize - page);
len = pages << info->pageshift;
+
+ // Get the data from the transfer buffer (s-g)
if (use_sg)
usb_stor_access_xfer_buf(buffer, len, us->srb,
&index, &offset, FROM_XFER_BUF);
@@ -468,6 +480,7 @@
/* update the pba<->lba maps for new_pba */
info->pba_to_lba[new_pba] = lba % 1000;
+ // Update the transfer buffer pointer (!s-g)
if (!use_sg)
buffer += len;
page = 0;
diff -u 2.6/drivers/usb/storage2/shuttle_usbat.c
2.6/drivers/usb/storage/shuttle_usbat.c
--- 2.6/drivers/usb/storage2/shuttle_usbat.c Tue Dec 2 16:15:06 2003
+++ 2.6/drivers/usb/storage/shuttle_usbat.c Tue Dec 2 16:29:00 2003
@@ -568,6 +568,13 @@
srb->transfersize);
}
+ // Since we only read in one block at a time, we have to create
+ // a bounce buffer if the transfer uses scatter-gather. We will
+ // move the data a piece at a time between the bounce buffer and
+ // the actual transfer buffer. If we're not using scatter-gather,
+ // we can simply update the transfer buffer pointer to get the
+ // same effect.
+
len = (65535/srb->transfersize) * srb->transfersize;
US_DEBUGP("Max read is %d bytes\n", len);
len = min(len, srb->request_bufflen);
@@ -614,8 +621,7 @@
if (result != USB_STOR_TRANSPORT_GOOD)
break;
- // Transfer the received data into the srb buffer
-
+ // Store the data (s-g) or update the pointer (!s-g)
if (srb->use_sg)
usb_stor_access_xfer_buf(buffer, len, srb,
&sg_segment, &sg_offset, TO_XFER_BUF);
----- End forwarded message -----
--
Matthew Dharm Home: [EMAIL PROTECTED]
Maintainer, Linux USB Mass Storage Driver
I think the problem is there's a nut loose on your keyboard.
-- Greg to Customer
User Friendly, 1/5/1999
pgp00000.pgp
Description: PGP signature
