On Tue, Jun 17, 2008 at 09:52:36PM +0300, Anssi Hannula wrote: > (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? > It looks like there is a common issue with HID FF devices. Pid driver tries to handle it by inserting waits till the control queue is cleared, other drivers are completely ignorant of this problem... I guess we need to implement a queue of events to be played and put it in hid-ff.c so it is available for all hid ff drivers. -- Dmitry -- 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
