wacom + runtime PM = AA deadlock

wacom + runtime PM = AA deadlock

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

thanks,
-- 
js
suse labs

Re: wacom + runtime PM = AA deadlock

[Alan Stern]

On Mon, 13 Sep 2010, Jiri Slaby wrote:

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

One possibility is for wacom_open simply not to acquire the lock until
after the usb_autopm_get_interface call returns.  And why does
wacom_open have that assignment to wacom->irq->dev?  The value was
already set in wacom_probe.

BTW, does anybody know why wacom_open calls usb_autopm_get_interface 
but wacom_close doesn't call usb_autopm_put_interface?

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

On Mon, 13 Sep 2010, Jiri Slaby wrote:

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

One possibility is for wacom_open simply not to acquire the lock until
after the usb_autopm_get_interface call returns.  And why does
wacom_open have that assignment to wacom->irq->dev?  The value was
already set in wacom_probe.

BTW, does anybody know why wacom_open calls usb_autopm_get_interface 
but wacom_close doesn't call usb_autopm_put_interface?

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Mon, 13 Sep 2010, Jiri Slaby wrote:

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

One possibility is for wacom_open simply not to acquire the lock until
after the usb_autopm_get_interface call returns.  And why does
wacom_open have that assignment to wacom->irq->dev?  The value was
already set in wacom_probe.

BTW, does anybody know why wacom_open calls usb_autopm_get_interface 
but wacom_close doesn't call usb_autopm_put_interface?

Alan Stern

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

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
+	 */
 
 	/* switch to wacom mode first */
 	wacom_query_tablet_data(intf, features);
@@ -583,8 +591,6 @@ static int wacom_resume(struct usb_interface *intf)
 	else
 		rv = 0;
 
-	mutex_unlock(&wacom->lock);
-
 	return rv;
 }
 

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Mon, 13 Sep 2010, Oliver Neukum wrote:

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

Is there any point in resuming the device during close() just in order
to kill the interrupt URB?  It seems counterproductive -- if the device 
had been suspended then there wouldn't be any interrupt URB to kill in 
the first place.

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Mon, 13 Sep 2010, Oliver Neukum wrote:

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

Is there any point in resuming the device during close() just in order
to kill the interrupt URB?  It seems counterproductive -- if the device 
had been suspended then there wouldn't be any interrupt URB to kill in 
the first place.

Alan Stern

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 13. September 2010, 17:17:54 schrieb Alan Stern:
> On Mon, 13 Sep 2010, Oliver Neukum wrote:
> 
> > 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()
> 
> Is there any point in resuming the device during close() just in order
> to kill the interrupt URB?  It seems counterproductive -- if the device 
> had been suspended then there wouldn't be any interrupt URB to kill in 
> the first place.

Suppose the device does not support remote wakeup. It would never
be autosuspended while it is open, but simply resetting the flag
would never reach the PM layer.

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 13. September 2010, 17:17:54 schrieb Alan Stern:
> On Mon, 13 Sep 2010, Oliver Neukum wrote:
> 
> > 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()
> 
> Is there any point in resuming the device during close() just in order
> to kill the interrupt URB?  It seems counterproductive -- if the device 
> had been suspended then there wouldn't be any interrupt URB to kill in 
> the first place.

Suppose the device does not support remote wakeup. It would never
be autosuspended while it is open, but simply resetting the flag
would never reach the PM layer.

	Regards
		Oliver

Re: wacom + runtime PM = AA deadlock

[Alan Stern]

On Mon, 13 Sep 2010, Oliver Neukum wrote:

> Am Montag, 13. September 2010, 17:17:54 schrieb Alan Stern:
> > On Mon, 13 Sep 2010, Oliver Neukum wrote:
> > 
> > > 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()
> > 
> > Is there any point in resuming the device during close() just in order
> > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > had been suspended then there wouldn't be any interrupt URB to kill in 
> > the first place.
> 
> Suppose the device does not support remote wakeup. It would never
> be autosuspended while it is open, but simply resetting the flag
> would never reach the PM layer.

Whoops, that's right.  I didn't see the assignment to 
needs_remote_wakeup.

How come wacom_open doesn't check to see if wacom->open is already set?

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Mon, 13 Sep 2010, Oliver Neukum wrote:

> Am Montag, 13. September 2010, 17:17:54 schrieb Alan Stern:
> > On Mon, 13 Sep 2010, Oliver Neukum wrote:
> > 
> > > 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()
> > 
> > Is there any point in resuming the device during close() just in order
> > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > had been suspended then there wouldn't be any interrupt URB to kill in 
> > the first place.
> 
> Suppose the device does not support remote wakeup. It would never
> be autosuspended while it is open, but simply resetting the flag
> would never reach the PM layer.

