[PATCH] USB storage: Add comments explaining new s-g usage

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.

drivers/usb/storage/datafab.c

@@ -116,7 +116,11 @@ static int datafab_read_data(struct us_data *us,
 	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 @@ static int datafab_read_data(struct us_data *us,
 		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 @@ static int datafab_write_data(struct us_data *us,
 	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 @@ static int datafab_write_data(struct us_data *us,
 		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 @@ static int datafab_write_data(struct us_data *us,
 			goto leave;
 		}
 
+		// Update the transfer buffer pointer (!s-g)
 		if (!use_sg)
 			buffer += len;
 

drivers/usb/storage/jumpshot.c

@@ -130,7 +130,11 @@ static int jumpshot_read_data(struct us_data *us,
 	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 @@ static int jumpshot_read_data(struct us_data *us,
 
 		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 @@ static int jumpshot_write_data(struct us_data *us,
 	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 @@ static int jumpshot_write_data(struct us_data *us,
 		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 @@ static int jumpshot_write_data(struct us_data *us,
 		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;
 

drivers/usb/storage/protocol.c

@@ -233,7 +233,11 @@ void usb_stor_transparent_scsi_command(Scsi_Cmnd *srb, struct us_data *us)
  ***********************************************************************/
 
 /* 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 usb_stor_access_xfer_buf(unsigned char *buffer,
 {
 	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 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
 					*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 usb_stor_access_xfer_buf(unsigned char *buffer,
 			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 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
 				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 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
 			}
 		}
 	}
+
+	/* 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)
 {

drivers/usb/storage/sddr09.c

@@ -673,7 +673,11 @@ sddr09_read_data(struct us_data *us,
 	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 @@ sddr09_read_data(struct us_data *us,
 			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 @@ sddr09_write_data(struct us_data *us,
 
 	// 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 @@ sddr09_write_data(struct us_data *us,
 
 		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 @@ sddr09_write_data(struct us_data *us,
 				buffer, blockbuffer);
 		if (result != USB_STOR_TRANSPORT_GOOD)
 			break;
+
+		// Update the transfer buffer pointer (!s-g)
 		if (!use_sg)
 			buffer += len;
 

drivers/usb/storage/sddr55.c

@@ -169,7 +169,11 @@ static int sddr55_read_data(struct us_data *us,
 	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 @@ static int sddr55_read_data(struct us_data *us,
 				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 @@ static int sddr55_write_data(struct us_data *us,
 	}
 
 	// 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 @@ static int sddr55_write_data(struct us_data *us,
 		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 @@ static int sddr55_write_data(struct us_data *us,
 		/* 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;

drivers/usb/storage/shuttle_usbat.c

@@ -568,6 +568,13 @@ int usbat_handle_read10(struct us_data *us,
 			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 @@ int usbat_handle_read10(struct us_data *us,
 		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);