Thread (1 message) 1 message, 1 author, 2010-09-13

Re: wacom + runtime PM = AA deadlock

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2010-09-13 17:10:47
Subsystem: input (keyboard, mouse, joystick, touchscreen) drivers, the rest · Maintainers: Dmitry Torokhov, Linus Torvalds

Possibly related (same subject, not in this thread)

On Mon, Sep 13, 2010 at 04:56:17PM +0200, Oliver Neukum wrote:
quoted hunk
Am Montag, 13. September 2010, 14:24:45 schrieb Jiri Slaby:
quoted
Hi,

by mistake when runtime PM is enabled by default for input devices, X
hangs on wacom open:
[<ffffffff814a00ea>] mutex_lock+0x1a/0x40
[<ffffffffa02bc94b>] wacom_resume+0x3b/0x90 [wacom]
[<ffffffff81327a32>] usb_resume_interface+0xd2/0x190
[<ffffffff81327b5d>] usb_resume_both+0x6d/0x110
[<ffffffff81327c24>] usb_runtime_resume+0x24/0x40
[<ffffffff8130a2cf>] __pm_runtime_resume+0x26f/0x450
[<ffffffff8130a23a>] __pm_runtime_resume+0x1da/0x450
[<ffffffff8130a53a>] pm_runtime_resume+0x2a/0x50
[<ffffffff81328176>] usb_autopm_get_interface+0x26/0x60
[<ffffffffa02bc626>] wacom_open+0x36/0x90 [wacom]

wacom_open took wacom->lock and calls usb_autopm_get_interface which in
turn calls wacom_resume which tries to aquire the lock again.

More details (dmesg including) at:
https://bugzilla.novell.com/show_bug.cgi?id=638506

Any ideas how to fix that properly?
PM in this driver looks broken. Please try this.

In short you want to drop the PM reference and depend on remote
wakeup and busy marking for this driver. Currently it gets a reference
on every open() but never drops it.

For locking you depend on the PM core's internal lock. You simply
make sure you have a PM reference during open() and close()

	Regards
		Oliver
diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 42ba369..e399a8a 100644
--- a/drivers/input/tablet/wacom_sys.c
+++ b/drivers/input/tablet/wacom_sys.c
@@ -121,6 +121,7 @@ static int wacom_open(struct input_dev *dev)
 
 	wacom->open = true;
 	wacom->intf->needs_remote_wakeup = 1;
+	usb_autopm_put_interface(wacom->intf);
 
 	mutex_unlock(&wacom->lock);
 	return 0;
@@ -129,11 +130,15 @@ static int wacom_open(struct input_dev *dev)
 static void wacom_close(struct input_dev *dev)
 {
 	struct wacom *wacom = input_get_drvdata(dev);
+	int r;
 
 	mutex_lock(&wacom->lock);
-	usb_kill_urb(wacom->irq);
+	r = usb_autopm_get_interface(wacom->intf);
 	wacom->open = false;
 	wacom->intf->needs_remote_wakeup = 0;
+	usb_kill_urb(wacom->irq);
+	if (!r)
+		usb_autopm_put_interface(wacom->intf);
 	mutex_unlock(&wacom->lock);
 }
 
@@ -573,7 +578,10 @@ static int wacom_resume(struct usb_interface *intf)
 	struct wacom_features *features = &wacom->wacom_wac.features;
 	int rv;
 
-	mutex_lock(&wacom->lock);
+	/*
+	 * no locking against open needed
+	 * as open holds a power reference
+	 */
Hmm, wacom_open may hold power reference, but what about close? Anyways,
isn't below enough?

I think this introduces significant change in behavior though - before
we did not do usb_autopm_put_interface() on successful open, basically
disabling autopm facilities, right?

-- 
Dmitry


Input: wacom - fix pm locking

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <redacted>
---

 drivers/input/tablet/wacom_sys.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)


diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 1e3af29..2806b2d 100644
--- a/drivers/input/tablet/wacom_sys.c
+++ b/drivers/input/tablet/wacom_sys.c
@@ -103,27 +103,26 @@ static void wacom_sys_irq(struct urb *urb)
 static int wacom_open(struct input_dev *dev)
 {
 	struct wacom *wacom = input_get_drvdata(dev);
+	int retval;
 
-	mutex_lock(&wacom->lock);
-
-	wacom->irq->dev = wacom->usbdev;
-
-	if (usb_autopm_get_interface(wacom->intf) < 0) {
-		mutex_unlock(&wacom->lock);
+	if (usb_autopm_get_interface(wacom->intf) < 0)
 		return -EIO;
-	}
+
+	mutex_lock(&wacom->lock);
 
 	if (usb_submit_urb(wacom->irq, GFP_KERNEL)) {
-		usb_autopm_put_interface(wacom->intf);
-		mutex_unlock(&wacom->lock);
-		return -EIO;
+		retval = -EIO;
+		goto out;
 	}
 
 	wacom->open = true;
 	wacom->intf->needs_remote_wakeup = 1;
 
+out:
 	mutex_unlock(&wacom->lock);
-	return 0;
+	usb_autopm_put_interface(wacom->intf);
+
+	return retval;
 }
 
 static void wacom_close(struct input_dev *dev)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help