Hi Matthew,
I ran into a hang during stress testing of usb-storage from 2.5.18,
and in the following review I found a few bugs. You've fixed some
of them in 2.5.19, here are the remaining points from my TODO list:
- First, please fix the bug I've introduced into
usb_stor_control_thread():
The assignment
host = us->srb->host;
happens too early. us->srb is NULL for US_ACT_EXIT actions.
The result is an oops during module unload. That assignment
should only happen for US_ACT_COMMAND actions.
- the definition of US_ACT_{COMMAND,EXIT} is duplicated in
scsiglue.c and usb.c. IMHO it belongs into a header file.
- merge us->queue_srb and us->srb. If scsi_driver.command_abort is
called before the usb worker thread has a chance to move the
command from queue_srb to srb, then
usb_stor_abort_transport() will BUG_ON(!(us->srb));
- sm_state access is not locked correctly:
* check the definitions of atomic_read() and atomic_set(): they are
just assignments. I'd replace them with a normal integer
assignment, using atomic_{set,read} obfuscates the code.
* usb_stor_abort_transport() does
atomic_set(&us->sm_state, US_STATE_ABORTING);
and usb_stor_control_thread() does
atomic_set(us->sm_state, US_STATE_RUNNING);
The assignments can overwrite each other.
I've moved the abort state into a seperate variable, and I perform
all changes to sm_state under the dev_semaphore spinlock.
- usb_stor_abort_transport can sleep, and is called by the
usb_stor_timeout_handler. The timeout handler runs at bottom
half context --> panic.
The timeout handler must schedule an event, and the event handler
then calls usb_stor_timeout_handler().
- the timer_lock is broken, a completion is needed instead of a
spinlock.
- AFAICS you haven't fixed the error I found during stress testing:
What if command_abort() is called, but the command finishes
with DID_ERROR?
Then complete(&(us->notify)) is never called, and the error
handler hangs.
I've added a wait queue, and use us->notify only for thread
creation and destruction.
- If the device initialization fails, then urb structures are
freed with kfree instead of usb_free_urb()
And one unreleated issue: the driver ate my harddisk, I must figure
out how to low-level format it. dd if=/dev/zero of=/dev/sda fails :-(
It seems that the ScanLogic firmware doesn't handle large write
requests, it fails & destroys the disk with requests > 128 kB.
Is that yet another odd bug of the ScanLogic firmware? Could you try
it with other devices?
I've worked around the issue by adding "max_sectors=128" in the scsi
host template. max_sectors=256 should work, too, but I don't want to
figure out the exact limit until I've found a way to low-level format.
As a reference, I've attached my patch against 2.5.18. It's only
partially tested, and I don't like the locking/memory barrier around
the simulated interrupt in usb_stor_abort_transport().
--
Manfred
<<<<<<<<<<<
diff -u 2.5/drivers/usb/storage/scsiglue.c build-2.5/drivers/usb/storage/scsiglue.c
--- 2.5/drivers/usb/storage/scsiglue.c Sun May 26 11:05:08 2002
+++ build-2.5/drivers/usb/storage/scsiglue.c Sat Jun 1 02:11:21 2002
@@ -51,16 +51,6 @@
#include <linux/slab.h>
-/*
- * kernel thread actions
- */
-
-#define US_ACT_COMMAND 1
-#define US_ACT_DEVICE_RESET 2
-#define US_ACT_BUS_RESET 3
-#define US_ACT_HOST_RESET 4
-#define US_ACT_EXIT 5
-
/***********************************************************************
* Host functions
***********************************************************************/
@@ -113,6 +103,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)
{
@@ -126,7 +117,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));
@@ -159,7 +154,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;
@@ -183,12 +179,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));
scsi_lock(srb->host);
return SUCCESS;
}
@@ -205,46 +198,75 @@
int result;
US_DEBUGP("device_reset() called\n" );
-
- /* if the device was removed, then we're already reset */
- if (atomic_read(&us->sm_state) == US_STATE_DETACHED)
- return SUCCESS;
-
scsi_unlock(srb->host);
+
/* lock the device pointers */
- down(&(us->dev_semaphore));
+ down(&us->dev_semaphore);
+
+ /* if the device was removed, then we're already reset */
+ if (us->sm_state == US_STATE_DETACHED) {
+ US_DEBUGP("-- device removed already\n");
+ result = SUCCESS;
+ goto out;
+ }
+ /* scsi_lock not needed, midlayer guarantees that no
+ * new commands will be forwarded until reset returns.
+ */
+ BUG_ON(us->srb != NULL);
us->srb = srb;
- atomic_set(&us->sm_state, US_STATE_RESETTING);
+
+ us->sm_state = US_STATE_RESETTING;
result = us->transport_reset(us);
- atomic_set(&us->sm_state, US_STATE_IDLE);
+ us->sm_state = US_STATE_IDLE;
+ US_DEBUGP("-- device reset done\n");
+ BUG_ON(us->srb != srb);
+ us->srb = NULL;
+ wake_up(&us->wq_cmd);
+
+out:
/* unlock the device pointers */
up(&(us->dev_semaphore));
scsi_lock(srb->host);
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 )
{
+ printk(KERN_INFO "usb-storage: bus_reset() requested but not implemented\n" );
+ return device_reset(srb);
+#if 0
+/* This resets the device port, and simulates the device
+ * disconnect/reconnect for all drivers which have claimed
+ * interfaces, including ourself.
+ * Right now broken due to deadlocks:
+ * - we must own dev_semaphore, otherwise the pusb_dev can disappear
+ * - we must own driver->serialize to avoid concurrent probes/disconnects
+ * - the locking order is first driver->serialize, then us->semaphore.
+ * - we can't honor the locking order, since we don't know the driver object
+ * pointer(s) upfront.
+ * */
struct us_data *us = (struct us_data *)srb->host->hostdata[0];
int i;
int result;
- struct usb_device *pusb_dev_save = us->pusb_dev;
+ struct usb_device *pusb_dev_save;
/* we use the usb_reset_device() function to handle this for us */
US_DEBUGP("bus_reset() called\n");
+ scsi_unlock(srb->host);
+ down(&usb_storage_driver.serialize);
+ down(&us->dev_semaphore);
/* if the device has been removed, this worked */
- if (atomic_read(&us->sm_state) == US_STATE_DETACHED) {
+ if (us->sm_state == US_STATE_DETACHED) {
US_DEBUGP("-- device removed already\n");
- return SUCCESS;
+ result = SUCCESS;
+ goto out;
}
/* attempt to reset the port */
- scsi_unlock(srb->host);
+ pusb_dev_save = us->pusb_dev;
result = usb_reset_device(pusb_dev_save);
US_DEBUGP("usb_reset_device returns %d\n", result);
if (result < 0) {
@@ -268,24 +290,26 @@
US_DEBUGP("Examining driver %s...", intf->driver->name);
/* simulate a disconnect and reconnect for all interfaces */
+ up(&intf->driver->serialize);
+ BUG(&inf->driver != usb_storage_driver);
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");
+out:
+ up(&us->dev_semaphore);
scsi_lock(srb->host);
return SUCCESS;
+#endif
}
-/* 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);
}
/***********************************************************************
@@ -318,7 +342,12 @@
us = us->next;
}
- /* release our lock on the data structures */
+ /* release our lock on the data structures
+ * 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 */
@@ -340,8 +369,8 @@
/* show the GUID of the device */
SPRINTF(" GUID: " GUID_FORMAT "\n", GUID_ARGS(us->guid));
- SPRINTF(" Attached: %s\n", (atomic_read(&us->sm_state) ==
- US_STATE_DETACHED) ? "Yes" : "No");
+ SPRINTF(" Attached: %s\n", (us->sm_state != US_STATE_DETACHED) ? "Yes" :
+"No");
+
/*
* Calculate start of next buffer, and return value.
@@ -379,6 +408,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 Sun May 26 11:04:45 2002
+++ build-2.5/drivers/usb/storage/transport.c Sat Jun 1 02:32:17 2002
@@ -362,13 +362,15 @@
}
/* 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.
*/
static int usb_stor_msg_common(struct us_data *us)
{
struct completion urb_done;
int status;
+ BUG_NOT_DOWN(&us->dev_semaphore);
+
/* set up data structures for the wakeup system */
init_completion(&urb_done);
@@ -379,26 +381,20 @@
us->current_urb->transfer_flags = USB_ASYNC_UNLINK;
/* submit the URB */
- status = usb_submit_urb(us->current_urb, GFP_NOIO);
+ 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) {
/* something went wrong */
return status;
}
- /* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
-
- /* avoid a race with usb_stor_abort_transport():
- * if the abort took place before we submitted
- * the URB, we must cancel it ourselves */
- if (us->current_urb->status == -EINPROGRESS)
- usb_unlink_urb(us->current_urb);
- }
-
- /* 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;
@@ -425,8 +421,8 @@
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);
/* fill the URB */
FILL_CONTROL_URB(us->current_urb, us->pusb_dev, pipe,
@@ -440,8 +436,6 @@
if (status >= 0)
status = us->current_urb->actual_length;
- /* release the lock and return status */
- up(&(us->current_urb_sem));
return status;
}
@@ -453,8 +447,8 @@
{
int status;
- /* lock the URB */
- down(&(us->current_urb_sem));
+ /* is the device access locked? */
+ BUG_NOT_DOWN(&us->dev_semaphore);
/* fill the URB */
FILL_BULK_URB(us->current_urb, us->pusb_dev, pipe, data, len,
@@ -466,8 +460,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;
}
@@ -535,7 +527,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;
}
@@ -582,6 +574,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)
@@ -813,37 +808,58 @@
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)
{
- 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 */
- if ( !(state == US_STATE_RUNNING || state == US_STATE_RESETTING)
- || !us->srb) {
- US_DEBUGP("-- invalid current state\n");
- return;
- }
- 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 */
+ /* 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().
+ */
+ down(&us->urb_sem);
+ spin_lock_irq(&us->queue_exclusion);
+ us->abort_cmd = 1;
+ mb();
+ if(us->sm_state == US_STATE_RUNNING || us->sm_state == US_STATE_RESETTING) {
+ /* the worker thread is processing the
+ * command right now, abort it.
+ */
+ spin_unlock_irq(&us->queue_exclusion);
- /* if we have an URB pending, cancel it */
- if (us->current_urb->status == -EINPROGRESS) {
- US_DEBUGP("-- cancelling URB\n");
- usb_unlink_urb(us->current_urb);
- }
+ /* if we have an URB pending, cancel it */
+ if (us->current_urb->status == -EINPROGRESS) {
+ US_DEBUGP("-- cancelling URB\n");
+ usb_unlink_urb(us->current_urb);
+ }
- /* if we are waiting for an IRQ, simulate it */
- else if (test_bit(IP_WANTED, &us->bitflags)) {
- US_DEBUGP("-- simulating missing IRQ\n");
- usb_stor_CBI_irq(us->irq_urb);
+ /* if we are waiting for an IRQ, simulate it */
+ if (test_bit(IP_WANTED, &us->bitflags)) {
+ US_DEBUGP("-- simulating missing IRQ\n");
+ usb_stor_CBI_irq(us->irq_urb);
+ }
+ spin_lock_irq(&us->queue_exclusion);
}
+ up(&us->urb_sem);
+ 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);
+ }
+ us->abort_cmd = 0;
+ mb();
+ spin_unlock_irq(&us->queue_exclusion);
}
/*
@@ -860,9 +876,9 @@
US_DEBUGP("-- IRQ state is %d\n", urb->status);
US_DEBUGP("-- Interrupt Status (0x%x, 0x%x)\n",
us->irqbuf[0], us->irqbuf[1]);
-
+ mb();
/* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ if (us->abort_cmd) {
/* was this a wanted interrupt? */
if (!test_and_clear_bit(IP_WANTED, &us->bitflags)) {
@@ -988,7 +1004,8 @@
down(&(us->ip_waitq));
/* has the current command been aborted? */
- if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ mb();
+ if (us->abort_cmd) {
US_DEBUGP("CBI interrupt aborted\n");
return USB_STOR_TRANSPORT_ABORTED;
}
@@ -1294,23 +1311,33 @@
struct us_timeout {
struct us_data *us;
- spinlock_t timer_lock;
+ struct completion done;
+ struct tq_struct event;
};
-/* The timeout event handler
- */
-static void usb_stor_timeout_handler(unsigned long to__)
+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 occurred\n");
+ US_DEBUGP("timeout event handler called\n");
/* abort the current request */
usb_stor_abort_transport(us);
- /* let the reset routine know we have finished */
- spin_unlock(&to->timer_lock);
+ complete(&to->done);
+}
+
+/* The timeout event handler
+ */
+static void usb_stor_timeout_handler(unsigned long to__)
+{
+ struct us_timeout *to = (struct us_timeout *) to__;
+
+ US_DEBUGP("Timeout occurred\n");
+
+ INIT_TQUEUE(&to->event, usb_stor_event_handler, to);
+ schedule_task(&to->event);
}
/* This is the common part of the device reset code.
@@ -1325,11 +1352,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
@@ -1365,8 +1393,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/usb.c build-2.5/drivers/usb/storage/usb.c
--- 2.5/drivers/usb/storage/usb.c Fri May 31 22:08:29 2002
+++ build-2.5/drivers/usb/storage/usb.c Sat Jun 1 00:36:54 2002
@@ -99,19 +99,9 @@
static int my_host_number;
-/*
- * kernel thread actions
- */
-
-#define US_ACT_COMMAND 1
-#define US_ACT_DEVICE_RESET 2
-#define US_ACT_BUS_RESET 3
-#define US_ACT_HOST_RESET 4
-#define US_ACT_EXIT 5
-
/* 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);
@@ -337,6 +327,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));
@@ -344,8 +337,11 @@
for(;;) {
struct Scsi_Host *host;
US_DEBUGP("*** thread sleeping.\n");
- 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");
@@ -355,17 +351,17 @@
/* 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);
+ US_DEBUGP("Got command %d.\n", action);
switch (action) {
case US_ACT_COMMAND:
/* reject the command if the direction indicator
* is UNKNOWN
*/
+ host = us->srb->host;
if (us->srb->sc_data_direction == SCSI_DATA_UNKNOWN) {
US_DEBUGP("UNKNOWN data direction\n");
us->srb->result = DID_ERROR << 16;
@@ -421,7 +417,7 @@
down(&(us->dev_semaphore));
/* our device has gone - pretend not ready */
- if (atomic_read(&us->sm_state) == US_STATE_DETACHED) {
+ if (us->sm_state == US_STATE_DETACHED) {
US_DEBUGP("Request is for removed device\n");
/* For REQUEST_SENSE, it's the data. But
* for anything else, it should look like
@@ -445,27 +441,29 @@
sizeof(usb_stor_sense_notready));
us->srb->result = CHECK_CONDITION << 1;
}
- } else { /* atomic_read(&us->sm_state) == STATE_DETACHED */
-
+ } else if ((us->srb->cmnd[0] == INQUIRY) &&
+ (us->flags & US_FL_FIX_INQUIRY)) {
/* Handle those devices which need us to fake
* their inquiry data */
- if ((us->srb->cmnd[0] == INQUIRY) &&
- (us->flags & US_FL_FIX_INQUIRY)) {
- unsigned char data_ptr[36] = {
- 0x00, 0x80, 0x02, 0x02,
- 0x1F, 0x00, 0x00, 0x00};
+ unsigned char data_ptr[36] = {
+ 0x00, 0x80, 0x02, 0x02,
+ 0x1F, 0x00, 0x00, 0x00};
- US_DEBUGP("Faking INQUIRY command\n");
- fill_inquiry_response(us, data_ptr, 36);
- us->srb->result = GOOD << 1;
+ US_DEBUGP("Faking INQUIRY command\n");
+ fill_inquiry_response(us, data_ptr, 36);
+ us->srb->result = GOOD << 1;
+ } else {
+ /* we've got a command, let's do it! */
+ US_DEBUG(usb_stor_show_command(us->srb));
+ us->sm_state = US_STATE_RUNNING;
+ mb();
+ if (us->abort_cmd) {
+ us->srb->result = DID_ABORT << 16;
} else {
- /* we've got a command, let's do it! */
- US_DEBUG(usb_stor_show_command(us->srb));
- atomic_set(&us->sm_state, US_STATE_RUNNING);
us->proto_handler(us->srb, us);
- atomic_set(&us->sm_state, US_STATE_IDLE);
}
- }
+ us->sm_state = US_STATE_IDLE;
+ }
/* unlock the device pointers */
up(&(us->dev_semaphore));
@@ -480,28 +478,18 @@
scsi_unlock(host);
} else {
US_DEBUGP("scsi command aborted\n");
+ scsi_lock(host);
us->srb = NULL;
- complete(&(us->notify));
+ scsi_unlock(host);
}
break;
-
- case US_ACT_DEVICE_RESET:
- break;
-
- case US_ACT_BUS_RESET:
- break;
-
- case US_ACT_HOST_RESET:
- break;
-
- } /* end switch on action */
-
- /* exit if we get a signal to exit */
- if (action == US_ACT_EXIT) {
+ case US_ACT_EXIT:
+ /* exit if we get a signal to exit */
US_DEBUGP("-- US_ACT_EXIT command received\n");
- break;
- }
+ goto out;
+ } /* end switch on action */
} /* for (;;) */
+out:
/* notify the exit routine that we're actually exiting now */
complete(&(us->notify));
@@ -523,13 +511,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;
}
@@ -550,12 +537,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;
}
@@ -593,6 +577,8 @@
intf[ifnum].altsetting + intf[ifnum].act_altsetting;
US_DEBUGP("act_altsettting 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));
@@ -725,7 +711,6 @@
/* establish the connection to the new device upon reconnect */
ss->ifnum = ifnum;
ss->pusb_dev = dev;
- atomic_set(&ss->sm_state, US_STATE_IDLE);
/* copy over the endpoint data */
if (ep_in)
@@ -752,6 +737,8 @@
/* Re-Initialize the device if it needs it */
if (unusual_dev && unusual_dev->initFunction)
(unusual_dev->initFunction)(ss);
+ BUG_ON(ss->sm_state != US_STATE_DETACHED);
+ ss->sm_state = US_STATE_IDLE;
/* unlock the device pointers */
up(&(ss->dev_semaphore));
@@ -780,9 +767,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;
@@ -920,7 +907,7 @@
default:
ss->transport_name = "Unknown";
- kfree(ss->current_urb);
+ usb_free_urb(ss->current_urb);
kfree(ss);
usb_put_dev(dev);
return NULL;
@@ -975,7 +962,7 @@
default:
ss->protocol_name = "Unknown";
- kfree(ss->current_urb);
+ usb_free_urb(ss->current_urb);
kfree(ss);
usb_put_dev(dev);
return NULL;
@@ -985,7 +972,7 @@
/* allocate an IRQ callback if one is needed */
if ((ss->protocol == US_PR_CBI) && usb_stor_allocate_irq(ss)) {
- kfree(ss->current_urb);
+ usb_free_urb(ss->current_urb);
kfree(ss);
usb_put_dev(dev);
return NULL;
@@ -1014,14 +1001,16 @@
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);
ss->pid = kernel_thread(usb_stor_control_thread, ss,
CLONE_VM);
if (ss->pid < 0) {
printk(KERN_WARNING USB_STORAGE
"Unable to start control thread\n");
- kfree(ss->current_urb);
+ usb_free_urb(ss->current_urb);
kfree(ss);
usb_put_dev(dev);
return NULL;
@@ -1062,6 +1051,8 @@
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");
@@ -1072,7 +1063,6 @@
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);
@@ -1080,7 +1070,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");
@@ -1092,7 +1081,7 @@
/* mark the device as gone */
usb_put_dev(ss->pusb_dev);
ss->pusb_dev = NULL;
- atomic_set(&ss->sm_state, US_STATE_DETACHED);
+ ss->sm_state = US_STATE_DETACHED;
/* unlock access to the device data structure */
up(&(ss->dev_semaphore));
@@ -1108,7 +1097,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 Sun May 26 11:05:08 2002
+++ build-2.5/drivers/usb/storage/usb.h Sat Jun 1 00:36:54 2002
@@ -103,11 +103,18 @@
#define US_FL_SCM_MULT_TARG 0x00000020 /* supports multiple targets */
#define US_FL_FIX_INQUIRY 0x00000040 /* INQUIRY response needs fixing */
-#define US_STATE_DETACHED 1 /* State machine states */
+#define US_STATE_BAD 0 /* State machine states */
+#define US_STATE_DETACHED 1
#define US_STATE_IDLE 2
#define US_STATE_RUNNING 3
#define US_STATE_RESETTING 4
-#define US_STATE_ABORTING 5
+
+/*
+ * kernel thread actions
+ */
+
+#define US_ACT_COMMAND 1
+#define US_ACT_EXIT 2
#define USB_STOR_STRING_LEN 32
@@ -156,10 +163,13 @@
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 int abort_cmd;
/* interrupt info for CBI devices -- only good if attached */
struct semaphore ip_waitq; /* for CBI interrupts */
@@ -167,13 +177,11 @@
#define IP_WANTED 1 /* is an IRQ expected? */
/* 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 */
@@ -210,4 +218,12 @@
#define sg_address(psg) ((psg)->address)
#endif
+#define BUG_NOT_DOWN(sem) \
+ do { \
+ if (!down_trylock(sem)) { \
+ up(sem); \
+ BUG(); \
+ } \
+ } while(0)
+
#endif
<<<<<<<<<<<
_______________________________________________________________
Don't miss the 2002 Sprint PCS Application Developer's Conference
August 25-28 in Las Vegas -- http://devcon.sprintpcs.com/adp/index.cfm
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
