So, my first BK patch. Once you figure out these tools, it's really not that bad... but the problem is the initial learning curve and the high shoot-yourself-in-the-foot potential.
Greg, this won't compile without the next changeset (which I'm about to
send). I figured that sending this via e-mail was going to be better than
attempting a 'bk push' (will that even work)? This is based on a clone of
your usb-2.5 tree... and I'm not even sure that's the right starting point.
What will happen if I try to 'bk push' this back to linuxusb.bitmover.net?
Will it even let me do that? I don't want to try it without some feedback
telling me it's okay, as I don't want to risk messing up a remote
repository -- I've already had to wipe and start over with my local
repositories a few times....
This material is _not_ in the CVS repository. It would be nice if someone
would update CVS apropriately.
Matt
# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
# ChangeSet 1.561 -> 1.562
# drivers/usb/storage/scsiglue.c 1.11 -> 1.12
# drivers/usb/storage/transport.c 1.17 -> 1.18
# drivers/usb/storage/usb.h 1.5 -> 1.6
# drivers/usb/storage/usb.c 1.16 -> 1.17
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/05/25 [EMAIL PROTECTED] 1.562
# Cleanup of abort mechanism. This version should be much more crash resistant (dare
I say crash-proof?)
# --------------------------------------------
#
diff -Nru a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
--- a/drivers/usb/storage/scsiglue.c Sat May 25 17:30:49 2002
+++ b/drivers/usb/storage/scsiglue.c Sat May 25 17:30:49 2002
@@ -4,7 +4,7 @@
* $Id: scsiglue.c,v 1.26 2002/04/22 03:39:43 mdharm Exp $
*
* Current development and maintenance by:
- * (c) 1999, 2000 Matthew Dharm ([EMAIL PROTECTED])
+ * (c) 1999-2002 Matthew Dharm ([EMAIL PROTECTED])
*
* Developed with the assistance of:
* (c) 2000 David L. Brown, Jr. ([EMAIL PROTECTED])
@@ -56,9 +56,6 @@
*/
#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
/***********************************************************************
diff -Nru a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
--- a/drivers/usb/storage/transport.c Sat May 25 17:30:49 2002
+++ b/drivers/usb/storage/transport.c Sat May 25 17:30:49 2002
@@ -351,6 +351,29 @@
* Data transfer routines
***********************************************************************/
+/*
+ * This is subtle, so pay attention:
+ * ---------------------------------
+ * 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
+ * lock on the current_urb to abort it.
+ *
+ * Once a function grabs the current_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
+ * 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
+ * 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.
+ */
+
/* This is the completion handler which will wake us up when an URB
* completes.
*/
@@ -363,6 +386,9 @@
/* This is the common part of the URB message submission code
* This function expects the current_urb_sem 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.
*/
static int usb_stor_msg_common(struct us_data *us)
{
@@ -385,16 +411,6 @@
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);
@@ -428,6 +444,12 @@
/* lock the URB */
down(&(us->current_urb_sem));
+ /* has the current command been aborted? */
+ if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ up(&(us->current_urb_sem));
+ return 0;
+ }
+
/* fill the URB */
FILL_CONTROL_URB(us->current_urb, us->pusb_dev, pipe,
(unsigned char*) &dr, data, size,
@@ -456,6 +478,12 @@
/* lock the URB */
down(&(us->current_urb_sem));
+ /* has the current command been aborted? */
+ if (atomic_read(&us->sm_state) == US_STATE_ABORTING) {
+ up(&(us->current_urb_sem));
+ return 0;
+ }
+
/* fill the URB */
FILL_BULK_URB(us->current_urb, us->pusb_dev, pipe, data, len,
usb_stor_blocking_completion, NULL);
@@ -823,24 +851,31 @@
/* 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;
- }
+ ASSERT((state == US_STATE_RUNNING || state == US_STATE_RESETTING));
+ ASSERT(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 */
+ * let's wake it up */
- /* if we have an URB pending, cancel it */
+ /* 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.
+ */
+ 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 */
- else if (test_bit(IP_WANTED, &us->bitflags)) {
+ /* 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);
}
diff -Nru a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
--- a/drivers/usb/storage/usb.c Sat May 25 17:30:49 2002
+++ b/drivers/usb/storage/usb.c Sat May 25 17:30:49 2002
@@ -102,9 +102,6 @@
*/
#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 */
@@ -341,10 +338,12 @@
for(;;) {
struct Scsi_Host *host;
US_DEBUGP("*** thread sleeping.\n");
+ atomic_set(&us->sm_state, US_STATE_IDLE);
if(down_interruptible(&us->sema))
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);
@@ -358,145 +357,131 @@
/* release the queue lock as fast as possible */
spin_unlock_irq(&us->queue_exclusion);
- switch (action) {
- case US_ACT_COMMAND:
- /* reject the command if the direction indicator
- * is UNKNOWN
- */
- if (us->srb->sc_data_direction == SCSI_DATA_UNKNOWN) {
- US_DEBUGP("UNKNOWN data direction\n");
- us->srb->result = DID_ERROR << 16;
- scsi_lock(host);
- us->srb->scsi_done(us->srb);
- us->srb = NULL;
- scsi_unlock(host);
- break;
- }
+ /* exit if we get a signal to exit */
+ if (action == US_ACT_EXIT) {
+ US_DEBUGP("-- US_ACT_EXIT command received\n");
+ break;
+ }
- /* reject if target != 0 or if LUN is higher than
- * the maximum known LUN
- */
- if (us->srb->target &&
- !(us->flags & US_FL_SCM_MULT_TARG)) {
- US_DEBUGP("Bad target number (%d/%d)\n",
- us->srb->target, us->srb->lun);
- us->srb->result = DID_BAD_TARGET << 16;
-
- scsi_lock(host);
- us->srb->scsi_done(us->srb);
- us->srb = NULL;
- scsi_unlock(host);
- break;
- }
+ ASSERT (action == US_ACT_COMMAND);
- if (us->srb->lun > us->max_lun) {
- US_DEBUGP("Bad LUN (%d/%d)\n",
- us->srb->target, us->srb->lun);
- us->srb->result = DID_BAD_TARGET << 16;
-
- scsi_lock(host);
- us->srb->scsi_done(us->srb);
- us->srb = NULL;
- scsi_unlock(host);
- break;
- }
-
- /* handle those devices which can't do a START_STOP */
- if ((us->srb->cmnd[0] == START_STOP) &&
- (us->flags & US_FL_START_STOP)) {
- US_DEBUGP("Skipping START_STOP command\n");
- us->srb->result = GOOD << 1;
+ /* reject the command if the direction indicator
+ * is UNKNOWN
+ */
+ if (us->srb->sc_data_direction == SCSI_DATA_UNKNOWN) {
+ US_DEBUGP("UNKNOWN data direction\n");
+ us->srb->result = DID_ERROR << 16;
+ scsi_lock(host);
+ us->srb->scsi_done(us->srb);
+ us->srb = NULL;
+ scsi_unlock(host);
+ continue;
+ }
- scsi_lock(host);
- us->srb->scsi_done(us->srb);
- us->srb = NULL;
- scsi_unlock(host);
- break;
- }
+ /* reject if target != 0 or if LUN is higher than
+ * the maximum known LUN
+ */
+ if (us->srb->target &&
+ !(us->flags & US_FL_SCM_MULT_TARG)) {
+ US_DEBUGP("Bad target number (%d/%d)\n",
+ us->srb->target, us->srb->lun);
+ us->srb->result = DID_BAD_TARGET << 16;
+
+ scsi_lock(host);
+ us->srb->scsi_done(us->srb);
+ us->srb = NULL;
+ scsi_unlock(host);
+ continue;
+ }
+
+ if (us->srb->lun > us->max_lun) {
+ US_DEBUGP("Bad LUN (%d/%d)\n",
+ us->srb->target, us->srb->lun);
+ us->srb->result = DID_BAD_TARGET << 16;
+
+ scsi_lock(host);
+ us->srb->scsi_done(us->srb);
+ us->srb = NULL;
+ scsi_unlock(host);
+ continue;
+ }
+
+ /* handle those devices which can't do a START_STOP */
+ if ((us->srb->cmnd[0] == START_STOP) &&
+ (us->flags & US_FL_START_STOP)) {
+ US_DEBUGP("Skipping START_STOP command\n");
+ us->srb->result = GOOD << 1;
+
+ scsi_lock(host);
+ us->srb->scsi_done(us->srb);
+ us->srb = NULL;
+ scsi_unlock(host);
+ continue;
+ }
- /* lock the device pointers */
- down(&(us->dev_semaphore));
+ /* lock the device pointers */
+ down(&(us->dev_semaphore));
- /* our device has gone - pretend not ready */
- if (atomic_read(&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
- * we auto-sensed for it.
- */
- if (us->srb->cmnd[0] == REQUEST_SENSE) {
- memcpy(us->srb->request_buffer,
- usb_stor_sense_notready,
- sizeof(usb_stor_sense_notready));
- us->srb->result = GOOD << 1;
- } else if(us->srb->cmnd[0] == INQUIRY) {
- unsigned char data_ptr[36] = {
- 0x20, 0x80, 0x02, 0x02,
- 0x1F, 0x00, 0x00, 0x00};
- US_DEBUGP("Faking INQUIRY command for
disconnected device\n");
- fill_inquiry_response(us, data_ptr, 36);
- us->srb->result = GOOD << 1;
- } else {
- memcpy(us->srb->sense_buffer,
- usb_stor_sense_notready,
- sizeof(usb_stor_sense_notready));
- us->srb->result = CHECK_CONDITION << 1;
- }
- } else { /* atomic_read(&us->sm_state) == STATE_DETACHED */
-
- /* 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};
-
- 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));
- atomic_set(&us->sm_state, US_STATE_RUNNING);
- us->proto_handler(us->srb, us);
- atomic_set(&us->sm_state, US_STATE_IDLE);
- }
+ /* our device has gone - pretend not ready */
+ if (atomic_read(&us->device_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
+ * we auto-sensed for it.
+ */
+ if (us->srb->cmnd[0] == REQUEST_SENSE) {
+ memcpy(us->srb->request_buffer,
+ usb_stor_sense_notready,
+ sizeof(usb_stor_sense_notready));
+ us->srb->result = GOOD << 1;
+ } else if(us->srb->cmnd[0] == INQUIRY) {
+ unsigned char data_ptr[36] = {
+ 0x20, 0x80, 0x02, 0x02,
+ 0x1F, 0x00, 0x00, 0x00};
+ US_DEBUGP("Faking INQUIRY command for disconnected
+device\n");
+ fill_inquiry_response(us, data_ptr, 36);
+ us->srb->result = GOOD << 1;
+ } else {
+ memcpy(us->srb->sense_buffer,
+ usb_stor_sense_notready,
+ sizeof(usb_stor_sense_notready));
+ us->srb->result = CHECK_CONDITION << 1;
}
+ } else { /* atomic_read(&us->device_state) == STATE_DETACHED */
- /* unlock the device pointers */
- up(&(us->dev_semaphore));
+ /* 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};
- /* 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);
- us->srb->scsi_done(us->srb);
- us->srb = NULL;
- scsi_unlock(host);
+ US_DEBUGP("Faking INQUIRY command\n");
+ fill_inquiry_response(us, data_ptr, 36);
+ us->srb->result = GOOD << 1;
} else {
- US_DEBUGP("scsi command aborted\n");
- us->srb = NULL;
- complete(&(us->notify));
+ /* we've got a command, let's do it! */
+ US_DEBUG(usb_stor_show_command(us->srb));
+ us->proto_handler(us->srb, us);
}
- break;
-
- case US_ACT_DEVICE_RESET:
- break;
-
- case US_ACT_BUS_RESET:
- break;
-
- case US_ACT_HOST_RESET:
- break;
+ }
- } /* end switch on action */
+ /* unlock the device pointers */
+ up(&(us->dev_semaphore));
- /* exit if we get a signal to exit */
- if (action == US_ACT_EXIT) {
- US_DEBUGP("-- US_ACT_EXIT command received\n");
- break;
+ /* 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);
+ 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));
}
} /* for (;;) */
@@ -722,7 +707,7 @@
/* establish the connection to the new device upon reconnect */
ss->ifnum = ifnum;
ss->pusb_dev = dev;
- atomic_set(&ss->sm_state, US_STATE_IDLE);
+ atomic_set(&ss->device_state, US_STATE_ATTACHED);
/* copy over the endpoint data */
if (ep_in)
@@ -1013,6 +998,7 @@
/* start up our control thread */
atomic_set(&ss->sm_state, US_STATE_IDLE);
+ atomic_set(&ss->device_state, US_STATE_ATTACHED);
ss->pid = kernel_thread(usb_stor_control_thread, ss,
CLONE_VM);
if (ss->pid < 0) {
diff -Nru a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h
--- a/drivers/usb/storage/usb.h Sat May 25 17:30:49 2002
+++ b/drivers/usb/storage/usb.h Sat May 25 17:30:49 2002
@@ -4,7 +4,7 @@
* $Id: usb.h,v 1.21 2002/04/21 02:57:59 mdharm Exp $
*
* Current development and maintenance by:
- * (c) 1999, 2000 Matthew Dharm ([EMAIL PROTECTED])
+ * (c) 1999-2002 Matthew Dharm ([EMAIL PROTECTED])
*
* Initial work by:
* (c) 1999 Michael Gee ([EMAIL PROTECTED])
@@ -102,11 +102,15 @@
#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_IDLE 2
-#define US_STATE_RUNNING 3
-#define US_STATE_RESETTING 4
-#define US_STATE_ABORTING 5
+/* device attached/detached states */
+#define US_STATE_DETACHED 1
+#define US_STATE_ATTACHED 2
+
+/* processing state machine states */
+#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
@@ -119,8 +123,13 @@
struct us_data {
struct us_data *next; /* next device */
- /* the device we're working with */
+ /* The device we're working with
+ * It's important to note:
+ * (o) you must hold dev_semaphore to change pusb_dev
+ * (o) device_state should change whenever pusb_dev does
+ */
struct semaphore dev_semaphore; /* protect pusb_dev */
+ atomic_t device_state; /* attached or detached */
struct usb_device *pusb_dev; /* this usb_device */
unsigned int flags; /* from filter initially */
--
Matthew Dharm Home: [EMAIL PROTECTED]
Senior Software Designer, Momentum Computer
C: They kicked your ass, didn't they?
S: They were cheating!
-- The Chief and Stef
User Friendly, 11/19/1997
msg06814/pgp00000.pgp
Description: PGP signature
