Currently a blocking read, select, or poll call will not return if a
joystick device is unplugged. This patch allows them to return.
This patch adds a wake_up_interruptible call to the joydev_disconnect
to interrupt both a blocking read and a blocking poll call. The read
routine was already setup to handle this situation, just nothing was
waking it up. The poll routine required only minimal changes to
report the error condition. It is important to find out when the
joystick has been disconnected instead of blocking forever.
I tested it with a USB joystick, or rather I keep on getting the
following messages and my application needed to know when the joystick
was removed to close the port and re-open it when the USB system has
completed rediscovering it.
hub 4-0:1.0: port 1 enable change, status 00000101
hub 4-0:1.0: port 1 disabled by hub (EMI?), re-enabling...
hub 4-0:1.0: port 1, status 0101, change 0002, 12 Mb/s
usb 4-1: USB disconnect, address 39
usb 4-1: usb_disable_device nuking all URBs
I can reliably enough reproduce the problem by standing up as the
static on my cloth chair followed by a discharge to the chair is
enough to cause the joystick to be disabled by the USB HUB even if I'm
only touching the chair and floor! The kernel then removes the old
joystick and redetects it as a new joystick.
I've also included a test program to verify that a blocking select,
poll, and read call will return if the joystick is unplugged.
--
David Fries <[email protected]>
http://fries.net/~david/pgpkey.txt
Signed-off-by: David Fries <[email protected]>
--- drivers/input/joydev.c.orig Tue Feb 1 08:39:52 2005
+++ drivers/input/joydev.c Tue Feb 1 08:40:15 2005
@@ -279,11 +279,14 @@ static ssize_t joydev_read(struct file *
/* No kernel lock - fine */
static unsigned int joydev_poll(struct file *file, poll_table *wait)
{
+ int mask = 0;
struct joydev_list *list = file->private_data;
poll_wait(file, &list->joydev->wait, wait);
- if (list->head != list->tail || list->startup < list->joydev->nabs + list->joydev->nkey)
- return POLLIN | POLLRDNORM;
- return 0;
+ if(!list->joydev->exist)
+ mask |= POLLERR;
+ if(list->head != list->tail || list->startup < list->joydev->nabs + list->joydev->nkey)
+ mask |= POLLIN | POLLRDNORM;
+ return mask;
}
static int joydev_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
@@ -469,9 +472,14 @@ static void joydev_disconnect(struct inp
joydev->exist = 0;
if (joydev->open)
+ {
input_close_device(handle);
+ wake_up_interruptible(&joydev->wait);
+ }
else
+ {
joydev_free(joydev);
+ }
}
static struct input_device_id joydev_blacklist[] = {
On Tue, 1 Feb 2005 08:52:15 -0600, David Fries <[email protected]> wrote:
> Currently a blocking read, select, or poll call will not return if a
> joystick device is unplugged. This patch allows them to return.
>
...
> static unsigned int joydev_poll(struct file *file, poll_table *wait)
> {
> + int mask = 0;
> struct joydev_list *list = file->private_data;
> poll_wait(file, &list->joydev->wait, wait);
> - if (list->head != list->tail || list->startup < list->joydev->nabs + list->joydev->nkey)
> - return POLLIN | POLLRDNORM;
> - return 0;
> + if(!list->joydev->exist)
> + mask |= POLLERR;
Probably need POLLHUP in addition (or instead of POLLERR).
> if (joydev->open)
> + {
> input_close_device(handle);
> + wake_up_interruptible(&joydev->wait);
> + }
> else
> + {
> joydev_free(joydev);
> + }
Opening braces should go on the same line as the statement (if (...) {).
--
Dmitry
On Tue, Feb 01, 2005 at 10:24:39AM -0500, Dmitry Torokhov wrote:
> On Tue, 1 Feb 2005 08:52:15 -0600, David Fries <[email protected]> wrote:
> > Currently a blocking read, select, or poll call will not return if a
> > joystick device is unplugged. This patch allows them to return.
> >
> ...
> > static unsigned int joydev_poll(struct file *file, poll_table *wait)
> > {
> > + int mask = 0;
> > struct joydev_list *list = file->private_data;
> > poll_wait(file, &list->joydev->wait, wait);
> > - if (list->head != list->tail || list->startup < list->joydev->nabs + list->joydev->nkey)
> > - return POLLIN | POLLRDNORM;
> > - return 0;
> > + if(!list->joydev->exist)
> > + mask |= POLLERR;
>
> Probably need POLLHUP in addition (or instead of POLLERR).
>
> > if (joydev->open)
> > + {
> > input_close_device(handle);
> > + wake_up_interruptible(&joydev->wait);
> > + }
> > else
> > + {
> > joydev_free(joydev);
> > + }
>
> Opening braces should go on the same line as the statement (if (...) {).
How about this patch?
[email protected], 2005-02-06 13:58:37+01:00, [email protected]
input: Fix poll() behavior of input handlers on disconnect.
Signed-off-by: Vojtech Pavlik <[email protected]>
evdev.c | 5 ++---
joydev.c | 10 +++++-----
mousedev.c | 6 +++---
tsdev.c | 6 ++----
4 files changed, 12 insertions(+), 15 deletions(-)
diff -Nru a/drivers/input/evdev.c b/drivers/input/evdev.c
--- a/drivers/input/evdev.c 2005-02-06 14:12:04 +01:00
+++ b/drivers/input/evdev.c 2005-02-06 14:12:04 +01:00
@@ -199,9 +199,8 @@
{
struct evdev_list *list = file->private_data;
poll_wait(file, &list->evdev->wait, wait);
- if (list->head != list->tail)
- return POLLIN | POLLRDNORM;
- return 0;
+ return ((list->head == list->tail) ? 0 : (POLLIN | POLLRDNORM)) |
+ (list->evdev->exist ? 0 : (POLLHUP | POLLERR));
}
static int evdev_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
--- a/drivers/input/joydev.c 2005-02-06 14:12:04 +01:00
+++ b/drivers/input/joydev.c 2005-02-06 14:12:04 +01:00
@@ -281,9 +281,8 @@
{
struct joydev_list *list = file->private_data;
poll_wait(file, &list->joydev->wait, wait);
- if (list->head != list->tail || list->startup < list->joydev->nabs + list->joydev->nkey)
- return POLLIN | POLLRDNORM;
- return 0;
+ return ((list->head != list->tail || list->startup < list->joydev->nabs + list->joydev->nkey) ?
+ (POLLIN | POLLRDNORM) : 0) | (list->joydev->exist ? 0 : (POLLHUP | POLLERR));
}
static int joydev_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
@@ -468,9 +467,10 @@
devfs_remove("input/js%d", joydev->minor);
joydev->exist = 0;
- if (joydev->open)
+ if (joydev->open) {
input_close_device(handle);
- else
+ wake_up_interruptible(&joydev->wait);
+ } else
joydev_free(joydev);
}
diff -Nru a/drivers/input/mousedev.c b/drivers/input/mousedev.c
--- a/drivers/input/mousedev.c 2005-02-06 14:12:04 +01:00
+++ b/drivers/input/mousedev.c 2005-02-06 14:12:04 +01:00
@@ -595,9 +595,8 @@
{
struct mousedev_list *list = file->private_data;
poll_wait(file, &list->mousedev->wait, wait);
- if (list->ready || list->buffer)
- return POLLIN | POLLRDNORM;
- return 0;
+ return ((list->ready || list->buffer) ? (POLLIN | POLLRDNORM) : 0) |
+ (list->mousedev->exist ? 0 : (POLLHUP | POLLERR));
}
static struct file_operations mousedev_fops = {
@@ -660,6 +659,7 @@
if (mousedev->open) {
input_close_device(handle);
+ wake_up_interruptible(&mousedev->wait);
} else {
if (mousedev_mix.open)
input_close_device(handle);
diff -Nru a/drivers/input/tsdev.c b/drivers/input/tsdev.c
--- a/drivers/input/tsdev.c 2005-02-06 14:12:04 +01:00
+++ b/drivers/input/tsdev.c 2005-02-06 14:12:04 +01:00
@@ -232,11 +232,9 @@
static unsigned int tsdev_poll(struct file *file, poll_table * wait)
{
struct tsdev_list *list = file->private_data;
-
poll_wait(file, &list->tsdev->wait, wait);
- if (list->head != list->tail)
- return POLLIN | POLLRDNORM;
- return 0;
+ return ((list->head == list->tail) ? 0 : (POLLIN | POLLRDNORM)) |
+ (list->tsdev->exist ? 0 : (POLLHUP | POLLERR));
}
static int tsdev_ioctl(struct inode *inode, struct file *file,
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Sunday 06 February 2005 08:12, Vojtech Pavlik wrote:
> On Tue, Feb 01, 2005 at 10:24:39AM -0500, Dmitry Torokhov wrote:
> > On Tue, 1 Feb 2005 08:52:15 -0600, David Fries <[email protected]> wrote:
> > > Currently a blocking read, select, or poll call will not return if a
> > > joystick device is unplugged. ?This patch allows them to return.
> > >
> > ...
> > > static unsigned int joydev_poll(struct file *file, poll_table *wait)
> > > {
> > > + ? ? ? int mask = 0;
> > > ? ? ? ?struct joydev_list *list = file->private_data;
> > > ? ? ? ?poll_wait(file, &list->joydev->wait, wait);
> > > - ? ? ? if (list->head != list->tail || list->startup < list->joydev->nabs + list->joydev->nkey)
> > > - ? ? ? ? ? ? ? return POLLIN | POLLRDNORM;
> > > - ? ? ? return 0;
> > > + ? ? ? if(!list->joydev->exist)
> > > + ? ? ? ? ? ? ? mask |= POLLERR;
> >
> > Probably need POLLHUP in addition (or instead of POLLERR).
> >
> > > ? ? ? ?if (joydev->open)
> > > + ? ? ? {
> > > ? ? ? ? ? ? ? ?input_close_device(handle);
> > > + ? ? ? ? ? ? ? wake_up_interruptible(&joydev->wait);
> > > + ? ? ? }
> > > ? ? ? ?else
> > > + ? ? ? {
> > > ? ? ? ? ? ? ? ?joydev_free(joydev);
> > > + ? ? ? }
> >
> > Opening braces should go on the same line as the statement (if (...) {).
> ?
> How about this patch?
Looks fine now. Hmm, wait a sec... Don't we also need kill_fasync calls in
disconnect routines as well?
--
Dmitry
On Sun, Feb 06, 2005 at 08:21:13PM -0500, Dmitry Torokhov wrote:
> On Sunday 06 February 2005 08:12, Vojtech Pavlik wrote:
> > On Tue, Feb 01, 2005 at 10:24:39AM -0500, Dmitry Torokhov wrote:
> > > On Tue, 1 Feb 2005 08:52:15 -0600, David Fries <[email protected]> wrote:
> > > > Currently a blocking read, select, or poll call will not return if a
> > > > joystick device is unplugged. ?This patch allows them to return.
> > > >
> > > ...
> > > > static unsigned int joydev_poll(struct file *file, poll_table *wait)
> > > > {
> > > > + ? ? ? int mask = 0;
> > > > ? ? ? ?struct joydev_list *list = file->private_data;
> > > > ? ? ? ?poll_wait(file, &list->joydev->wait, wait);
> > > > - ? ? ? if (list->head != list->tail || list->startup < list->joydev->nabs + list->joydev->nkey)
> > > > - ? ? ? ? ? ? ? return POLLIN | POLLRDNORM;
> > > > - ? ? ? return 0;
> > > > + ? ? ? if(!list->joydev->exist)
> > > > + ? ? ? ? ? ? ? mask |= POLLERR;
> > >
> > > Probably need POLLHUP in addition (or instead of POLLERR).
> > >
> > > > ? ? ? ?if (joydev->open)
> > > > + ? ? ? {
> > > > ? ? ? ? ? ? ? ?input_close_device(handle);
> > > > + ? ? ? ? ? ? ? wake_up_interruptible(&joydev->wait);
> > > > + ? ? ? }
> > > > ? ? ? ?else
> > > > + ? ? ? {
> > > > ? ? ? ? ? ? ? ?joydev_free(joydev);
> > > > + ? ? ? }
> > >
> > > Opening braces should go on the same line as the statement (if (...) {).
> > ?
> > How about this patch?
>
> Looks fine now. Hmm, wait a sec... Don't we also need kill_fasync calls in
> disconnect routines as well?
Yes, we do.
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Sun, Feb 06, 2005 at 08:21:13PM -0500, Dmitry Torokhov wrote:
> > > Opening braces should go on the same line as the statement (if (...) {).
> > ?
> > How about this patch?
>
> Looks fine now. Hmm, wait a sec... Don't we also need kill_fasync calls in
> disconnect routines as well?
This should do it:
[email protected], 2005-02-07 13:19:59+01:00, [email protected]
input: Do a kill_fasync() in input handlers on device disconnect
to notify a client using poll() that the device is gone.
Signed-off-by: Vojtech Pavlik <[email protected]>
evdev.c | 3 +++
joydev.c | 3 +++
mousedev.c | 3 +++
tsdev.c | 3 +++
4 files changed, 12 insertions(+)
diff -Nru a/drivers/input/evdev.c b/drivers/input/evdev.c
--- a/drivers/input/evdev.c 2005-02-07 13:20:15 +01:00
+++ b/drivers/input/evdev.c 2005-02-07 13:20:15 +01:00
@@ -440,6 +440,7 @@
static void evdev_disconnect(struct input_handle *handle)
{
struct evdev *evdev = handle->private;
+ struct evdev_list *list;
class_simple_device_remove(MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + evdev->minor));
devfs_remove("input/event%d", evdev->minor);
@@ -448,6 +449,8 @@
if (evdev->open) {
input_close_device(handle);
wake_up_interruptible(&evdev->wait);
+ list_for_each_entry(list, &evdev->list, node)
+ kill_fasync(&list->fasync, SIGIO, POLLHUP | POLLERR);
} else
evdev_free(evdev);
}
diff -Nru a/drivers/input/joydev.c b/drivers/input/joydev.c
--- a/drivers/input/joydev.c 2005-02-07 13:20:15 +01:00
+++ b/drivers/input/joydev.c 2005-02-07 13:20:15 +01:00
@@ -462,6 +462,7 @@
static void joydev_disconnect(struct input_handle *handle)
{
struct joydev *joydev = handle->private;
+ struct joydev_list *list;
class_simple_device_remove(MKDEV(INPUT_MAJOR, JOYDEV_MINOR_BASE + joydev->minor));
devfs_remove("input/js%d", joydev->minor);
@@ -470,6 +471,8 @@
if (joydev->open) {
input_close_device(handle);
wake_up_interruptible(&joydev->wait);
+ list_for_each_entry(list, &joydev->list, node)
+ kill_fasync(&list->fasync, SIGIO, POLLHUP | POLLERR);
} else
joydev_free(joydev);
}
diff -Nru a/drivers/input/mousedev.c b/drivers/input/mousedev.c
--- a/drivers/input/mousedev.c 2005-02-07 13:20:15 +01:00
+++ b/drivers/input/mousedev.c 2005-02-07 13:20:15 +01:00
@@ -652,6 +652,7 @@
static void mousedev_disconnect(struct input_handle *handle)
{
struct mousedev *mousedev = handle->private;
+ struct mousedev_list *list;
class_simple_device_remove(MKDEV(INPUT_MAJOR, MOUSEDEV_MINOR_BASE + mousedev->minor));
devfs_remove("input/mouse%d", mousedev->minor);
@@ -660,6 +661,8 @@
if (mousedev->open) {
input_close_device(handle);
wake_up_interruptible(&mousedev->wait);
+ list_for_each_entry(list, &mousedev->list, node)
+ kill_fasync(&list->fasync, SIGIO, POLLHUP | POLLERR);
} else {
if (mousedev_mix.open)
input_close_device(handle);
diff -Nru a/drivers/input/tsdev.c b/drivers/input/tsdev.c
--- a/drivers/input/tsdev.c 2005-02-07 13:20:15 +01:00
+++ b/drivers/input/tsdev.c 2005-02-07 13:20:15 +01:00
@@ -424,6 +424,7 @@
static void tsdev_disconnect(struct input_handle *handle)
{
struct tsdev *tsdev = handle->private;
+ struct tsdev_list *list;
class_simple_device_remove(MKDEV(INPUT_MAJOR, TSDEV_MINOR_BASE + tsdev->minor));
devfs_remove("input/ts%d", tsdev->minor);
@@ -433,6 +434,8 @@
if (tsdev->open) {
input_close_device(handle);
wake_up_interruptible(&tsdev->wait);
+ list_for_each_entry(list, &tsdev->list, node)
+ kill_fasync(&list->fasync, SIGIO, POLLHUP | POLLERR);
} else
tsdev_free(tsdev);
}
--
Vojtech Pavlik
SuSE Labs, SuSE CR
On Mon, 7 Feb 2005 13:20:33 +0100, Vojtech Pavlik <[email protected]> wrote:
> On Sun, Feb 06, 2005 at 08:21:13PM -0500, Dmitry Torokhov wrote:
> > > > Opening braces should go on the same line as the statement (if (...) {).
> > >
> > > How about this patch?
> >
> > Looks fine now. Hmm, wait a sec... Don't we also need kill_fasync calls in
> > disconnect routines as well?
>
> This should do it:
>
Not quite...
> + list_for_each_entry(list, &evdev->list, node)
> + kill_fasync(&list->fasync, SIGIO, POLLHUP | POLLERR);
Wrong band constants - for SIGIO POLL_HUP and POLL_ERR should be used.
/*
* SIGPOLL si_codes
*/
#define POLL_IN (__SI_POLL|1) /* data input available */
#define POLL_OUT (__SI_POLL|2) /* output buffers available */
#define POLL_MSG (__SI_POLL|3) /* input message available */
#define POLL_ERR (__SI_POLL|4) /* i/o error */
#define POLL_PRI (__SI_POLL|5) /* high priority input available */
#define POLL_HUP (__SI_POLL|6) /* device disconnected */
--
Dmitry
On Mon, Feb 07, 2005 at 01:20:33PM +0100, Vojtech Pavlik wrote:
> On Sun, Feb 06, 2005 at 08:21:13PM -0500, Dmitry Torokhov wrote:
>
> This should do it:
>
> [email protected], 2005-02-07 13:19:59+01:00, [email protected]
> input: Do a kill_fasync() in input handlers on device disconnect
> to notify a client using poll() that the device is gone.
>
> Signed-off-by: Vojtech Pavlik <[email protected]>
...
>
> --
> Vojtech Pavlik
> SuSE Labs, SuSE CR
I just checked it against my joystick_select test program which I
included in an earlier e-mail and the blocking read, poll, and select
return when I unplug the joystick. Thanks for fixing the other
drivers, I didn't think of looking at them.
--
David Fries <[email protected]>
http://fries.net/~david/pgpkey.txt
On Mon, Feb 07, 2005 at 09:22:19AM -0500, Dmitry Torokhov wrote:
> On Mon, 7 Feb 2005 13:20:33 +0100, Vojtech Pavlik <[email protected]> wrote:
> > On Sun, Feb 06, 2005 at 08:21:13PM -0500, Dmitry Torokhov wrote:
> > > > > Opening braces should go on the same line as the statement (if (...) {).
> > > >
> > > > How about this patch?
> > >
> > > Looks fine now. Hmm, wait a sec... Don't we also need kill_fasync calls in
> > > disconnect routines as well?
> >
> > This should do it:
> >
>
> Not quite...
>
> > + list_for_each_entry(list, &evdev->list, node)
> > + kill_fasync(&list->fasync, SIGIO, POLLHUP | POLLERR);
>
> Wrong band constants - for SIGIO POLL_HUP and POLL_ERR should be used.
Obviously only one of them, since they're not designed for ORing. I used POLL_HUP then.
> /*
> * SIGPOLL si_codes
> */
> #define POLL_IN (__SI_POLL|1) /* data input available */
> #define POLL_OUT (__SI_POLL|2) /* output buffers available */
> #define POLL_MSG (__SI_POLL|3) /* input message available */
> #define POLL_ERR (__SI_POLL|4) /* i/o error */
> #define POLL_PRI (__SI_POLL|5) /* high priority input available */
> #define POLL_HUP (__SI_POLL|6) /* device disconnected */
--
Vojtech Pavlik
SuSE Labs, SuSE CR