On Tue, Jan 29 2008 at 18:34 +0200, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, 2008-01-29 at 10:36 -0500, Alan Stern wrote:
>> On Tue, 29 Jan 2008, Boaz Harrosh wrote:
>>
>>> --- a/drivers/usb/storage/transport.c
>>> +++ b/drivers/usb/storage/transport.c
>>> @@ -462,18 +462,24 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
>>> * Common used function. Transfer a complete command
>>> * via usb_stor_bulk_transfer_sglist() above. Set cmnd resid
>>> */
>>> -int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
>>> - struct scsi_cmnd* srb)
>>> +int usb_stor_bulk_srb_length(struct us_data* us, unsigned int pipe,
>>> + struct scsi_cmnd* srb, unsigned length)
>>> {
>>> unsigned int partial;
>>> int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb),
>>> - scsi_sg_count(srb), scsi_bufflen(srb),
>>> + scsi_sg_count(srb), length,
>>> &partial);
>>>
>>> scsi_set_resid(srb, scsi_bufflen(srb) - partial);
>>> return result;
>>> }
>>>
>>> +int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
>>> + struct scsi_cmnd* srb)
>>> +{
>>> + return usb_stor_bulk_srb_length(us, pipe, srb, scsi_bufflen(srb));
>>> +}
>>> +
>> I don't like this patch very much. Why add another layer of
>> indirection when the two subroutines do hardly any work? Leave
>> usb_stor_bulk_srb() the way it was, and add usb_stor_bulk_srb_length()
>> as a separate routine that simply calls usb_stor_bulk_transfer_sglist()
>> and scsi_set_resid().
>>
>> BTW, the standard coding style calls for a blank line after the list of
>> local variables at the start of a function or block.
>
> There's another bug in the transport.c conversion in that the residuals
> are updated with bogus data in several error cases, since
> usb_stor_bulk_transfer_sglist() only sets the actual length if the urb
> is actually sent.
>
> I'm not sure if this is is the solution to the problem at hand, but it
> definitely fixes another bug in the code.
>
> James
>
> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> index d9f4912..bab0858 100644
> --- a/drivers/usb/storage/transport.c
> +++ b/drivers/usb/storage/transport.c
> @@ -465,7 +465,7 @@ static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned int pipe,
> int usb_stor_bulk_srb(struct us_data* us, unsigned int pipe,
> struct scsi_cmnd* srb)
> {
> - unsigned int partial;
> + unsigned int partial = scsi_get_resid(srb);
> int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb),
> scsi_sg_count(srb), scsi_bufflen(srb),
> &partial);
>
>
> -
But then this is weird because it is not what usb_stor_bulk_transfer_sg() is doing
which was the one called before.
I have such a device and I get one reset but then every thing works nice.
This is with debug on. I'll try to make it fail.
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
