Fix deadlocks in autosuspend

14 messages, 2 authors, 2018-03-17 · open the first message on its own page

Fix deadlocks in autosuspend

From: Marcus Folkesson <marcus.folkesson@gmail.com>
Date: 2018-02-28 13:38:15

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

From: Marcus Folkesson <marcus.folkesson@gmail.com>
Date: 2018-02-28 13:38:18

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 3/6] input: pagasus_notetaker: fix deadlock in autosuspend

From: Marcus Folkesson <marcus.folkesson@gmail.com>
Date: 2018-02-28 13:38:20

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 5/6] input: usbtouchscreen: fix deadlock in autosuspend

From: Marcus Folkesson <marcus.folkesson@gmail.com>
Date: 2018-02-28 13:38:23

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

From: Marcus Folkesson <marcus.folkesson@gmail.com>
Date: 2018-02-28 13:38:33

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

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

From: Marcus Folkesson <marcus.folkesson@gmail.com>
Date: 2018-02-28 13:39:00

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 2/6] input: synaptics_usb: do not rely on input_dev->users

From: Marcus Folkesson <marcus.folkesson@gmail.com>
Date: 2018-02-28 13:39:25

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

Re: Fix deadlocks in autosuspend

From: Marcus Folkesson <marcus.folkesson@gmail.com>
Date: 2018-03-16 13:41:44

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

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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2018-03-17 18:10:52

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.

quoted hunk

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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2018-03-17 18:11:02

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.

quoted hunk

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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2018-03-17 18:11:06

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.

quoted hunk

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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2018-03-17 18:11:13

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.

quoted hunk

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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2018-03-17 18:11:21

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.

quoted hunk

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

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date: 2018-03-17 18:11:27

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.

quoted hunk

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help