usb storage cleanup

PATCH Manfred Spraul 5 participants 11 messages Jul 3, 2002 – Jul 4, 2002

Subsystems (2)

USB USB mass storage driver

lore

Share & tools

lore

Manfred Spraul

author

2002-07-03 21:12 UTC #0 lore raw

Attached is a cleanup for usb-storage.

Mainly it's an update to the synchonization: there were a few small races left. I've added a complete changelog into usb.h.

The patch is vs 2.5.24-dj2. I've tested it with my SaroTech device, please test it with other usb storage devices.

--
Manfred

drivers/usb/storage/freecom.c +8 -8

diff -u 2.5/drivers/usb/storage/freecom.c build-2.5/drivers/usb/storage/freecom.c
@@ -191,7 +191,7 @@
                                 result, partial);
 
 		/* has the current command been aborted? */
-		if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+		if (us->abort_cmd) {
 			US_DEBUGP("freecom_readdata(): transfer aborted\n");
 			return US_BULK_TRANSFER_ABORTED;
 		}

@@ -232,7 +232,7 @@
                                 result, partial);
 
 		/* has the current command been aborted? */
-		if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+		if (us->abort_cmd) {
 			US_DEBUGP("freecom_writedata(): transfer aborted\n");
 			return US_BULK_TRANSFER_ABORTED;
 		}

@@ -295,7 +295,7 @@
                                 result, partial);
 
 		/* we canceled this transfer */
-		if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+		if (us->abort_cmd) {
 			US_DEBUGP("freecom_transport(): transfer aborted\n");
 			return US_BULK_TRANSFER_ABORTED;
 		}

@@ -310,7 +310,7 @@
         US_DEBUGP("foo Status result %d %d\n", result, partial);
 
 	/* we canceled this transfer */
-	if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+	if (us->abort_cmd) {
 		US_DEBUGP("freecom_transport(): transfer aborted\n");
 		return US_BULK_TRANSFER_ABORTED;
 	}

@@ -348,7 +348,7 @@
 					result, partial);
 
 			/* we canceled this transfer */
-			if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+			if (us->abort_cmd) {
 				US_DEBUGP("freecom_transport(): transfer aborted\n");
 				return US_BULK_TRANSFER_ABORTED;
 			}

@@ -363,7 +363,7 @@
 		US_DEBUGP("bar Status result %d %d\n", result, partial);
 
 		/* we canceled this transfer */
-		if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+		if (us->abort_cmd) {
 			US_DEBUGP("freecom_transport(): transfer aborted\n");
 			return US_BULK_TRANSFER_ABORTED;
 		}

@@ -424,7 +424,7 @@
                                 FCM_PACKET_LENGTH, &partial);
 		US_DEBUG(pdump ((void *) fst, partial));
 
-		if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+		if (us->abort_cmd) {
                         US_DEBUGP ("freecom_transport: transfer aborted\n");
                         return US_BULK_TRANSFER_ABORTED;
                 }

@@ -453,7 +453,7 @@
                 result = usb_stor_bulk_msg (us, fst, ipipe,
                                 FCM_PACKET_LENGTH, &partial);
 
-		if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+		if (us->abort_cmd) {
                         US_DEBUGP ("freecom_transport: transfer aborted\n");
                         return US_BULK_TRANSFER_ABORTED;
                 }

drivers/usb/storage/scsiglue.c +38 ~~-71~~

175 lines not on this page · raw message

drivers/usb/storage/transport.c +106 ~~-89~~

395 lines not on this page · raw message

drivers/usb/storage/transport.h +1 -1

diff -u 2.5/drivers/usb/storage/transport.h build-2.5/drivers/usb/storage/transport.h
@@ -149,7 +149,7 @@
 
 extern unsigned int usb_stor_transfer_length(Scsi_Cmnd*);
 extern void usb_stor_invoke_transport(Scsi_Cmnd*, struct us_data*);