Whoops, that's right.  I didn't see the assignment to 
needs_remote_wakeup.

How come wacom_open doesn't check to see if wacom->open is already set?

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Mon, 13 Sep 2010, Oliver Neukum wrote:

> Am Montag, 13. September 2010, 17:17:54 schrieb Alan Stern:
> > On Mon, 13 Sep 2010, Oliver Neukum wrote:
> > 
> > > 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()
> > 
> > Is there any point in resuming the device during close() just in order
> > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > had been suspended then there wouldn't be any interrupt URB to kill in 
> > the first place.
> 
> Suppose the device does not support remote wakeup. It would never
> be autosuspended while it is open, but simply resetting the flag
> would never reach the PM layer.

Whoops, that's right.  I didn't see the assignment to 
needs_remote_wakeup.

How come wacom_open doesn't check to see if wacom->open is already set?

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Monday, September 13, 2010 01:02:16 pm Alan Stern wrote:
> On Mon, 13 Sep 2010, Oliver Neukum wrote:
> > Am Montag, 13. September 2010, 17:17:54 schrieb Alan Stern:
> > > On Mon, 13 Sep 2010, Oliver Neukum wrote:
> > > > 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()
> > > 
> > > Is there any point in resuming the device during close() just in order
> > > to kill the interrupt URB?  It seems counterproductive -- if the device
> > > had been suspended then there wouldn't be any interrupt URB to kill in
> > > the first place.
> > 
> > Suppose the device does not support remote wakeup. It would never
> > be autosuspended while it is open, but simply resetting the flag
> > would never reach the PM layer.
> 
> Whoops, that's right.  I didn't see the assignment to
> needs_remote_wakeup.
> 
> How come wacom_open doesn't check to see if wacom->open is already set?

No need - input core will not call dev->open() twice.

-- 
Dmitry

Re: wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Monday, September 13, 2010 01:02:16 pm Alan Stern wrote:
> On Mon, 13 Sep 2010, Oliver Neukum wrote:
> > Am Montag, 13. September 2010, 17:17:54 schrieb Alan Stern:
> > > On Mon, 13 Sep 2010, Oliver Neukum wrote:
> > > > 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()
> > > 
> > > Is there any point in resuming the device during close() just in order
> > > to kill the interrupt URB?  It seems counterproductive -- if the device
> > > had been suspended then there wouldn't be any interrupt URB to kill in
> > > the first place.
> > 
> > Suppose the device does not support remote wakeup. It would never
> > be autosuspended while it is open, but simply resetting the flag
> > would never reach the PM layer.
> 
> Whoops, that's right.  I didn't see the assignment to
> needs_remote_wakeup.
> 
> How come wacom_open doesn't check to see if wacom->open is already set?

No need - input core will not call dev->open() twice.

-- 
Dmitry

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 13. September 2010, 22:02:16 schrieb Alan Stern:
> > > Is there any point in resuming the device during close() just in order
> > > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > > had been suspended then there wouldn't be any interrupt URB to kill in 
> > > the first place.
> > 
> > Suppose the device does not support remote wakeup. It would never
> > be autosuspended while it is open, but simply resetting the flag
> > would never reach the PM layer.
> 
> Whoops, that's right.  I didn't see the assignment to 
> needs_remote_wakeup.

Should I have used usb_autopm_get_interface_no_resume()?

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 13. September 2010, 22:02:16 schrieb Alan Stern:
> > > Is there any point in resuming the device during close() just in order
> > > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > > had been suspended then there wouldn't be any interrupt URB to kill in 
> > > the first place.
> > 
> > Suppose the device does not support remote wakeup. It would never
> > be autosuspended while it is open, but simply resetting the flag
> > would never reach the PM layer.
> 
> Whoops, that's right.  I didn't see the assignment to 
> needs_remote_wakeup.

Should I have used usb_autopm_get_interface_no_resume()?

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> Am Montag, 13. September 2010, 22:02:16 schrieb Alan Stern:
> > > > Is there any point in resuming the device during close() just in order
> > > > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > > > had been suspended then there wouldn't be any interrupt URB to kill in 
> > > > the first place.
> > > 
> > > Suppose the device does not support remote wakeup. It would never
> > > be autosuspended while it is open, but simply resetting the flag
> > > would never reach the PM layer.
> > 
> > Whoops, that's right.  I didn't see the assignment to 
> > needs_remote_wakeup.
> 
> Should I have used usb_autopm_get_interface_no_resume()?

