On Thu, Jan 31 2008 at 19:49 +0200, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 31 Jan 2008, Boaz Harrosh wrote:
>
>> @@ -228,9 +228,14 @@ void usb_stor_set_xfer_buf(unsigned char *buffer,
>> {
>> unsigned int offset = 0;
>> struct scatterlist *sg = NULL;
>> + unsigned int count;
>>
>> - usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>> + count = usb_stor_access_xfer_buf(buffer, buflen, srb, &sg, &offset,
>> TO_XFER_BUF);
>> - if (buflen < scsi_bufflen(srb))
>> - scsi_set_resid(srb, scsi_bufflen(srb) - buflen);
>> +
>> + /* Check for underflow */
>> + if (buflen > scsi_bufflen(srb))
>> + count = buflen;
>> +
>> + scsi_set_resid(srb, scsi_bufflen(srb) - count);
>> }
>
> This last "if" statement doesn't look right. And since you know that
> count will never be larger than scsi_bufflen(srb), you don't have to
> check for underflow at all. Just leave out the "if" and call
> scsi_set_resid() directly.
>
> Alan Stern
>
I was thinking about that hard. And I disagree. It could be that we have
sg-list length (The total of all sg->length) that does not match
scsi_bufflen() - More or less. The code in usb_stor_access_xfer_buf() will
now correctly attempt to transfer according to buflen and what ever is available
at the passed sg's. Now this can be less or it can be more. SCSI standard defines
this as underflow/overflow. When overflow should be reported as negative values.
and an error status. (BUT not CHECK_CONDITION)
so the code is actualy
if (buflen > scsi_bufflen(srb))
scsi_set_resid(srb, scsi_bufflen(srb) - buflen); /* Negative overflow */
else
scsi_set_resid(srb, scsi_bufflen(srb) - count);
So this is actually code equivalent to above. Minus the extra call site.
The SCSI standard does allow these conditions and the system should cope
and go on. So a BUG_ON() is not accepted, I think.
But, ok, I mixed up with the comment I'll resend. underflow => overflow.
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