-extern void usb_stor_abort_transport(struct us_data*);
+extern void usb_stor_abort_transport(struct us_data*, int call_scsidone);
 extern int usb_stor_transfer_partial(struct us_data*, char*, int);
 
 extern int usb_stor_bulk_msg(struct us_data *us, void *data, int pipe,

drivers/usb/storage/usb.c +43 ~~-41~~

247 lines not on this page · raw message

drivers/usb/storage/usb.h +51 ~~-12~~

108 lines not on this page · raw message

Matthew Dharm

2002-07-03 21:41 UTC #1 lore raw

I don't understand what this patch is trying to do...

You're reverting our new state machine changes... why?

You're reverting the new mechanism to determine device state... why?

You're removing the entire bus_reset() logic... why?

This patch undoes most of the work done in the last few months. I _strongly_ oppose the patch without some better explanations.

About the only think I agree with is the addition of some BUG_ON() calls.

Matt

Signature · 6 lines

Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

I need a computer?
					-- Customer
User Friendly, 2/19/1998

Manfred Spraul

author

2002-07-03 22:16 UTC #2 lore raw

Quoted 4 lines · Matthew Dharm

I don't understand what this patch is trying to do...
You're reverting our new state machine changes... why?

Because the state machine doesn't work. I've degraded it into a debugging state. I've described it in a mail I send to you and linux-usb-devel a few weeks ago, without any reply.

E.g. queue_command stored new commands in ->queue_srb. The worker thread then moved it from queue_srb to srb and set sm_state to RUNNING.

But what if command_abort() is called before the worker thread is scheduled?

State machines and asynchroneous command aborts are incompatible, that why I've moved command abortion out of sm_state.

Quoted 3 lines

You're reverting the new mechanism to determine device state... why?

Unnesessary duplication. Device disconnected is equivalent to ->pusb_dev==NULL. Why do you need a special variable?

Quoted 3 lines

You're removing the entire bus_reset() logic... why?

You are right, that change is not correct.
Do you remember the reasons that lead to the current implementation?

Hmm. Are you sure that the code can't cause data losses with unrelated devices?
Suppose I have an usb hub installed, and behind that hub 2 usb disks. If bus_reset is called for the scsi controller that represents one disk, won't that affect the data transfer that go to the other disk?

Quoted 3 lines

This patch undoes most of the work done in the last few months. I _strongly_ oppose the patch without some better explanations.

I've sent you a mail on 06/02 with details about all changes.

http://www.geocrawler.com/archives/3/2571/2002/6/600/8821396/

You did not reply, thus I assumed that you were too busy and I fixed everything myself.

The only new change is removing the call to usb_stor_CBI_irq() and replacing it with "up(&us->ip_waitq);" from usb_stor_abort_transport. Setting sm_state and then calling usb_stor_CBI_irq() is a synchronization nightmare.
Situation: command is completed by the hardware and aborted by the scsi midlayer at the same time. usb_stor_abort_transport() could run on cpu1, _CBI_irq() on cpu2. Now imagine you run on Alpha, where both reads and writes are reordered. Initially I tried to fix it with memory barriers, but the new version is much simpler.

--
Manfred

Matthew Dharm

2002-07-04 00:03 UTC #3 lore raw

Quoted 10 lines · Manfred Spraul

Quoted 4 lines · Matthew Dharm
I don't understand what this patch is trying to do...
You're reverting our new state machine changes... why?
Because the state machine doesn't work. I've degraded it into a debugging state.
I've described it in a mail I send to you and linux-usb-devel a few weeks ago, without any reply.

I've got that mail, and it's on my todo list.

Quoted 4 lines

E.g. queue_command stored new commands in ->queue_srb. The worker thread then moved it from queue_srb to srb and set sm_state to RUNNING.
But what if command_abort() is called before the worker thread is scheduled?

Then we have a serious problem, because the aborts are on the order of several seconds. If the thread hasn't gotten scheduled by then it _should_ cause a BUG_ON.

Quoted 2 lines

State machines and asynchroneous command aborts are incompatible, that why I've moved command abortion out of sm_state.

I disagree here. I think the clear state machine is the -only- way to get this right. We tried it without the state machine, and all we did was find more and more corner cases which are not handled.

Quoted 4 lines

Quoted 1 line
You're reverting the new mechanism to determine device state... why?
Unnesessary duplication. Device disconnected is equivalent to ->pusb_dev==NULL. Why do you need a special variable?

Because relying on a pointer has caused problems in the past, especially when there are concerns that the pointer might be invalid.

Quoted 10 lines

Quoted 2 lines
You're removing the entire bus_reset() logic... why?
You are right, that change is not correct.
Do you remember the reasons that lead to the current implementation?
Hmm. Are you sure that the code can't cause data losses with unrelated devices?
Suppose I have an usb hub installed, and behind that hub 2 usb disks. If bus_reset is called for the scsi controller that represents one disk, won't that affect the data transfer that go to the other disk?

The hub isn't reset, only the target device is.

Quoted 9 lines

Quoted 2 lines
This patch undoes most of the work done in the last few months. I _strongly_ oppose the patch without some better explanations.
I've sent you a mail on 06/02 with details about all changes.
http://www.geocrawler.com/archives/3/2571/2002/6/600/8821396/
You did not reply, thus I assumed that you were too busy and I fixed everything myself.

I see.. thus skipping the 4 patches which address most of these issues which are in my queue.

Look, I might not be that speedy on this, but did it at least occur to you to contact _any_ of the other usb-storage people? Bjorn? Stern?

Quoted 9 lines

The only new change is removing the call to usb_stor_CBI_irq() and replacing it with "up(&us->ip_waitq);" from usb_stor_abort_transport. Setting sm_state and then calling usb_stor_CBI_irq() is a synchronization nightmare.
Situation: command is completed by the hardware and aborted by the scsi midlayer at the same time. usb_stor_abort_transport() could run on cpu1, _CBI_irq() on cpu2. Now imagine you run on Alpha, where both reads and writes are reordered. Initially I tried to fix it with memory barriers, but the new version is much simpler.

The only requirement in this condition is that the command state be consistent at the end -- either completed or aborted. I don't see how the current code fails this requirement...

Matt

Signature · 8 lines

Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

A:  The most ironic oxymoron wins ...
DP: "Microsoft Works"
A:  Uh, okay, you win.
					-- A.J. & Dust Puppy
User Friendly, 1/18/1998

Manfred Spraul

author

2002-07-04 17:12 UTC #4 lore raw

Quoted 10 lines · Matthew Dharm

Quoted 4 lines
E.g. queue_command stored new commands in ->queue_srb. The worker thread then moved it from queue_srb to srb and set sm_state to RUNNING.
But what if command_abort() is called before the worker thread is scheduled?
Then we have a serious problem, because the aborts are on the order of several seconds. If the thread hasn't gotten scheduled by then it _should_ cause a BUG_ON.

First of all, it's dead ugly. usb-storage crashes if the scheduler is overloaded. IMHO that's a bug, especially since it's simple to avoid it.

And what about storage_disconnect()?

Test case: user pulls out the usb cable while a transfer is in progress. urb submitted to the device, reply not yet received.
Result: storage_disconnect() would hang for 20 seconds until command_abort() is called.
I've fixed that by adding usb_stor_abort_transport() into storage_disconnect(). But that means that abort_transport() could run in the window between queuecommand() and the scheduling of the worker thread.

Read through my proposal: With current_urb_sem [I've called it urb_sem, but it's the same concept], the synchronization between abort and new commands is guaranteed.

The only difference is that I've moved testing ->abort_cmd and down(&->urb_sem) into usb_stor_msg_common: Requesting that the callers must acquire the semaphore and check abort_cmd() is unnecessary code duplication and just asks for bugs.

Quoted 10 lines

Quoted 5 lines
Quoted 2 lines
You're reverting the new mechanism to determine device state... why?
Unnesessary duplication. Device disconnected is equivalent to ->pusb_dev==NULL. Why do you need a special variable?
Because relying on a pointer has caused problems in the past, especially when there are concerns that the pointer might be invalid.

Could you explain a bit more? How could the pointer become invalid?

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
You're removing the entire bus_reset() logic... why?

Quoted 9 lines

Quoted 5 lines
You are right, that change is not correct.
Do you remember the reasons that lead to the current implementation?
Hmm. Are you sure that the code can't cause data losses with unrelated devices?
Suppose I have an usb hub installed, and behind that hub 2 usb disks. If bus_reset is called for the scsi controller that represents one disk, won't that affect the data transfer that go to the other disk?
The hub isn't reset, only the target device is.

You are right.

That leaves one problem: a real disconnect in the middle of host_reset(), i.e. after checking us->bitflags or reading pusb_dev.

It should be possible to handle that case, too: usb_device structures are refcounted.

Quoted 9 lines

Quoted 3 lines
The only new change is removing the call to usb_stor_CBI_irq() and replacing it with "up(&us->ip_waitq);" from usb_stor_abort_transport. Setting sm_state and then calling usb_stor_CBI_irq() is a synchronization nightmare.
Situation: command is completed by the hardware and aborted by the scsi midlayer at the same time. usb_stor_abort_transport() could run on cpu1, _CBI_irq() on cpu2. Now imagine you run on Alpha, where both reads and writes are reordered. Initially I tried to fix it with memory barriers, but the new version is much simpler.
The only requirement in this condition is that the command state be consistent at the end -- either completed or aborted. I don't see how the current code fails this requirement...

My version is shorter ;-)
usb_stor_CBI_irq() containes 2 independent parts: only part only for command aborts, one part for normal interrupts. By splitting the function several lines of exception handling became unnecessary.