That actually would work.  It's a good idea.  The only drawback (not a
big one) is that if the device _was_ suspended with remote wakeup
enabled, doing this wouldn't turn off remote wakeup.  I think that
doesn't matter.

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> Am Montag, 13. September 2010, 22:02:16 schrieb Alan Stern:
> > > > Is there any point in resuming the device during close() just in order
> > > > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > > > had been suspended then there wouldn't be any interrupt URB to kill in 
> > > > the first place.
> > > 
> > > Suppose the device does not support remote wakeup. It would never
> > > be autosuspended while it is open, but simply resetting the flag
> > > would never reach the PM layer.
> > 
> > Whoops, that's right.  I didn't see the assignment to 
> > needs_remote_wakeup.
> 
> Should I have used usb_autopm_get_interface_no_resume()?

That actually would work.  It's a good idea.  The only drawback (not a
big one) is that if the device _was_ suspended with remote wakeup
enabled, doing this wouldn't turn off remote wakeup.  I think that
doesn't matter.

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Dienstag, 14. September 2010, 16:01:14 schrieb Alan Stern:
> On Tue, 14 Sep 2010, Oliver Neukum wrote:
> 
> > Am Montag, 13. September 2010, 22:02:16 schrieb Alan Stern:
> > > > > Is there any point in resuming the device during close() just in order
> > > > > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > > > > had been suspended then there wouldn't be any interrupt URB to kill in 
> > > > > the first place.
> > > > 
> > > > Suppose the device does not support remote wakeup. It would never
> > > > be autosuspended while it is open, but simply resetting the flag
> > > > would never reach the PM layer.
> > > 
> > > Whoops, that's right.  I didn't see the assignment to 
> > > needs_remote_wakeup.
> > 
> > Should I have used usb_autopm_get_interface_no_resume()?
> 
> That actually would work.  It's a good idea.  The only drawback (not a
> big one) is that if the device _was_ suspended with remote wakeup
> enabled, doing this wouldn't turn off remote wakeup.  I think that
> doesn't matter.

I am afraid it does matter as devices whose remote wakeup is enabled
may draw more power.

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> > > Should I have used usb_autopm_get_interface_no_resume()?
> > 
> > That actually would work.  It's a good idea.  The only drawback (not a
> > big one) is that if the device _was_ suspended with remote wakeup
> > enabled, doing this wouldn't turn off remote wakeup.  I think that
> > doesn't matter.
> 
> I am afraid it does matter as devices whose remote wakeup is enabled
> may draw more power.

I doubt it, or at least, not very much more.  They are still limited by 
the USB spec as to the total amount of current they can draw while 
suspended.

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> > > Should I have used usb_autopm_get_interface_no_resume()?
> > 
> > That actually would work.  It's a good idea.  The only drawback (not a
> > big one) is that if the device _was_ suspended with remote wakeup
> > enabled, doing this wouldn't turn off remote wakeup.  I think that
> > doesn't matter.
> 
> I am afraid it does matter as devices whose remote wakeup is enabled
> may draw more power.

I doubt it, or at least, not very much more.  They are still limited by 
the USB spec as to the total amount of current they can draw while 
suspended.

Alan Stern

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Dienstag, 14. September 2010, 17:23:15 schrieb Alan Stern:
> On Tue, 14 Sep 2010, Oliver Neukum wrote:
> 
> > > > Should I have used usb_autopm_get_interface_no_resume()?
> > > 
> > > That actually would work.  It's a good idea.  The only drawback (not a
> > > big one) is that if the device _was_ suspended with remote wakeup
> > > enabled, doing this wouldn't turn off remote wakeup.  I think that
> > > doesn't matter.
> > 
> > I am afraid it does matter as devices whose remote wakeup is enabled
> > may draw more power.
> 
> I doubt it, or at least, not very much more.  They are still limited by 
> the USB spec as to the total amount of current they can draw while 
> suspended.

7.2.3 Sources of remote wakeup may draw 2.5mA when suspended
as opposed to 0.5 mA

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Dienstag, 14. September 2010, 17:23:15 schrieb Alan Stern:
> On Tue, 14 Sep 2010, Oliver Neukum wrote:
> 
> > > > Should I have used usb_autopm_get_interface_no_resume()?
> > > 
> > > That actually would work.  It's a good idea.  The only drawback (not a
> > > big one) is that if the device _was_ suspended with remote wakeup
> > > enabled, doing this wouldn't turn off remote wakeup.  I think that
> > > doesn't matter.
> > 
> > I am afraid it does matter as devices whose remote wakeup is enabled
> > may draw more power.
> 
> I doubt it, or at least, not very much more.  They are still limited by 
> the USB spec as to the total amount of current they can draw while 
> suspended.

7.2.3 Sources of remote wakeup may draw 2.5mA when suspended
as opposed to 0.5 mA

	Regards
		Oliver

