From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750743AbWHCCmU (ORCPT ); Wed, 2 Aug 2006 22:42:20 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750858AbWHCCmU (ORCPT ); Wed, 2 Aug 2006 22:42:20 -0400 Received: from 1wt.eu ([62.212.114.60]:14093 "EHLO 1wt.eu") by vger.kernel.org with ESMTP id S1750743AbWHCCmT (ORCPT ); Wed, 2 Aug 2006 22:42:19 -0400 Date: Thu, 3 Aug 2006 04:33:53 +0200 From: Willy Tarreau To: Pete Zaitcev Cc: benjamin.cherian.kernel@gmail.com, linux-kernel@vger.kernel.org, linux-usb-devel@lists.sourceforge.net Subject: Re: Bug with USB proc_bulk in 2.4 kernel Message-ID: <20060803023352.GA18264@1wt.eu> References: <200607271521.38217.benjamin.cherian.kernel@gmail.com> <20060730003527.19bec8ce.zaitcev@redhat.com> <200607311141.29600.benjamin.cherian.kernel@gmail.com> <20060802195100.GA16290@1wt.eu> <20060802180216.da6c0d1e.zaitcev@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20060802180216.da6c0d1e.zaitcev@redhat.com> User-Agent: Mutt/1.5.11 Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 02, 2006 at 06:02:16PM -0700, Pete Zaitcev wrote: > On Wed, 2 Aug 2006 21:51:00 +0200, Willy Tarreau wrote: > > > Pete, do you consider your work as appropriate for mainline ? In this > > case, I can queue it for 2.4.34. > > Yes, please use the attached patch. The one I sent for testing > missed necessary exports. This is also tested with TEAC CD-210PU. > I suppose the worst that can happen is that someone will complain > about a regression in 2008 :-) OK, fine :-) Would you be so kind as to give me a short description about what it really does (2-3 lines) and a signed-off line so that I can feed the changelog appropriately please ? Otherwise, my explanations from your previous mails might not completely match reality. Thanks, Willy > -- Pete > > diff -urp -X dontdiff linux-2.4.32/drivers/usb/devices.c linux-2.4.32-wk/drivers/usb/devices.c > --- linux-2.4.32/drivers/usb/devices.c 2004-11-17 03:54:21.000000000 -0800 > +++ linux-2.4.32-wk/drivers/usb/devices.c 2006-07-24 22:30:54.000000000 -0700 > @@ -392,7 +392,7 @@ static char *usb_dump_desc(char *start, > * Grab device's exclusive_access mutex to prevent its driver or > * devio from using this device while we are accessing it. > */ > - down (&dev->exclusive_access); > + usb_excl_lock(dev, 3, 0); > > start = usb_dump_device_descriptor(start, end, &dev->descriptor); > > @@ -411,7 +411,7 @@ static char *usb_dump_desc(char *start, > } > > out: > - up (&dev->exclusive_access); > + usb_excl_unlock(dev, 3); > return start; > } > > diff -urp -X dontdiff linux-2.4.32/drivers/usb/devio.c linux-2.4.32-wk/drivers/usb/devio.c > --- linux-2.4.32/drivers/usb/devio.c 2006-04-13 19:02:30.000000000 -0700 > +++ linux-2.4.32-wk/drivers/usb/devio.c 2006-07-30 00:27:13.000000000 -0700 > @@ -623,7 +623,12 @@ static int proc_bulk(struct dev_state *p > free_page((unsigned long)tbuf); > return -EINVAL; > } > + if (usb_excl_lock(dev, 1, 1) != 0) { > + free_page((unsigned long)tbuf); > + return -ERESTARTSYS; > + } > i = usb_bulk_msg(dev, pipe, tbuf, len1, &len2, tmo); > + usb_excl_unlock(dev, 1); > if (!i && len2) { > if (copy_to_user(bulk.data, tbuf, len2)) { > free_page((unsigned long)tbuf); > @@ -637,7 +642,12 @@ static int proc_bulk(struct dev_state *p > return -EFAULT; > } > } > + if (usb_excl_lock(dev, 2, 1) != 0) { > + free_page((unsigned long)tbuf); > + return -ERESTARTSYS; > + } > i = usb_bulk_msg(dev, pipe, tbuf, len1, &len2, tmo); > + usb_excl_unlock(dev, 2); > } > free_page((unsigned long)tbuf); > if (i < 0) { > @@ -1160,12 +1170,6 @@ static int usbdev_ioctl_exclusive(struct > inode->i_mtime = CURRENT_TIME; > break; > > - case USBDEVFS_BULK: > - ret = proc_bulk(ps, (void *)arg); > - if (ret >= 0) > - inode->i_mtime = CURRENT_TIME; > - break; > - > case USBDEVFS_RESETEP: > ret = proc_resetep(ps, (void *)arg); > if (ret >= 0) > @@ -1259,8 +1263,13 @@ static int usbdev_ioctl(struct inode *in > ret = proc_disconnectsignal(ps, (void *)arg); > break; > > - case USBDEVFS_CONTROL: > case USBDEVFS_BULK: > + ret = proc_bulk(ps, (void *)arg); > + if (ret >= 0) > + inode->i_mtime = CURRENT_TIME; > + break; > + > + case USBDEVFS_CONTROL: > case USBDEVFS_RESETEP: > case USBDEVFS_RESET: > case USBDEVFS_CLEAR_HALT: > @@ -1272,9 +1281,9 @@ static int usbdev_ioctl(struct inode *in > case USBDEVFS_RELEASEINTERFACE: > case USBDEVFS_IOCTL: > ret = -ERESTARTSYS; > - if (down_interruptible(&ps->dev->exclusive_access) == 0) { > + if (usb_excl_lock(ps->dev, 3, 1) == 0) { > ret = usbdev_ioctl_exclusive(ps, inode, cmd, arg); > - up(&ps->dev->exclusive_access); > + usb_excl_unlock(ps->dev, 3); > } > break; > > diff -urp -X dontdiff linux-2.4.32/drivers/usb/storage/transport.c linux-2.4.32-wk/drivers/usb/storage/transport.c > --- linux-2.4.32/drivers/usb/storage/transport.c 2005-04-03 18:42:19.000000000 -0700 > +++ linux-2.4.32-wk/drivers/usb/storage/transport.c 2006-07-24 22:58:58.000000000 -0700 > @@ -628,16 +628,16 @@ void usb_stor_invoke_transport(Scsi_Cmnd > int result; > > /* > - * Grab device's exclusive_access mutex to prevent libusb/usbfs from > + * Grab device's exclusive access lock to prevent libusb/usbfs from > * sending out a command in the middle of ours (if libusb sends a > * get_descriptor or something on pipe 0 after our CBW and before > * our CSW, and then we get a stall, we have trouble). > */ > - down(&(us->pusb_dev->exclusive_access)); > + usb_excl_lock(us->pusb_dev, 3, 0); > > /* send the command to the transport layer */ > result = us->transport(srb, us); > - up(&(us->pusb_dev->exclusive_access)); > + usb_excl_unlock(us->pusb_dev, 3); > > /* if the command gets aborted by the higher layers, we need to > * short-circuit all other processing > @@ -757,9 +757,9 @@ void usb_stor_invoke_transport(Scsi_Cmnd > srb->use_sg = 0; > > /* issue the auto-sense command */ > - down(&(us->pusb_dev->exclusive_access)); > + usb_excl_lock(us->pusb_dev, 3, 0); > temp_result = us->transport(us->srb, us); > - up(&(us->pusb_dev->exclusive_access)); > + usb_excl_unlock(us->pusb_dev, 3); > > /* let's clean up right away */ > srb->request_buffer = old_request_buffer; > diff -urp -X dontdiff linux-2.4.32/drivers/usb/usb.c linux-2.4.32-wk/drivers/usb/usb.c > --- linux-2.4.32/drivers/usb/usb.c 2004-11-17 03:54:21.000000000 -0800 > +++ linux-2.4.32-wk/drivers/usb/usb.c 2006-08-02 17:44:50.000000000 -0700 > @@ -989,7 +989,8 @@ struct usb_device *usb_alloc_dev(struct > INIT_LIST_HEAD(&dev->filelist); > > init_MUTEX(&dev->serialize); > - init_MUTEX(&dev->exclusive_access); > + spin_lock_init(&dev->excl_lock); > + init_waitqueue_head(&dev->excl_wait); > > dev->bus->op->allocate(dev); > > @@ -2380,6 +2381,59 @@ struct list_head *usb_bus_get_list(void) > } > #endif > > +int usb_excl_lock(struct usb_device *dev, unsigned int type, int interruptible) > +{ > + DECLARE_WAITQUEUE(waita, current); > + > + add_wait_queue(&dev->excl_wait, &waita); > + if (interruptible) > + set_current_state(TASK_INTERRUPTIBLE); > + else > + set_current_state(TASK_UNINTERRUPTIBLE); > + > + for (;;) { > + spin_lock_irq(&dev->excl_lock); > + switch (type) { > + case 1: /* 1 - read */ > + case 2: /* 2 - write */ > + case 3: /* 3 - control: excludes both read and write */ > + if ((dev->excl_type & type) == 0) { > + dev->excl_type |= type; > + spin_unlock_irq(&dev->excl_lock); > + set_current_state(TASK_RUNNING); > + remove_wait_queue(&dev->excl_wait, &waita); > + return 0; > + } > + break; > + default: > + spin_unlock_irq(&dev->excl_lock); > + return -EINVAL; > + } > + spin_unlock_irq(&dev->excl_lock); > + > + if (interruptible) { > + schedule(); > + if (signal_pending(current)) { > + remove_wait_queue(&dev->excl_wait, &waita); > + return 1; > + } > + set_current_state(TASK_INTERRUPTIBLE); > + } else { > + schedule(); > + set_current_state(TASK_UNINTERRUPTIBLE); > + } > + } > +} > + > +void usb_excl_unlock(struct usb_device *dev, unsigned int type) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&dev->excl_lock, flags); > + dev->excl_type &= ~type; > + wake_up(&dev->excl_wait); > + spin_unlock_irqrestore(&dev->excl_lock, flags); > +} > > /* > * Init > @@ -2473,5 +2527,8 @@ EXPORT_SYMBOL(usb_unlink_urb); > EXPORT_SYMBOL(usb_control_msg); > EXPORT_SYMBOL(usb_bulk_msg); > > +EXPORT_SYMBOL(usb_excl_lock); > +EXPORT_SYMBOL(usb_excl_unlock); > + > EXPORT_SYMBOL(usb_devfs_handle); > MODULE_LICENSE("GPL"); > diff -urp -X dontdiff linux-2.4.32/include/linux/usb.h linux-2.4.32-wk/include/linux/usb.h > --- linux-2.4.32/include/linux/usb.h 2005-12-22 17:08:01.000000000 -0800 > +++ linux-2.4.32-wk/include/linux/usb.h 2006-07-24 22:30:02.000000000 -0700 > @@ -828,8 +828,19 @@ struct usb_device { > > atomic_t refcnt; /* Reference count */ > struct semaphore serialize; > - struct semaphore exclusive_access; /* prevent driver & proc accesses */ > - /* from overlapping cmds at device */ > + > + /* > + * This is our custom open-coded lock, similar to r/w locks in concept. > + * It prevents drivers and /proc access from simultaneous access. > + * Type: > + * 0 - unlocked > + * 1 - locked for reads > + * 2 - locked for writes > + * 3 - locked for everything > + */ > + wait_queue_head_t excl_wait; > + spinlock_t excl_lock; > + unsigned excl_type; > > unsigned int toggle[2]; /* one bit for each endpoint ([0] = IN, [1] = OUT) */ > unsigned int halted[2]; /* endpoint halts; one bit per endpoint # & direction; */ > @@ -904,6 +915,8 @@ extern void usb_destroy_configuration(st > > int usb_get_current_frame_number (struct usb_device *usb_dev); > > +int usb_excl_lock(struct usb_device *dev, unsigned int type, int interruptible); > +void usb_excl_unlock(struct usb_device *dev, unsigned int type); > > /** > * usb_make_path - returns stable device path in the usb tree