linux-kernel - [PATCH] [RFC] BTUSB: be quiet on device disconnect

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 09 Aug 2011 17:16:28 +0200
From:	Paul Bolle <pebolle@...cali.nl>
To:	Marcel Holtmann <marcel@...tmann.org>,
	"Gustavo F. Padovan" <padovan@...fusion.mobi>,
	Greg Kroah-Hartman <gregkh@...e.de>
Cc:	linux-bluetooth@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: [PATCH] [RFC] BTUSB: be quiet on device disconnect

Disabling the bluetooth usb device embedded in (some) ThinkPads tends to
lead to errors like these:
    btusb_bulk_complete: hci0 urb ffff88011b9bfd68 failed to resubmit (19)
    btusb_intr_complete: hci0 urb ffff88011b46a318 failed to resubmit (19)
    btusb_bulk_complete: hci0 urb ffff88011b46a000 failed to resubmit (19)

That is because usb_disconnect() doesn't "quiesces" pending urbs.

Disconnecting a device is a normal thing to happen so it's no big deal
that usb_submit_urb() returns -ENODEV. The simplest way to get rid of
these errors is to stop treating that return as an error. Trivial,
actually.

While we're at it, add comments to be explicit about the reasons we're
not complaining about -EPERM and -ENODEV.

Signed-off-by: Paul Bolle <pebolle@...cali.nl>
---
0) This patch seems to put an end to a pet peeve of mine: the errors in
the logs of a ThinkPad when I disable its embedded bluetooth.

1) I added Greg and linux-usb@...r.kernel.org because usb_submit_urb()
doesn't specify the meaning of its negative return values. I traced
-EPERM to usb_hcd_link_urb_to_ep(). That returns -EPERM if the urb is
"being killed". Did I trace the calls made by usb_submit_urb()
correctly? I also just assumed that -ENODEV always means device got
disconnected or something comparable. Is that correct too?

2) Sent as an RFC because btusb's btusb_*_complete() functions puzzle
me. If I read their code correctly these three functions will be an
urb's completion handler when usb_submit_urb() is called on that urb.
This means they will be called when usb_submit_urb() is done doing its
stuff. But they themselves also call usb_submit_urb()! So, apparently,
there's this endless loop:
    usb_submit_urb()
        btusb_*_complete()
           usb_submit_urb()
               [...]

It seems that usb_submit_urb() returning -EPERM is the expected way for
this loop to end. Am I reading this correctly? If so, doesn't this need
some comments to explain that? But perhaps calling usb_submit_urb() in a
completion handler is a common pattern for usb devices.

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

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 91d13a9..9e4448e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -256,7 +256,9 @@ static void btusb_intr_complete(struct urb *urb)
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
 	if (err < 0) {
-		if (err != -EPERM)
+		/* -EPERM: urb is being killed;
+		 * -ENODEV: device got disconnected */
+		if (err != -EPERM && err != -ENODEV)
 			BT_ERR("%s urb %p failed to resubmit (%d)",
 						hdev->name, urb, -err);
 		usb_unanchor_urb(urb);
@@ -341,7 +343,9 @@ static void btusb_bulk_complete(struct urb *urb)
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
 	if (err < 0) {
-		if (err != -EPERM)
+		/* -EPERM: urb is being killed;
+		 * -ENODEV: device got disconnected */
+		if (err != -EPERM && err != -ENODEV)
 			BT_ERR("%s urb %p failed to resubmit (%d)",
 						hdev->name, urb, -err);
 		usb_unanchor_urb(urb);
@@ -431,7 +435,9 @@ static void btusb_isoc_complete(struct urb *urb)
 
 	err = usb_submit_urb(urb, GFP_ATOMIC);
 	if (err < 0) {
-		if (err != -EPERM)
+		/* -EPERM: urb is being killed;
+		 * -ENODEV: device got disconnected */
+		if (err != -EPERM && err != -ENODEV)
 			BT_ERR("%s urb %p failed to resubmit (%d)",
 						hdev->name, urb, -err);
 		usb_unanchor_urb(urb);
-- 
1.7.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/