Hi,
Running latest -git (head 91525300baf162e83e923b09ca286f9205e21522) and

connecting my cf usb storage device yields and endless stream of:
Initializing USB Mass Storage driver...

scsi6 : SCSI emulation for USB Mass Storage devices

usb-storage: device found at 2

usb-storage: waiting for device to settle before scanning

usbcore: registered new interface driver usb-storage

USB Mass Storage support registered.

scsi 6:0:0:0: Direct-Access     Generic  STORAGE DEVICE   0125 PQ: 0

ANSI: 0

sd 6:0:0:0: [sdb] 4001760 512-byte hardware sectors (2049 MB)

sd 6:0:0:0: [sdb] Write Protect is off

sd 6:0:0:0: [sdb] Mode Sense: 02 00 00 00

sd 6:0:0:0: [sdb] Assuming drive cache: write through

sd 6:0:0:0: [sdb] 4001760 512-byte hardware sectors (2049 MB)

sd 6:0:0:0: [sdb] Write Protect is off

sd 6:0:0:0: [sdb] Mode Sense: 02 00 00 00

sd 6:0:0:0: [sdb] Assuming drive cache: write through

 sdb: sdb1

sd 6:0:0:0: [sdb] Attached SCSI removable disk

sd 6:0:0:0: Attached scsi generic sg1 type 0

scsi 6:0:0:1: Direct-Access     Generic  STORAGE DEVICE   0125 PQ: 0

ANSI: 0

usb 5-1: reset high speed USB device using ehci_hcd and address 2

usb 5-1: reset high speed USB device using ehci_hcd and address 2

usb 5-1: reset high speed USB device using ehci_hcd and address 2

usb 5-1: reset high speed USB device using ehci_hcd and address 2

usb 5-1: reset high speed USB device using ehci_hcd and address 2

usb 5-1: reset high speed USB device using ehci_hcd and address 2

usb 5-1: reset high speed USB device using ehci_hcd and address 2

[...]
until I disconnect it. The device doesn't work. Worked fine in 2.6.24.

I'm attaching boot messages and my .config.
--

Jens Axboe

That's a bit wierd, as we haven't added any USB patches to the -git tree

yet after 2.6.24 :)
Could this be caused by some scsi changes perhaps?
thanks,
greg k-h

--

Yes it is ;)
Jens could you test the patch below? if it works I'll submit a proper

patch. Please forgive me for the bug.
Matthew, Greg, Is there a way to extract from messages the usb storage transport

used? I'm guessing it is freecom do to the fact that I find a bug there.
Thanks

Boaz
---

 drivers/usb/storage/freecom.c   |    4 ++--

 drivers/usb/storage/transport.c |   12 +++++++++---

 drivers/usb/storage/transport.h |    3 ++-

 3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/storage/freecom.c b/drivers/usb/storage/freecom.c

index f5a4e8d..8d77603 100644

--- a/drivers/usb/storage/freecom.c

+++ b/drivers/usb/storage/freecom.c

