Page MenuHomeFreeBSD

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

Authored by wulf on Dec 31 2020, 3:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 7, 8:00 PM
Unknown Object (File)
Feb 25 2024, 6:11 AM
Unknown Object (File)
Feb 14 2024, 7:27 AM
Unknown Object (File)
Jan 4 2024, 7:09 PM
Unknown Object (File)
Dec 20 2023, 8:25 AM
Unknown Object (File)
Dec 11 2023, 1:49 AM
Unknown Object (File)
Oct 21 2023, 11:07 PM
Unknown Object (File)
Oct 5 2023, 11:45 PM
Subscribers
None

Details

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

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

wulf requested review of this revision.Dec 31 2020, 3:10 PM
wulf created this revision.
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.

sys/dev/evdev/evdev_private.h
107

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

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 added inline comments.
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.

This revision is now accepted and ready to land.Dec 31 2020, 4:06 PM
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.

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?

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?

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.

This revision now requires review to proceed.Jan 6 2021, 12:18 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jan 7 2021, 11:21 PM
This revision was automatically updated to reflect the committed changes.