Re: wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> Am Dienstag, 14. September 2010, 17:23:15 schrieb Alan Stern:
> > On Tue, 14 Sep 2010, Oliver Neukum wrote:
> > 
> > > > > Should I have used usb_autopm_get_interface_no_resume()?
> > > > 
> > > > That actually would work.  It's a good idea.  The only drawback (not a
> > > > big one) is that if the device _was_ suspended with remote wakeup
> > > > enabled, doing this wouldn't turn off remote wakeup.  I think that
> > > > doesn't matter.
> > > 
> > > I am afraid it does matter as devices whose remote wakeup is enabled
> > > may draw more power.
> > 
> > I doubt it, or at least, not very much more.  They are still limited by 
> > the USB spec as to the total amount of current they can draw while 
> > suspended.
> 
> 7.2.3 Sources of remote wakeup may draw 2.5mA when suspended
> as opposed to 0.5 mA

Okay, then we may as well wake it up in order to turn off remote 
wakeup.  For drivers that don't need remote wakeup, it's better to use 
the _no_resume form.

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> Am Dienstag, 14. September 2010, 17:23:15 schrieb Alan Stern:
> > On Tue, 14 Sep 2010, Oliver Neukum wrote:
> > 
> > > > > Should I have used usb_autopm_get_interface_no_resume()?
> > > > 
> > > > That actually would work.  It's a good idea.  The only drawback (not a
> > > > big one) is that if the device _was_ suspended with remote wakeup
> > > > enabled, doing this wouldn't turn off remote wakeup.  I think that
> > > > doesn't matter.
> > > 
> > > I am afraid it does matter as devices whose remote wakeup is enabled
> > > may draw more power.
> > 
> > I doubt it, or at least, not very much more.  They are still limited by 
> > the USB spec as to the total amount of current they can draw while 
> > suspended.
> 
> 7.2.3 Sources of remote wakeup may draw 2.5mA when suspended
> as opposed to 0.5 mA

Okay, then we may as well wake it up in order to turn off remote 
wakeup.  For drivers that don't need remote wakeup, it's better to use 
the _no_resume form.

Alan Stern

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> Am Dienstag, 14. September 2010, 17:23:15 schrieb Alan Stern:
> > On Tue, 14 Sep 2010, Oliver Neukum wrote:
> > 
> > > > > Should I have used usb_autopm_get_interface_no_resume()?
> > > > 
> > > > That actually would work.  It's a good idea.  The only drawback (not a
> > > > big one) is that if the device _was_ suspended with remote wakeup
> > > > enabled, doing this wouldn't turn off remote wakeup.  I think that
> > > > doesn't matter.
> > > 
> > > I am afraid it does matter as devices whose remote wakeup is enabled
> > > may draw more power.
> > 
> > I doubt it, or at least, not very much more.  They are still limited by 
> > the USB spec as to the total amount of current they can draw while 
> > suspended.
> 
> 7.2.3 Sources of remote wakeup may draw 2.5mA when suspended
> as opposed to 0.5 mA

Okay, then we may as well wake it up in order to turn off remote 
wakeup.  For drivers that don't need remote wakeup, it's better to use 
the _no_resume form.

Alan Stern

Re: wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> > > Should I have used usb_autopm_get_interface_no_resume()?
> > 
> > That actually would work.  It's a good idea.  The only drawback (not a
> > big one) is that if the device _was_ suspended with remote wakeup
> > enabled, doing this wouldn't turn off remote wakeup.  I think that
> > doesn't matter.
> 
> I am afraid it does matter as devices whose remote wakeup is enabled
> may draw more power.

I doubt it, or at least, not very much more.  They are still limited by 
the USB spec as to the total amount of current they can draw while 
suspended.

Alan Stern

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Dienstag, 14. September 2010, 16:01:14 schrieb Alan Stern:
> On Tue, 14 Sep 2010, Oliver Neukum wrote:
> 
> > Am Montag, 13. September 2010, 22:02:16 schrieb Alan Stern:
> > > > > Is there any point in resuming the device during close() just in order
> > > > > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > > > > had been suspended then there wouldn't be any interrupt URB to kill in 
> > > > > the first place.
> > > > 
> > > > Suppose the device does not support remote wakeup. It would never
> > > > be autosuspended while it is open, but simply resetting the flag
> > > > would never reach the PM layer.
> > > 
> > > Whoops, that's right.  I didn't see the assignment to 
> > > needs_remote_wakeup.
> > 
> > Should I have used usb_autopm_get_interface_no_resume()?
> 
> That actually would work.  It's a good idea.  The only drawback (not a
> big one) is that if the device _was_ suspended with remote wakeup
> enabled, doing this wouldn't turn off remote wakeup.  I think that
> doesn't matter.

I am afraid it does matter as devices whose remote wakeup is enabled
may draw more power.

	Regards
		Oliver

Re: wacom + runtime PM = AA deadlock

