Re: [PATCH 2/2] USB: storage: use sg_miter_* APIs to access scsi buffer

[Ming Lei]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/