Messages in this thread | | | Date | Thu, 04 Jul 2002 19:12:40 +0200 | From | Manfred Spraul <> | Subject | Re: usb storage cleanup |
| |
Matthew Dharm wrote: > >>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.
> >>>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?
> >>>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. > 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.
> >>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
- 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/
| |