[Alan Stern]

On Tue, 14 Sep 2010, Oliver Neukum wrote:

> Am Montag, 13. September 2010, 22:02:16 schrieb Alan Stern:
> > > > Is there any point in resuming the device during close() just in order
> > > > to kill the interrupt URB?  It seems counterproductive -- if the device 
> > > > had been suspended then there wouldn't be any interrupt URB to kill in 
> > > > the first place.
> > > 
> > > Suppose the device does not support remote wakeup. It would never
> > > be autosuspended while it is open, but simply resetting the flag
> > > would never reach the PM layer.
> > 
> > Whoops, that's right.  I didn't see the assignment to 
> > needs_remote_wakeup.
> 
> Should I have used usb_autopm_get_interface_no_resume()?

That actually would work.  It's a good idea.  The only drawback (not a
big one) is that if the device _was_ suspended with remote wakeup
enabled, doing this wouldn't turn off remote wakeup.  I think that
doesn't matter.

Alan Stern

Re: wacom + runtime PM = AA deadlock

[Alan Stern]

On Mon, 13 Sep 2010, Oliver Neukum wrote:

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

Is there any point in resuming the device during close() just in order
to kill the interrupt URB?  It seems counterproductive -- if the device 
had been suspended then there wouldn't be any interrupt URB to kill in 
the first place.

Alan Stern

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@gmail.com>

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 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)

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@gmail.com>

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 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)

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 13. September 2010, 19:10:47 schrieb Dmitry Torokhov:

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

Right. Which makes no sense at all. You'd better remove anything related
to runtime PM and not set supports_autosuspend for that.

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 13. September 2010, 19:10:47 schrieb Dmitry Torokhov:

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

Right. Which makes no sense at all. You'd better remove anything related
to runtime PM and not set supports_autosuspend for that.

	Regards
		Oliver

Re: wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Mon, Sep 13, 2010 at 09:20:23PM +0200, Oliver Neukum wrote:
> Am Montag, 13. September 2010, 19:10:47 schrieb Dmitry Torokhov:
> 
> > 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?
> 
> Right. Which makes no sense at all. You'd better remove anything related
> to runtime PM and not set supports_autosuspend for that.
> 

That not what I meant, I do not want to remove autopm, it's just it was
effectively disabled and if we fix it we might start getting some
regression reports ;)

-- 
Dmitry

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Mon, Sep 13, 2010 at 09:20:23PM +0200, Oliver Neukum wrote:
> Am Montag, 13. September 2010, 19:10:47 schrieb Dmitry Torokhov:
> 
> > 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?
> 
> Right. Which makes no sense at all. You'd better remove anything related
> to runtime PM and not set supports_autosuspend for that.
> 

That not what I meant, I do not want to remove autopm, it's just it was
effectively disabled and if we fix it we might start getting some
regression reports ;)

-- 
Dmitry

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Dienstag, 14. September 2010, 02:52:10 schrieb Dmitry Torokhov:
> On Mon, Sep 13, 2010 at 09:20:23PM +0200, Oliver Neukum wrote:
> > Am Montag, 13. September 2010, 19:10:47 schrieb Dmitry Torokhov:
> > 
> > > 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?
> > 
> > Right. Which makes no sense at all. You'd better remove anything related
> > to runtime PM and not set supports_autosuspend for that.
> > 
> 
> That not what I meant, I do not want to remove autopm, it's just it was
> effectively disabled and if we fix it we might start getting some
> regression reports ;)

True. So currently we have

- a deadlock
- disabled runtime power management

We need to fix the deadlock. We can fix it retaining a disabled
runtime power management. Or we can fix it fixing the runtime
power management at the same time. However this opens
the door to regressions. So for now I really suggest removing
it from the driver and reintroduce it properly for the next merge
window.

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Dienstag, 14. September 2010, 02:52:10 schrieb Dmitry Torokhov:
> On Mon, Sep 13, 2010 at 09:20:23PM +0200, Oliver Neukum wrote:
> > Am Montag, 13. September 2010, 19:10:47 schrieb Dmitry Torokhov:
> > 
> > > 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?
> > 
> > Right. Which makes no sense at all. You'd better remove anything related
> > to runtime PM and not set supports_autosuspend for that.
> > 
> 
> That not what I meant, I do not want to remove autopm, it's just it was
> effectively disabled and if we fix it we might start getting some
> regression reports ;)

True. So currently we have

- a deadlock
- disabled runtime power management

