lkml.org 
[lkml]   [2004]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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]
    \
     
     \ /
      Last update: 2005-03-22 14:02    [W:0.048 / U:0.844 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site