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-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