We need to fix the deadlock. We can fix it retaining a disabled
runtime power management. Or we can fix it fixing the runtime
power management at the same time. However this opens
the door to regressions. So for now I really suggest removing
it from the driver and reintroduce it properly for the next merge
window.

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Tue, Sep 14, 2010 at 08:07:39AM +0200, Oliver Neukum wrote:
> Am Dienstag, 14. September 2010, 02:52:10 schrieb Dmitry Torokhov:
> > On Mon, Sep 13, 2010 at 09:20:23PM +0200, Oliver Neukum wrote:
> > > Am Montag, 13. September 2010, 19:10:47 schrieb Dmitry Torokhov:
> > > 
> > > > 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?
> > > 
> > > Right. Which makes no sense at all. You'd better remove anything related
> > > to runtime PM and not set supports_autosuspend for that.
> > > 
> > 
> > That not what I meant, I do not want to remove autopm, it's just it was
> > effectively disabled and if we fix it we might start getting some
> > regression reports ;)
> 
> True. So currently we have
> 
> - a deadlock
> - disabled runtime power management
> 
> We need to fix the deadlock. We can fix it retaining a disabled
> runtime power management. Or we can fix it fixing the runtime
> power management at the same time. However this opens
> the door to regressions. So for now I really suggest removing
> it from the driver and reintroduce it properly for the next merge
> window.
> 

Lost track of this issue for a while. So I think we still need to fix
the deadlock for .36 and I think that the following will do that. Then
we'll adjust the driver to actually enable runtime PM for .37.

Thanks.

-- 
Dmitry

Input: wacom - fix runtime PM related deadlock

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

When runtime PM is enabled by default for input devices, X hangs in
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() takes wacom->lock and calls usb_autopm_get_interface(),
which in turn calls wacom_resume() which tries to acquire the lock
again.

The fix is to call usb_autopm_get_interface() first, before we take
the lock.

Since we do not do usb_autopm_put_interface() until wacom_close()
is called runtime PM is effectively disabled for the driver, however
changing it now would risk regressions so the complete fix will
have to wait till the next merge window.

Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

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


diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 1e3af29..02de653 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 = 0;
 
-	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;
+	if (retval)
+		usb_autopm_put_interface(wacom->intf);
+	return retval;
 }
 
 static void wacom_close(struct input_dev *dev)
@@ -135,6 +134,8 @@ static void wacom_close(struct input_dev *dev)
 	wacom->open = false;
 	wacom->intf->needs_remote_wakeup = 0;
 	mutex_unlock(&wacom->lock);
+
+	usb_autopm_put_interface(wacom->intf);
 }
 
 static int wacom_parse_hid(struct usb_interface *intf, struct hid_descriptor *hid_desc,

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 4. Oktober 2010, 18:13:20 schrieb Dmitry Torokhov:
> Lost track of this issue for a while. So I think we still need to fix
> the deadlock for .36 and I think that the following will do that. Then
> we'll adjust the driver to actually enable runtime PM for .37.

That's a viable plan.

	Regards
		Oliver

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 4. Oktober 2010, 18:13:20 schrieb Dmitry Torokhov:
> Lost track of this issue for a while. So I think we still need to fix
> the deadlock for .36 and I think that the following will do that. Then
> we'll adjust the driver to actually enable runtime PM for .37.

That's a viable plan.

	Regards
		Oliver

Re: wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Mon, Oct 04, 2010 at 08:33:53PM +0200, Oliver Neukum wrote:
> Am Montag, 4. Oktober 2010, 18:13:20 schrieb Dmitry Torokhov:
> > Lost track of this issue for a while. So I think we still need to fix
> > the deadlock for .36 and I think that the following will do that. Then
> > we'll adjust the driver to actually enable runtime PM for .37.
> 
> That's a viable plan.
> 

I take it as an "acked-by" for this patch, right?

-- 
Dmitry

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Mon, Oct 04, 2010 at 08:33:53PM +0200, Oliver Neukum wrote:
> Am Montag, 4. Oktober 2010, 18:13:20 schrieb Dmitry Torokhov:
> > Lost track of this issue for a while. So I think we still need to fix
> > the deadlock for .36 and I think that the following will do that. Then
> > we'll adjust the driver to actually enable runtime PM for .37.
> 
> That's a viable plan.
> 

I take it as an "acked-by" for this patch, right?

-- 
Dmitry

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 4. Oktober 2010, 20:38:49 schrieb Dmitry Torokhov:
> On Mon, Oct 04, 2010 at 08:33:53PM +0200, Oliver Neukum wrote:
> > Am Montag, 4. Oktober 2010, 18:13:20 schrieb Dmitry Torokhov:
> > > Lost track of this issue for a while. So I think we still need to fix
> > > the deadlock for .36 and I think that the following will do that. Then
> > > we'll adjust the driver to actually enable runtime PM for .37.
> > 
> > That's a viable plan.
> > 
> 
> I take it as an "acked-by" for this patch, right?

Sure. Just in case:

Acked-by: Oliver Neukum <oneukum@suse.de>

	HTH
		Oliver

Re: wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Mon, Oct 04, 2010 at 09:24:24PM +0200, Oliver Neukum wrote:
> Am Montag, 4. Oktober 2010, 20:38:49 schrieb Dmitry Torokhov:
> > On Mon, Oct 04, 2010 at 08:33:53PM +0200, Oliver Neukum wrote:
> > > Am Montag, 4. Oktober 2010, 18:13:20 schrieb Dmitry Torokhov:
> > > > Lost track of this issue for a while. So I think we still need to fix
> > > > the deadlock for .36 and I think that the following will do that. Then
> > > > we'll adjust the driver to actually enable runtime PM for .37.
> > > 
> > > That's a viable plan.
> > > 
> > 
> > I take it as an "acked-by" for this patch, right?
> 
> Sure. Just in case:
> 
> Acked-by: Oliver Neukum <oneukum@suse.de>
> 

Thanks Oliver. And the patch below is for .37 properly enabling runtime
PM for wacom.

-- 
Dmitry

Input: wacom - properly enable runtime PM

We need to always call usb_autopm_put_interface() in wacom_open(),
not only when initialization fails, otherwise the device will be
marked as PM-busy and will never be put in suspended state.

Based on patch by Oliver Neukum.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/tablet/wacom_sys.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)


diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 02de653..fc38149 100644
--- a/drivers/input/tablet/wacom_sys.c
+++ b/drivers/input/tablet/wacom_sys.c
@@ -120,14 +120,16 @@ static int wacom_open(struct input_dev *dev)
 
 out:
 	mutex_unlock(&wacom->lock);
-	if (retval)
-		usb_autopm_put_interface(wacom->intf);
+	usb_autopm_put_interface(wacom->intf);
 	return retval;
 }
 
 static void wacom_close(struct input_dev *dev)
 {
 	struct wacom *wacom = input_get_drvdata(dev);
+	int autopm_error;
+
+	autopm_error = usb_autopm_get_interface(wacom->intf);
 
 	mutex_lock(&wacom->lock);
 	usb_kill_urb(wacom->irq);
@@ -135,7 +137,8 @@ static void wacom_close(struct input_dev *dev)
 	wacom->intf->needs_remote_wakeup = 0;
 	mutex_unlock(&wacom->lock);
 
-	usb_autopm_put_interface(wacom->intf);
+	if (!autopm_error)
+		usb_autopm_put_interface(wacom->intf);
 }
 
 static int wacom_parse_hid(struct usb_interface *intf, struct hid_descriptor *hid_desc,

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Mon, Oct 04, 2010 at 09:24:24PM +0200, Oliver Neukum wrote:
> Am Montag, 4. Oktober 2010, 20:38:49 schrieb Dmitry Torokhov:
> > On Mon, Oct 04, 2010 at 08:33:53PM +0200, Oliver Neukum wrote:
> > > Am Montag, 4. Oktober 2010, 18:13:20 schrieb Dmitry Torokhov:
> > > > Lost track of this issue for a while. So I think we still need to fix
> > > > the deadlock for .36 and I think that the following will do that. Then
> > > > we'll adjust the driver to actually enable runtime PM for .37.
> > > 
> > > That's a viable plan.
> > > 
> > 
> > I take it as an "acked-by" for this patch, right?
> 
> Sure. Just in case:
> 
> Acked-by: Oliver Neukum <oneukum@suse.de>
> 

Thanks Oliver. And the patch below is for .37 properly enabling runtime
PM for wacom.

-- 
Dmitry

Input: wacom - properly enable runtime PM

We need to always call usb_autopm_put_interface() in wacom_open(),
not only when initialization fails, otherwise the device will be
marked as PM-busy and will never be put in suspended state.

Based on patch by Oliver Neukum.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/tablet/wacom_sys.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)


diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 02de653..fc38149 100644
--- a/drivers/input/tablet/wacom_sys.c
+++ b/drivers/input/tablet/wacom_sys.c
@@ -120,14 +120,16 @@ static int wacom_open(struct input_dev *dev)
 
 out:
 	mutex_unlock(&wacom->lock);
