Fix deadlocks in autosuspend

Fix deadlocks in autosuspend

[Marcus Folkesson]

Hello,

I have not recieved any feedback on these so I resend them.

I got this deadlock on my own driver (pxrc) when using the same
construction.

Please have a look

Here is a clip from 
[PATCH v3] input: pxrc: new driver for PhoenixRC Flight Controller Adapter [1]
that describes the problem.

-------------------------------8<----------------------------------------------
Also, I think we have a deadlock in the synaptics_usb driver.

When the device is suspended and someone is open the device, the input
subsystem will call input_open_device() which takes the
input_dev->mutex and then call input_dev->open().

synusb_open() has a call to usb_autopm_get_interface() which will
result in a call to the registered resume-function if the device is
suspended. (see Documentation/driver-api/usb/power-manaement.rst).

In the case of snaptics_usb, it will take the input_dev->mutex in the
resume function.

I have no synaptic mouse, but tested to put the same code into my
driver just to confirm, and got the following dump:

[ 9215.626476] INFO: task input-events:8590 blocked for more than 120 seconds.
[ 9215.626495]       Not tainted 4.15.0-rc8-ARCH+ #6
[ 9215.626500] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 9215.626507] input-events    D    0  8590   4394 0x00000004
[ 9215.626520] Call Trace:
[ 9215.626546]  ? __schedule+0x236/0x850
[ 9215.626559]  schedule+0x2f/0x90
[ 9215.626569]  schedule_preempt_disabled+0x11/0x20
[ 9215.626579]  __mutex_lock.isra.0+0x1aa/0x520
[ 9215.626609]  ? usb_runtime_suspend+0x70/0x70 [usbcore]
[ 9215.626622]  ? pxrc_resume+0x37/0x70 [pxrc]
[ 9215.626632]  pxrc_resume+0x37/0x70 [pxrc]
[ 9215.626655]  usb_resume_interface.isra.2+0x39/0xe0 [usbcore]
[ 9215.626676]  usb_resume_both+0xd2/0x120 [usbcore]
[ 9215.626688]  __rpm_callback+0xb6/0x1f0
[ 9215.626699]  rpm_callback+0x1f/0x70
[ 9215.626718]  ? usb_runtime_suspend+0x70/0x70 [usbcore]
[ 9215.626726]  rpm_resume+0x4e2/0x7f0
[ 9215.626737]  rpm_resume+0x582/0x7f0
[ 9215.626749]  __pm_runtime_resume+0x3a/0x50
[ 9215.626767]  usb_autopm_get_interface+0x1d/0x50 [usbcore]
[ 9215.626780]  pxrc_open+0x17/0x8d [pxrc]
[ 9215.626791]  input_open_device+0x70/0xa0
[ 9215.626804]  evdev_open+0x183/0x1c0 [evdev]
[ 9215.626819]  chrdev_open+0xa0/0x1b0
[ 9215.626830]  ? cdev_put.part.1+0x20/0x20
[ 9215.626840]  do_dentry_open+0x1ad/0x2c0
[ 9215.626855]  path_openat+0x576/0x1300
[ 9215.626868]  ? alloc_set_pte+0x22c/0x520
[ 9215.626883]  ? filemap_map_pages+0x19b/0x340
[ 9215.626893]  do_filp_open+0x9b/0x110
[ 9215.626908]  ? __check_object_size+0x9d/0x190
[ 9215.626920]  ? __alloc_fd+0xaf/0x160
[ 9215.626931]  ? do_sys_open+0x1bd/0x250
[ 9215.626942]  do_sys_open+0x1bd/0x250
[ 9215.626956]  entry_SYSCALL_64_fastpath+0x20/0x83
[ 9215.626967] RIP: 0033:0x7fbf6358f7ae


tablet/pegasus_notetaker.c and touchscreen/usbtouchscreen.c has the same
construction (taking input_dev->mutex in resume/suspend and call
usb_autopm_get_interface() in open()).

I will create a separate "pm_mutex" to use instead of input_dev->mutex
to get rid of the lockups in those drivers 

-------------------------------8<----------------------------------------------


[1] https://lkml.org/lkml/2018/1/20/191

Thanks,

Best regards
Marcus Folkesson

[RESEND PATCH 1/6] input: synaptics_usb: fix deadlock in autosuspend

[Marcus Folkesson]

usb_autopm_get_interface() that is called in synusb_open() does an
autoresume if the device is suspended.

input_dev->mutex used in synusb_resume() is in this case already
taken by the input subsystem and will cause a deadlock.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/input/mouse/synaptics_usb.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
index cb7d15d826d0..2c66913cf5a2 100644
--- a/drivers/input/mouse/synaptics_usb.c
+++ b/drivers/input/mouse/synaptics_usb.c
@@ -82,6 +82,9 @@ struct synusb {
 	struct urb *urb;
 	unsigned char *data;
 
+	/* serialize access to open/suspend */
+	struct mutex pm_mutex;
+
 	/* input device related data structures */
 	struct input_dev *input;
 	char name[128];
@@ -252,6 +255,7 @@ static int synusb_open(struct input_dev *dev)
 		return retval;
 	}
 
+	mutex_lock(&synusb->pm_mutex);
 	retval = usb_submit_urb(synusb->urb, GFP_KERNEL);
 	if (retval) {
 		dev_err(&synusb->intf->dev,
@@ -264,6 +268,7 @@ static int synusb_open(struct input_dev *dev)
 	synusb->intf->needs_remote_wakeup = 1;
 
 out:
+	mutex_unlock(&synusb->pm_mutex);
 	usb_autopm_put_interface(synusb->intf);
 	return retval;
 }
@@ -275,8 +280,10 @@ static void synusb_close(struct input_dev *dev)
 
 	autopm_error = usb_autopm_get_interface(synusb->intf);
 
+	mutex_lock(&synusb->pm_mutex);
 	usb_kill_urb(synusb->urb);
 	synusb->intf->needs_remote_wakeup = 0;
+	mutex_unlock(&synusb->pm_mutex);
 
 	if (!autopm_error)
 		usb_autopm_put_interface(synusb->intf);
@@ -315,6 +322,7 @@ static int synusb_probe(struct usb_interface *intf,
 	synusb->udev = udev;
 	synusb->intf = intf;
 	synusb->input = input_dev;
+	mutex_init(&synusb->pm_mutex);
 
 	synusb->flags = id->driver_info;
 	if (synusb->flags & SYNUSB_COMBO) {
@@ -466,11 +474,10 @@ static void synusb_disconnect(struct usb_interface *intf)
 static int synusb_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct synusb *synusb = usb_get_intfdata(intf);
-	struct input_dev *input_dev = synusb->input;
 
-	mutex_lock(&input_dev->mutex);
+	mutex_lock(&synusb->pm_mutex);
 	usb_kill_urb(synusb->urb);
-	mutex_unlock(&input_dev->mutex);
+	mutex_unlock(&synusb->pm_mutex);
 
 	return 0;
 }
@@ -481,14 +488,14 @@ static int synusb_resume(struct usb_interface *intf)
 	struct input_dev *input_dev = synusb->input;
 	int retval = 0;
 
-	mutex_lock(&input_dev->mutex);
+	mutex_lock(&synusb->pm_mutex);
 
 	if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
 	    usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
 		retval = -EIO;
 	}
 
-	mutex_unlock(&input_dev->mutex);
+	mutex_unlock(&synusb->pm_mutex);
 
 	return retval;
 }
