Re: Bug with USB proc_bulk in 2.4 kernel and possibly bug in proc_ioctl in 2.6

Re: Bug with USB proc_bulk in 2.4 kernel and possibly bug in proc_ioctl in 2.6

[Pete Zaitcev]

On Fri, 7 Jul 2006 17:24:56 -0700, Benjamin Cherian <benjamin.cherian.kernel@gmail.com> wrote:

> I believe that there is a bug in the proc_bulk function in drivers/usb/devio.c 
> in the current 2.4 kernel. In the 2.4.28 proc_ioctl was changed so that the 
> device would be locked before proc_bulk was called (See line 1275 of 
> devio.c). In 2.4.27, no locking occurred. However, this leads to problems if 
> one thread is continuously attempting to read and another one needs to write 
> because the write thread can never access the device. []

The decision to cripple the devio in this way was conscious. The problem
scenario with 2.4.27 and earlier was how many devices would fail if
a control and other transfer were submitted simultaneously. In other
words, when desktop components (e.g. HAL) rescanned /proc/bus/usb/devices,
some other devices would throw errors. The most notorious example
would be TEAC CD-210PU, because of how widespread and popular it is.

In other words, I crippled your application for the sake of a bunch
of users of other devices. You have to realize though, that your
concerns weren't voiced in the time and it was widely believed that
user-mode drivers did not submit several URBs at once (primarily
because the queueing in HCDs of 2.4 was a suspect).

> I would have submitted a patch, but it seems like the locking/unlocking 
> mechanism is different in 2.4 and 2.6 and I wasn't sure if I would break 
> other stuff.

If you do send a patch, you need to account for the serialization
somehow. The linux-usb-devel people considered a few ideas, such
as per-device blacklists and locking flags, backport of string caching
from 2.6 (to alleviate the most common case of /proc/usb/usb/devices).

But it seems to me that the best approach would be if you made
a special case for bulk+bulk and locked out control transfers somehow.

Another option is to look at an in-kernel driver architecture for
your device. What is it, anyway?

-- Pete

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

Hi,

On Saturday 08 July 2006 13:28, you wrote:
> The decision to cripple the devio in this way was conscious. The problem
> scenario with 2.4.27 and earlier was how many devices would fail if
> a control and other transfer were submitted simultaneously. In other
> words, when desktop components (e.g. HAL) rescanned /proc/bus/usb/devices,
> some other devices would throw errors. The most notorious example
> would be TEAC CD-210PU, because of how widespread and popular it is.
>
> In other words, I crippled your application for the sake of a bunch
> of users of other devices. You have to realize though, that your
> concerns weren't voiced in the time and it was widely believed that
> user-mode drivers did not submit several URBs at once (primarily
> because the queueing in HCDs of 2.4 was a suspect).

I understand your decision but this does not follow the USB spec. According 
to the spec, two threads should be able to perform operations on the same 
device at the same time.

> But it seems to me that the best approach would be if you made
> a special case for bulk+bulk and locked out control transfers somehow.

This is exactly what I suggested in my first email. It just involves adding a 
couple of lines of code, but I'm not sure how the unlock function works in 
2.4. In 2.6, the device is unlocked WITHIN proc_bulk before usb_bulk_mesg is 
called and is locked after usb_bulk_mesg returns. Therefore, the device is 
still locked during control transfers.

Thanks,

Ben

Re: Bug with USB proc_bulk in 2.4 kernel

[Pete Zaitcev]

On Mon, 10 Jul 2006 12:58:32 -0700, Benjamin Cherian <benjamin.cherian.kernel@gmail.com> wrote:

> I understand your decision but this does not follow the USB spec. According 
> to the spec, two threads should be able to perform operations on the same 
> device at the same time.

This consideration is wholly irrelevant to the pertinent topic,
because the very issue arises from devices being non-compliant.
If we want them to operate, we have to apply workarounds, which
may but us outside of the spec envelope. So I am not even going
to argue if the spec in fact mandates this behaviour.

> > But it seems to me that the best approach would be if you made
> > a special case for bulk+bulk and locked out control transfers somehow.
> 
> This is exactly what I suggested in my first email. It just involves adding a 
> couple of lines of code, but I'm not sure how the unlock function works in 
> 2.4. In 2.6, the device is unlocked WITHIN proc_bulk before usb_bulk_mesg is 
> called and is locked after usb_bulk_mesg returns. Therefore, the device is 
> still locked during control transfers.

