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:

quoted

Hi Anssi,

On Sun, Jun 15, 2008 at 10:01:55PM +0300, Anssi Hannula wrote:

quoted

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