@@ -496,9 +503,8 @@ static int synusb_resume(struct usb_interface *intf)
 static int synusb_pre_reset(struct usb_interface *intf)
 {
 	struct synusb *synusb = usb_get_intfdata(intf);
-	struct input_dev *input_dev = synusb->input;
 
-	mutex_lock(&input_dev->mutex);
+	mutex_lock(&synusb->pm_mutex);
 	usb_kill_urb(synusb->urb);
 
 	return 0;
@@ -515,7 +521,7 @@ static int synusb_post_reset(struct usb_interface *intf)
 		retval = -EIO;
 	}
 
-	mutex_unlock(&input_dev->mutex);
+	mutex_unlock(&synusb->pm_mutex);
 
 	return retval;
 }
-- 
2.16.2

[RESEND PATCH 2/6] input: synaptics_usb: do not rely on input_dev->users

[Marcus Folkesson]

If the device is unused and suspended, a call to open will cause the
device to autoresume through the call to usb_autopm_get_interface().

input_dev->users is already incremented by the input subsystem,
therefore this expression will always be evaluated to true:

	if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
	    usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
		retval = -EIO;
	}

The same URB will then be fail when resubmitted in synusb_open().

Introduce synusb->is_open to keep track of the state instead.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/input/mouse/synaptics_usb.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
index 2c66913cf5a2..e2b726751220 100644
--- a/drivers/input/mouse/synaptics_usb.c
+++ b/drivers/input/mouse/synaptics_usb.c
@@ -84,6 +84,7 @@ struct synusb {
 
 	/* serialize access to open/suspend */
 	struct mutex pm_mutex;
+	bool is_open;
 
 	/* input device related data structures */
 	struct input_dev *input;
@@ -266,6 +267,7 @@ static int synusb_open(struct input_dev *dev)
 	}
 
 	synusb->intf->needs_remote_wakeup = 1;
+	synusb->is_open = 1;
 
 out:
 	mutex_unlock(&synusb->pm_mutex);
@@ -283,6 +285,7 @@ static void synusb_close(struct input_dev *dev)
 	mutex_lock(&synusb->pm_mutex);
 	usb_kill_urb(synusb->urb);
 	synusb->intf->needs_remote_wakeup = 0;
+	synusb->is_open = 0;
 	mutex_unlock(&synusb->pm_mutex);
 
 	if (!autopm_error)