-	if (retval)
-		usb_autopm_put_interface(wacom->intf);
+	usb_autopm_put_interface(wacom->intf);
 	return retval;
 }
 
 static void wacom_close(struct input_dev *dev)
 {
 	struct wacom *wacom = input_get_drvdata(dev);
+	int autopm_error;
+
+	autopm_error = usb_autopm_get_interface(wacom->intf);
 
 	mutex_lock(&wacom->lock);
 	usb_kill_urb(wacom->irq);
@@ -135,7 +137,8 @@ static void wacom_close(struct input_dev *dev)
 	wacom->intf->needs_remote_wakeup = 0;
 	mutex_unlock(&wacom->lock);
 
-	usb_autopm_put_interface(wacom->intf);
+	if (!autopm_error)
+		usb_autopm_put_interface(wacom->intf);
 }
 
 static int wacom_parse_hid(struct usb_interface *intf, struct hid_descriptor *hid_desc,

Re: [linux-pm] wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Dienstag, 5. Oktober 2010, 07:41:40 schrieb Dmitry Torokhov:

> Thanks Oliver. And the patch below is for .37 properly enabling runtime
> PM for wacom.

Acked-by: Oliver Neukum <oneukum@suse.de>

	Regards
		Oliver

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Dienstag, 5. Oktober 2010, 07:41:40 schrieb Dmitry Torokhov:

> Thanks Oliver. And the patch below is for .37 properly enabling runtime
> PM for wacom.

Acked-by: Oliver Neukum <oneukum@suse.de>

	Regards
		Oliver

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

Am Montag, 4. Oktober 2010, 20:38:49 schrieb Dmitry Torokhov:
> On Mon, Oct 04, 2010 at 08:33:53PM +0200, Oliver Neukum wrote:
> > Am Montag, 4. Oktober 2010, 18:13:20 schrieb Dmitry Torokhov:
> > > Lost track of this issue for a while. So I think we still need to fix
> > > the deadlock for .36 and I think that the following will do that. Then
> > > we'll adjust the driver to actually enable runtime PM for .37.
> > 
> > That's a viable plan.
> > 
> 
> I take it as an "acked-by" for this patch, right?

Sure. Just in case:

Acked-by: Oliver Neukum <oneukum@suse.de>

	HTH
		Oliver

Re: wacom + runtime PM = AA deadlock

[Dmitry Torokhov]

On Tue, Sep 14, 2010 at 08:07:39AM +0200, Oliver Neukum wrote:
> Am Dienstag, 14. September 2010, 02:52:10 schrieb Dmitry Torokhov:
> > On Mon, Sep 13, 2010 at 09:20:23PM +0200, Oliver Neukum wrote:
> > > Am Montag, 13. September 2010, 19:10:47 schrieb Dmitry Torokhov:
> > > 
> > > > 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?
> > > 
> > > Right. Which makes no sense at all. You'd better remove anything related
> > > to runtime PM and not set supports_autosuspend for that.
> > > 
> > 
> > That not what I meant, I do not want to remove autopm, it's just it was
> > effectively disabled and if we fix it we might start getting some
> > regression reports ;)
> 
> True. So currently we have
> 
> - a deadlock
> - disabled runtime power management
> 
> We need to fix the deadlock. We can fix it retaining a disabled
> runtime power management. Or we can fix it fixing the runtime
> power management at the same time. However this opens
> the door to regressions. So for now I really suggest removing
> it from the driver and reintroduce it properly for the next merge
> window.
> 

Lost track of this issue for a while. So I think we still need to fix
the deadlock for .36 and I think that the following will do that. Then
we'll adjust the driver to actually enable runtime PM for .37.

Thanks.

-- 
Dmitry

Input: wacom - fix runtime PM related deadlock

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

When runtime PM is enabled by default for input devices, X hangs in
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() takes wacom->lock and calls usb_autopm_get_interface(),
which in turn calls wacom_resume() which tries to acquire the lock
again.

The fix is to call usb_autopm_get_interface() first, before we take
the lock.

Since we do not do usb_autopm_put_interface() until wacom_close()
is called runtime PM is effectively disabled for the driver, however
changing it now would risk regressions so the complete fix will
have to wait till the next merge window.

Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

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


diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 1e3af29..02de653 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 = 0;
 
-	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;
+	if (retval)
+		usb_autopm_put_interface(wacom->intf);
+	return retval;
 }
 
 static void wacom_close(struct input_dev *dev)
@@ -135,6 +134,8 @@ static void wacom_close(struct input_dev *dev)
 	wacom->open = false;
 	wacom->intf->needs_remote_wakeup = 0;
 	mutex_unlock(&wacom->lock);
+
+	usb_autopm_put_interface(wacom->intf);
 }
 
 static int wacom_parse_hid(struct usb_interface *intf, struct hid_descriptor *hid_desc,

Re: wacom + runtime PM = AA deadlock

[Oliver Neukum]

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
+	 */
 
 	/* switch to wacom mode first */
 	wacom_query_tablet_data(intf, features);
@@ -583,8 +591,6 @@ static int wacom_resume(struct usb_interface *intf)
 	else
 		rv = 0;
 
-	mutex_unlock(&wacom->lock);
-
 	return rv;
 }
 

wacom + runtime PM = AA deadlock

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

thanks,
-- 
js
suse labs