On Tue, Jan 29 2008 at 17:36 +0200, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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.
>
> Alan Stern
>
> -
Me neither, it's not a proper patch just a shut to try and find the reported
bug. I will submit a proper bug later.
Thanks for the review, you are right on all accounts
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