@@ -485,12 +488,11 @@ static int synusb_suspend(struct usb_interface *intf, pm_message_t message)
 static int synusb_resume(struct usb_interface *intf)
 {
 	struct synusb *synusb = usb_get_intfdata(intf);
-	struct input_dev *input_dev = synusb->input;
 	int retval = 0;
 
 	mutex_lock(&synusb->pm_mutex);
 
-	if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
+	if ((synusb->is_open || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
 	    usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
 		retval = -EIO;
 	}
@@ -513,10 +515,9 @@ static int synusb_pre_reset(struct usb_interface *intf)
 static int synusb_post_reset(struct usb_interface *intf)
 {
 	struct synusb *synusb = usb_get_intfdata(intf);
-	struct input_dev *input_dev = synusb->input;
 	int retval = 0;
 
-	if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
+	if ((synusb->is_open || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
 	    usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
 		retval = -EIO;
 	}
-- 
2.16.2

[RESEND PATCH 3/6] input: pagasus_notetaker: fix deadlock in autosuspend

[Marcus Folkesson]

usb_autopm_get_interface() that is called in pegasus_open() does an
autoresume if the device is suspended.

input_dev->mutex used in pegasus_resume() is in this case already
taken by the input subsystem and will cause a deadlock.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/input/tablet/pegasus_notetaker.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index 47de5a81172f..9ab1ed5e20e7 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -41,6 +41,7 @@
 #include <linux/usb/input.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
+#include <linux/mutex.h>
 
 /* USB HID defines */
 #define USB_REQ_GET_REPORT		0x01
@@ -76,6 +77,10 @@ struct pegasus {
 	struct usb_device *usbdev;
 	struct usb_interface *intf;
 	struct urb *irq;
+
+	/* serialize access to open/suspend */
+	struct mutex pm_mutex;
+
 	char name[128];
 	char phys[64];
 	struct work_struct init;
@@ -216,6 +221,7 @@ static int pegasus_open(struct input_dev *dev)
 	if (error)
 		return error;
 
+	mutex_lock(&pegasus->pm_mutex);
 	pegasus->irq->dev = pegasus->usbdev;
 	if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
 		error = -EIO;
@@ -226,12 +232,14 @@ static int pegasus_open(struct input_dev *dev)
 	if (error)
 		goto err_kill_urb;
 
+	mutex_unlock(&pegasus->pm_mutex);
 	return 0;
 
 err_kill_urb:
 	usb_kill_urb(pegasus->irq);
 	cancel_work_sync(&pegasus->init);
 err_autopm_put:
+	mutex_unlock(&pegasus->pm_mutex);
 	usb_autopm_put_interface(pegasus->intf);
 	return error;
 }
@@ -240,8 +248,11 @@ static void pegasus_close(struct input_dev *dev)
 {
 	struct pegasus *pegasus = input_get_drvdata(dev);
 
+	mutex_lock(&pegasus->pm_mutex);
 	usb_kill_urb(pegasus->irq);
 	cancel_work_sync(&pegasus->init);
+	mutex_unlock(&pegasus->pm_mutex);
+
 	usb_autopm_put_interface(pegasus->intf);
 }
 
@@ -274,6 +285,8 @@ static int pegasus_probe(struct usb_interface *intf,
 		goto err_free_mem;
 	}
 
+	mutex_init(&pegasus->pm_mutex);
+
 	pegasus->usbdev = dev;
 	pegasus->dev = input_dev;
 	pegasus->intf = intf;
@@ -388,10 +401,10 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
 {
 	struct pegasus *pegasus = usb_get_intfdata(intf);
 
-	mutex_lock(&pegasus->dev->mutex);
+	mutex_lock(&pegasus->pm_mutex);
 	usb_kill_urb(pegasus->irq);
 	cancel_work_sync(&pegasus->init);
-	mutex_unlock(&pegasus->dev->mutex);
+	mutex_unlock(&pegasus->pm_mutex);
 
 	return 0;
 }
@@ -401,10 +414,10 @@ static int pegasus_resume(struct usb_interface *intf)
 	struct pegasus *pegasus = usb_get_intfdata(intf);
 	int retval = 0;
 
-	mutex_lock(&pegasus->dev->mutex);
+	mutex_lock(&pegasus->pm_mutex);
 	if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
 		retval = -EIO;
-	mutex_unlock(&pegasus->dev->mutex);
+	mutex_unlock(&pegasus->pm_mutex);
 
 	return retval;
 }
@@ -414,14 +427,14 @@ static int pegasus_reset_resume(struct usb_interface *intf)
 	struct pegasus *pegasus = usb_get_intfdata(intf);
 	int retval = 0;
 
-	mutex_lock(&pegasus->dev->mutex);
+	mutex_lock(&pegasus->pm_mutex);
 	if (pegasus->dev->users) {
 		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
 					  NOTETAKER_LED_MOUSE);
 		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
 			retval = -EIO;
 	}
-	mutex_unlock(&pegasus->dev->mutex);
+	mutex_unlock(&pegasus->pm_mutex);
 
 	return retval;
 }
-- 
2.16.2

[RESEND PATCH 4/6] input: pegasus_notetaker: do not rely on input_dev->users

[Marcus Folkesson]

If the device is unused and suspended, a call to open will cause the
device to autoresume through the call to usb_autopm_get_interface().

input_dev->users is already incremented by the input subsystem,
therefore this expression will always be evaluated to true:

	if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
		retval = -EIO;

The same URB will then be fail when resubmitted in pegasus_open().

Introduce pegasus->is_open to keep track of the state instead.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/input/tablet/pegasus_notetaker.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index 9ab1ed5e20e7..74c93e09c337 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -80,6 +80,7 @@ struct pegasus {
 
 	/* serialize access to open/suspend */
 	struct mutex pm_mutex;
+	bool is_open;
 
 	char name[128];
 	char phys[64];
@@ -232,6 +233,7 @@ static int pegasus_open(struct input_dev *dev)
 	if (error)
 		goto err_kill_urb;
 
+	pegasus->is_open = 1;
 	mutex_unlock(&pegasus->pm_mutex);
 	return 0;
 
@@ -251,6 +253,7 @@ static void pegasus_close(struct input_dev *dev)
 	mutex_lock(&pegasus->pm_mutex);
 	usb_kill_urb(pegasus->irq);
 	cancel_work_sync(&pegasus->init);
+	pegasus->is_open = 0;
 	mutex_unlock(&pegasus->pm_mutex);
 
 	usb_autopm_put_interface(pegasus->intf);
@@ -415,7 +418,7 @@ static int pegasus_resume(struct usb_interface *intf)
 	int retval = 0;
 
 	mutex_lock(&pegasus->pm_mutex);
-	if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
+	if (pegasus->is_open && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
 		retval = -EIO;
 	mutex_unlock(&pegasus->pm_mutex);
 
@@ -428,7 +431,7 @@ static int pegasus_reset_resume(struct usb_interface *intf)
 	int retval = 0;
 
 	mutex_lock(&pegasus->pm_mutex);
-	if (pegasus->dev->users) {
+	if (pegasus->is_open) {
 		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
 					  NOTETAKER_LED_MOUSE);
 		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
-- 
2.16.2

[RESEND PATCH 5/6] input: usbtouchscreen: fix deadlock in autosuspend

[Marcus Folkesson]

usb_autopm_get_interface() that is called in usbtouch_open() does an
autoresume if the device is suspended.

input_dev->mutex used in usbtouch_resume() is in this case already
taken by the input subsystem and will cause a deadlock.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/input/touchscreen/usbtouchscreen.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
index 2c41107240de..e964658203d8 100644
--- a/drivers/input/touchscreen/usbtouchscreen.c
+++ b/drivers/input/touchscreen/usbtouchscreen.c
@@ -54,6 +54,7 @@
 #include <linux/usb.h>
 #include <linux/usb/input.h>
 #include <linux/hid.h>
+#include <linux/mutex.h>
 
 
 #define DRIVER_VERSION		"v0.6"
@@ -112,6 +113,7 @@ struct usbtouch_usb {
 	struct usb_interface *interface;
 	struct input_dev *input;
 	struct usbtouch_device_info *type;
+	struct mutex pm_mutex;  /* serialize access to open/suspend */
 	char name[128];
 	char phys[64];
 	void *priv;
@@ -1455,6 +1457,7 @@ static int usbtouch_open(struct input_dev *input)
 	if (r < 0)
 		goto out;
 
+	mutex_lock(&usbtouch->pm_mutex);
 	if (!usbtouch->type->irq_always) {
 		if (usb_submit_urb(usbtouch->irq, GFP_KERNEL)) {
 			r = -EIO;
@@ -1464,6 +1467,7 @@ static int usbtouch_open(struct input_dev *input)
 
 	usbtouch->interface->needs_remote_wakeup = 1;
 out_put:
+	mutex_unlock(&usbtouch->pm_mutex);
 	usb_autopm_put_interface(usbtouch->interface);
 out:
 	return r;
@@ -1474,8 +1478,11 @@ static void usbtouch_close(struct input_dev *input)
 	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
 	int r;
 
+	mutex_lock(&usbtouch->pm_mutex);
 	if (!usbtouch->type->irq_always)
 		usb_kill_urb(usbtouch->irq);
+	mutex_lock(&usbtouch->pm_mutex);
+
 	r = usb_autopm_get_interface(usbtouch->interface);
 	usbtouch->interface->needs_remote_wakeup = 0;
 	if (!r)
@@ -1498,10 +1505,10 @@ static int usbtouch_resume(struct usb_interface *intf)
 	struct input_dev *input = usbtouch->input;
 	int result = 0;
 
-	mutex_lock(&input->mutex);
+	mutex_lock(&usbtouch->pm_mutex);
 	if (input->users || usbtouch->type->irq_always)
 		result = usb_submit_urb(usbtouch->irq, GFP_NOIO);
-	mutex_unlock(&input->mutex);
+	mutex_unlock(&usbtouch->pm_mutex);
 
 	return result;
 }
@@ -1524,10 +1531,10 @@ static int usbtouch_reset_resume(struct usb_interface *intf)
 	}
 
 	/* restart IO if needed */
-	mutex_lock(&input->mutex);
+	mutex_lock(&usbtouch->pm_mutex);
 	if (input->users)
 		err = usb_submit_urb(usbtouch->irq, GFP_NOIO);
-	mutex_unlock(&input->mutex);
+	mutex_unlock(&usbtouch->pm_mutex);
 
 	return err;
 }
-- 
2.16.2

[RESEND PATCH 6/6] input: usbtouchscreen: do not rely on input_dev->users

[Marcus Folkesson]

If the device is unused and suspended, a call to open will cause the
device to autoresume through the call to usb_autopm_get_interface().

input_dev->users is already incremented by the input subsystem,
therefore this expression will always be evaluated to true:

	if (input->users || usbtouch->type->irq_always)
		result = usb_submit_urb(usbtouch->irq, GFP_NOIO);

The same URB will then be fail when resubmitted in usbtouch_open().

Introduce usbtouch->is_open to keep track of the state instead.

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 drivers/input/touchscreen/usbtouchscreen.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
index e964658203d8..0356ad792e40 100644
--- a/drivers/input/touchscreen/usbtouchscreen.c
+++ b/drivers/input/touchscreen/usbtouchscreen.c
@@ -114,6 +114,7 @@ struct usbtouch_usb {
 	struct input_dev *input;
 	struct usbtouch_device_info *type;
 	struct mutex pm_mutex;  /* serialize access to open/suspend */
+	bool is_open;
 	char name[128];
 	char phys[64];
 	void *priv;
@@ -1466,6 +1467,7 @@ static int usbtouch_open(struct input_dev *input)
 	}
 
 	usbtouch->interface->needs_remote_wakeup = 1;
+	usbtouch->is_open = 1;
 out_put:
 	mutex_unlock(&usbtouch->pm_mutex);
 	usb_autopm_put_interface(usbtouch->interface);
@@ -1481,6 +1483,7 @@ static void usbtouch_close(struct input_dev *input)
 	mutex_lock(&usbtouch->pm_mutex);
 	if (!usbtouch->type->irq_always)
 		usb_kill_urb(usbtouch->irq);
+	usbtouch->is_open = 0;
 	mutex_lock(&usbtouch->pm_mutex);
 
 	r = usb_autopm_get_interface(usbtouch->interface);
@@ -1502,11 +1505,10 @@ static int usbtouch_suspend
 static int usbtouch_resume(struct usb_interface *intf)
 {
 	struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
-	struct input_dev *input = usbtouch->input;
 	int result = 0;
 
 	mutex_lock(&usbtouch->pm_mutex);
-	if (input->users || usbtouch->type->irq_always)
+	if (usbtouch->is_open || usbtouch->type->irq_always)
 		result = usb_submit_urb(usbtouch->irq, GFP_NOIO);
 	mutex_unlock(&usbtouch->pm_mutex);
 
@@ -1516,7 +1518,6 @@ static int usbtouch_resume(struct usb_interface *intf)
 static int usbtouch_reset_resume(struct usb_interface *intf)
 {
 	struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
-	struct input_dev *input = usbtouch->input;
 	int err = 0;
 
 	/* reinit the device */
@@ -1532,7 +1533,7 @@ static int usbtouch_reset_resume(struct usb_interface *intf)
 
 	/* restart IO if needed */
 	mutex_lock(&usbtouch->pm_mutex);
-	if (input->users)
+	if (usbtouch->is_open)
 		err = usb_submit_urb(usbtouch->irq, GFP_NOIO);
 	mutex_unlock(&usbtouch->pm_mutex);
 
-- 
2.16.2

Re: Fix deadlocks in autosuspend

[Marcus Folkesson]

[-- Attachment #1: Type: text/plain, Size: 3659 bytes --]

Ping.

Would someone please have a look?

Thanks,
Marcus Folkesson

On Wed, Feb 28, 2018 at 02:37:57PM +0100, Marcus Folkesson wrote:
> Hello,
> 
> I have not recieved any feedback on these so I resend them.
> 
> I got this deadlock on my own driver (pxrc) when using the same
> construction.
> 
> Please have a look
> 
> Here is a clip from 
> [PATCH v3] input: pxrc: new driver for PhoenixRC Flight Controller Adapter [1]
> that describes the problem.
> 
> -------------------------------8<----------------------------------------------
> Also, I think we have a deadlock in the synaptics_usb driver.
> 
> When the device is suspended and someone is open the device, the input
> subsystem will call input_open_device() which takes the
> input_dev->mutex and then call input_dev->open().
> 
> synusb_open() has a call to usb_autopm_get_interface() which will
> result in a call to the registered resume-function if the device is
> suspended. (see Documentation/driver-api/usb/power-manaement.rst).
> 
> In the case of snaptics_usb, it will take the input_dev->mutex in the
> resume function.
> 
> I have no synaptic mouse, but tested to put the same code into my
> driver just to confirm, and got the following dump:
> 
> [ 9215.626476] INFO: task input-events:8590 blocked for more than 120 seconds.
> [ 9215.626495]       Not tainted 4.15.0-rc8-ARCH+ #6
> [ 9215.626500] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [ 9215.626507] input-events    D    0  8590   4394 0x00000004
> [ 9215.626520] Call Trace:
> [ 9215.626546]  ? __schedule+0x236/0x850
> [ 9215.626559]  schedule+0x2f/0x90
> [ 9215.626569]  schedule_preempt_disabled+0x11/0x20
> [ 9215.626579]  __mutex_lock.isra.0+0x1aa/0x520
> [ 9215.626609]  ? usb_runtime_suspend+0x70/0x70 [usbcore]
> [ 9215.626622]  ? pxrc_resume+0x37/0x70 [pxrc]
> [ 9215.626632]  pxrc_resume+0x37/0x70 [pxrc]
> [ 9215.626655]  usb_resume_interface.isra.2+0x39/0xe0 [usbcore]
> [ 9215.626676]  usb_resume_both+0xd2/0x120 [usbcore]
> [ 9215.626688]  __rpm_callback+0xb6/0x1f0
> [ 9215.626699]  rpm_callback+0x1f/0x70
> [ 9215.626718]  ? usb_runtime_suspend+0x70/0x70 [usbcore]
> [ 9215.626726]  rpm_resume+0x4e2/0x7f0
> [ 9215.626737]  rpm_resume+0x582/0x7f0
> [ 9215.626749]  __pm_runtime_resume+0x3a/0x50
> [ 9215.626767]  usb_autopm_get_interface+0x1d/0x50 [usbcore]
> [ 9215.626780]  pxrc_open+0x17/0x8d [pxrc]
> [ 9215.626791]  input_open_device+0x70/0xa0
> [ 9215.626804]  evdev_open+0x183/0x1c0 [evdev]
> [ 9215.626819]  chrdev_open+0xa0/0x1b0
> [ 9215.626830]  ? cdev_put.part.1+0x20/0x20
> [ 9215.626840]  do_dentry_open+0x1ad/0x2c0
> [ 9215.626855]  path_openat+0x576/0x1300
> [ 9215.626868]  ? alloc_set_pte+0x22c/0x520
> [ 9215.626883]  ? filemap_map_pages+0x19b/0x340
> [ 9215.626893]  do_filp_open+0x9b/0x110
> [ 9215.626908]  ? __check_object_size+0x9d/0x190
> [ 9215.626920]  ? __alloc_fd+0xaf/0x160
> [ 9215.626931]  ? do_sys_open+0x1bd/0x250
> [ 9215.626942]  do_sys_open+0x1bd/0x250
> [ 9215.626956]  entry_SYSCALL_64_fastpath+0x20/0x83
> [ 9215.626967] RIP: 0033:0x7fbf6358f7ae
> 
> 
> tablet/pegasus_notetaker.c and touchscreen/usbtouchscreen.c has the same
> construction (taking input_dev->mutex in resume/suspend and call
> usb_autopm_get_interface() in open()).
> 
> I will create a separate "pm_mutex" to use instead of input_dev->mutex
> to get rid of the lockups in those drivers 
> 
> -------------------------------8<----------------------------------------------
> 
> 
> [1] https://lkml.org/lkml/2018/1/20/191
> 
> Thanks,
> 
> Best regards
> Marcus Folkesson
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RESEND PATCH 1/6] input: synaptics_usb: fix deadlock in autosuspend

[Dmitry Torokhov]

On Wed, Feb 28, 2018 at 02:37:58PM +0100, Marcus Folkesson wrote:
> usb_autopm_get_interface() that is called in synusb_open() does an
> autoresume if the device is suspended.
> 
> input_dev->mutex used in synusb_resume() is in this case already
> taken by the input subsystem and will cause a deadlock.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Applied, thank you.

> ---
>  drivers/input/mouse/synaptics_usb.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
> index cb7d15d826d0..2c66913cf5a2 100644
> --- a/drivers/input/mouse/synaptics_usb.c
> +++ b/drivers/input/mouse/synaptics_usb.c
> @@ -82,6 +82,9 @@ struct synusb {
>  	struct urb *urb;
>  	unsigned char *data;
>  
> +	/* serialize access to open/suspend */
> +	struct mutex pm_mutex;
> +
>  	/* input device related data structures */
>  	struct input_dev *input;
>  	char name[128];
> @@ -252,6 +255,7 @@ static int synusb_open(struct input_dev *dev)
>  		return retval;
>  	}
>  
> +	mutex_lock(&synusb->pm_mutex);
>  	retval = usb_submit_urb(synusb->urb, GFP_KERNEL);
>  	if (retval) {
>  		dev_err(&synusb->intf->dev,
> @@ -264,6 +268,7 @@ static int synusb_open(struct input_dev *dev)
>  	synusb->intf->needs_remote_wakeup = 1;
>  
>  out:
> +	mutex_unlock(&synusb->pm_mutex);
>  	usb_autopm_put_interface(synusb->intf);
>  	return retval;
>  }
> @@ -275,8 +280,10 @@ static void synusb_close(struct input_dev *dev)
>  
>  	autopm_error = usb_autopm_get_interface(synusb->intf);
>  
> +	mutex_lock(&synusb->pm_mutex);
>  	usb_kill_urb(synusb->urb);
>  	synusb->intf->needs_remote_wakeup = 0;
> +	mutex_unlock(&synusb->pm_mutex);
>  
>  	if (!autopm_error)
>  		usb_autopm_put_interface(synusb->intf);
> @@ -315,6 +322,7 @@ static int synusb_probe(struct usb_interface *intf,
>  	synusb->udev = udev;
>  	synusb->intf = intf;
>  	synusb->input = input_dev;
> +	mutex_init(&synusb->pm_mutex);
>  
>  	synusb->flags = id->driver_info;
>  	if (synusb->flags & SYNUSB_COMBO) {
> @@ -466,11 +474,10 @@ static void synusb_disconnect(struct usb_interface *intf)
>  static int synusb_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>  	struct synusb *synusb = usb_get_intfdata(intf);
> -	struct input_dev *input_dev = synusb->input;
>  
> -	mutex_lock(&input_dev->mutex);
> +	mutex_lock(&synusb->pm_mutex);
>  	usb_kill_urb(synusb->urb);
> -	mutex_unlock(&input_dev->mutex);
> +	mutex_unlock(&synusb->pm_mutex);
>  
>  	return 0;
>  }
> @@ -481,14 +488,14 @@ static int synusb_resume(struct usb_interface *intf)
>  	struct input_dev *input_dev = synusb->input;
>  	int retval = 0;
>  
> -	mutex_lock(&input_dev->mutex);
> +	mutex_lock(&synusb->pm_mutex);
>  
>  	if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
>  	    usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
>  		retval = -EIO;
>  	}
>  
> -	mutex_unlock(&input_dev->mutex);
> +	mutex_unlock(&synusb->pm_mutex);
>  
>  	return retval;
>  }
> @@ -496,9 +503,8 @@ static int synusb_resume(struct usb_interface *intf)
>  static int synusb_pre_reset(struct usb_interface *intf)
>  {
>  	struct synusb *synusb = usb_get_intfdata(intf);
> -	struct input_dev *input_dev = synusb->input;
>  
> -	mutex_lock(&input_dev->mutex);
> +	mutex_lock(&synusb->pm_mutex);
>  	usb_kill_urb(synusb->urb);
>  
>  	return 0;
> @@ -515,7 +521,7 @@ static int synusb_post_reset(struct usb_interface *intf)
>  		retval = -EIO;
>  	}
>  
> -	mutex_unlock(&input_dev->mutex);
> +	mutex_unlock(&synusb->pm_mutex);
>  
>  	return retval;
>  }
> -- 
> 2.16.2
> 

-- 
Dmitry

Re: [RESEND PATCH 2/6] input: synaptics_usb: do not rely on input_dev->users

[Dmitry Torokhov]

On Wed, Feb 28, 2018 at 02:37:59PM +0100, Marcus Folkesson wrote:
> If the device is unused and suspended, a call to open will cause the
> device to autoresume through the call to usb_autopm_get_interface().
> 
> input_dev->users is already incremented by the input subsystem,
> therefore this expression will always be evaluated to true:
> 
> 	if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> 	    usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
> 		retval = -EIO;
> 	}
> 
> The same URB will then be fail when resubmitted in synusb_open().
> 
> Introduce synusb->is_open to keep track of the state instead.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Applied, thank you.

> ---
>  drivers/input/mouse/synaptics_usb.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/mouse/synaptics_usb.c b/drivers/input/mouse/synaptics_usb.c
> index 2c66913cf5a2..e2b726751220 100644
> --- a/drivers/input/mouse/synaptics_usb.c
> +++ b/drivers/input/mouse/synaptics_usb.c
> @@ -84,6 +84,7 @@ struct synusb {
>  
>  	/* serialize access to open/suspend */
>  	struct mutex pm_mutex;
> +	bool is_open;
>  
>  	/* input device related data structures */
>  	struct input_dev *input;
> @@ -266,6 +267,7 @@ static int synusb_open(struct input_dev *dev)
>  	}
>  
>  	synusb->intf->needs_remote_wakeup = 1;
> +	synusb->is_open = 1;
>  
>  out:
>  	mutex_unlock(&synusb->pm_mutex);
> @@ -283,6 +285,7 @@ static void synusb_close(struct input_dev *dev)
>  	mutex_lock(&synusb->pm_mutex);
>  	usb_kill_urb(synusb->urb);
>  	synusb->intf->needs_remote_wakeup = 0;
> +	synusb->is_open = 0;
>  	mutex_unlock(&synusb->pm_mutex);
>  
>  	if (!autopm_error)
> @@ -485,12 +488,11 @@ static int synusb_suspend(struct usb_interface *intf, pm_message_t message)
>  static int synusb_resume(struct usb_interface *intf)
>  {
>  	struct synusb *synusb = usb_get_intfdata(intf);
> -	struct input_dev *input_dev = synusb->input;
>  	int retval = 0;
>  
>  	mutex_lock(&synusb->pm_mutex);
>  
> -	if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> +	if ((synusb->is_open || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
>  	    usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
>  		retval = -EIO;
>  	}
> @@ -513,10 +515,9 @@ static int synusb_pre_reset(struct usb_interface *intf)
>  static int synusb_post_reset(struct usb_interface *intf)
>  {
>  	struct synusb *synusb = usb_get_intfdata(intf);
> -	struct input_dev *input_dev = synusb->input;
>  	int retval = 0;
>  
> -	if ((input_dev->users || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
> +	if ((synusb->is_open || (synusb->flags & SYNUSB_IO_ALWAYS)) &&
>  	    usb_submit_urb(synusb->urb, GFP_NOIO) < 0) {
>  		retval = -EIO;
>  	}
> -- 
> 2.16.2
> 

-- 
Dmitry

Re: [RESEND PATCH 3/6] input: pagasus_notetaker: fix deadlock in autosuspend

[Dmitry Torokhov]

On Wed, Feb 28, 2018 at 02:38:00PM +0100, Marcus Folkesson wrote:
> usb_autopm_get_interface() that is called in pegasus_open() does an
> autoresume if the device is suspended.
> 
> input_dev->mutex used in pegasus_resume() is in this case already
> taken by the input subsystem and will cause a deadlock.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Applied, thank you.

> ---
>  drivers/input/tablet/pegasus_notetaker.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index 47de5a81172f..9ab1ed5e20e7 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -41,6 +41,7 @@
>  #include <linux/usb/input.h>
>  #include <linux/slab.h>
>  #include <linux/workqueue.h>
> +#include <linux/mutex.h>
>  
>  /* USB HID defines */
>  #define USB_REQ_GET_REPORT		0x01
> @@ -76,6 +77,10 @@ struct pegasus {
>  	struct usb_device *usbdev;
>  	struct usb_interface *intf;
>  	struct urb *irq;
> +
> +	/* serialize access to open/suspend */
> +	struct mutex pm_mutex;
> +
>  	char name[128];
>  	char phys[64];
>  	struct work_struct init;
> @@ -216,6 +221,7 @@ static int pegasus_open(struct input_dev *dev)
>  	if (error)
>  		return error;
>  
> +	mutex_lock(&pegasus->pm_mutex);
>  	pegasus->irq->dev = pegasus->usbdev;
>  	if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
>  		error = -EIO;
> @@ -226,12 +232,14 @@ static int pegasus_open(struct input_dev *dev)
>  	if (error)
>  		goto err_kill_urb;
>  
> +	mutex_unlock(&pegasus->pm_mutex);
>  	return 0;
>  
>  err_kill_urb:
>  	usb_kill_urb(pegasus->irq);
>  	cancel_work_sync(&pegasus->init);
>  err_autopm_put:
> +	mutex_unlock(&pegasus->pm_mutex);
>  	usb_autopm_put_interface(pegasus->intf);
>  	return error;
>  }
> @@ -240,8 +248,11 @@ static void pegasus_close(struct input_dev *dev)
>  {
>  	struct pegasus *pegasus = input_get_drvdata(dev);
>  
> +	mutex_lock(&pegasus->pm_mutex);
>  	usb_kill_urb(pegasus->irq);
>  	cancel_work_sync(&pegasus->init);
> +	mutex_unlock(&pegasus->pm_mutex);
> +
>  	usb_autopm_put_interface(pegasus->intf);
>  }
>  
> @@ -274,6 +285,8 @@ static int pegasus_probe(struct usb_interface *intf,
>  		goto err_free_mem;
>  	}
>  
> +	mutex_init(&pegasus->pm_mutex);
> +
>  	pegasus->usbdev = dev;
>  	pegasus->dev = input_dev;
>  	pegasus->intf = intf;
> @@ -388,10 +401,10 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
>  {
>  	struct pegasus *pegasus = usb_get_intfdata(intf);
>  
> -	mutex_lock(&pegasus->dev->mutex);
> +	mutex_lock(&pegasus->pm_mutex);
>  	usb_kill_urb(pegasus->irq);
>  	cancel_work_sync(&pegasus->init);
> -	mutex_unlock(&pegasus->dev->mutex);
> +	mutex_unlock(&pegasus->pm_mutex);
>  
>  	return 0;
>  }
> @@ -401,10 +414,10 @@ static int pegasus_resume(struct usb_interface *intf)
>  	struct pegasus *pegasus = usb_get_intfdata(intf);
>  	int retval = 0;
>  
> -	mutex_lock(&pegasus->dev->mutex);
> +	mutex_lock(&pegasus->pm_mutex);
>  	if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
>  		retval = -EIO;
> -	mutex_unlock(&pegasus->dev->mutex);
> +	mutex_unlock(&pegasus->pm_mutex);
>  
>  	return retval;
>  }
> @@ -414,14 +427,14 @@ static int pegasus_reset_resume(struct usb_interface *intf)
>  	struct pegasus *pegasus = usb_get_intfdata(intf);
>  	int retval = 0;
>  
> -	mutex_lock(&pegasus->dev->mutex);
> +	mutex_lock(&pegasus->pm_mutex);
>  	if (pegasus->dev->users) {
>  		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
>  					  NOTETAKER_LED_MOUSE);
>  		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
>  			retval = -EIO;
>  	}
> -	mutex_unlock(&pegasus->dev->mutex);
> +	mutex_unlock(&pegasus->pm_mutex);
>  
>  	return retval;
>  }
> -- 
> 2.16.2
> 

-- 
Dmitry

Re: [RESEND PATCH 4/6] input: pegasus_notetaker: do not rely on input_dev->users

[Dmitry Torokhov]

On Wed, Feb 28, 2018 at 02:38:01PM +0100, Marcus Folkesson wrote:
> If the device is unused and suspended, a call to open will cause the
> device to autoresume through the call to usb_autopm_get_interface().
> 
> input_dev->users is already incremented by the input subsystem,
> therefore this expression will always be evaluated to true:
> 
> 	if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> 		retval = -EIO;
> 
> The same URB will then be fail when resubmitted in pegasus_open().
> 
> Introduce pegasus->is_open to keep track of the state instead.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Applied, thank you.

> ---
>  drivers/input/tablet/pegasus_notetaker.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index 9ab1ed5e20e7..74c93e09c337 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -80,6 +80,7 @@ struct pegasus {
>  
>  	/* serialize access to open/suspend */
>  	struct mutex pm_mutex;
> +	bool is_open;
>  
>  	char name[128];
>  	char phys[64];
> @@ -232,6 +233,7 @@ static int pegasus_open(struct input_dev *dev)
>  	if (error)
>  		goto err_kill_urb;
>  
> +	pegasus->is_open = 1;
>  	mutex_unlock(&pegasus->pm_mutex);
>  	return 0;
>  
> @@ -251,6 +253,7 @@ static void pegasus_close(struct input_dev *dev)
>  	mutex_lock(&pegasus->pm_mutex);
>  	usb_kill_urb(pegasus->irq);
>  	cancel_work_sync(&pegasus->init);
> +	pegasus->is_open = 0;
>  	mutex_unlock(&pegasus->pm_mutex);
>  
>  	usb_autopm_put_interface(pegasus->intf);
> @@ -415,7 +418,7 @@ static int pegasus_resume(struct usb_interface *intf)
>  	int retval = 0;
>  
>  	mutex_lock(&pegasus->pm_mutex);
> -	if (pegasus->dev->users && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> +	if (pegasus->is_open && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
>  		retval = -EIO;
>  	mutex_unlock(&pegasus->pm_mutex);
>  
> @@ -428,7 +431,7 @@ static int pegasus_reset_resume(struct usb_interface *intf)
>  	int retval = 0;
>  
>  	mutex_lock(&pegasus->pm_mutex);
> -	if (pegasus->dev->users) {
> +	if (pegasus->is_open) {
>  		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
>  					  NOTETAKER_LED_MOUSE);
>  		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> -- 
> 2.16.2
> 

-- 
Dmitry

Re: [RESEND PATCH 5/6] input: usbtouchscreen: fix deadlock in autosuspend

[Dmitry Torokhov]

On Wed, Feb 28, 2018 at 02:38:02PM +0100, Marcus Folkesson wrote:
> usb_autopm_get_interface() that is called in usbtouch_open() does an
> autoresume if the device is suspended.
> 
> input_dev->mutex used in usbtouch_resume() is in this case already
> taken by the input subsystem and will cause a deadlock.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/usbtouchscreen.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
> index 2c41107240de..e964658203d8 100644
> --- a/drivers/input/touchscreen/usbtouchscreen.c
> +++ b/drivers/input/touchscreen/usbtouchscreen.c
> @@ -54,6 +54,7 @@
>  #include <linux/usb.h>
>  #include <linux/usb/input.h>
>  #include <linux/hid.h>
> +#include <linux/mutex.h>
>  
>  
>  #define DRIVER_VERSION		"v0.6"
> @@ -112,6 +113,7 @@ struct usbtouch_usb {
>  	struct usb_interface *interface;
>  	struct input_dev *input;
>  	struct usbtouch_device_info *type;
> +	struct mutex pm_mutex;  /* serialize access to open/suspend */
>  	char name[128];
>  	char phys[64];
>  	void *priv;
> @@ -1455,6 +1457,7 @@ static int usbtouch_open(struct input_dev *input)
>  	if (r < 0)
>  		goto out;
>  
> +	mutex_lock(&usbtouch->pm_mutex);
>  	if (!usbtouch->type->irq_always) {
>  		if (usb_submit_urb(usbtouch->irq, GFP_KERNEL)) {
>  			r = -EIO;
> @@ -1464,6 +1467,7 @@ static int usbtouch_open(struct input_dev *input)
>  
>  	usbtouch->interface->needs_remote_wakeup = 1;
>  out_put:
> +	mutex_unlock(&usbtouch->pm_mutex);
>  	usb_autopm_put_interface(usbtouch->interface);
>  out:
>  	return r;
> @@ -1474,8 +1478,11 @@ static void usbtouch_close(struct input_dev *input)
>  	struct usbtouch_usb *usbtouch = input_get_drvdata(input);
>  	int r;
>  
> +	mutex_lock(&usbtouch->pm_mutex);
>  	if (!usbtouch->type->irq_always)
>  		usb_kill_urb(usbtouch->irq);
> +	mutex_lock(&usbtouch->pm_mutex);
> +
>  	r = usb_autopm_get_interface(usbtouch->interface);
>  	usbtouch->interface->needs_remote_wakeup = 0;
>  	if (!r)
> @@ -1498,10 +1505,10 @@ static int usbtouch_resume(struct usb_interface *intf)
>  	struct input_dev *input = usbtouch->input;
>  	int result = 0;
>  
> -	mutex_lock(&input->mutex);
> +	mutex_lock(&usbtouch->pm_mutex);
>  	if (input->users || usbtouch->type->irq_always)
>  		result = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> -	mutex_unlock(&input->mutex);
> +	mutex_unlock(&usbtouch->pm_mutex);
>  
>  	return result;
>  }
> @@ -1524,10 +1531,10 @@ static int usbtouch_reset_resume(struct usb_interface *intf)
>  	}
>  
>  	/* restart IO if needed */
> -	mutex_lock(&input->mutex);
> +	mutex_lock(&usbtouch->pm_mutex);
>  	if (input->users)
>  		err = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> -	mutex_unlock(&input->mutex);
> +	mutex_unlock(&usbtouch->pm_mutex);
>  
>  	return err;
>  }
> -- 
> 2.16.2
> 

-- 
Dmitry

Re: [RESEND PATCH 6/6] input: usbtouchscreen: do not rely on input_dev->users

[Dmitry Torokhov]

On Wed, Feb 28, 2018 at 02:38:03PM +0100, Marcus Folkesson wrote:
> If the device is unused and suspended, a call to open will cause the
> device to autoresume through the call to usb_autopm_get_interface().
> 
> input_dev->users is already incremented by the input subsystem,
> therefore this expression will always be evaluated to true:
> 
> 	if (input->users || usbtouch->type->irq_always)
> 		result = usb_submit_urb(usbtouch->irq, GFP_NOIO);
> 
> The same URB will then be fail when resubmitted in usbtouch_open().
> 
> Introduce usbtouch->is_open to keep track of the state instead.
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Applied, thank you.

> ---
>  drivers/input/touchscreen/usbtouchscreen.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
> index e964658203d8..0356ad792e40 100644
> --- a/drivers/input/touchscreen/usbtouchscreen.c
> +++ b/drivers/input/touchscreen/usbtouchscreen.c
> @@ -114,6 +114,7 @@ struct usbtouch_usb {
>  	struct input_dev *input;
>  	struct usbtouch_device_info *type;
>  	struct mutex pm_mutex;  /* serialize access to open/suspend */
> +	bool is_open;
>  	char name[128];
>  	char phys[64];
>  	void *priv;
> @@ -1466,6 +1467,7 @@ static int usbtouch_open(struct input_dev *input)
>  	}
>  
>  	usbtouch->interface->needs_remote_wakeup = 1;
> +	usbtouch->is_open = 1;
>  out_put:
>  	mutex_unlock(&usbtouch->pm_mutex);
>  	usb_autopm_put_interface(usbtouch->interface);
> @@ -1481,6 +1483,7 @@ static void usbtouch_close(struct input_dev *input)
>  	mutex_lock(&usbtouch->pm_mutex);
>  	if (!usbtouch->type->irq_always)
>  		usb_kill_urb(usbtouch->irq);
> +	usbtouch->is_open = 0;
>  	mutex_lock(&usbtouch->pm_mutex);
>  
>  	r = usb_autopm_get_interface(usbtouch->interface);
> @@ -1502,11 +1505,10 @@ static int usbtouch_suspend
>  static int usbtouch_resume(struct usb_interface *intf)
>  {
>  	struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
> -	struct input_dev *input = usbtouch->input;
>  	int result = 0;
>  
>  	mutex_lock(&usbtouch->pm_mutex);
> -	if (input->users || usbtouch->type->irq_always)
> +	if (usbtouch->is_open || usbtouch->type->irq_always)
>  		result = usb_submit_urb(usbtouch->irq, GFP_NOIO);
>  	mutex_unlock(&usbtouch->pm_mutex);
>  
> @@ -1516,7 +1518,6 @@ static int usbtouch_resume(struct usb_interface *intf)
>  static int usbtouch_reset_resume(struct usb_interface *intf)
>  {
>  	struct usbtouch_usb *usbtouch = usb_get_intfdata(intf);
> -	struct input_dev *input = usbtouch->input;
>  	int err = 0;
>  
>  	/* reinit the device */
> @@ -1532,7 +1533,7 @@ static int usbtouch_reset_resume(struct usb_interface *intf)
>  
>  	/* restart IO if needed */
>  	mutex_lock(&usbtouch->pm_mutex);
> -	if (input->users)
> +	if (usbtouch->is_open)
>  		err = usb_submit_urb(usbtouch->irq, GFP_NOIO);
>  	mutex_unlock(&usbtouch->pm_mutex);
>  
> -- 
> 2.16.2
> 

-- 
Dmitry