I explained why this technique is not applicable to 2.4 in the part
of the message you failed to quote. To recap, if bulk methods were
simply to drop locks with a "couple of lines of code", they would allow
control transfers to happen simultaneously with bulk transfers.
It happens to work in 2.6 only because of its string caching feature.
Indeed, Stuart Hayes of Dell reported that he can cause TEAC CD-210PU
to break under 2.6.16 in the same way it did in 2.4.27, simply by doing
"lsusb -v" instead of "cat /proc/bus/usb/devices". The issue is still
with us. Greg chose to bypass it in 2.6, instead of fixing it, due
to the amount of work involved.

This is a fixable issue, but it's not two lines of code. And its
impact is very limited. If you, the person who actually suffers, are
not willing to do the necessary work, then why would anyone else be?
Still, I would be happy to consider your patch, if it were to emerge.

-- Pete

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

Hi Pete,

> This consideration is wholly irrelevant to the pertinent topic,
> because the very issue arises from devices being non-compliant.
> If we want them to operate, we have to apply workarounds, which
> may but us outside of the spec envelope. So I am not even going
> to argue if the spec in fact mandates this behaviour.

I understand the need to apply workarounds.  But here is why the spec is 
relevant.  By applying workarounds you have *broken* the functionality of 
Linux with respect to the USB specification.  In my opinion, workarounds 
should be constrained to fixing the problem rather than creating new ones.


> > This is exactly what I suggested in my first email. It just involves
> > adding a couple of lines of code, but I'm not sure how the unlock
> > function works in 2.4. In 2.6, the device is unlocked WITHIN proc_bulk
> > before usb_bulk_mesg is called and is locked after usb_bulk_mesg returns.
> > Therefore, the device is still locked during control transfers.
>
> I explained why this technique is not applicable to 2.4 in the part
> of the message you failed to quote. To recap, if bulk methods were
> simply to drop locks with a "couple of lines of code", they would allow
> control transfers to happen simultaneously with bulk transfers.
> It happens to work in 2.6 only because of its string caching feature.
> Indeed, Stuart Hayes of Dell reported that he can cause TEAC CD-210PU
> to break under 2.6.16 in the same way it did in 2.4.27, simply by doing
> "lsusb -v" instead of "cat /proc/bus/usb/devices". The issue is still
> with us. Greg chose to bypass it in 2.6, instead of fixing it, due
> to the amount of work involved.

Honestly, you provided very little information in your initial email.  I had 
to dig through the kernel source to make sense of what was going on.  Even 
for the above, I'm not understanding why "lsusb -v" causes problems under 
2.6.16.  Does the "-v" option cause the kernel to send control packets to the 
device?  You have to understand that the USB driver stack under Linux is not 
my domain of expertise and I'm coming to you with a problem that is not of my 
doing.  It would help a great deal to receive more details from you.


> This is a fixable issue, but it's not two lines of code. And its
> impact is very limited. If you, the person who actually suffers, are
> not willing to do the necessary work, then why would anyone else be?
> Still, I would be happy to consider your patch, if it were to emerge.

It is really looking like you are backing me into a corner to make the change 
myself.  However, before doing so I'd like to say that I am disappointed that 
the kernel developer list has not been more accommodating to this issue.  I 
understand that this problem only affects very few people.  However, please 
consider the spirit of what has happened here.  My device adheres to the 
specification.  In an effort to fix other devices that do not adhere to the 
specification you have broken my device.  Then I am being asked to make 
modifications to the OS, when clearly this is not my domain.  We all want 
Linux to be a primetime operating system and this sort of interaction with 
the developers list is *not* representative of that desire.

Besides, the device that is being broken is a 10x CD-ROM drive!  Who even uses 
these anymore? :-)

Regards,

Benjamin Cherian

Re: Bug with USB proc_bulk in 2.4 kernel

[Pete Zaitcev]

On Mon, 17 Jul 2006 14:35:21 -0700, Benjamin Cherian <benjamin.cherian.kernel@gmail.com> wrote:

I'm skipping the discussion of the spec, but going further, here's
what we have:

