Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1765023AbYA2Qen (ORCPT ); Tue, 29 Jan 2008 11:34:43 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753206AbYA2Qe2 (ORCPT ); Tue, 29 Jan 2008 11:34:28 -0500 Received: from accolon.hansenpartnership.com ([76.243.235.52]:51435 "EHLO accolon.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751765AbYA2Qe0 (ORCPT ); Tue, 29 Jan 2008 11:34:26 -0500 Subject: Re: [BUG] 2.6.24-git usb reset problems From: James Bottomley To: Alan Stern Cc: Boaz Harrosh , Greg KH , Jens Axboe , Matthew Dharm , linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, linux-scsi@vger.kernel.org In-Reply-To: References: Content-Type: text/plain Date: Tue, 29 Jan 2008 11:34:15 -0500 Message-Id: <1201624455.3069.10.camel@localhost.localdomain> Mime-Version: 1.0 X-Mailer: Evolution 2.12.3 (2.12.3-1.fc8) Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2796 Lines: 70 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/