[PATCH] Regression: Revert "Bluetooth: use buffer priority to mark URB_ISO_ASAP flag"

[PATCH] Regression: Revert "Bluetooth: use buffer priority to mark URB_ISO_ASAP flag"

[Keith Packard]

This reverts commit b8aabfc92249b239c425da7e4ca85b7e4855e984.

URB_ISO_ASAP may only be used with iso end points, but this patch was
attempting to use it with a bulk endpoint. When DEBUG is defined in
the USB stack, it checks for invalid flags and returns -EINVAL instead
of actually transmitting the data.

Signed-off-by: Keith Packard <keithp@keithp.com>
---

Ok, so this is a comedy of errors. The Bluetooth code is trying to use
an iso-specific flag (URB_ISO_ASAP) on a bulk endpoint, which is
invalid, but normally harmless. Unless the USB code is compiled for
debugging, in which case it checks a bunch for incorrect usage and
discards URBs which are wrong.

As far as I'm concerned, #ifdef DEBUG code should never ever change
the behaviour of the code, and so I'd say the USB code is even more
broken than the bluetooth code...

In any case, to reproduce this issue, enable CONFIG_USB_DEBUG and then
try to use bluetooth (sdptool browse). You'll get error messages from
the USB stack:

	usb 1-1.1.3: BOGUS urb flags, 2 --> 0

Apply this patch and bluetooth starts working again.

 drivers/bluetooth/btusb.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index f00f596..b7c4f4e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -726,9 +726,6 @@ static int btusb_send_frame(struct sk_buff *skb)
 		usb_fill_bulk_urb(urb, data->udev, pipe,
 				skb->data, skb->len, btusb_tx_complete, skb);
 
-		if (skb->priority >= HCI_PRIO_MAX - 1)
-			urb->transfer_flags  = URB_ISO_ASAP;
-
 		hdev->stat.acl_tx++;
 		break;
 
-- 
1.7.8.3

Re: [PATCH] Regression: Revert "Bluetooth: use buffer priority to mark URB_ISO_ASAP flag"

[Marcel Holtmann]

Hi Keith,

> URB_ISO_ASAP may only be used with iso end points, but this patch was
> attempting to use it with a bulk endpoint. When DEBUG is defined in
> the USB stack, it checks for invalid flags and returns -EINVAL instead
> of actually transmitting the data.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---

a version of this patch should be already upstream or in stable trees.
If you are sending this again, then it might slipped through. Johan?

> Ok, so this is a comedy of errors. The Bluetooth code is trying to use
> an iso-specific flag (URB_ISO_ASAP) on a bulk endpoint, which is
> invalid, but normally harmless. Unless the USB code is compiled for
> debugging, in which case it checks a bunch for incorrect usage and
> discards URBs which are wrong.
> 
> As far as I'm concerned, #ifdef DEBUG code should never ever change
> the behaviour of the code, and so I'd say the USB code is even more
> broken than the bluetooth code...

And yes, this one is hilarious.

Regards

Marcel

Re: [PATCH] Regression: Revert "Bluetooth: use buffer priority to mark URB_ISO_ASAP flag"

[Johan Hedberg]

Hi Marcel,

On Mon, Jan 23, 2012, Marcel Holtmann wrote:
> > URB_ISO_ASAP may only be used with iso end points, but this patch was
> > attempting to use it with a bulk endpoint. When DEBUG is defined in
> > the USB stack, it checks for invalid flags and returns -EINVAL instead
> > of actually transmitting the data.
> > 
> > Signed-off-by: Keith Packard <keithp@keithp.com>
> > ---
> 
> a version of this patch should be already upstream or in stable trees.
> If you are sending this again, then it might slipped through. Johan?

It's here:

http://git.kernel.org/?p=linux/kernel/git/jh/bluetooth-next.git;a=commitdiff;h=e2bcdc65928af5f2b509a8c4d8b4469901f6522f

Johan

Re: [PATCH] Regression: Revert "Bluetooth: use buffer priority to mark URB_ISO_ASAP flag"

[Alan Stern]

On Sun, 22 Jan 2012, Keith Packard wrote:

> Ok, so this is a comedy of errors. The Bluetooth code is trying to use
> an iso-specific flag (URB_ISO_ASAP) on a bulk endpoint, which is
> invalid, but normally harmless. Unless the USB code is compiled for
> debugging, in which case it checks a bunch for incorrect usage and
> discards URBs which are wrong.
> 
> As far as I'm concerned, #ifdef DEBUG code should never ever change
> the behaviour of the code, and so I'd say the USB code is even more
> broken than the bluetooth code...

Well, if the DEBUG code never changed any behaviors at all, it would be 
pretty useless, wouldn't it?

