Re: [BUG] 2.6.24-git usb reset problems

[James Bottomley]

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);


--
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/