> It is really looking like you are backing me into a corner to make the change 
> myself.  However, before doing so I'd like to say that I am disappointed that 
> the kernel developer list has not been more accommodating to this issue.

I understand, this is not a good situation. The problem is, it's 2.4.
It is upgraded very slowly, if at all. You came around about a year after
the fact. At the time, my initial approaches threw similar regressions
(with ADSL modems). Of course it's very tempting for me to off-load both
the work and the responsibility on you.

> Besides, the device that is being broken is a 10x CD-ROM drive!
> Who even uses these anymore? :-)

This was my reaction too, when Dell people came knocking. But apparently,
that thing is very popular. Also, they backed up their request with
a bag of money. In the end, I was glad I did "fix" that thing. Later,
it turned out to be OEM-ed by NEC, Fujitsu, and others; our own QA
uses it a lot as well. This happens because the 210PU offers significant
savings in power and space in OEM applications, and for casual users,
it's a perfect jump drive.

It's the same kind of question as, "who even uses 2.4 anymore".

By the way, did you consider an in-kernel driver? For me, it seems much
safer to reimplement the whole thing that way than to monkey with devio
again and risk more regressions.

Another option would be to change USBDEVFS_BULK to USBDEVFS_SUBMITURB.
Did you look at doing that?

Yours,
-- Pete

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

Pete,

> It's the same kind of question as, "who even uses 2.4 anymore".
We asked to same question to the user who told us about this bug :-). It 
happened after the user built his own kernel (2.4.32) for Fedora Core 1, 
which uses a much older version.
> By the way, did you consider an in-kernel driver? For me, it seems much
> safer to reimplement the whole thing that way than to monkey with devio
> again and risk more regressions.
We're currently using libusb. We don't have to time to patch and maintain a 
driver that's actually in the tree. And our customers are definitely not 
going to patch and build their own kernel either.

> Another option would be to change USBDEVFS_BULK to USBDEVFS_SUBMITURB.
> Did you look at doing that?
We did that as well. But when you try to reap an URB there is no timeout. So 
if something goes wrong you're stuck waiting for the operation to finish or 
for the user to physically unplug the device.

>Of course it's very tempting for me to off-load both
>the work and the responsibility on you.

All right then. I'll send you a patch that backports the string caching 
mechanism from 2.6 in a few days. Would you be able to test it with the 
210PU?

Thanks,

Ben

Re: Bug with USB proc_bulk in 2.4 kernel

[Pete Zaitcev]

On Tue, 18 Jul 2006 10:04:54 -0700, Benjamin Cherian <benjamin.cherian.kernel@gmail.com> wrote:

> > Another option would be to change USBDEVFS_BULK to USBDEVFS_SUBMITURB.
> > Did you look at doing that?

> We did that as well. But when you try to reap an URB there is no timeout. So 
> if something goes wrong you're stuck waiting for the operation to finish or 
> for the user to physically unplug the device.

This can't be right. Surely alarm(2) should work?

> >Of course it's very tempting for me to off-load both
> >the work and the responsibility on you.
> 
> All right then. I'll send you a patch that backports the string caching 
> mechanism from 2.6 in a few days. Would you be able to test it with the 
> 210PU?

Yes, that would be fine.

Although I am starting to think about creating a custom locking
scheme in devio.c after all. It seems like less work.

-- Pete

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

Pete,

Thanks for the reply.

> > All right then. I'll send you a patch that backports the string caching
> > mechanism from 2.6 in a few days. Would you be able to test it with the
> > 210PU?
>
> Yes, that would be fine.
We should get back to you in a couple weeks with this.

> Although I am starting to think about creating a custom locking
> scheme in devio.c after all. It seems like less work.
What's your timeframe for this? Good luck with it.

Thanks,

Ben

Re: Bug with USB proc_bulk in 2.4 kernel

[Pete Zaitcev]

On Thu, 20 Jul 2006 10:43:59 -0700, Benjamin Cherian <benjamin.cherian.kernel@gmail.com> wrote:

> > Although I am starting to think about creating a custom locking
> > scheme in devio.c after all. It seems like less work.

> What's your timeframe for this? Good luck with it.

OK, now I hate my life, I hate you, I hate Stuart, but most of all
I hate the anonymous Japanese who wrote the microcode for TEAC CD-210PU.
Anyway, please test the attached patch. Does it do what you want?

