* [RESEND PATCH 1/6] input: synaptics_usb: fix deadlock in autosuspend
2018-02-28 13:37 Fix deadlocks in autosuspend Marcus Folkesson
@ 2018-02-28 13:37 ` Marcus Folkesson
2018-03-17 18:10 ` Dmitry Torokhov
2018-02-28 13:37 ` [RESEND PATCH 2/6] input: synaptics_usb: do not rely on input_dev->users Marcus Folkesson
` (5 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Marcus Folkesson @ 2018-02-28 13:37 UTC (permalink / raw)
To: Dmitry Torokhov, Arvind Yadav; +Cc: linux-input, linux-kernel, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RESEND PATCH 1/6] input: synaptics_usb: fix deadlock in autosuspend
2018-02-28 13:37 ` [RESEND PATCH 1/6] input: synaptics_usb: fix deadlock " Marcus Folkesson
@ 2018-03-17 18:10 ` Dmitry Torokhov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Torokhov @ 2018-03-17 18:10 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RESEND PATCH 2/6] input: synaptics_usb: do not rely on input_dev->users
2018-02-28 13:37 Fix deadlocks in autosuspend Marcus Folkesson
2018-02-28 13:37 ` [RESEND PATCH 1/6] input: synaptics_usb: fix deadlock " Marcus Folkesson
@ 2018-02-28 13:37 ` Marcus Folkesson
2018-03-17 18:10 ` Dmitry Torokhov
2018-02-28 13:38 ` [RESEND PATCH 3/6] input: pagasus_notetaker: fix deadlock in autosuspend Marcus Folkesson
` (4 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Marcus Folkesson @ 2018-02-28 13:37 UTC (permalink / raw)
To: Dmitry Torokhov, Arvind Yadav; +Cc: linux-input, linux-kernel, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RESEND PATCH 2/6] input: synaptics_usb: do not rely on input_dev->users
2018-02-28 13:37 ` [RESEND PATCH 2/6] input: synaptics_usb: do not rely on input_dev->users Marcus Folkesson
@ 2018-03-17 18:10 ` Dmitry Torokhov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Torokhov @ 2018-03-17 18:10 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RESEND PATCH 3/6] input: pagasus_notetaker: fix deadlock in autosuspend
2018-02-28 13:37 Fix deadlocks in autosuspend Marcus Folkesson
2018-02-28 13:37 ` [RESEND PATCH 1/6] input: synaptics_usb: fix deadlock " Marcus Folkesson
2018-02-28 13:37 ` [RESEND PATCH 2/6] input: synaptics_usb: do not rely on input_dev->users Marcus Folkesson
@ 2018-02-28 13:38 ` Marcus Folkesson
2018-03-17 18:10 ` Dmitry Torokhov
2018-02-28 13:38 ` [RESEND PATCH 4/6] input: pegasus_notetaker: do not rely on input_dev->users Marcus Folkesson
` (3 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Marcus Folkesson @ 2018-02-28 13:38 UTC (permalink / raw)
To: Dmitry Torokhov, Arvind Yadav; +Cc: linux-input, linux-kernel, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RESEND PATCH 3/6] input: pagasus_notetaker: fix deadlock in autosuspend
2018-02-28 13:38 ` [RESEND PATCH 3/6] input: pagasus_notetaker: fix deadlock in autosuspend Marcus Folkesson
@ 2018-03-17 18:10 ` Dmitry Torokhov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Torokhov @ 2018-03-17 18:10 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RESEND PATCH 4/6] input: pegasus_notetaker: do not rely on input_dev->users
2018-02-28 13:37 Fix deadlocks in autosuspend Marcus Folkesson
` (2 preceding siblings ...)
2018-02-28 13:38 ` [RESEND PATCH 3/6] input: pagasus_notetaker: fix deadlock in autosuspend Marcus Folkesson
@ 2018-02-28 13:38 ` Marcus Folkesson
2018-03-17 18:11 ` Dmitry Torokhov
2018-02-28 13:38 ` [RESEND PATCH 5/6] input: usbtouchscreen: fix deadlock in autosuspend Marcus Folkesson
` (2 subsequent siblings)
6 siblings, 1 reply; 14+ messages in thread
From: Marcus Folkesson @ 2018-02-28 13:38 UTC (permalink / raw)
To: Dmitry Torokhov, Arvind Yadav; +Cc: linux-input, linux-kernel, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RESEND PATCH 4/6] input: pegasus_notetaker: do not rely on input_dev->users
2018-02-28 13:38 ` [RESEND PATCH 4/6] input: pegasus_notetaker: do not rely on input_dev->users Marcus Folkesson
@ 2018-03-17 18:11 ` Dmitry Torokhov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Torokhov @ 2018-03-17 18:11 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RESEND PATCH 5/6] input: usbtouchscreen: fix deadlock in autosuspend
2018-02-28 13:37 Fix deadlocks in autosuspend Marcus Folkesson
` (3 preceding siblings ...)
2018-02-28 13:38 ` [RESEND PATCH 4/6] input: pegasus_notetaker: do not rely on input_dev->users Marcus Folkesson
@ 2018-02-28 13:38 ` Marcus Folkesson
2018-03-17 18:11 ` Dmitry Torokhov
2018-02-28 13:38 ` [RESEND PATCH 6/6] input: usbtouchscreen: do not rely on input_dev->users Marcus Folkesson
2018-03-16 13:41 ` Fix deadlocks in autosuspend Marcus Folkesson
6 siblings, 1 reply; 14+ messages in thread
From: Marcus Folkesson @ 2018-02-28 13:38 UTC (permalink / raw)
To: Dmitry Torokhov, Arvind Yadav; +Cc: linux-input, linux-kernel, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RESEND PATCH 5/6] input: usbtouchscreen: fix deadlock in autosuspend
2018-02-28 13:38 ` [RESEND PATCH 5/6] input: usbtouchscreen: fix deadlock in autosuspend Marcus Folkesson
@ 2018-03-17 18:11 ` Dmitry Torokhov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Torokhov @ 2018-03-17 18:11 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RESEND PATCH 6/6] input: usbtouchscreen: do not rely on input_dev->users
2018-02-28 13:37 Fix deadlocks in autosuspend Marcus Folkesson
` (4 preceding siblings ...)
2018-02-28 13:38 ` [RESEND PATCH 5/6] input: usbtouchscreen: fix deadlock in autosuspend Marcus Folkesson
@ 2018-02-28 13:38 ` Marcus Folkesson
2018-03-17 18:11 ` Dmitry Torokhov
2018-03-16 13:41 ` Fix deadlocks in autosuspend Marcus Folkesson
6 siblings, 1 reply; 14+ messages in thread
From: Marcus Folkesson @ 2018-02-28 13:38 UTC (permalink / raw)
To: Dmitry Torokhov, Arvind Yadav; +Cc: linux-input, linux-kernel, 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RESEND PATCH 6/6] input: usbtouchscreen: do not rely on input_dev->users
2018-02-28 13:38 ` [RESEND PATCH 6/6] input: usbtouchscreen: do not rely on input_dev->users Marcus Folkesson
@ 2018-03-17 18:11 ` Dmitry Torokhov
0 siblings, 0 replies; 14+ messages in thread
From: Dmitry Torokhov @ 2018-03-17 18:11 UTC (permalink / raw)
To: Marcus Folkesson; +Cc: Arvind Yadav, linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Fix deadlocks in autosuspend
2018-02-28 13:37 Fix deadlocks in autosuspend Marcus Folkesson
` (5 preceding siblings ...)
2018-02-28 13:38 ` [RESEND PATCH 6/6] input: usbtouchscreen: do not rely on input_dev->users Marcus Folkesson
@ 2018-03-16 13:41 ` Marcus Folkesson
6 siblings, 0 replies; 14+ messages in thread
From: Marcus Folkesson @ 2018-03-16 13:41 UTC (permalink / raw)
To: Dmitry Torokhov, Arvind Yadav; +Cc: linux-input, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 14+ messages in thread