(Added Jiri Kosina due to the hid problem I describe near the end) Dmitry Torokhov wrote: > Hi Anssi, > > On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote: >> Hi all! >> >> It seems a new spinlock input_dev->event_lock has been added [1] to the >> input subsystem since the force feedback support was reworked. >> >> However, the force feedback subsystem sleeps on events in multiple >> places, e.g. ff-core.c uses a mutex, and hid-pidff driver waits for hid >> io (otherwise commands were lost, IIRC; if necessary I'll test again). >> >> ff_device->mutex is used to shield effects[], so it is locked when >> handling EV_FF events, on flushes, and on effect upload and erase ioctls. >> >> Maybe we should make EV_FF handling atomic? For effect uploading we >> could either make it completely atomic, or lock only for reserving the >> effect slot, then release the lock, and mark it as ready after upload is >> complete. >> Making even the upload completely atomic would mean that no force >> feedback events/ioctl() would sleep, which AFAIK would be a plus for >> userspace ff applications. On the other hand, hid-pidff (device managed >> mode) driver doesn't know whether effect upload was successful until it >> has received a report from the device, so it wouldn't be able to report >> failure immediately. Other drivers would, though. >> >> What do you think? >> > > I think something the patch below is what is needed. EV_FF handling is > already atomic because of event_lock (and it is here to stay), but > uploading does not need to be atomic, only installing into effect > table needs the lock. Any change you could test the patch? I dont have > any FF devices. It seems to be ok, but not enough. The hid-pidff.c driver also waits on pidff_playback_pid(). However, I now see that the wait is probably only necessary because just the report pointer is passed to usbhid_submit_report(). But fixing it properly seems non-trivial (to me). E.g. the problem sequence is: - playback_pid() gets called to stop effect 1. - it sets control_report->field[X]->value[X] = 1; - it submits control_report - thus usbhid_submit_report() stores a pointer to the report - playback_pid() gets immediately called again for effect 2. - it sets control_report->field[X]->value[X] = 2; - thus the previous report hasn't yet been submitted, but the report content has already changed, thus effect 1 is never stopped. Any idea how this should be solved properly? -- Anssi Hannula -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