-- 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-24 22:39:32.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-07-24 22:41:19.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
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

Re: Bug with USB proc_bulk in 2.4 kernel

[Willy Tarreau]

Hi Pete,

On Mon, Jul 24, 2006 at 11:07:32PM -0700, Pete Zaitcev wrote:
> On Thu, 20 Jul 2006 10:43:59 -0700, Benjamin Cherian <benjamin.cherian.kernel@gmail.com> wrote:
> 
> > > Although I am starting to think about creating a custom locking
> > > scheme in devio.c after all. It seems like less work.
> 
> > What's your timeframe for this? Good luck with it.
> 
> OK, now I hate my life, I hate you, I hate Stuart, but most of all
> I hate the anonymous Japanese who wrote the microcode for TEAC CD-210PU.
> Anyway, please test the attached patch. Does it do what you want?

I'm very glad that you're still maintaining 2.4 code so much actively. Do
you think of any possible side effects that non-TEAC users might encounter ?
If you feel more comfortable after some particular corner-case tests on other
hardware, please ask.

> -- Pete

Cheers,
Willy

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

On Monday 24 July 2006 23:07, Pete Zaitcev wrote:
> Anyway, please test the attached patch. Does it do what you want?

Sorry to say that it doesnt. When calling usb_get_string_simple in libusb, the 
program segfaults with the patched kernel. I believe that 
usb_get_string_simple eventually calls usbdev_ioctl.

Thanks,

Ben

Re: Bug with USB proc_bulk in 2.4 kernel

[Pete Zaitcev]

On Thu, 27 Jul 2006 15:21:37 -0700, Benjamin Cherian <benjamin.cherian.kernel@gmail.com> wrote:

> On Monday 24 July 2006 23:07, Pete Zaitcev wrote:
> > Anyway, please test the attached patch. Does it do what you want?
> 
> Sorry to say that it doesnt. When calling usb_get_string_simple in libusb,
> the program segfaults with the patched kernel. I believe that 
> usb_get_string_simple eventually calls usbdev_ioctl.

Thanks for testing, I'll look into it.

By the way, you didn't tell me if you tried to use alarm(2) across
submitted URBs. This is the technique ADSL modem drivers use. They
also have to have input and output streams running simultaneously.

-- Pete

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

Hi Pete,
Thanks for the reply.
> By the way, you didn't tell me if you tried to use alarm(2) across
> submitted URBs. This is the technique ADSL modem drivers use. They
> also have to have input and output streams running simultaneously.

I'm making a library to operate my device, and I don't want to use alarm, 
because the user might need to use alarm in his own applications.

Thanks again,
Ben

Re: Bug with USB proc_bulk in 2.4 kernel

[Pete Zaitcev]

On Thu, 27 Jul 2006 15:21:37 -0700, Benjamin Cherian <benjamin.cherian.kernel@gmail.com> wrote:
> On Monday 24 July 2006 23:07, Pete Zaitcev wrote:

> > Anyway, please test the attached patch. Does it do what you want?
> 
> Sorry to say that it doesnt. When calling usb_get_string_simple in libusb,
> the program segfaults with the patched kernel. I believe that 
> usb_get_string_simple eventually calls usbdev_ioctl.

Indeed, I should've tested better, sorry. How about now?

-- 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-07-24 22:41:19.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
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

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

Hi Pete,

>How about now?

Much better! For me, everything seems to be working. No segfaults, and 
everything seems to work.

Thanks,

Ben

Re: Bug with USB proc_bulk in 2.4 kernel

[Willy Tarreau]

Hi Benjamin, Hi Pete,

On Mon, Jul 31, 2006 at 11:41:28AM -0700, Benjamin Cherian wrote:
> Hi Pete,
> 
> >How about now?
> 
> Much better! For me, everything seems to be working. No segfaults, and 
> everything seems to work.

That's very good news.

Pete, do you consider your work as appropriate for mainline ? In this
case, I can queue it for 2.4.34.

> Thanks,
> 
> Ben

Thanks guys,
Willy

Re: Bug with USB proc_bulk in 2.4 kernel

[Pete Zaitcev]

On Wed, 2 Aug 2006 21:51:00 +0200, Willy Tarreau <w@1wt.eu> 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 :-)

-- 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

Re: Bug with USB proc_bulk in 2.4 kernel

[Willy Tarreau]

