| Message ID | 20191127101008.GA327265@nuka.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | BUG: ff_effects lost after a fork | expand |
> On Nov 27, 2019, at 2:10 AM, Mathieu Maret <mathieu.maret@gmail.com> wrote: > > Hi, > > I'm using evdev for vibrator interface. > I can register ff_effect and play them. > But, if I do any kind of fork, all the effects are flush and cannot be > used. > > You can find, below, an example of such a program. > From some trace have put in the kernel, it's seems that at the end of > the system() call, evdev_flush get called. > > evdev_flush() will call flush_effects() that will remove all the > registered effects. > > I've only one device with vibrator and it's a imx6 4.1.15 kernel. But > code looks the same that in linus master that why I'm posting it here. I > hope that it will not waste people time > Hi everyone, I’m also hitting this bug with games that use force-feedback steering wheels under Wine/Proton. It typically shows up as EVIOCSFF ioctls failing with EINVAL, since all the effects were unexpectedly flushed. The problem is that input_ff_flush() is called whenever a file descriptor is closed, but there can be multiple descriptors open to the same file description (through fork(), dup(), etc). input_ff_flush() removes all effects added by that file description, which the users of the other descriptors certainly don't expect. As for the fix, maybe fd_ops->flush() shouldn’t be implemented at all? In the current design, effects “belong” to a file description (a struct file *), not a descriptor. This seems sensible to me: a process could open a device, upload an effect, then fork(), and it makes sense that the child would have full control of the effects created by the parent. It seems to me like nothing should be done when a descriptor is closed, and input_ff_flush() should be called only when the whole struct file is released. I’ve attached a modified copy of Mathieu’s code, which reproduces the problem for me with a Logitech G25 steering wheel. Brendan Shanks CodeWeavers #include <errno.h> #include <fcntl.h> #include <linux/input.h> #include <stdio.h> #include <string.h> #include <sys/stat.h> #include <sys/types.h> #include <unistd.h> #include <stdlib.h> #define DEV_PATH "/dev/input/event10" int main(int argc, char *argv[]) { int fd = open(DEV_PATH, O_RDWR); if (fd < 0) { printf("Cannot open " DEV_PATH); } // Register an effect struct ff_effect effects; memset(&effects, 0, sizeof(effects)); effects.type = FF_CONSTANT; effects.id = -1; effects.replay.length = 1000; effects.replay.delay = 0; if (ioctl(fd, EVIOCSFF, &effects) < 0) { printf("Cannot upload effect %s\n", strerror(errno)); return -1; } int fd2 = dup(fd); close(fd2); // ioctl fails with EINVAL if (ioctl(fd, EVIOCSFF, &effects) < 0) { printf("Cannot upload effect %s\n", strerror(errno)); return -1; } close(fd); return 0; }
On Tue, Apr 21, 2020 at 11:53:19AM -0700, Brendan Shanks wrote: > > > On Nov 27, 2019, at 2:10 AM, Mathieu Maret <mathieu.maret@gmail.com> > > wrote: > > > > Hi, > > > > I'm using evdev for vibrator interface. I can register ff_effect > > and play them. But, if I do any kind of fork, all the effects are > > flush and cannot be used. > > > > You can find, below, an example of such a program. From some trace > > have put in the kernel, it's seems that at the end of the system() > > call, evdev_flush get called. > > > > evdev_flush() will call flush_effects() that will remove all the > > registered effects. > > > > I've only one device with vibrator and it's a imx6 4.1.15 kernel. > > But code looks the same that in linus master that why I'm posting it > > here. I hope that it will not waste people time > > > > Hi everyone, > > I’m also hitting this bug with games that use force-feedback steering > wheels under Wine/Proton. It typically shows up as EVIOCSFF ioctls > failing with EINVAL, since all the effects were unexpectedly flushed. > > The problem is that input_ff_flush() is called whenever a file > descriptor is closed, but there can be multiple descriptors open to > the same file description (through fork(), dup(), etc). > input_ff_flush() removes all effects added by that file description, > which the users of the other descriptors certainly don't expect. > > As for the fix, maybe fd_ops->flush() shouldn’t be implemented at all? > In the current design, effects “belong” to a file description (a > struct file *), not a descriptor. This seems sensible to me: a process > could open a device, upload an effect, then fork(), and it makes sense > that the child would have full control of the effects created by the > parent. It seems to me like nothing should be done when a descriptor > is closed, and input_ff_flush() should be called only when the whole > struct file is released. > Yes, I agree, we should drop setting fops->flush and move the code into evdev_release(). This will ensure that the effects are stopped and erased once file is closed, but will allow passing the descriptors around. Can I get a real patch for this please? Thanks.
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index e578a75..6e6002d 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -415,6 +415,8 @@ static int evdev_release(struct inode *inode, struct file *file) struct evdev_client *client = file->private_data; struct evdev *evdev = client->evdev; + evdev_flush(file, NULL); + mutex_lock(&evdev->mutex); evdev_ungrab(evdev, client); mutex_unlock(&evdev->mutex); @@ -1107,7 +1109,7 @@ static const struct file_operations evdev_fops = { .compat_ioctl = evdev_ioctl_compat, #endif .fasync = evdev_fasync, - .flush = evdev_flush, +// .flush = evdev_flush, .llseek = no_llseek, };
Hi, I'm using evdev for vibrator interface. I can register ff_effect and play them. But, if I do any kind of fork, all the effects are flush and cannot be used. You can find, below, an example of such a program. From some trace have put in the kernel, it's seems that at the end of the system() call, evdev_flush get called. evdev_flush() will call flush_effects() that will remove all the registered effects. I've only one device with vibrator and it's a imx6 4.1.15 kernel. But code looks the same that in linus master that why I'm posting it here. I hope that it will not waste people time For the moment, I'm using this nasty workaround: * C program example #include <errno.h> #include <fcntl.h> #include <linux/input.h> #include <stdio.h> #include <string.h> #include <sys/stat.h> #include <sys/types.h> #include <unistd.h> #include <stdlib.h> #define DEV_PATH "/dev/input/event1" int main(int argc, char *argv[]) { int fd = open(DEV_PATH, O_RDWR); if (fd < 0) { printf("Cannot open " DEV_PATH); } // Register an effect struct ff_effect effects; memset(&effects, 0, sizeof(effects)); effects.type = FF_RUMBLE; effects.id = -1; effects.u.rumble.strong_magnitude = 0x8000; effects.u.rumble.weak_magnitude = 0; effects.replay.length = 1000; effects.replay.delay = 0; if (ioctl(fd, EVIOCSFF, &effects) < 0) { printf("Cannot upload effect %s\n", strerror(errno)); return -1; } // Play this effect struct input_event input; memset(&input, 0, sizeof(input)); input.type = EV_FF; input.code = effects.id; input.value = 1; if (write(fd, &input, sizeof(input)) != sizeof(input)) { printf("Cannot write %s\n", strerror(errno)); return -1; } printf("Forking\n"); system("sleep 1"); // Comment this line to have the second effect played // Play effect again : Nothing is played memset(&input, 0, sizeof(input)); input.type = EV_FF; input.code = effects.id; input.value = 1; if (write(fd, &input, sizeof(input)) != sizeof(input)) { printf("Cannot write %s\n", strerror(errno)); return -1; } close(fd); return 0; }