The branch main has been updated by wulf: URL: https://cgit.FreeBSD.org/src/commit/?id=d276eae674d22214d6a58d1f4871053ceb0cb9f5
commit d276eae674d22214d6a58d1f4871053ceb0cb9f5 Author: Vladimir Kondratyev <[email protected]> AuthorDate: 2020-04-21 10:26:58 +0000 Commit: Vladimir Kondratyev <[email protected]> CommitDate: 2021-01-07 23:18:41 +0000 evdev: Make open(2) and close(3) handlers sleepable. At the beginning of evdev 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. While this works good with PS/2 and USB, this does not work with I2C. Unlike PS/2 and USB, I2C open()/close() handlers do unbound sleeps while waiting for I2C bus to release and while performing IO. This change uses epoch(9) for traversing evdev client list in interrupt handler to avoid the LOR thus making possible to convert evdev client list lock to sleepable sx. While here add brief locking protocol description. Reviewed by: markj Differential revision: https://reviews.freebsd.org/D27865 --- sys/dev/evdev/cdev.c | 23 ++++----- sys/dev/evdev/evdev.c | 83 ++++++++++++++++++++++-------- sys/dev/evdev/evdev.h | 3 ++ sys/dev/evdev/evdev_private.h | 114 +++++++++++++++++++++++++++++------------- 4 files changed, 156 insertions(+), 67 deletions(-) diff --git a/sys/dev/evdev/cdev.c b/sys/dev/evdev/cdev.c index cd40d1f218a7..c4550362ebce 100644 --- a/sys/dev/evdev/cdev.c +++ b/sys/dev/evdev/cdev.c @@ -32,6 +32,7 @@ #include <sys/param.h> #include <sys/bitstring.h> #include <sys/conf.h> +#include <sys/epoch.h> #include <sys/filio.h> #include <sys/fcntl.h> #include <sys/kernel.h> @@ -126,7 +127,7 @@ evdev_open(struct cdev *dev, int oflags, int devtype, struct thread *td) knlist_init_mtx(&client->ec_selp.si_note, &client->ec_buffer_mtx); /* Avoid race with evdev_unregister */ - EVDEV_LOCK(evdev); + EVDEV_LIST_LOCK(evdev); if (dev->si_drv1 == NULL) ret = ENODEV; else @@ -134,13 +135,9 @@ evdev_open(struct cdev *dev, int oflags, int devtype, struct thread *td) if (ret != 0) evdev_revoke_client(client); - /* - * Unlock evdev here because non-sleepable lock held - * while calling devfs_set_cdevpriv upsets WITNESS - */ - EVDEV_UNLOCK(evdev); + EVDEV_LIST_UNLOCK(evdev); - if (!ret) + if (ret == 0) ret = devfs_set_cdevpriv(client, evdev_dtor); if (ret != 0) { @@ -156,11 +153,13 @@ evdev_dtor(void *data) { struct evdev_client *client = (struct evdev_client *)data; - EVDEV_LOCK(client->ec_evdev); + EVDEV_LIST_LOCK(client->ec_evdev); if (!client->ec_revoked) evdev_dispose_client(client->ec_evdev, client); - EVDEV_UNLOCK(client->ec_evdev); + EVDEV_LIST_UNLOCK(client->ec_evdev); + if (client->ec_evdev->ev_lock_type != EV_LOCK_MTX) + epoch_wait_preempt(INPUT_EPOCH); knlist_clear(&client->ec_selp.si_note, 0); seldrain(&client->ec_selp); knlist_destroy(&client->ec_selp.si_note); @@ -547,12 +546,12 @@ evdev_ioctl(struct cdev *dev, u_long cmd, caddr_t data, int fflag, if (*(int *)data != 0) return (EINVAL); - EVDEV_LOCK(evdev); + EVDEV_LIST_LOCK(evdev); if (dev->si_drv1 != NULL && !client->ec_revoked) { evdev_dispose_client(evdev, client); evdev_revoke_client(client); } - EVDEV_UNLOCK(evdev); + EVDEV_LIST_UNLOCK(evdev); return (0); case EVIOCSCLOCKID: @@ -717,7 +716,7 @@ void evdev_revoke_client(struct evdev_client *client) { - EVDEV_LOCK_ASSERT(client->ec_evdev); + EVDEV_LIST_LOCK_ASSERT(client->ec_evdev); client->ec_revoked = true; } diff --git a/sys/dev/evdev/evdev.c b/sys/dev/evdev/evdev.c index 8d520e3ac09e..e76abbc816d3 100644 --- a/sys/dev/evdev/evdev.c +++ b/sys/dev/evdev/evdev.c @@ -31,12 +31,15 @@ #include <sys/param.h> #include <sys/bitstring.h> +#include <sys/ck.h> #include <sys/conf.h> +#include <sys/epoch.h> #include <sys/kdb.h> #include <sys/kernel.h> #include <sys/malloc.h> #include <sys/module.h> #include <sys/proc.h> +#include <sys/sx.h> #include <sys/sysctl.h> #include <sys/systm.h> @@ -295,12 +298,14 @@ evdev_register_common(struct evdev_dev *evdev) evdev->ev_shortname, evdev->ev_name, evdev->ev_serial); /* Initialize internal structures */ - LIST_INIT(&evdev->ev_clients); + CK_SLIST_INIT(&evdev->ev_clients); + sx_init(&evdev->ev_list_lock, "evsx"); if (evdev_event_supported(evdev, EV_REP) && bit_test(evdev->ev_flags, EVDEV_FLAG_SOFTREPEAT)) { /* Initialize callout */ - callout_init_mtx(&evdev->ev_rep_callout, evdev->ev_lock, 0); + callout_init_mtx(&evdev->ev_rep_callout, + evdev->ev_state_lock, 0); if (evdev->ev_rep[REP_DELAY] == 0 && evdev->ev_rep[REP_PERIOD] == 0) { @@ -332,6 +337,8 @@ evdev_register_common(struct evdev_dev *evdev) evdev_sysctl_create(evdev); bail_out: + if (ret != 0) + sx_destroy(&evdev->ev_list_lock); return (ret); } @@ -340,8 +347,11 @@ evdev_register(struct evdev_dev *evdev) { int ret; - evdev->ev_lock_type = EV_LOCK_INTERNAL; - evdev->ev_lock = &evdev->ev_mtx; + if (bit_test(evdev->ev_flags, EVDEV_FLAG_EXT_EPOCH)) + evdev->ev_lock_type = EV_LOCK_EXT_EPOCH; + else + evdev->ev_lock_type = EV_LOCK_INTERNAL; + evdev->ev_state_lock = &evdev->ev_mtx; mtx_init(&evdev->ev_mtx, "evmtx", NULL, MTX_DEF); ret = evdev_register_common(evdev); @@ -356,7 +366,7 @@ evdev_register_mtx(struct evdev_dev *evdev, struct mtx *mtx) { evdev->ev_lock_type = EV_LOCK_MTX; - evdev->ev_lock = mtx; + evdev->ev_state_lock = mtx; return (evdev_register_common(evdev)); } @@ -370,22 +380,23 @@ evdev_unregister(struct evdev_dev *evdev) sysctl_ctx_free(&evdev->ev_sysctl_ctx); - EVDEV_LOCK(evdev); + EVDEV_LIST_LOCK(evdev); evdev->ev_cdev->si_drv1 = NULL; /* Wake up sleepers */ - LIST_FOREACH_SAFE(client, &evdev->ev_clients, ec_link, tmp) { + CK_SLIST_FOREACH_SAFE(client, &evdev->ev_clients, ec_link, tmp) { evdev_revoke_client(client); evdev_dispose_client(evdev, client); EVDEV_CLIENT_LOCKQ(client); evdev_notify_event(client); EVDEV_CLIENT_UNLOCKQ(client); } - EVDEV_UNLOCK(evdev); + EVDEV_LIST_UNLOCK(evdev); - /* destroy_dev can sleep so release lock */ + /* release lock to avoid deadlock with evdev_dtor */ ret = evdev_cdev_destroy(evdev); evdev->ev_cdev = NULL; - if (ret == 0 && evdev->ev_lock_type == EV_LOCK_INTERNAL) + sx_destroy(&evdev->ev_list_lock); + if (ret == 0 && evdev->ev_lock_type != EV_LOCK_MTX) mtx_destroy(&evdev->ev_mtx); evdev_free_absinfo(evdev->ev_absinfo); @@ -689,7 +700,7 @@ evdev_modify_event(struct evdev_dev *evdev, uint16_t type, uint16_t code, } else { /* Start/stop callout for evdev repeats */ if (bit_test(evdev->ev_key_states, code) == !*value && - !LIST_EMPTY(&evdev->ev_clients)) { + !CK_SLIST_EMPTY(&evdev->ev_clients)) { if (*value == KEY_EVENT_DOWN) evdev_start_repeat(evdev, code); else @@ -817,6 +828,7 @@ static void evdev_propagate_event(struct evdev_dev *evdev, uint16_t type, uint16_t code, int32_t value) { + struct epoch_tracker et; struct evdev_client *client; debugf(evdev, "%s pushed event %d/%d/%d", @@ -825,7 +837,14 @@ evdev_propagate_event(struct evdev_dev *evdev, uint16_t type, uint16_t code, EVDEV_LOCK_ASSERT(evdev); /* Propagate event through all clients */ - LIST_FOREACH(client, &evdev->ev_clients, ec_link) { + if (evdev->ev_lock_type == EV_LOCK_INTERNAL) + epoch_enter_preempt(INPUT_EPOCH, &et); + + KASSERT( + evdev->ev_lock_type == EV_LOCK_MTX || in_epoch(INPUT_EPOCH) != 0, + ("Input epoch has not been entered\n")); + + CK_SLIST_FOREACH(client, &evdev->ev_clients, ec_link) { if (evdev->ev_grabber != NULL && evdev->ev_grabber != client) continue; @@ -835,6 +854,8 @@ evdev_propagate_event(struct evdev_dev *evdev, uint16_t type, uint16_t code, evdev_notify_event(client); EVDEV_CLIENT_UNLOCKQ(client); } + if (evdev->ev_lock_type == EV_LOCK_INTERNAL) + epoch_exit_preempt(INPUT_EPOCH, &et); evdev->ev_event_count++; } @@ -932,6 +953,7 @@ int evdev_inject_event(struct evdev_dev *evdev, uint16_t type, uint16_t code, int32_t value) { + struct epoch_tracker et; int ret = 0; switch (type) { @@ -962,11 +984,16 @@ evdev_inject_event(struct evdev_dev *evdev, uint16_t type, uint16_t code, case EV_ABS: case EV_SW: push: - if (evdev->ev_lock_type != EV_LOCK_INTERNAL) + if (evdev->ev_lock_type == EV_LOCK_MTX) EVDEV_LOCK(evdev); + else if (evdev->ev_lock_type == EV_LOCK_EXT_EPOCH) + epoch_enter_preempt(INPUT_EPOCH, &et); ret = evdev_push_event(evdev, type, code, value); - if (evdev->ev_lock_type != EV_LOCK_INTERNAL) + if (evdev->ev_lock_type == EV_LOCK_MTX) EVDEV_UNLOCK(evdev); + else if (evdev->ev_lock_type == EV_LOCK_EXT_EPOCH) + epoch_exit_preempt(INPUT_EPOCH, &et); + break; default: @@ -983,16 +1010,16 @@ evdev_register_client(struct evdev_dev *evdev, struct evdev_client *client) debugf(evdev, "adding new client for device %s", evdev->ev_shortname); - EVDEV_LOCK_ASSERT(evdev); + EVDEV_LIST_LOCK_ASSERT(evdev); - if (LIST_EMPTY(&evdev->ev_clients) && evdev->ev_methods != NULL && + if (CK_SLIST_EMPTY(&evdev->ev_clients) && evdev->ev_methods != NULL && evdev->ev_methods->ev_open != NULL) { debugf(evdev, "calling ev_open() on device %s", evdev->ev_shortname); ret = evdev->ev_methods->ev_open(evdev); } if (ret == 0) - LIST_INSERT_HEAD(&evdev->ev_clients, client, ec_link); + CK_SLIST_INSERT_HEAD(&evdev->ev_clients, client, ec_link); return (ret); } @@ -1001,18 +1028,27 @@ evdev_dispose_client(struct evdev_dev *evdev, struct evdev_client *client) { debugf(evdev, "removing client for device %s", evdev->ev_shortname); - EVDEV_LOCK_ASSERT(evdev); + EVDEV_LIST_LOCK_ASSERT(evdev); - LIST_REMOVE(client, ec_link); - if (LIST_EMPTY(&evdev->ev_clients)) { + CK_SLIST_REMOVE(&evdev->ev_clients, client, evdev_client, ec_link); + if (CK_SLIST_EMPTY(&evdev->ev_clients)) { if (evdev->ev_methods != NULL && evdev->ev_methods->ev_close != NULL) (void)evdev->ev_methods->ev_close(evdev); if (evdev_event_supported(evdev, EV_REP) && - bit_test(evdev->ev_flags, EVDEV_FLAG_SOFTREPEAT)) + bit_test(evdev->ev_flags, EVDEV_FLAG_SOFTREPEAT)) { + if (evdev->ev_lock_type != EV_LOCK_MTX) + EVDEV_LOCK(evdev); evdev_stop_repeat(evdev); + if (evdev->ev_lock_type != EV_LOCK_MTX) + EVDEV_UNLOCK(evdev); + } } + if (evdev->ev_lock_type != EV_LOCK_MTX) + EVDEV_LOCK(evdev); evdev_release_client(evdev, client); + if (evdev->ev_lock_type != EV_LOCK_MTX) + EVDEV_UNLOCK(evdev); } int @@ -1046,10 +1082,15 @@ evdev_release_client(struct evdev_dev *evdev, struct evdev_client *client) static void evdev_repeat_callout(void *arg) { + struct epoch_tracker et; struct evdev_dev *evdev = (struct evdev_dev *)arg; + if (evdev->ev_lock_type == EV_LOCK_EXT_EPOCH) + epoch_enter_preempt(INPUT_EPOCH, &et); evdev_send_event(evdev, EV_KEY, evdev->ev_rep_key, KEY_EVENT_REPEAT); evdev_send_event(evdev, EV_SYN, SYN_REPORT, 1); + if (evdev->ev_lock_type == EV_LOCK_EXT_EPOCH) + epoch_exit_preempt(INPUT_EPOCH, &et); if (evdev->ev_rep[REP_PERIOD]) callout_reset(&evdev->ev_rep_callout, diff --git a/sys/dev/evdev/evdev.h b/sys/dev/evdev/evdev.h index f584e52fc8e4..30d6a106d8b3 100644 --- a/sys/dev/evdev/evdev.h +++ b/sys/dev/evdev/evdev.h @@ -30,6 +30,7 @@ #define _DEV_EVDEV_EVDEV_H #include <sys/types.h> +#include <sys/epoch.h> #include <sys/kbio.h> #include <dev/evdev/input.h> #include <dev/kbd/kbdreg.h> @@ -87,6 +88,8 @@ extern int evdev_sysmouse_t_axis; * for MT protocol type B reports */ #define EVDEV_FLAG_MT_AUTOREL 0x02 /* Autorelease MT-slots not listed in * current MT protocol type B report */ +#define EVDEV_FLAG_EXT_EPOCH 0x03 /* evdev_push_* is allways called with + * input (global) epoch entered */ #define EVDEV_FLAG_MAX 0x1F #define EVDEV_FLAG_CNT (EVDEV_FLAG_MAX + 1) diff --git a/sys/dev/evdev/evdev_private.h b/sys/dev/evdev/evdev_private.h index d7f0b4eab62f..66a059c763bc 100644 --- a/sys/dev/evdev/evdev_private.h +++ b/sys/dev/evdev/evdev_private.h @@ -31,12 +31,15 @@ #define _DEV_EVDEV_EVDEV_PRIVATE_H #include <sys/bitstring.h> +#include <sys/ck.h> +#include <sys/epoch.h> #include <sys/kbio.h> #include <sys/lock.h> #include <sys/malloc.h> #include <sys/mutex.h> #include <sys/queue.h> #include <sys/selinfo.h> +#include <sys/sx.h> #include <sys/sysctl.h> #include <dev/evdev/evdev.h> @@ -75,10 +78,33 @@ enum evdev_clock_id EV_CLOCK_BOOTTIME /* monotonic, suspend-awared */ }; +/* + * Locking. + * + * Internal evdev structures are protected with next locks: + * State lock (s) - Internal state. The data it protects is changed + * by incoming evdev events and some ioctls. + * Client list epoch (l) - Read access to client list. + * Client list lock (l) - Write access to client list. + * Client queue locks (q) - One lock per client to serialize access to data + * available through character device node. + * + * Depending on evdev_register_() suffix evdev can run in following modes: + * 1. Internal epoch. evdev_register(). All locks are internal. + * 2. External epoch. Evdev expects to be run under input epoch entered by + * parent driver. The mode is enabled with EVDEV_FLAG_EXT_EPOCH flag. + * 3. External mutex. evdev_register_mtx(). Evdev uses mutex provided by parent + * driver as both "State lock" and "Client list lock". This mode is + * deprecated as it causes ev_open and ev_close handlers to be called with + * parent driver mutex taken. + */ +#define INPUT_EPOCH global_epoch_preempt + enum evdev_lock_type { - EV_LOCK_INTERNAL = 0, /* Internal evdev mutex */ + EV_LOCK_INTERNAL = 0, /* Internal epoch */ EV_LOCK_MTX, /* Driver`s mutex */ + EV_LOCK_EXT_EPOCH, /* External epoch */ }; struct evdev_dev @@ -89,10 +115,11 @@ struct evdev_dev struct cdev * ev_cdev; int ev_unit; enum evdev_lock_type ev_lock_type; - struct mtx * ev_lock; - struct mtx ev_mtx; + struct mtx * ev_state_lock; /* State lock */ + struct mtx ev_mtx; /* Internal state lock */ + struct sx ev_list_lock; /* Client list lock */ struct input_id ev_id; - struct evdev_client * ev_grabber; + struct evdev_client * ev_grabber; /* (s) */ size_t ev_report_size; /* Supported features: */ @@ -105,31 +132,31 @@ struct evdev_dev bitstr_t bit_decl(ev_led_flags, LED_CNT); bitstr_t bit_decl(ev_snd_flags, SND_CNT); bitstr_t bit_decl(ev_sw_flags, SW_CNT); - struct input_absinfo * ev_absinfo; + struct input_absinfo * ev_absinfo; /* (s) */ bitstr_t bit_decl(ev_flags, EVDEV_FLAG_CNT); /* Repeat parameters & callout: */ - int ev_rep[REP_CNT]; - struct callout ev_rep_callout; - uint16_t ev_rep_key; + int ev_rep[REP_CNT]; /* (s) */ + struct callout ev_rep_callout; /* (s) */ + uint16_t ev_rep_key; /* (s) */ /* State: */ - bitstr_t bit_decl(ev_key_states, KEY_CNT); - bitstr_t bit_decl(ev_led_states, LED_CNT); - bitstr_t bit_decl(ev_snd_states, SND_CNT); - bitstr_t bit_decl(ev_sw_states, SW_CNT); - bool ev_report_opened; + bitstr_t bit_decl(ev_key_states, KEY_CNT); /* (s) */ + bitstr_t bit_decl(ev_led_states, LED_CNT); /* (s) */ + bitstr_t bit_decl(ev_snd_states, SND_CNT); /* (s) */ + bitstr_t bit_decl(ev_sw_states, SW_CNT); /* (s) */ + bool ev_report_opened; /* (s) */ /* KDB state: */ bool ev_kdb_active; bitstr_t bit_decl(ev_kdb_led_states, LED_CNT); /* Multitouch protocol type B state: */ - struct evdev_mt * ev_mt; + struct evdev_mt * ev_mt; /* (s) */ /* Counters: */ - uint64_t ev_event_count; - uint64_t ev_report_count; + uint64_t ev_event_count; /* (s) */ + uint64_t ev_report_count; /* (s) */ /* Parent driver callbacks: */ const struct evdev_methods * ev_methods; @@ -139,47 +166,66 @@ struct evdev_dev struct sysctl_ctx_list ev_sysctl_ctx; LIST_ENTRY(evdev_dev) ev_link; - LIST_HEAD(, evdev_client) ev_clients; + CK_SLIST_HEAD(, evdev_client) ev_clients; /* (l) */ }; #define SYSTEM_CONSOLE_LOCK &Giant -#define EVDEV_LOCK(evdev) mtx_lock((evdev)->ev_lock) -#define EVDEV_UNLOCK(evdev) mtx_unlock((evdev)->ev_lock) +#define EVDEV_LOCK(evdev) mtx_lock((evdev)->ev_state_lock) +#define EVDEV_UNLOCK(evdev) mtx_unlock((evdev)->ev_state_lock) #define EVDEV_LOCK_ASSERT(evdev) do { \ - if ((evdev)->ev_lock != SYSTEM_CONSOLE_LOCK) \ - mtx_assert((evdev)->ev_lock, MA_OWNED); \ + if ((evdev)->ev_state_lock != SYSTEM_CONSOLE_LOCK) \ + mtx_assert((evdev)->ev_state_lock, MA_OWNED); \ } while (0) #define EVDEV_ENTER(evdev) do { \ - if ((evdev)->ev_lock_type == EV_LOCK_INTERNAL) \ + if ((evdev)->ev_lock_type != EV_LOCK_MTX) \ EVDEV_LOCK(evdev); \ else \ EVDEV_LOCK_ASSERT(evdev); \ } while (0) #define EVDEV_EXIT(evdev) do { \ - if ((evdev)->ev_lock_type == EV_LOCK_INTERNAL) \ + if ((evdev)->ev_lock_type != EV_LOCK_MTX) \ EVDEV_UNLOCK(evdev); \ } while (0) +#define EVDEV_LIST_LOCK(evdev) do { \ + if ((evdev)->ev_lock_type == EV_LOCK_MTX) \ + EVDEV_LOCK(evdev); \ + else \ + sx_xlock(&(evdev)->ev_list_lock); \ +} while (0) +#define EVDEV_LIST_UNLOCK(evdev) do { \ + if ((evdev)->ev_lock_type == EV_LOCK_MTX) \ + EVDEV_UNLOCK(evdev); \ + else \ + sx_unlock(&(evdev)->ev_list_lock); \ +} while (0) +#define EVDEV_LIST_LOCK_ASSERT(evdev) do { \ + if ((evdev)->ev_lock_type == EV_LOCK_MTX) \ + EVDEV_LOCK_ASSERT(evdev); \ + else \ + sx_assert(&(evdev)->ev_list_lock, MA_OWNED); \ +} while (0) + struct evdev_client { struct evdev_dev * ec_evdev; - struct mtx ec_buffer_mtx; + struct mtx ec_buffer_mtx; /* Client queue lock */ size_t ec_buffer_size; - size_t ec_buffer_head; - size_t ec_buffer_tail; - size_t ec_buffer_ready; + size_t ec_buffer_head; /* (q) */ + size_t ec_buffer_tail; /* (q) */ + size_t ec_buffer_ready; /* (q) */ enum evdev_clock_id ec_clock_id; - struct selinfo ec_selp; + struct selinfo ec_selp; /* (q) */ struct sigio * ec_sigio; - bool ec_async; - bool ec_revoked; - bool ec_blocked; - bool ec_selected; + bool ec_async; /* (q) */ + bool ec_revoked; /* (l) */ + bool ec_blocked; /* (q) */ + bool ec_selected; /* (q) */ - LIST_ENTRY(evdev_client) ec_link; + CK_SLIST_ENTRY(evdev_client) ec_link; /* (l) */ - struct input_event ec_buffer[]; + struct input_event ec_buffer[]; /* (q) */ }; #define EVDEV_CLIENT_LOCKQ(client) mtx_lock(&(client)->ec_buffer_mtx) _______________________________________________ [email protected] mailing list https://lists.freebsd.org/mailman/listinfo/dev-commits-src-all To unsubscribe, send any mail to "[email protected]"
