| Message ID | e9e2b6b9-83d6-fab7-88db-9e0e6a2b0185@m-reimer.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/22/2016 08:22 PM, Manuel Reimer wrote: > Hello, > > I had a deeper look into the uinput kernel module crash problem, which > happens when a game loads effects and the driver exits while the game is > still running. > > The actual problem is, that "input_unregister_device", which is called > in "uinput_destroy_device", seems to include some "cleanup routine" > which includes erasing loaded force feedback effects. This cleanup > routine causes the "uinput_dev_erase_effect" routine to be called, which > will wait for the, already exited, daemon to answer this request. > > The existing code contains a device state test, but this test is placed > wrong. My patch moves the device state test, which was in > "uinput_request_send", to the "uinput_request_submit" function. This way > the erase requests, triggered by "input_unregister_device", are rejected > immediately, so they no longer cause any trouble. > > Should I have added a one-line comment above the device state test, > mentioning the cleanup routine in "input_unregister_device"? > > Signed-off-by: Manuel Reimer <mail@m-reimer.de> > > --- a/drivers/input/misc/uinput.c 2016-05-13 16:06:31.919351656 +0200 > +++ b/drivers/input/misc/uinput.c 2016-05-22 19:33:30.814403482 +0200 > @@ -117,11 +117,6 @@ static int uinput_request_send(struct ui > if (retval) > return retval; > > - if (udev->state != UIST_CREATED) { > - retval = -ENODEV; > - goto out; > - } > - > init_completion(&request->done); > > /* > @@ -130,7 +125,6 @@ static int uinput_request_send(struct ui > */ > uinput_dev_event(udev->dev, EV_UINPUT, request->code, request->id); > > - out: > mutex_unlock(&udev->mutex); > return retval; > } > @@ -140,6 +134,9 @@ static int uinput_request_submit(struct > { > int error; > > + if (udev->state != UIST_CREATED) > + return -ENODEV; > + > error = uinput_request_reserve_slot(udev, request); > if (error) > return error; > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Hello, did I use the correct list of recipients? This is my first try to fix something in kernel code, so it would really help me to get some suggestions about the right way to get something reviewed. Thanks in advance Manuel -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, May 22, 2016 at 08:22:23PM +0200, Manuel Reimer wrote: > Hello, > > I had a deeper look into the uinput kernel module crash problem, > which happens when a game loads effects and the driver exits while > the game is still running. > > The actual problem is, that "input_unregister_device", which is > called in "uinput_destroy_device", seems to include some "cleanup > routine" which includes erasing loaded force feedback effects. This > cleanup routine causes the "uinput_dev_erase_effect" routine to be > called, which will wait for the, already exited, daemon to answer > this request. There are 2 cases when we destroy the device: - when daemon is exiting. In this case you are absolutely right, we have no hope of talking to the device and should not be trying to send requests to it; - when userspace explicitly requested the device to be destroyed via UI_DEV_DESTROY ioctl. In this case I believe the kernel should attempt orderly device shutdown which includes stopping all FF effects. I guess we'll need to introduce another bit of state and adjust behavior accordingly. > > The existing code contains a device state test, but this test is > placed wrong. My patch moves the device state test, which was in > "uinput_request_send", to the "uinput_request_submit" function. This The device state is supposed to be protected by udev->mutex, your change violates this constraint. > > way the erase requests, triggered by "input_unregister_device", are > rejected immediately, so they no longer cause any trouble. > > Should I have added a one-line comment above the device state test, > mentioning the cleanup routine in "input_unregister_device"? > > Signed-off-by: Manuel Reimer <mail@m-reimer.de> > > --- a/drivers/input/misc/uinput.c 2016-05-13 16:06:31.919351656 +0200 > +++ b/drivers/input/misc/uinput.c 2016-05-22 19:33:30.814403482 +0200 > @@ -117,11 +117,6 @@ static int uinput_request_send(struct ui > if (retval) > return retval; > > - if (udev->state != UIST_CREATED) { > - retval = -ENODEV; > - goto out; > - } > - > init_completion(&request->done); > > /* > @@ -130,7 +125,6 @@ static int uinput_request_send(struct ui > */ > uinput_dev_event(udev->dev, EV_UINPUT, request->code, request->id); > > - out: > mutex_unlock(&udev->mutex); > return retval; > } > @@ -140,6 +134,9 @@ static int uinput_request_submit(struct > { > int error; > > + if (udev->state != UIST_CREATED) > + return -ENODEV; > + > error = uinput_request_reserve_slot(udev, request); > if (error) > return error; Thanks.
--- a/drivers/input/misc/uinput.c 2016-05-13 16:06:31.919351656 +0200 +++ b/drivers/input/misc/uinput.c 2016-05-22 19:33:30.814403482 +0200 @@ -117,11 +117,6 @@ static int uinput_request_send(struct ui if (retval) return retval; - if (udev->state != UIST_CREATED) { - retval = -ENODEV; - goto out; - } - init_completion(&request->done); /* @@ -130,7 +125,6 @@ static int uinput_request_send(struct ui */ uinput_dev_event(udev->dev, EV_UINPUT, request->code, request->id); - out: mutex_unlock(&udev->mutex); return retval; } @@ -140,6 +134,9 @@ static int uinput_request_submit(struct { int error; + if (udev->state != UIST_CREATED) + return -ENODEV; + error = uinput_request_reserve_slot(udev, request); if (error) return error;
Hello, I had a deeper look into the uinput kernel module crash problem, which happens when a game loads effects and the driver exits while the game is still running. The actual problem is, that "input_unregister_device", which is called in "uinput_destroy_device", seems to include some "cleanup routine" which includes erasing loaded force feedback effects. This cleanup routine causes the "uinput_dev_erase_effect" routine to be called, which will wait for the, already exited, daemon to answer this request. The existing code contains a device state test, but this test is placed wrong. My patch moves the device state test, which was in "uinput_request_send", to the "uinput_request_submit" function. This way the erase requests, triggered by "input_unregister_device", are rejected immediately, so they no longer cause any trouble. Should I have added a one-line comment above the device state test, mentioning the cleanup routine in "input_unregister_device"? Signed-off-by: Manuel Reimer <mail@m-reimer.de> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html