In this case, the change in behavior involves printing out a kernel 
message announcing a bug.  Causing the submission to fail also helps 
draw people's attention to the bug.  Under the circumstances, I think 
this is understandable and not truly broken.

Alan Stern

Re: [PATCH] Regression: Revert "Bluetooth: use buffer priority to mark URB_ISO_ASAP flag"

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

On Mon, 23 Jan 2012 10:23:14 -0500 (EST), Alan Stern <stern@rowland.harvard.edu> wrote:

> Well, if the DEBUG code never changed any behaviors at all, it would be 
> pretty useless, wouldn't it?

A WARN_ON would have been sufficient to catch attention without breaking
an otherwise working system.

This is certainly the first time I've ever encountered DEBUG code that
did anything more than print out information to help developers debug
stuff...

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH] Regression: Revert "Bluetooth: use buffer priority to mark URB_ISO_ASAP flag"

[Pavel Machek]

Hi!

> This reverts commit b8aabfc92249b239c425da7e4ca85b7e4855e984.
> 
> URB_ISO_ASAP may only be used with iso end points, but this patch was
> attempting to use it with a bulk endpoint. When DEBUG is defined in
> the USB stack, it checks for invalid flags and returns -EINVAL instead
> of actually transmitting the data.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

This patch fixes a regression, and it is not in 3.3.0-rc2.

(And for the record I also believe that #ifdef debug should not
introduce extra error returns. WARN_ON() should be enough. Don't break
systems for people who try to be nice and enable debugging.)
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] USB: debugging code shouldn't alter control flow

[Alan Stern]

People have complained that debugging code shouldn't alter the flow of
control; it should restrict itself to printing out warnings and error
messages.  Bowing to popular opinion, this patch (as1518) changes the
debugging checks in usb_submit_urb() to follow this guideline.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-by: Keith Packard <keithp@keithp.com>
CC: Pavel Machek <pavel@ucw.cz>

---

 drivers/usb/core/urb.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

Index: usb-3.3/drivers/usb/core/urb.c
===================================================================
--- usb-3.3.orig/drivers/usb/core/urb.c
+++ usb-3.3/drivers/usb/core/urb.c
@@ -403,20 +403,17 @@ int usb_submit_urb(struct urb *urb, gfp_
 	 * cause problems in HCDs if they get it wrong.
 	 */
 	{
-	unsigned int	orig_flags = urb->transfer_flags;
 	unsigned int	allowed;
 	static int pipetypes[4] = {
 		PIPE_CONTROL, PIPE_ISOCHRONOUS, PIPE_BULK, PIPE_INTERRUPT
 	};
 
 	/* Check that the pipe's type matches the endpoint's type */
-	if (usb_pipetype(urb->pipe) != pipetypes[xfertype]) {
-		dev_err(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
+	if (usb_pipetype(urb->pipe) != pipetypes[xfertype])
+		dev_WARN(&dev->dev, "BOGUS urb xfer, pipe %x != type %x\n",
 			usb_pipetype(urb->pipe), pipetypes[xfertype]);
-		return -EPIPE;		/* The most suitable error code :-) */
-	}
 
-	/* enforce simple/standard policy */
+	/* Check against a simple/standard policy */
 	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
 			URB_FREE_BUFFER);
 	switch (xfertype) {
@@ -435,14 +432,12 @@ int usb_submit_urb(struct urb *urb, gfp_
 		allowed |= URB_ISO_ASAP;
 		break;
 	}
-	urb->transfer_flags &= allowed;
+	allowed &= urb->transfer_flags;
 
-	/* fail if submitter gave bogus flags */
-	if (urb->transfer_flags != orig_flags) {
-		dev_err(&dev->dev, "BOGUS urb flags, %x --> %x\n",
-			orig_flags, urb->transfer_flags);
-		return -EINVAL;
-	}
+	/* warn if submitter gave bogus flags */
+	if (allowed != urb->transfer_flags)
+		dev_WARN(&dev->dev, "BOGUS urb flags, %x --> %x\n",
+			urb->transfer_flags, allowed);
 	}
 #endif
 	/*

Re: [PATCH] USB: debugging code shouldn't alter control flow

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

On Thu, 2 Feb 2012 15:38:14 -0500 (EST), Alan Stern <stern@rowland.harvard.edu> wrote:

> -	urb->transfer_flags &= allowed;
> +	allowed &= urb->transfer_flags;

Minor bikeshed here, but 'allowed' is not the right name for the value
at this point; perhaps just
        
	/* warn if submitter gave bogus flags */
	if ((urb->transfer_flags & allowed) != urb->transfer_flags)

might be better?

Otherwise, this looks nice to me.

Acked-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]