--
Manfred

Matthew Dharm

2002-07-04 19:47 UTC #5 lore raw

Quoted 16 lines · Manfred Spraul

Quoted 11 lines · Matthew Dharm
Quoted 5 lines
E.g. queue_command stored new commands in ->queue_srb. The worker thread then moved it from queue_srb to srb and set sm_state to RUNNING.
But what if command_abort() is called before the worker thread is scheduled?
Then we have a serious problem, because the aborts are on the order of several seconds. If the thread hasn't gotten scheduled by then it _should_ cause a BUG_ON.
First of all, it's dead ugly. usb-storage crashes if the scheduler is overloaded. IMHO that's a bug, especially since it's simple to avoid it.

Quoted 6 lines

And what about storage_disconnect()?
Test case: user pulls out the usb cable while a transfer is in progress. urb submitted to the device, reply not yet received.
Result: storage_disconnect() would hang for 20 seconds until command_abort() is called.

No, not quite. The HCD accelerates the URBs to completion if the device
is removed with an URB pending on it. It therefore shouldn't hang for the timeout -- if you're seeing this behavior, then the HCD is broken.

Quoted 8 lines

Read through my proposal: With current_urb_sem [I've called it urb_sem, but it's the same concept], the synchronization between abort and new commands is guaranteed.
The only difference is that I've moved testing ->abort_cmd and down(&->urb_sem) into usb_stor_msg_common: Requesting that the callers must acquire the semaphore and check abort_cmd() is unnecessary code duplication and just asks for bugs.

