Re: Sleeping inside spinlock in force feedback input event code

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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