Hi Matt,
I've just added both sets of patches to CVS.
regards,
Stephen.
[antonia] ~/unix/linux-usb/storage > patch -p4 <~/temp
patching file scsiglue.c
Hunk #1 succeeded at 4 with fuzz 1.
patching file transport.c
patching file usb.c
patching file usb.h
Hunk #1 succeeded at 4 with fuzz 1.
[antonia] ~/unix/linux-usb/storage > patch -p4 < ~/temp
patching file debug.h
On Sat, 25 May 2002, Matthew Dharm wrote:
> 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 */
>
>
--
/------------------------------------+-------------------------\
|Stephen J. Gowdy | SLAC, MailStop 34, |
|http://www.slac.stanford.edu/~gowdy/ | 2575 Sand Hill Road, |
|http://calendar.yahoo.com/gowdy | Menlo Park CA 94025, USA |
|EMail: [EMAIL PROTECTED] | Tel: +1 650 926 3144 |
\------------------------------------+-------------------------/
_______________________________________________________________
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
