Messages in this thread Patch in this message | | | Date | Sat, 10 Apr 2004 17:09:57 -0700 | From | Matthew Dharm <> | Subject | Re: [linux-usb-devel] Patch for usb-storage in 2.4 |
| |
Off the cuff, this doesn't look bad... tho it does a lot.
The reordering of locking in device_reset and bus reset definately looks good, especially for the corner-cases (yanked device, etc.).
I'm uncertain if your handling of the io_request_lock is right.... but getting information on how to handle that has been like pulling teeth, so I'm inclined to trust your wide-scale testing on this.
Was there a reason to add more do-nothing code to host_reset?
Is it really safe to remove the irq_urb_sem?
Also, please make sure everything is CC'ed to the usb-storage list (on this message).
Matt
On Fri, Apr 09, 2004 at 07:59:43PM -0700, Pete Zaitcev wrote: > Guys, > > this works dandy for us in RHL and RHEL, *especially* on bigger SMP boxes. > It does not fix all the bugs in that code, but I consider it a good start, > and unless someone speaks up, it goes to Marcelo after 2.4.26 is out. > Or maybe before, depending how long it takes him to release 2.4.26. > Please review, test, etc. > > I am working on two new bugs in usb-storage right now, but I'd like > to have things going in gradually, in self-contained chunks, Viro style. > Those two are not fixed yet anyway. > > -- Pete > > diff -urN -X dontdiff linux-2.4.26-rc2/drivers/usb/storage/scsiglue.c linux-2.4.26-rc2-nip/drivers/usb/storage/scsiglue.c > --- linux-2.4.26-rc2/drivers/usb/storage/scsiglue.c 2004-04-09 17:28:30.000000000 -0700 > +++ linux-2.4.26-rc2-nip/drivers/usb/storage/scsiglue.c 2004-04-09 17:31:44.000000000 -0700 > @@ -218,7 +218,14 @@ > US_DEBUGP("device_reset() called\n" ); > > spin_unlock_irq(&io_request_lock); > + down(&(us->dev_semaphore)); > + if (!us->pusb_dev) { > + up(&(us->dev_semaphore)); > + spin_lock_irq(&io_request_lock); > + return SUCCESS; > + } > rc = us->transport_reset(us); > + up(&(us->dev_semaphore)); > spin_lock_irq(&io_request_lock); > return rc; > } > @@ -235,27 +242,32 @@ > /* we use the usb_reset_device() function to handle this for us */ > US_DEBUGP("bus_reset() called\n"); > > + spin_unlock_irq(&io_request_lock); > + > + down(&(us->dev_semaphore)); > + > /* if the device has been removed, this worked */ > if (!us->pusb_dev) { > US_DEBUGP("-- device removed already\n"); > + up(&(us->dev_semaphore)); > + spin_lock_irq(&io_request_lock); > return SUCCESS; > } > > - spin_unlock_irq(&io_request_lock); > - > /* release the IRQ, if we have one */ > - down(&(us->irq_urb_sem)); > if (us->irq_urb) { > US_DEBUGP("-- releasing irq URB\n"); > result = usb_unlink_urb(us->irq_urb); > US_DEBUGP("-- usb_unlink_urb() returned %d\n", result); > } > - up(&(us->irq_urb_sem)); > > /* attempt to reset the port */ > if (usb_reset_device(us->pusb_dev) < 0) { > - spin_lock_irq(&io_request_lock); > - return FAILED; > + /* > + * Do not return errors, or else the error handler might > + * invoke host_reset, which is not implemented. > + */ > + goto bail_out; > } > > /* FIXME: This needs to lock out driver probing while it's working > @@ -286,28 +298,36 @@ > up(&intf->driver->serialize); > } > > +bail_out: > /* re-allocate the IRQ URB and submit it to restore connectivity > * for CBI devices > */ > if (us->protocol == US_PR_CBI) { > - down(&(us->irq_urb_sem)); > us->irq_urb->dev = us->pusb_dev; > result = usb_submit_urb(us->irq_urb); > US_DEBUGP("usb_submit_urb() returns %d\n", result); > - up(&(us->irq_urb_sem)); > } > - > + > + up(&(us->dev_semaphore)); > + > spin_lock_irq(&io_request_lock); > > US_DEBUGP("bus_reset() complete\n"); > return SUCCESS; > } > > -/* FIXME: This doesn't do anything right now */ > static int host_reset( Scsi_Cmnd *srb ) > { > - printk(KERN_CRIT "usb-storage: host_reset() requested but not implemented\n" ); > - return FAILED; > + struct us_data *us = (struct us_data *)srb->host->hostdata[0]; > + > + spin_unlock_irq(&io_request_lock); > + down(&(us->dev_semaphore)); > + printk(KERN_CRIT "usb-storage: host_reset() requested but hardly implemented\n" ); > + up(&(us->dev_semaphore)); > + spin_lock_irq(&io_request_lock); > + US_DEBUGP("host_reset() complete\n"); > + > + return SUCCESS; > } > > /*********************************************************************** > diff -urN -X dontdiff linux-2.4.26-rc2/drivers/usb/storage/usb.c linux-2.4.26-rc2-nip/drivers/usb/storage/usb.c > --- linux-2.4.26-rc2/drivers/usb/storage/usb.c 2004-02-26 14:09:59.000000000 -0800 > +++ linux-2.4.26-rc2-nip/drivers/usb/storage/usb.c 2004-04-09 17:31:45.000000000 -0700 > @@ -501,6 +501,9 @@ > * strucuture is current. This includes the ep_int field, which gives us > * the endpoint for the interrupt. > * Returns non-zero on failure, zero on success > + * > + * ss->dev_semaphore is expected taken, except for a newly minted, > + * unregistered device. > */ > static int usb_stor_allocate_irq(struct us_data *ss) > { > @@ -510,13 +513,9 @@ > > US_DEBUGP("Allocating IRQ for CBI transport\n"); > > - /* lock access to the data structure */ > - down(&(ss->irq_urb_sem)); > - > /* allocate the URB */ > ss->irq_urb = usb_alloc_urb(0); > if (!ss->irq_urb) { > - up(&(ss->irq_urb_sem)); > US_DEBUGP("couldn't allocate interrupt URB"); > return 1; > } > @@ -537,12 +536,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; > } > > @@ -772,7 +768,6 @@ > 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)); > > @@ -1063,7 +1058,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); > @@ -1071,7 +1065,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"); > diff -urN -X dontdiff linux-2.4.26-rc2/drivers/usb/storage/usb.h linux-2.4.26-rc2-nip/drivers/usb/storage/usb.h > --- linux-2.4.26-rc2/drivers/usb/storage/usb.h 2003-11-29 19:23:15.000000000 -0800 > +++ linux-2.4.26-rc2-nip/drivers/usb/storage/usb.h 2004-04-09 17:51:44.000000000 -0700 > @@ -116,7 +116,7 @@ > struct us_data *next; /* next device */ > > /* the device we're working with */ > - struct semaphore dev_semaphore; /* protect pusb_dev */ > + struct semaphore dev_semaphore; /* protect many things */ > struct usb_device *pusb_dev; /* this usb_device */ > > unsigned int flags; /* from filter initially */ > @@ -162,7 +162,6 @@ > atomic_t 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 */ > > > ------------------------------------------------------- > This SF.Net email is sponsored by: IBM Linux Tutorials > Free Linux tutorial presented by Daniel Robbins, President and CEO of > GenToo technologies. Learn everything from fundamentals to system > administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click > _______________________________________________ > linux-usb-devel@lists.sourceforge.net > To unsubscribe, use the last form field at: > https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
-- Matthew Dharm Home: mdharm-usb@one-eyed-alien.net Maintainer, Linux USB Mass Storage Driver
Department of Justice agent. I have come to purify the flock. -- DOJ agent User Friendly, 5/22/1998 [unhandled content-type:application/pgp-signature] | |