On Fri, Nov 1, 2013 at 10:54 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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 = sg_next(miter.piter.sg); >> } >> + cnt += len; >> } >> - *sgptr = sg; >> + sg_miter_stop(&miter); >> >> - /* Return the amount actually transferred */ > > Why remove this comment? With this patch, the comment becomes uncessary since the code is very simple and it is obvious. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html