ChangeSet 1.1504.2.27, 2003/12/09 11:48:52-08:00, stern@rowland.harvard.edu [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 | 15 +++++++++++++-- drivers/usb/storage/jumpshot.c | 15 +++++++++++++-- drivers/usb/storage/protocol.c | 34 +++++++++++++++++++++++++++++++--- drivers/usb/storage/sddr09.c | 17 +++++++++++++++-- drivers/usb/storage/sddr55.c | 17 +++++++++++++++-- drivers/usb/storage/shuttle_usbat.c | 10 ++++++++-- 6 files changed, 95 insertions(+), 13 deletions(-) diff -Nru a/drivers/usb/storage/datafab.c b/drivers/usb/storage/datafab.c --- a/drivers/usb/storage/datafab.c Mon Dec 29 14:23:59 2003 +++ b/drivers/usb/storage/datafab.c Mon Dec 29 14:23:59 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 -Nru a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c --- a/drivers/usb/storage/jumpshot.c Mon Dec 29 14:23:59 2003 +++ b/drivers/usb/storage/jumpshot.c Mon Dec 29 14:23:59 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 -Nru a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c --- a/drivers/usb/storage/protocol.c Mon Dec 29 14:23:59 2003 +++ b/drivers/usb/storage/protocol.c Mon Dec 29 14:23:59 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 -Nru a/drivers/usb/storage/sddr09.c b/drivers/usb/storage/sddr09.c --- a/drivers/usb/storage/sddr09.c Mon Dec 29 14:23:59 2003 +++ b/drivers/usb/storage/sddr09.c Mon Dec 29 14:23:59 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 -Nru a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c --- a/drivers/usb/storage/sddr55.c Mon Dec 29 14:23:59 2003 +++ b/drivers/usb/storage/sddr55.c Mon Dec 29 14:23:59 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 -Nru a/drivers/usb/storage/shuttle_usbat.c b/drivers/usb/storage/shuttle_usbat.c --- a/drivers/usb/storage/shuttle_usbat.c Mon Dec 29 14:23:59 2003 +++ b/drivers/usb/storage/shuttle_usbat.c Mon Dec 29 14:23:59 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);