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
diff -u 2.5/drivers/usb/storage/freecom.c build-2.5/drivers/usb/storage/freecom.c
--- 2.5/drivers/usb/storage/freecom.c Tue Jun 4 20:39:19 2002
+++ build-2.5/drivers/usb/storage/freecom.c Wed Jul 3 20:31:14 2002
@@ -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;
}
diff -u 2.5/drivers/usb/storage/scsiglue.c build-2.5/drivers/usb/storage/scsiglue.c
--- 2.5/drivers/usb/storage/scsiglue.c Tue Jun 25 20:24:18 2002
+++ build-2.5/drivers/usb/storage/scsiglue.c Wed Jul 3 20:35:42 2002
@@ -104,6 +104,7 @@
* NOTE: There is no contention here, because we're already deregistered
* the driver and we're doing each virtual host in turn, not in parallel
* Synchronization: BLK, no spinlock.
+ * The function sleeps, no need for _irqsave.
*/
static int release(struct Scsi_Host *psh)
{
@@ -117,7 +118,11 @@
* notification that it's exited.
*/
US_DEBUGP("-- sending US_ACT_EXIT command to thread\n");
+
+ spin_lock_irq(&us->queue_exclusion);
us->action = US_ACT_EXIT;
+ BUG_ON(us->srb != NULL);
+ spin_unlock_irq(&us->queue_exclusion);
up(&(us->sema));
wait_for_completion(&(us->notify));
@@ -150,7 +155,8 @@
spin_lock_irqsave(&us->queue_exclusion, flags);
/* enqueue the command */
- us->queue_srb = srb;
+ BUG_ON(us->srb != NULL);
+ us->srb = srb;
srb->scsi_done = done;
us->action = US_ACT_COMMAND;
@@ -174,12 +180,9 @@
US_DEBUGP("command_abort() called\n");
- if (atomic_read(&us->sm_state) == US_STATE_RUNNING) {
+ if (us->srb) {
scsi_unlock(srb->host);
- usb_stor_abort_transport(us);
-
- /* wait for us to be done */
- wait_for_completion(&(us->notify));
+ usb_stor_abort_transport(us, 0);
scsi_lock(srb->host);
return SUCCESS;
}
@@ -198,16 +201,25 @@
US_DEBUGP("device_reset() called\n" );
/* if the device was removed, then we're already reset */
- if (!test_bit(DEV_ATTACHED, &us->bitflags))
- return SUCCESS;
scsi_unlock(srb->host);
/* lock the device pointers */
down(&(us->dev_semaphore));
- us->srb = srb;
- atomic_set(&us->sm_state, US_STATE_RESETTING);
- result = us->transport_reset(us);
- atomic_set(&us->sm_state, US_STATE_IDLE);
+ /* previous commands were stopped with command_abort(),
+ * and the scsi midlayer prevents new commands from comming
+ * in until device_reset returns.
+ */
+ BUG_ON(us->srb != NULL);
+ BUG_ON(us->sm_state != US_STATE_IDLE);
+ if (us->pusb_dev == NULL) {
+ result = SUCCESS;
+ } else {
+ us->srb = srb;
+ us->sm_state = US_STATE_RESETTING;
+ result = us->transport_reset(us);
+ us->srb = NULL;
+ us->sm_state = US_STATE_IDLE;
+ }
/* unlock the device pointers */
up(&(us->dev_semaphore));
@@ -215,68 +227,18 @@
return result;
}
-/* This resets the device port, and simulates the device
- * disconnect/reconnect for all drivers which have claimed
- * interfaces, including ourself. */
+/* FIXME: is that the best we can do? */
static int bus_reset( Scsi_Cmnd *srb )
{
- struct us_data *us = (struct us_data *)srb->host->hostdata[0];
- int i;
- int result;
- struct usb_device *pusb_dev_save = us->pusb_dev;
-
- /* we use the usb_reset_device() function to handle this for us */
- US_DEBUGP("bus_reset() called\n");
-
- /* if the device has been removed, this worked */
- if (!test_bit(DEV_ATTACHED, &us->bitflags)) {
- US_DEBUGP("-- device removed already\n");
- return SUCCESS;
- }
-
- /* attempt to reset the port */
- scsi_unlock(srb->host);
- result = usb_reset_device(pusb_dev_save);
- US_DEBUGP("usb_reset_device returns %d\n", result);
- if (result < 0) {
- scsi_lock(srb->host);
- return FAILED;
- }
-
- /* FIXME: This needs to lock out driver probing while it's working
- * or we can have race conditions */
- /* Is that still true? I don't see how... AS */
- for (i = 0; i < pusb_dev_save->actconfig->bNumInterfaces; i++) {
- struct usb_interface *intf =
- &pusb_dev_save->actconfig->interface[i];
- const struct usb_device_id *id;
-
- /* if this is an unclaimed interface, skip it */
- if (!intf->driver) {
- continue;
- }
-
- US_DEBUGP("Examining driver %s...", intf->driver->name);
-
- /* simulate a disconnect and reconnect for all interfaces */
- US_DEBUGPX("simulating disconnect/reconnect.\n");
- down(&intf->driver->serialize);
- intf->driver->disconnect(pusb_dev_save, intf->private_data);
- id = usb_match_id(pusb_dev_save, intf, intf->driver->id_table);
- intf->driver->probe(pusb_dev_save, i, id);
- up(&intf->driver->serialize);
- }
- US_DEBUGP("bus_reset() complete\n");
- scsi_lock(srb->host);
- return SUCCESS;
+ printk(KERN_INFO "usb-storage: bus_reset() requested but not implemented\n" );
+ return device_reset(srb);
}
-/* FIXME: This doesn't do anything right now */
+/* FIXME: is that the best we can do? */
static int host_reset( Scsi_Cmnd *srb )
{
- printk(KERN_CRIT "usb-storage: host_reset() requested but not implemented\n" );
- bus_reset(srb);
- return FAILED;
+ printk(KERN_INFO "usb-storage: host_reset() requested but not implemented\n" );
+ return device_reset(srb);
}
/***********************************************************************
@@ -309,7 +271,12 @@
us = us->next;
}
- /* release our lock on the data structures */
+ /* Release our lock on the data structures early.
+ * The only function that frees us structures is
+ * usb_stor_exit(), and that happens after
+ * scsi_unregister_host, i.e. never while the
+ * /proc interface is in use.
+ */
up(&us_list_semaphore);
/* if we couldn't find it, we return an error */
@@ -331,8 +298,7 @@
/* show the GUID of the device */
SPRINTF(" GUID: " GUID_FORMAT "\n", GUID_ARGS(us->guid));
- SPRINTF(" Attached: %s\n", (test_bit(DEV_ATTACHED, &us->bitflags)
- ? "Yes" : "No"));
+ SPRINTF(" Attached: %s\n", (us->pusb_dev != NULL) ? "Yes" : "No");
/*
* Calculate start of next buffer, and return value.
@@ -370,6 +336,7 @@
this_id: -1,
sg_tablesize: SG_ALL,
+ max_sectors: 128,
cmd_per_lun: 1,
present: 0,
unchecked_isa_dma: FALSE,
diff -u 2.5/drivers/usb/storage/transport.c build-2.5/drivers/usb/storage/transport.c
--- 2.5/drivers/usb/storage/transport.c Tue Jun 4 20:39:19 2002
+++ build-2.5/drivers/usb/storage/transport.c Wed Jul 3 20:36:17 2002
@@ -357,21 +357,21 @@
* We're very concered about races with a command abort. Hanging this code is
* a sure fire way to hang the kernel.
*
- * The abort function first sets the machine state, then tries to acquire the
+ * The abort function first sets abort_cmd, then tries to acquire the
* lock on the current_urb to abort it.
*
- * Once a function grabs the current_urb_sem, then it -MUST- test the state to
+ * Once a function grabs the urb_sem, then it -MUST- test the state to
* see if we just got aborted before the lock was grabbed. If so, it's
* essential to release the lock and return.
*
- * The idea is that, once the current_urb_sem is held, the state can't really
+ * The idea is that, once the urb_sem is held, the state can't really
* change anymore without also engaging the usb_unlink_urb() call _after_ the
* URB is actually submitted.
*
- * So, we've guaranteed that (after the sm_state test), if we do submit the
- * URB it will get aborted when we release the current_urb_sem. And we've
+ * So, we've guaranteed that (after the abort_cmd test), if we do submit the
+ * URB it will get aborted when we release the urb_sem. And we've
* also guaranteed that if the abort code was called before we held the
- * current_urb_sem, we can safely get out before the URB is submitted.
+ * urb_sem, we can safely get out before the URB is submitted.
*/
/* This is the completion handler which will wake us up when an URB
@@ -385,7 +385,7 @@
}
/* This is the common part of the URB message submission code
- * This function expects the current_urb_sem to be held upon entry.
+ * This function expects the dev_semaphore to be held upon entry.
*
* All URBs from the usb-storage driver _must_ pass through this function
* (or something like it) for the abort mechanisms to work properly.
@@ -395,6 +395,8 @@
struct completion urb_done;
int status;
+ BUG_NOT_DOWN(&us->dev_semaphore);
+
/* set up data structures for the wakeup system */
init_completion(&urb_done);
@@ -405,16 +407,19 @@
us->current_urb->transfer_flags = USB_ASYNC_UNLINK;
/* submit the URB */
- status = usb_submit_urb(us->current_urb, GFP_NOIO);
- if (status) {
- /* something went wrong */
- return status;
+ down(&us->urb_sem);
+ if (us->abort_cmd) {
+ /* we are in abort mode, do not submit the new urb */
+ status = -ENOENT;
+ } else {
+ status = usb_submit_urb(us->current_urb, GFP_NOIO);
}
+ up(&us->urb_sem);
+ if (status)
+ return status;
/* wait for the completion of the URB */
- up(&(us->current_urb_sem));
wait_for_completion(&urb_done);
- down(&(us->current_urb_sem));
/* return the URB status */
return us->current_urb->status;
@@ -441,12 +446,11 @@
dr->wIndex = cpu_to_le16(index);
dr->wLength = cpu_to_le16(size);
- /* lock the URB */
- down(&(us->current_urb_sem));
+ /* is the device access locked? */
+ BUG_NOT_DOWN(&us->dev_semaphore);
/* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
- up(&(us->current_urb_sem));
+ if (us->abort_cmd) {
return 0;
}
@@ -462,8 +466,6 @@
if (status >= 0)
status = us->current_urb->actual_length;
- /* release the lock and return status */
- up(&(us->current_urb_sem));
return status;
}
@@ -475,14 +477,12 @@
{
int status;
- /* lock the URB */
- down(&(us->current_urb_sem));
+ /* is the device access locked? */
+ BUG_NOT_DOWN(&us->dev_semaphore);
/* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
- up(&(us->current_urb_sem));
+ if (us->abort_cmd)
return 0;
- }
/* fill the URB */
FILL_BULK_URB(us->current_urb, us->pusb_dev, pipe, data, len,
@@ -494,8 +494,6 @@
/* return the actual length of the data transferred */
*act_len = us->current_urb->actual_length;
- /* release the lock and return status */
- up(&(us->current_urb_sem));
return status;
}
@@ -571,7 +569,7 @@
}
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_transfer_partial(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -618,6 +616,9 @@
/* calculate how much we want to transfer */
transfer_amount = usb_stor_transfer_length(srb);
+ US_DEBUGP("usb_stor_transfer(): request for %d/%d bytes\n",
+ transfer_amount, srb->request_bufflen);
+
/* was someone foolish enough to request more data than available
* buffer space? */
if (transfer_amount > srb->request_bufflen)
@@ -849,44 +850,65 @@
srb->sense_buffer[0] = 0x0;
}
-/* Abort the currently running scsi command or device reset.
+
+/*
+ * Abort the currently running scsi command or device reset.
+ * The function waits until the currently executing command
+ * has finished.
*/
-void usb_stor_abort_transport(struct us_data *us)
+void usb_stor_abort_transport(struct us_data *us, int call_scsidone)
{
- int state = atomic_read(&us->sm_state);
-
US_DEBUGP("usb_stor_abort_transport called\n");
- /* If the current state is wrong or if there's
- * no srb, then there's nothing to do */
- BUG_ON((state != US_STATE_RUNNING && state != US_STATE_RESETTING));
- BUG_ON(!(us->srb));
-
- /* set state to abort */
- atomic_set(&us->sm_state, US_STATE_ABORTING);
-
- /* If the state machine is blocked waiting for an URB or an IRQ,
- * let's wake it up */
-
- /* If we have an URB pending, cancel it. Note that we guarantee with
- * the current_urb_sem that either (a) an URB has just been submitted,
- * or (b) the URB will never get submitted. But, because of the use
- * of us->sm_state and current_urb_sem, we can't get an URB sumbitted
- * _after_ we set the state to US_STATE_ABORTING and this section of
- * code runs. Thus we avoid deadlocks.
+ /* urb_sem is required to synchronize usb_unlink_urb()
+ * and submit_urb():
+ * The caller must guarantee that the urb that is
+ * passed to unlink_urb was submitted with submit_urb().
+ * Without the lock, we could call unlink_urb in the middle
+ * of submit_urb().
+ *
+ * urb_sem also doubles as the memory barrier that
+ * makes the update to abort_cmd visible on all cpus in the
+ * system.
+ */
+ down(&us->urb_sem);
+ if (call_scsidone)
+ us->abort_cmd = ABORT_CALLSCSIDONE;
+ else
+ us->abort_cmd = ABORT_NOSCSIDONE;
+
+ /* Check if the worker thread is processing the
+ * command right now, abort it if yes.
*/
- down(&(us->current_urb_sem));
+
if (us->current_urb->status == -EINPROGRESS) {
US_DEBUGP("-- cancelling URB\n");
usb_unlink_urb(us->current_urb);
}
- up(&(us->current_urb_sem));
/* If we are waiting for an IRQ, simulate it */
- if (test_bit(IP_WANTED, &us->bitflags)) {
+ if (test_and_clear_bit(IP_WANTED, &us->bitflags)) {
+ struct us_data *us = (struct us_data *)us->irq_urb->context;
US_DEBUGP("-- simulating missing IRQ\n");
- usb_stor_CBI_irq(us->irq_urb);
+ /* wake up the command thread */
+ up(&us->ip_waitq);
}
+ up(&us->urb_sem);
+
+ spin_lock_irq(&us->queue_exclusion);
+ while (us->srb) {
+ DECLARE_WAITQUEUE(wait, current);
+ current->state = TASK_UNINTERRUPTIBLE;
+ add_wait_queue(&us->wq_cmd, &wait);
+ spin_unlock_irq(&us->queue_exclusion);
+ schedule();
+ spin_lock_irq(&us->queue_exclusion);
+ remove_wait_queue(&us->wq_cmd, &wait);
+ }
+ spin_unlock_irq(&us->queue_exclusion);
+ down(&us->urb_sem);
+ us->abort_cmd = 0;
+ up(&us->urb_sem);
}
/*
@@ -904,21 +926,6 @@
US_DEBUGP("-- Interrupt Status (0x%x, 0x%x)\n",
us->irqbuf[0], us->irqbuf[1]);
- /* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
-
- /* was this a wanted interrupt? */
- if (!test_and_clear_bit(IP_WANTED, &us->bitflags)) {
- US_DEBUGP("ERROR: Unwanted interrupt received!\n");
- return;
- }
- US_DEBUGP("-- command aborted\n");
-
- /* wake up the command thread */
- up(&us->ip_waitq);
- return;
- }
-
/* is the device removed? */
if (urb->status == -ENOENT) {
US_DEBUGP("-- device has been removed\n");
@@ -987,7 +994,7 @@
}
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_control_msg(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -999,7 +1006,7 @@
usb_sndctrlpipe(us->pusb_dev, 0));
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_control_msg(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1036,7 +1043,7 @@
down(&(us->ip_waitq));
/* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("CBI interrupt aborted\n");
return USB_STOR_TRANSPORT_ABORTED;
}
@@ -1104,7 +1111,7 @@
US_DEBUGP("Call to usb_stor_control_msg() returned %d\n", result);
if (result < 0) {
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_CB_transport(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1116,7 +1123,7 @@
usb_sndctrlpipe(us->pusb_dev, 0));
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_CB_transport(): transfer
aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1225,7 +1232,7 @@
US_DEBUGP("Bulk command transfer result=%d\n", result);
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_Bulk_transport(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1236,7 +1243,7 @@
result = usb_stor_clear_halt(us, pipe);
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_Bulk_transport(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1273,7 +1280,7 @@
&partial);
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_Bulk_transport(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1284,7 +1291,7 @@
result = usb_stor_clear_halt(us, pipe);
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_Bulk_transport(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1295,7 +1302,7 @@
US_BULK_CS_WRAP_LEN, &partial);
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_Bulk_transport(): transfer aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1306,7 +1313,7 @@
result = usb_stor_clear_halt(us, pipe);
/* did we abort this command? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
US_DEBUGP("usb_stor_Bulk_transport(): transfer
aborted\n");
return US_BULK_TRANSFER_ABORTED;
}
@@ -1358,23 +1365,33 @@
struct us_timeout {
struct us_data *us;
- spinlock_t timer_lock;
+ struct completion done;
+ struct tq_struct event;
};
+static void usb_stor_event_handler(void* to__)
+{
+ struct us_timeout *to = (struct us_timeout *) to__;
+ struct us_data *us = to->us;
+
+ US_DEBUGP("timeout event handler called\n");
+
+ /* abort the current request */
+ usb_stor_abort_transport(us, 1);
+
+ complete(&to->done);
+}
+
/* The timeout event handler
*/
static void usb_stor_timeout_handler(unsigned long to__)
{
struct us_timeout *to = (struct us_timeout *) to__;
- struct us_data *us = to->us;
US_DEBUGP("Timeout occurred\n");
- /* abort the current request */
- usb_stor_abort_transport(us);
-
- /* let the reset routine know we have finished */
- spin_unlock(&to->timer_lock);
+ INIT_TQUEUE(&to->event, usb_stor_event_handler, to);
+ schedule_task(&to->event);
}
/* This is the common part of the device reset code.
@@ -1389,11 +1406,12 @@
u16 value, u16 index, void *data, u16 size)
{
int result;
- struct us_timeout timeout_data = {us, SPIN_LOCK_UNLOCKED};
+ struct us_timeout timeout_data;
struct timer_list timeout_list;
/* prepare the timeout handler */
- spin_lock(&timeout_data.timer_lock);
+ timeout_data.us = us;
+ init_completion(&timeout_data.done);
init_timer(&timeout_list);
/* A 20-second timeout may seem rather long, but a LaCie
@@ -1429,8 +1447,7 @@
/* prevent the timer from coming back to haunt us */
if (!del_timer(&timeout_list)) {
- /* the handler has already started; wait for it to finish */
- spin_lock(&timeout_data.timer_lock);
+ wait_for_completion(&timeout_data.done);
/* change the abort into a timeout */
if (result == -ENOENT)
result = -ETIMEDOUT;
diff -u 2.5/drivers/usb/storage/transport.h build-2.5/drivers/usb/storage/transport.h
--- 2.5/drivers/usb/storage/transport.h Sun May 26 11:05:08 2002
+++ build-2.5/drivers/usb/storage/transport.h Wed Jul 3 20:34:01 2002
@@ -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,
diff -u 2.5/drivers/usb/storage/usb.c build-2.5/drivers/usb/storage/usb.c
--- 2.5/drivers/usb/storage/usb.c Tue Jun 25 20:28:28 2002
+++ build-2.5/drivers/usb/storage/usb.c Wed Jul 3 22:45:47 2002
@@ -101,7 +101,7 @@
/* The list of structures and the protective lock for them */
struct us_data *us_list;
-struct semaphore us_list_semaphore;
+DECLARE_MUTEX(us_list_semaphore);
static void * storage_probe(struct usb_device *dev, unsigned int ifnum,
const struct usb_device_id *id);
@@ -326,6 +326,9 @@
/* set up for wakeups by new commands */
init_MUTEX_LOCKED(&us->sema);
+ down(&us->dev_semaphore);
+ us->sm_state = US_STATE_IDLE;
+ up(&us->dev_semaphore);
/* signal that we've started the thread */
complete(&(us->notify));
@@ -333,12 +336,13 @@
for(;;) {
struct Scsi_Host *host;
US_DEBUGP("*** thread sleeping.\n");
- atomic_set(&us->sm_state, US_STATE_IDLE);
- if(down_interruptible(&us->sema))
+ wake_up(&us->wq_cmd);
+ if(down_interruptible(&us->sema)) {
+ printk(KERN_ERR "Received unexpected signal in
+usb_stor_control_thread().\n");
break;
+ }
US_DEBUGP("*** thread awakened.\n");
- atomic_set(&us->sm_state, US_STATE_RUNNING);
/* lock access to the queue element */
spin_lock_irq(&us->queue_exclusion);
@@ -346,9 +350,6 @@
/* take the command off the queue */
action = us->action;
us->action = 0;
- us->srb = us->queue_srb;
- host = us->srb->host;
-
/* release the queue lock as fast as possible */
spin_unlock_irq(&us->queue_exclusion);
@@ -359,6 +360,8 @@
}
BUG_ON(action != US_ACT_COMMAND);
+ host = us->srb->host;
+
/* reject the command if the direction indicator
* is UNKNOWN
@@ -416,9 +419,10 @@
/* lock the device pointers */
down(&(us->dev_semaphore));
+ us->sm_state = US_STATE_RUNNING;
/* our device has gone - pretend not ready */
- if (!test_bit(DEV_ATTACHED, &us->bitflags)) {
+ if (us->pusb_dev == NULL) {
US_DEBUGP("Request is for removed device\n");
/* For REQUEST_SENSE, it's the data. But
* for anything else, it should look like
@@ -442,7 +446,7 @@
sizeof(usb_stor_sense_notready));
us->srb->result = CHECK_CONDITION << 1;
}
- } else { /* test_bit(DEV_ATTACHED, &us->bitflags) */
+ } else { /* us->pusb_dev != NULL */
/* Handle those devices which need us to fake
* their inquiry data */
@@ -461,23 +465,22 @@
us->proto_handler(us->srb, us);
}
}
+ us->sm_state = US_STATE_IDLE;
/* unlock the device pointers */
up(&(us->dev_semaphore));
/* indicate that the command is done */
- if (us->srb->result != DID_ABORT << 16) {
- US_DEBUGP("scsi cmd done, result=0x%x\n",
- us->srb->result);
- scsi_lock(host);
+ scsi_lock(host);
+ US_DEBUGP("scsi cmd done, result=0x%x\n", us->srb->result);
+ if (us->abort_cmd != ABORT_NOSCSIDONE) {
+ /* If we are in error recovery mode,
+ * no scsi_done callback needed.
+ */
us->srb->scsi_done(us->srb);
- us->srb = NULL;
- scsi_unlock(host);
- } else {
- US_DEBUGP("scsi command aborted\n");
- us->srb = NULL;
- complete(&(us->notify));
}
+ us->srb = NULL;
+ scsi_unlock(host);
} /* for (;;) */
/* notify the exit routine that we're actually exiting now */
@@ -500,13 +503,12 @@
US_DEBUGP("Allocating IRQ for CBI transport\n");
- /* lock access to the data structure */
- down(&(ss->irq_urb_sem));
+ /* check that the device is locked properly */
+ BUG_NOT_DOWN(&ss->dev_semaphore);
/* allocate the URB */
ss->irq_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!ss->irq_urb) {
- up(&(ss->irq_urb_sem));
US_DEBUGP("couldn't allocate interrupt URB");
return 1;
}
@@ -527,12 +529,9 @@
US_DEBUGP("usb_submit_urb() returns %d\n", result);
if (result) {
usb_free_urb(ss->irq_urb);
- up(&(ss->irq_urb_sem));
return 2;
}
- /* unlock the data structure and return success */
- up(&(ss->irq_urb_sem));
return 0;
}
@@ -569,6 +568,8 @@
intf[ifnum].altsetting + intf[ifnum].act_altsetting;
US_DEBUGP("act_altsetting is %d\n", intf[ifnum].act_altsetting);
+ BUG_NOT_DOWN(&usb_storage_driver.serialize);
+
/* clear the temporary strings */
memset(mf, 0, sizeof(mf));
memset(prod, 0, sizeof(prod));
@@ -686,10 +687,11 @@
* already on the system
*/
ss = us_list;
- while ((ss != NULL) &&
- (test_bit(DEV_ATTACHED, &ss->bitflags) ||
- !GUID_EQUAL(guid, ss->guid)))
+ while (ss != NULL) {
+ if (ss->pusb_dev == NULL && GUID_EQUAL(guid, ss->guid))
+ break;
ss = ss->next;
+ }
if (ss != NULL) {
/* Existing device -- re-connect */
@@ -701,8 +703,8 @@
/* establish the connection to the new device upon reconnect */
ss->ifnum = ifnum;
+ BUG_ON(ss->pusb_dev != NULL);
ss->pusb_dev = dev;
- set_bit(DEV_ATTACHED, &ss->bitflags);
/* copy over the endpoint data */
ss->ep_in = ep_in->bEndpointAddress &
@@ -749,9 +751,9 @@
init_completion(&(ss->notify));
init_MUTEX_LOCKED(&(ss->ip_waitq));
spin_lock_init(&ss->queue_exclusion);
- init_MUTEX(&(ss->irq_urb_sem));
- init_MUTEX(&(ss->current_urb_sem));
- init_MUTEX(&(ss->dev_semaphore));
+ init_MUTEX(&ss->urb_sem);
+ init_MUTEX_LOCKED(&(ss->dev_semaphore));
+ init_waitqueue_head(&ss->wq_cmd);
/* copy over the subclass and protocol data */
ss->subclass = subclass;
@@ -969,9 +971,11 @@
if (unusual_dev && unusual_dev->initFunction)
unusual_dev->initFunction(ss);
+ /* Now we are ready */
+ up(&ss->dev_semaphore);
+
/* start up our control thread */
- atomic_set(&ss->sm_state, US_STATE_IDLE);
- set_bit(DEV_ATTACHED, &ss->bitflags);
+ ss->sm_state = US_STATE_BAD;
ss->pid = kernel_thread(usb_stor_control_thread, ss,
CLONE_VM);
if (ss->pid < 0) {
@@ -1019,20 +1023,17 @@
/* we come here if there are any problems */
BadDevice:
US_DEBUGP("storage_probe() failed\n");
- down(&ss->irq_urb_sem);
if (ss->irq_urb) {
usb_unlink_urb(ss->irq_urb);
usb_free_urb(ss->irq_urb);
ss->irq_urb = NULL;
}
- up(&ss->irq_urb_sem);
if (ss->current_urb) {
usb_unlink_urb(ss->current_urb);
usb_free_urb(ss->current_urb);
ss->current_urb = NULL;
}
- clear_bit(DEV_ATTACHED, &ss->bitflags);
ss->pusb_dev = NULL;
if (new_device)
kfree(ss);
@@ -1050,17 +1051,21 @@
US_DEBUGP("storage_disconnect() called\n");
+ BUG_NOT_DOWN(&usb_storage_driver.serialize);
+
/* this is the odd case -- we disconnected but weren't using it */
if (!ss) {
US_DEBUGP("-- device was not in use\n");
return;
}
+ /* kill a pending command */
+ usb_stor_abort_transport(ss, 1);
+
/* lock access to the device data structure */
down(&(ss->dev_semaphore));
/* release the IRQ, if we have one */
- down(&(ss->irq_urb_sem));
if (ss->irq_urb) {
US_DEBUGP("-- releasing irq URB\n");
result = usb_unlink_urb(ss->irq_urb);
@@ -1068,7 +1073,6 @@
usb_free_urb(ss->irq_urb);
ss->irq_urb = NULL;
}
- up(&(ss->irq_urb_sem));
/* free up the main URB for this device */
US_DEBUGP("-- releasing main URB\n");
@@ -1080,7 +1084,6 @@
/* mark the device as gone */
usb_put_dev(ss->pusb_dev);
ss->pusb_dev = NULL;
- clear_bit(DEV_ATTACHED, &ss->bitflags);
/* unlock access to the device data structure */
up(&(ss->dev_semaphore));
@@ -1096,7 +1099,6 @@
/* initialize internal global data elements */
us_list = NULL;
- init_MUTEX(&us_list_semaphore);
my_host_number = 0;
/* register the driver, return -1 if error */
diff -u 2.5/drivers/usb/storage/usb.h build-2.5/drivers/usb/storage/usb.h
--- 2.5/drivers/usb/storage/usb.h Tue Jun 25 20:28:28 2002
+++ build-2.5/drivers/usb/storage/usb.h Wed Jul 3 20:35:04 2002
@@ -39,6 +39,28 @@
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ * Changelog:
+ * - remove the DEV_ATTACHED bitflag, it duplicates (->pusb_dev !=NULL)
+ * - make sm_state a normal variable, atomic_t do not provide synchronization
+ * for assignments
+ * - merge queue_srb and srb, that created a hole for crashes if a device_reset()
+ * happens too early. Add BUG_ON tests to catch errors.
+ * - device aborts happen asynchroneously, make abort_cmd a seperate variable
+ * - add BUG_ON tests to catch missing semaphore calls
+ * - bus_reset dropped for now, synchronization is too fragile, no real need for
+ * it
+ * - restrict commands to 128 sectors, large commands cause errors with my SaroTech
+ * USB enclosure
+ * - move urb_sem into usb_stor_msg_common(), asking the callers to lock & check for
+ * abort_cmd is too error prone.
+ * - add an explicit waitqueue for the device reset handler, reuse of the
+ * completion caused deadlocks in command_abort().
+ * - move the _timeout handler for usb_stor to process context.
+ * - fixoops during module unload.
+ * - abort a possibly pending command during storage_disconnect(), could cause
+ * excessive delays if the cable is pulled out while a command is pending.
+ * (C) Manfred Spraul <[EMAIL PROTECTED]> 2002
*/
#ifndef _USB_H_
@@ -107,13 +129,16 @@
/* kernel thread actions */
#define US_ACT_COMMAND 1
-#define US_ACT_EXIT 5
+#define US_ACT_EXIT 2
-/* processing state machine states */
+/* processing state machine states
+ * For debugging only, it can be wrong because queue_command()
+ * queues new requests asynchroneously.
+ * */
+#define US_STATE_BAD 0
#define US_STATE_IDLE 1
#define US_STATE_RUNNING 2
#define US_STATE_RESETTING 3
-#define US_STATE_ABORTING 4
#define USB_STOR_STRING_LEN 32
@@ -127,9 +152,12 @@
struct us_data *next; /* next device */
/* The device we're working with
- * It's important to note:
- * (o) you must hold dev_semaphore to change pusb_dev
- * (o) DEV_ATTACHED in bitflags should change whenever pusb_dev does
+ * It's important to note that you must hold dev_semaphore
+ * to change or use pusb_dev.
+ *
+ * Virtually the complete device access is serialized through
+ * dev_semaphore, the only notable exception is command abortion
+ * which is serialized by urb_sem.
*/
struct semaphore dev_semaphore; /* protect pusb_dev */
struct usb_device *pusb_dev; /* this usb_device */
@@ -166,25 +194,28 @@
Scsi_Cmnd *srb; /* current srb */
/* thread information */
- Scsi_Cmnd *queue_srb; /* the single queue slot */
int action; /* what to do */
int pid; /* control thread */
- atomic_t sm_state;
+ unsigned long sm_state;
+ wait_queue_head_t wq_cmd; /* command completion queue */
+ struct semaphore urb_sem; /* synchronize {unlink,submit}_urb */
+
+ unsigned long abort_cmd;
+/* 0 means do not abort */
+#define ABORT_CALLSCSIDONE 1
+#define ABORT_NOSCSIDONE 2
/* interrupt info for CBI devices -- only good if attached */
struct semaphore ip_waitq; /* for CBI interrupts */
- unsigned long bitflags; /* single-bit flags: */
+ unsigned long bitflags; /* atomic bitflags */
#define IP_WANTED 1 /* is an IRQ expected? */
-#define DEV_ATTACHED 2 /* is the dev. attached?*/
/* interrupt communications data */
- struct semaphore irq_urb_sem; /* to protect irq_urb */
struct urb *irq_urb; /* for USB int requests */
unsigned char irqbuf[2]; /* buffer for USB IRQ */
unsigned char irqdata[2]; /* data from USB IRQ */
/* control and bulk communications data */
- struct semaphore current_urb_sem; /* to protect irq_urb */
struct urb *current_urb; /* non-int USB requests */
/* the semaphore for sleeping the control thread */
@@ -221,4 +252,12 @@
#define sg_address(psg) ((psg)->address)
#endif
+#define BUG_NOT_DOWN(sem) \
+ do { \
+ if (!down_trylock(sem)) { \
+ up(sem); \
+ BUG(); \
+ } \
+ } while(0)
+
#endif
