On Tue, Feb 05 2008 at 17:42 +0200, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 5 Feb 2008, Boaz Harrosh wrote:
>
>>> However the interface to usb_stor_access_xfer_buf() will have to change
>>> slightly. Right now if it sees that *sgptr is NULL, it assumes this
>>> means it should start at the beginning of the s-g buffer. But with
>>> Boaz's change, *sgptr == NULL means the transfer has reached the end of
>>> the buffer. So I'll have to go through and audit all the callers.
>>>
>>> Alan Stern
>>>
>>> -
>> No it does not, this as not changed. Please look again.
>
> You look again. Your patched code goes like this:
>
> struct scatterlist *sg = *sgptr;
>
> if (!sg)
> sg = (struct scatterlist *) srb->request_buffer;
>
> Hence if *sgptr is NULL upon entry, it is taken to mean that the
> transfer should start at the beginning of the s-g buffer.
>
> /* 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 && sg) {
>
> Hence if sg is NULL, it indicates the end of the buffer has been
> reached. And then down near the end of the routine:
>
> *sgptr = sg;
>
> Hence if the end is reached and the caller makes another call to try
> transferring more data, the additional data will get stored back at the
> beginning of the buffer.
>
That behavior did not change. In the likely event of sg-length matching
bufflen the last call to sg_next will return NULL, and will be returned
in *sgptr. The end condition of an outside caller is either sum of
returned counts reaching some target count, or *sgptr return to NULL.
The code before the sg change would have *indexptr >= some_sg_count, but
now we do not have an index we have a pointer and the termination condition
is *sgptr == NULL.
So I guess you are afraid that calling code that was converted from index
to pointer, was done wrong, and where something did *indexptr >= some_sg_count
before, does not do *sgptr == NULL now.
So I guess, yes you are welcome to check. I did not do the conversion so
I can not comment.
>> Note that this patch was tested and working. It is a bug
>> in v2.2.24 and it should be accepted already. One way or
>> the other.
>>
>> Callers of usb_stor_access_xfer_buf() need not change.
>> Matthew Dharm should decide if he wants the WARN_ON in
>> usb_stor_set_xfer_buf() or not and be done with it.
>>
>> I have found and fixed the bug, but it is not a SCSI
>> related bug, and it is not do to any scsi changes. It
>> is a bug from the SG changes of early 2.6.24. Please
>> take it through the USB tree. Feel free to change it
>> the way you like it, and submit it.
>
> I will post a new version of this which handles all these issues.
> Expect it in a day or so.
>
Please do. Thanks, that would be better.
Don't forget to also submit a patch for current head-of-line. It's exactly
the same fix but has diff conflicts with surrounding code.
> Alan Stern
>
Boaz
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
