Re: wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Mon, Sep 13, 2010 at 04:56:17PM +0200, Oliver Neukum wrote:
> Am Montag, 13. September 2010, 14:24:45 schrieb Jiri Slaby:
> > 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@xxxxxxxxx>

Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
---

 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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/