On Wed, Aug 02, 2006 at 06:02:16PM -0700, Pete Zaitcev wrote:
> On Wed, 2 Aug 2006 21:51:00 +0200, Willy Tarreau <w@1wt.eu> 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

Re: Bug with USB proc_bulk in 2.4 kernel

[Pete Zaitcev]

Replace the semaphore exclusive_access with an open-coded lock for
the special use. The lock can be taken for: read, write, and both.
This way, two bulk URBs can be submitted simultaneously with ioctl
USBDEVFS_BULK: one read and one write. Such access was possible
before 2.4.28.

Signed-off-By: Pete Zaitcev <zaitcev@redhat.com>

---

The semaphore was introduced in 2.4.28 for the purpose of exclusion
between access from device.c (cat /proc/bus/usb/devices), devio.c
through libusb, and in-kernel driver. Currently, only usb-storage
observes the locking protocol. The most popular device which locks
in case of simultaneous access is TEAC CD-210PU.

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 22:56:53.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,61 @@ 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);
+			set_current_state(TASK_RUNNING);
+			remove_wait_queue(&dev->excl_wait, &waita);
+			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 +2529,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

Re: Bug with USB proc_bulk in 2.4 kernel

[Willy Tarreau]

On Wed, Aug 02, 2006 at 11:00:58PM -0700, Pete Zaitcev wrote:
> Replace the semaphore exclusive_access with an open-coded lock for
> the special use. The lock can be taken for: read, write, and both.
> This way, two bulk URBs can be submitted simultaneously with ioctl
> USBDEVFS_BULK: one read and one write. Such access was possible
> before 2.4.28.
> 
> Signed-off-By: Pete Zaitcev <zaitcev@redhat.com>
> 
> ---
> 
> The semaphore was introduced in 2.4.28 for the purpose of exclusion
> between access from device.c (cat /proc/bus/usb/devices), devio.c
> through libusb, and in-kernel driver. Currently, only usb-storage
> observes the locking protocol. The most popular device which locks
> in case of simultaneous access is TEAC CD-210PU.

Perfect. Patch queued, thanks Pete.

Willy

Re: Bug with USB proc_bulk in 2.4 kernel

[Willy Tarreau]

On Fri, Aug 04, 2006 at 09:57:04AM -0700, Benjamin Cherian wrote:
> Willy,
> 
> On Wednesday 02 August 2006 23:29, Willy Tarreau wrote:
> > Perfect. Patch queued, thanks Pete.
> 
> Do you know if it would be possible for this to get into 2.4.33? I know its 
> already at rc3, but it would be nice if this patch could be pushed out sooner 
> since 2.4.34 will probably not be out for a while. I would really appreciate 
> it. Thanks to both you and Pete for your help.

The problem is that Marcelo is very very busy those days (as you might have
noticed from the delay between each release), and there are a good bunch of
security fixes in -rc3 which should wait too much in -rc. Maybe an -rc4
would be OK, but I don't know if Marcelo has enough time to spend on yet
another RC. Otherwise, if I produce a 2.4.34-pre1 about one week after 2.4.33,
does that fit your needs ? I already have a few fixes waiting which might be
worth a first pre-release.

> Ben

Regards,
Willy

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

Willy,

On Wednesday 02 August 2006 23:29, Willy Tarreau wrote:
> Perfect. Patch queued, thanks Pete.

Do you know if it would be possible for this to get into 2.4.33? I know its 
already at rc3, but it would be nice if this patch could be pushed out sooner 
since 2.4.34 will probably not be out for a while. I would really appreciate 
it. Thanks to both you and Pete for your help.

Ben

Re: Bug with USB proc_bulk in 2.4 kernel

[Benjamin Cherian]

Willy,
Sorry I didn't notice your email till now.

On Friday 04 August 2006 09:55, Willy Tarreau wrote:
> The problem is that Marcelo is very very busy those days (as you might have
> noticed from the delay between each release), and there are a good bunch of
> security fixes in -rc3 which should wait too much in -rc. Maybe an -rc4
> would be OK, but I don't know if Marcelo has enough time to spend on yet
> another RC. Otherwise, if I produce a 2.4.34-pre1 about one week after
> 2.4.33, does that fit your needs ? I already have a few fixes waiting which
> might be worth a first pre-release.