The reason it was moved up was to save lots of processing (including memory allocation paths) if the abort had already been requested. Which is a common case.

Quoted 11 lines

Quoted 9 lines
Quoted 4 lines
Quoted 1 line
You're reverting the new mechanism to determine device state... why?
Unnesessary duplication. Device disconnected is equivalent to ->pusb_dev==NULL. Why do you need a special variable?
Because relying on a pointer has caused problems in the past, especially when there are concerns that the pointer might be invalid.
Could you explain a bit more? How could the pointer become invalid?

There was a large discussion of this in relation to another driver on linux-usb-devel... basically, the pointer is just an address to a structure owned by an HCD... it's entirely possible for the structure to go away on us unexpectedly. We _should_ get the notification via the _disconnect() call, but real-world experience showed us that this wasn't always happening.

Matt

Signature · 6 lines

Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

I need a computer?
					-- Customer
User Friendly, 2/19/1998

Arnaldo Carvalho de Melo

2002-07-04 19:52 UTC #6 lore raw

Em Thu, Jul 04, 2002 at 12:50:12PM -0700, Matthew Dharm escreveu:

Quoted 11 lines

Quoted 4 lines
Quoted 2 lines
Because relying on a pointer has caused problems in the past, especially when there are concerns that the pointer might be invalid.
Could you explain a bit more? How could the pointer become invalid?
There was a large discussion of this in relation to another driver on linux-usb-devel... basically, the pointer is just an address to a structure owned by an HCD... it's entirely possible for the structure to go away on us unexpectedly. We _should_ get the notification via the _disconnect() call, but real-world experience showed us that this wasn't always happening.

Humm, I believed that USB was using refcounting

- Arnaldo

Greg KH

2002-07-04 20:01 UTC #7 lore raw

Quoted 6 lines · Arnaldo Carvalho de Melo

Em Thu, Jul 04, 2002 at 12:50:12PM -0700, Matthew Dharm escreveu:
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Because relying on a pointer has caused problems in the past, especially when there are concerns that the pointer might be invalid.
Could you explain a bit more? How could the pointer become invalid?

Quoted 8 lines

