Re: [BUG] 2.6.24-git usb reset problems

[Boaz Harrosh <bharrosh@panasas.com>]

On Tue, Jan 29 2008 at 18:34 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> 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