It would be nice if you could do another rc, because otherwise it seems the 
patch wouldn't get into a stable kernel for a long time. But if it's really 
too much work for Marcelo, I would understand you putting it in th 2.4.34 
pre-release.

Thanks again,
Ben

Re: Bug with USB proc_bulk in 2.4 kernel

[Willy Tarreau]

Hi Benjamin,

On Wed, Aug 09, 2006 at 10:01:41AM -0700, Benjamin Cherian wrote:
> Willy,
> Sorry I didn't notice your email till now.

no problem.

> On Friday 04 August 2006 09:55, Willy Tarreau wrote:
> > The problem is that Marcelo is very very busy those days (as you might have
> > noticed from the delay between each release), and there are a good bunch of
> > security fixes in -rc3 which should wait too much in -rc. Maybe an -rc4
> > would be OK, but I don't know if Marcelo has enough time to spend on yet
> > another RC. Otherwise, if I produce a 2.4.34-pre1 about one week after
> > 2.4.33, does that fit your needs ? I already have a few fixes waiting which
> > might be worth a first pre-release.
> 
> It would be nice if you could do another rc, because otherwise it seems the 
> patch wouldn't get into a stable kernel for a long time. But if it's really 
> too much work for Marcelo, I would understand you putting it in th 2.4.34 
> pre-release.

The real problem is that Marcelo seems to be *very* busy. I failed to get
any contact from him from one week now. It's even becoming worrying, I hope
he's alright. I don't know how much time it would take him to produce another
release, but it will for sure delay 2.4.33 for a few weeks during which Marcelo
will still be bound to this time-consuming task. It's left to him to decide
anyway. In normal ciscumstances, I would for sure add another -rc, but delays
get stretched too far given the number of vulnerabilities fixed in 2.4.33.

> Thanks again,
> Ben

Regards,
Willy

Re: Bug with USB proc_bulk in 2.4 kernel

[Marcelo Tosatti]

On Wed, Aug 09, 2006 at 07:02:01PM +0200, Willy Tarreau wrote:
> Hi Benjamin,
> 
> On Wed, Aug 09, 2006 at 10:01:41AM -0700, Benjamin Cherian wrote:
> > Willy,
> > Sorry I didn't notice your email till now.
> 
> no problem.
> 
> > On Friday 04 August 2006 09:55, Willy Tarreau wrote:
> > > The problem is that Marcelo is very very busy those days (as you might have
> > > noticed from the delay between each release), and there are a good bunch of
> > > security fixes in -rc3 which should wait too much in -rc. Maybe an -rc4
> > > would be OK, but I don't know if Marcelo has enough time to spend on yet
> > > another RC. Otherwise, if I produce a 2.4.34-pre1 about one week after
> > > 2.4.33, does that fit your needs ? I already have a few fixes waiting which
> > > might be worth a first pre-release.
> > 
> > It would be nice if you could do another rc, because otherwise it seems the 
> > patch wouldn't get into a stable kernel for a long time. But if it's really 
> > too much work for Marcelo, I would understand you putting it in th 2.4.34 
> > pre-release.
> 
> The real problem is that Marcelo seems to be *very* busy. I failed to get
> any contact from him from one week now. It's even becoming worrying, I hope
> he's alright. I don't know how much time it would take him to produce another
> release, but it will for sure delay 2.4.33 for a few weeks during which Marcelo
> will still be bound to this time-consuming task. It's left to him to decide
> anyway. In normal ciscumstances, I would for sure add another -rc, but delays
> get stretched too far given the number of vulnerabilities fixed in 2.4.33.

The reason for so much delay was a serious accident which resulted in 2
weeks of hospitalization with no Internet connnectivity (and no mental
state for work).

Sorry about that.

Re: Bug with USB proc_bulk in 2.4 kernel

[Willy Tarreau]

On Fri, Aug 11, 2006 at 09:14:57PM -0300, Marcelo Tosatti wrote:
> 
> The reason for so much delay was a serious accident which resulted in 2
> weeks of hospitalization with no Internet connnectivity (and no mental
> state for work).
> 
> Sorry about that.

Hey Marcelo,

you don't have to apologize for this !!! I think that everybody's already
very happy that you released 2.4.33 as soon as you left the hospital !

I wish you a good and fast recovery.

Cheers,
Willy