Re: Sleeping inside spinlock in force feedback input event code

[Anssi Hannula <anssi.hannula@gmail.com>]

(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