Quoted 6 lines
There was a large discussion of this in relation to another driver on linux-usb-devel... basically, the pointer is just an address to a structure owned by an HCD... it's entirely possible for the structure to go away on us unexpectedly. We _should_ get the notification via the _disconnect() call, but real-world experience showed us that this wasn't always happening.
Humm, I believed that USB was using refcounting

Parts of it is, parts aren't :(

Soon, due to the struct device and struct driver conversion everything should be properly reference counted. But I'm not really aware of the problem that Matt is referring to. Matt, what structure is disappearing?

thanks,

greg k-h

David Brownell

2002-07-04 20:19 UTC #8 lore raw

In reply to Matthew Dharm

Quoted 7 lines

Quoted 2 lines
Test case: user pulls out the usb cable while a transfer is in progress. urb submitted to the device, reply not yet received.
Result: storage_disconnect() would hang for 20 seconds until command_abort() is called.
No, not quite. The HCD accelerates the URBs to completion if the device
is removed with an URB pending on it. It therefore shouldn't hang for the timeout -- if you're seeing this behavior, then the HCD is broken.

Actually all of the interesting work is triggered by khubd, and then the device driver.

Khubd calls usb_disconnect() for the device. That disconnects each driver (which is supposed to wait until all urbs it's submitted have completed, and not submit any more URBS).

Only at the very end of this does the HCD hear anything about devices going away. If there's any URB still submitted at that point it's not a bug in the HCD at all ... but in a device driver that didn't implement disconnect() correctly.

- Dave

Matthew Dharm

2002-07-04 21:14 UTC #9 lore raw

Now... wait a second here....

You say the dirver "is supposed to wait until all urbs" are completed... but we're talking about the case where the device is now gone...

So how do the urbs complete? Something is causing them to complete when the cable is removed -- that's the behavior I've observed, at least.

Matt

Quoted 26 lines · David Brownell

Quoted 9 lines
Quoted 4 lines
Test case: user pulls out the usb cable while a transfer is in progress. urb submitted to the device, reply not yet received.
Result: storage_disconnect() would hang for 20 seconds until command_abort() is called.
No, not quite. The HCD accelerates the URBs to completion if the device
is removed with an URB pending on it. It therefore shouldn't hang for the timeout -- if you're seeing this behavior, then the HCD is broken.
Actually all of the interesting work is triggered by khubd, and then the device driver.
Khubd calls usb_disconnect() for the device. That disconnects each driver (which is supposed to wait until all urbs it's submitted have completed, and not submit any more URBS).
Only at the very end of this does the HCD hear anything about devices going away. If there's any URB still submitted at that point it's not a bug in the HCD at all ... but in a device driver that didn't implement disconnect() correctly.
- Dave

Signature · 6 lines

Matthew Dharm                              Home: [email protected] 
Maintainer, Linux USB Mass Storage Driver

Way to go, lava boy.
					-- Stef to Greg
User Friendly, 3/26/1998

David Brownell

2002-07-04 22:00 UTC #10 lore raw

Quoted 7 lines · Matthew Dharm

Now... wait a second here....
You say the dirver "is supposed to wait until all urbs" are completed... but we're talking about the case where the device is now gone...
So how do the urbs complete? Something is causing them to complete when the cable is removed -- that's the behavior I've observed, at least.

Normally I'd expect the device driver to forcibly unlink anything it's started. Failing that, the host controller will often fail the transfer if the device is gone. (But I seem to recall recent speculation about maybe one of the UHCIs not doing that.)

In either case, the device driver should block until all its pending URBs complete.

- Dave

Quoted 30 lines

Matt
Quoted 24 lines · David Brownell
Quoted 7 lines
Quoted 2 lines
Test case: user pulls out the usb cable while a transfer is in progress. urb submitted to the device, reply not yet received.
Result: storage_disconnect() would hang for 20 seconds until command_abort() is called.
No, not quite. The HCD accelerates the URBs to completion if the device
is removed with an URB pending on it. It therefore shouldn't hang for the timeout -- if you're seeing this behavior, then the HCD is broken.
Actually all of the interesting work is triggered by khubd, and then the device driver.
Khubd calls usb_disconnect() for the device. That disconnects each driver (which is supposed to wait until all urbs it's submitted have completed, and not submit any more URBS).
Only at the very end of this does the HCD hear anything about devices going away. If there's any URB still submitted at that point it's not a bug in the HCD at all ... but in a device driver that didn't implement disconnect() correctly.
- Dave