@@ -132,7 +132,7 @@ freecom_readdata (struct scsi_cmnd *srb, struct us_data *us,
 	/* Now transfer all of our blocks. */

 	US_DEBUGP("Start of read\n");

-	result = usb_stor_bulk_srb(us, ipipe, srb);

+	result = usb_stor_bulk_srb_length(us, ipipe, srb, count);

 	US_DEBUGP("freecom_readdata done!\n");
 	if (result > USB_STOR_XFER_SHORT)

@@ -165,7 +165,7 @@ freecom_writedata (struct scsi_cmnd *srb, struct us_data *us,
 	/* Now transfer all of our blocks. */

 	US_DEBUGP("Start of write\n");

-	result = usb_stor_bulk_srb(us, opipe, srb);

+	result = usb_stor_bulk_srb_length(us, opipe, srb, count);
 	US_DEBUGP("freecom_writedata done!\n");

 	if (result > USB_STOR_XFER_SHORT)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c

index d9f4912..5072235 100644

--- 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 par...
[ message continues ]

The freecom transport is exceptionally rare these days.  It's primary user

was a device made by a company that went out of business.
Matt
--=20

Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=

net=20

Maintainer, Linux USB Mass Storage Driver
Way to go, lava boy.

					-- Stef to Greg

User Friendly, 3/26/1998

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

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

--

Um, yes it was.  The original code did this
sb_stor_bulk_transfer_sg(..., &srp->resid, ...)
Which was at liberty not to touch resid, which it chose not to do in the

error legs.
Your new code does
int partial; <- stack uninitialised

sb_stor_bulk_transfer_sglist(..., &partial, ...);

scsi_set_resid(srb, scsi_bufflen(srb) - partial);
If the function doesn't touch partial, as it doesn't in the error legs,

resid now gets set with rubbish.
Actually, my code is still wrong .. we have to set it to

scsi_bufflen(srb) - scsi_resid(srb) so that it comes back the same if
James
--

Sorry I still don't see it.
original code did sb_stor_bulk_transfer_sg(..., &srp->resid, ...)
but sb_stor_bulk_transfer_sg does the:

 int partial; <- stack uninitialised

 sb_stor_bulk_transfer_sglist(..., &partial, ...);
and then unconditionally sets *residual = length_left;

I do not see an "error legs" case in sb_stor_bulk_transfer_sg().
I have just cut and pasted the !use_sg from sb_stor_bulk_transfer_sg names

and all
Boaz
--

This is really programming 101. This:
static int usb_stor_bulk_transfer_sglist(struct us_data *us, unsigned

int pipe,

		struct scatterlist *sg, int num_sg, unsigned int length,

		unsigned int *act_len)

{

	int result;
	/* don't submit s-g requests during abort/disconnect processing */

	if (us->flags & ABORTING_OR_DISCONNECTING)

		return USB_STOR_XFER_ERROR;
The return USB_STOR_XFER_ERROR; is called an error leg.  It returns

without updating *act_len thus leaving &partial uninitialised.
James
--

Yes you are right this is a bug, but it is a bug that was there before.

perhaps the stack is just different now then what it used to be.
Jens could you please try that:
diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c

index d9f4912..b18a5e6 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 = 0;

 	int result = usb_stor_bulk_transfer_sglist(us, pipe, scsi_sglist(srb),

 				      scsi_sg_count(srb), scsi_bufflen(srb),

 				      &partial);
--

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

No difference, still just a lot of resets.
--

Jens Axboe
--

Where you able to figure out which usb storage transport is used?
in drivers/usb/storage/usb.c you have get_protocol() and get_transport()

functions. I'm not sure if these get stored in sysfs perhaps. This will

pinpoint better where to look. Let me research a bit. 
Thanks for testing

Boaz
--

From inspection of code it should be in /proc/scsi somewhere look for:

	SPRINTF("     Protocol: %s\n", us->protocol_name);

	SPRINTF("    Transport: %s\n", us->transport_name);
(it's proc_info() in drivers/usb/storage/scsiglue.c)
Thanks

Boaz

--

Did the quick'n easy and dumped it. Protocol is 'Transparent SCSI' and

transport is 'Bulk'
--

Jens Axboe
--

You can recompile your kernel with CONFIG_USB_DEBUG and CONFIG_STORAGE_DEBUG

That should tell the reason for the resets.
	Regards

		Oliver

--

I can not see what it is. Yes CONFIG_USB_STORAGE_DEBUG will help.

I have a device here that uses the same trasport/protocol I'll

try to reproduce the failure here.
Boaz

--

This is the most common combination of transport and protocol. If it were

broken in a larger number of cases, we'd hear more reports. You'll probably

need the debug output to figure out what's wrong.
	Regards

		Oliver

--

Sure, I'll do that. Will post the results tonight.
--

Jens Axboe
--

OK, fresh boot with CONFIG_USB_DEBUG and CONFIG_STORAGE_DEBUG. Plugged

in the device, waited 10 seconds or so and pulled it out. These are the

messages.
It all looks good until the MODE_SENSE command, where it only transfers

4 of 192 bytes.
usb usb5: usb resume

ehci_hcd 0000:00:1d.7: resume root hub

hub 5-0:1.0: hub_resume

hub 5-0:1.0: state 7 ports 8 chg 0000 evt 0000

ehci_hcd 0000:00:1d.7: GetStatus port 1 status 001803 POWER sig=j CSC CONNECT

hub 5-0:1.0: port 1, status 0501, change 0001, 480 Mb/s

hub 5-0:1.0: debounce: port 1: total 100ms stable 100ms status 0x501

ehci_hcd 0000:00:1d.7: port 1 high speed

ehci_hcd 0000:00:1d.7: GetStatus port 1 status 001005 POWER sig=se0 PE CONNECT

usb 5-1: new high speed USB device using ehci_hcd and address 2

ehci_hcd 0000:00:1d.7: port 1 high speed

ehci_hcd 0000:00:1d.7: GetStatus port 1 status 001005 POWER sig=se0 PE CONNECT

usb 5-1: default language 0x0409

usb 5-1: uevent

usb 5-1: usb_probe_device

usb 5-1: configuration #1 chosen from 1 choice

usb 5-1: adding 5-1:1.0 (config #1, interface 0)

usb 5-1:1.0: uevent

drivers/usb/core/inode.c: creating file '002'

usb 5-1: new device strings: Mfr=0, Product=3, SerialNumber=4

usb 5-1: Product: Flash Reader

usb 5-1: SerialNumber: 02206

Initializing USB Mass Storage driver...

usb-storage 5-1:1.0: usb_probe_interface

usb-storage 5-1:1.0: usb_probe_interface - got id

usb-storage: USB Mass Storage device detected

usb-storage: -- associate_dev

usb-storage: Vendor: 0x05e3, Product: 0x0760, Revision: 0x0125

usb-storage: Interface Subclass: 0x06, Protocol: 0x50

usb trans Bulk

usb-storage: Transport: Bulk

usb proto Transparent SCSI

usb-storage: Protocol: Transparent SCSI

scsi6 : SCSI emulation for USB Mass Storage devices

usb-storage: *** thread sleeping.

usb-storage: device found at 2

usb-storage: waiting for device to settle before scanning

usbcore: registered new interface driver usb-storage

USB Mass Storage support registered.

usb-storage: usb_stor_control_msg: rq=fe rqtype=a1 value=000...
[ message continues ]

No, that's not the problem.  This is the problem:
For some reason, usb_sg_init is boned during auto-sense.
Matt
--=20

Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=

net=20

Maintainer, Linux USB Mass Storage Driver
It was a new hope.

					-- Dust Puppy

User Friendly, 12/25/1998

OK, that's implicating the scsi_eh_prep_cmnd() in the auto sense

code ... that was also an update in 2.6.24
James
--

yeah, already found the bug - it's assuming ->request_buffer holds the

sglist, oops. Preparing a fix.
--

Jens Axboe
--

ok here goes, this saves and restores the sg table correctly. it also

fixes the usb bug for me.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c

index 547e85a..12770ef 100644

--- a/drivers/scsi/scsi_error.c

+++ b/drivers/scsi/scsi_error.c

@@ -622,13 +622,15 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,

 	ses->use_sg = scmd->use_sg;

 	ses->resid = scmd->resid;

 	ses->result = scmd->result;

+	memcpy(&ses->sense_sgl, &scmd->sg_table, sizeof(ses->sense_sgl));
 	if (sense_bytes) {

 		scmd->request_bufflen = min_t(unsigned,

 		                       SCSI_SENSE_BUFFERSIZE, sense_bytes);

 		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,

 		                                       scmd->request_bufflen);

-		scmd->request_buffer = &ses->sense_sgl;

+		scmd->sg_table.sgl = &ses->sense_sgl;

+		scmd->sg_table.nents = scmd->sg_table.orig_nents = 1;

 		scmd->sc_data_direction = DMA_FROM_DEVICE;

 		scmd->use_sg = 1;

 		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));

@@ -679,6 +681,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)

 	scmd->request_bufflen = ses->bufflen;

 	scmd->request_buffer = ses->buffer;

 	scmd->use_sg = ses->use_sg;

+	memcpy(&scmd->sg_table, &ses->sg_table, sizeof(scmd->sg_table));

 	scmd->resid = ses->resid;

 	scmd->result = ses->result;

 }

diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h

index d21b891..d43dc83 100644

--- a/include/scsi/scsi_eh.h

+++ b/include/scsi/scsi_eh.h

@@ -75,6 +75,7 @@ struct scsi_eh_save {
 	void *buffer;

 	unsigned bufflen;

+	struct sg_table sg_table;

 	unsigned short use_sg;

 	int resid;
--

Jens Axboe
--

I can confirm this patch fixes the errors I was seeing with current
With kind regards,
Geert Uytterhoeven

Software Architect
Sony Network and Software Technology Center Europe

The Corporate Village

James, we need a fix for this pushed asap. So either we should merge the

below now, or push the bidi patchset that also fixes this. It all
--

Jens Axboe
--

The SCSI patch set (including the bidirectional pieces) is waiting in

scsi-misc ... just for forms sake, could you confirm that it actually

fixes the problem and I'll push it.
James
--

Certainly, I'll give it a spin tonight.
--

Jens Axboe
--

Confirmed, pulling your scsi-misc branch into current -git makes the

problem go away as well.
--

Jens Axboe
--

Ok this is not in Linus tree is it? Hence I did not have that failure.
Boaz
--

actually James bidi tree has a fix for this in the scsi_data_buffer patch.
what you sent is not enough there are other places. look at this patch I

sent to the list.
http://www.spinics.net/lists/linux-scsi/msg21938.html
Could we take the 2 SG patches and submit them through the scsi

bidi tree? It is much more natural to have them in one tree as one

patchset then try coordinate with git-merge. Actually if you look at it,

the biggest change is to SCSI. So I think it is more natural this way
Boaz

--

What 2 sg patches do you mean?
--

Jens Axboe
--

Yes in this patchset I have taken your sg branch at the time, and

rebased it ontop of scsi_data_buffer patch. Because I felt that

it is more natural for this patch to come after the scsi total

cleanup that is scsi_data_buffer. Then the extraction to sg_table

is simple and trivial.
What I meant to point out with this patch is that all the exact same

places that are touched there should be fixed when moving to sg_table.
I mean the patches that where in sg branch of the linux-block tree, But

I see that it is now to late, and that they are in Linus already
James the most simple is to submit the scsi_data_buff patch that fixes

all these places. If not do you want that I send in fixes?
Boaz

--

Yes it is, it's in current -git! James, comments on the patch? Will you

push it or shall I?
--

Jens Axboe
--

I will ... but it will cause an explosion in the bidirectional tree

again.  I think the bidi updates also fix this.  However, give me time

to rebase and verify.
James
--

OK thanks, just wanted to make sure that we didn't both expect each

other to handle it :-)
--

Jens Axboe
--

Yes, confirm that; I think this is already fixed in scsi-misc-2.6.

Could you pull from
master.kernel.org:/pub/scm/linux/kernel/git/jejb/scsi-misc-2.6.git
and verify with your device?
Thanks,
James
--

I still don't see these changes, I wanted to check, make sure...

Are there mirrors on the way to here?
James I would like to remind you that one small piece is missing

from the bidi tree, as I saw it from here, it's the few patches from

scsi-pending. Mainly arm will break which is a grate loss.
I'm going home now, I'll review all the patches tomorrow and compare

to what I have, to make sure nothing was forgotten. What a waste of a day

I pulled from Linus this morning apparently minutes before the merge, and

chased a none existent bug in my tree. Sigh
Bye

Boaz
--

It's where the problem starts, otherwise there would not be a need to
My guess would be that sg == NULL, hence the -EINVAL.
--

Jens Axboe
--

MODE_SENSE has nothing to do with it.  A short MODE_SENSE response is

perfectly valid, and the log shows it was handled correctly and subsequent

commands worked just fine.  It's not until the TUR fails that we get the

problem.
Matt
--=20

Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.=

net=20

Maintainer, Linux USB Mass Storage Driver
P:  Nine more messages in admin.policy.

M: I know, I'm typing as fast as I can!

					-- Pitr and Mike

User Friendly, 11/27/97

Yep, trace below:
WARNING: at drivers/usb/storage/transport.c:426

usb_stor_bulk_transfer_sglist()

Pid: 12536, comm: usb-storage Not tainted 2.6.24 #74

 [<7810541a>] show_trace_log_lvl+0x1a/0x30

 [<78105e82>] show_trace+0x12/0x20

 [<7810689c>] dump_stack+0x6c/0x80

 [<f83e267e>] usb_stor_bulk_transfer_sglist+0x14e/0x160 [usb_storage]

 [<f83e2730>] usb_stor_bulk_srb_length+0x30/0x50 [usb_storage]

 [<f83e2762>] usb_stor_bulk_srb+0x12/0x20 [usb_storage]

 [<f83e2ce0>] usb_stor_Bulk_transport+0x190/0x3d0 [usb_storage]

 [<f83e30d6>] usb_stor_invoke_transport+0x1b6/0x320 [usb_storage]

 [<f83e1a38>] usb_stor_transparent_scsi_command+0x8/0x10 [usb_storage]

 [<f83e3813>] usb_stor_control_thread+0x143/0x2c0 [usb_storage]

 [<7813bc02>] kthread+0x42/0x70

 [<78104fab>] kernel_thread_helper+0x7/0x1c

 =======================

usb-storage: usb_stor_bulk_transfer_sglist: xfer 18 bytes, 1 entries, sg

00000000

usb-storage: usb_sg_init returned -22
--

Jens Axboe
--

<snip>
I get something similar but better:

usb-storage: Command MODE_SENSE (6 bytes)

usb-storage:  1a 00 3f 00 c0 00

usb-storage: Bulk Command S 0x43425355 T 0x6 L 192 F 128 Trg 0 LUN 0 CL 6

usb-storage: usb_stor_bulk_transfer_buf: xfer 31 bytes

usb-storage: Status code 0; transferred 31/31

usb-storage: -- transfer complete

usb-storage: Bulk command transfer result=0

usb-storage: usb_stor_bulk_transfer_sglist: xfer 192 bytes, 1 entries

usb-storage: Status code -121; transferred 36/192

usb-storage: -- short read transfer

usb-storage: Bulk data transfer result 0x1

usb-storage: Attempting to get CSW...
So I get 36 bytes, then code goes on into one reset, and every thing is then

fine.
Could you put us out of our mesery and revert that patch:

[SCSI] usb: transport - convert to accessors and !use_sg code path removal

(6d416e6173394defda5933e419e805b696681b7e)
to make sure this is it. I hate to do this to you, but I cannot reproduce the

failure down here. If it works please send a log with the debugs on perhaps

we can compare.
You will need to configure out the CONFIG_USB_STORAG_* they will not compile

you should have only have CONFIG_USB_STORAGE & CONFIG_USB_STORAGE_DEBUG. it should

support your HW.
Thanks Jens

Boaz
--

Sorry for the other noise. Thanks I'll have a look.

--

Heh, I guess it could! I'll double check, I reproduced it with two

distinct boots before posting.
--

Jens Axboe
--