evdev: Make open(2) and close(3) handlers sleepable.

[wulf]

Summary

Initially there was a LOR between hardware driver's and evdev client list locks as they were taken in different order at driver's interrupt and evdev open()/close() handlers.

The LOR was fixed with introduction of evdev_register_mtx() function which allowed to use a hardware driver's lock as evdev client list lock.
This works well with PS/2 and USB but not with I2C. Unlike PS/2 and USB, I2C open()/close() handlers want to do unbound sleeps while waiting for I2C bus to release and just to perform IO.
sysutil/iichid solved this problem with using a taskqueue.
This change uses different approach. It uses epoch(9) in interrupt handler for traversing evdev client list to avoid the LOR thus making possible to convert evdev client list lock to sleepable sx.

Besides that this change allows open() handler to be interrupted by a signal
It is possible that the client list lock is taken by other process for too long due to e.g. IO timeouts. Allow user to terminate open() in this case.

This revision is subset of https://reviews.freebsd.org/D27777

[• hselasky]

sys/dev/evdev/evdev_private.h

107

I think one EPOCH for all of EVDEV's will be plenty. Can you explain a bit why you think there will be a need for multiple EPOCH's.

[wulf]

sys/dev/evdev/evdev_private.h

107

For now it reuses global_epoch_preempt; So the number of new epochs is 0.

[wulf]

sys/dev/evdev/evdev_private.h

107

And external epoch is needed to avoid epoch recursion as hidbus uses the same technique to avoid LORs in interrupt handler. In this case evdev is called with global_epoch_preempt already entered.

[markj]

sys/dev/evdev/evdev_private.h

103

I think a comment explaining the locking protocols would be useful here. After reading the code for some time it makes sense to me, but it is not obvious why you write

if (client->ec_evdev->ev_lock_type != EV_LOCK_MTX)
                epoch_wait_preempt(client->ec_evdev->ev_epoch);

instead of

if (client->ec_evdev->ev_lock_type == EV_LOCK_EPOCH)
                epoch_wait_preempt(client->ec_evdev->ev_epoch);

for instance.

[wulf]

I think a comment explaining the locking protocols would be useful here.

Done: https://github.com/wulf7/freebsd/commit/78929f79a9e0ae8a6ed489328f1434eefd3504cc#diff-3c26e72ff877510bd53406aa46fbcd8ded4899f2f4d1374f24c5a9f47ff0aafcR81

After reading the code for some time it makes sense to me, but it is not obvious why you write

Somehow I missed it. Fixed. Thanks!
https://github.com/wulf7/freebsd/commit/78929f79a9e0ae8a6ed489328f1434eefd3504cc#diff-3894c24bb75181f63107befdd057eb124b1c5ea2f2c62730aab798eee2a1a819R855

[• hselasky]

sys/dev/evdev/evdev_private.h

107

Beware that number of EPOCHs in the system is limited:
sys/kern/subr_epoch.c:#define MAX_EPOCHS 64

And that there is some overhead creating many epochs for small things like HID and EVDEV.

If you can create one global EPOCH for these things related to input, that would be great.

[wulf]

sys/dev/evdev/evdev_private.h

107

Do you suggest to create dedicated input epoch and hard-code it's name into HID and EVDEV like network stack does?

[• hselasky]

Yes, the lists that are protected are very rarely updated, and one global EPOCH for the input subsystem would suffice plenty!

@markj : What do you think?

[wulf]

Implement @hselasky suggestion to make epoch name fixed. For now it is global_epoch_preempt. Can be trivially changed latter.
This obsoletes earlier added evdev_register_epoch() function and replaces it with EVDEV_FLAG_EXT_EPOCH flag.

While here, KASSERT epoch use while traversing evdev client list to catch missing epoch_enter() in the cases when evdev events are produced not by interrupt handler but by e.g. callout or ioctl handlers.