[PATCH libevdev 4/4] Transparently use EVIOCSMASK to disable event codes
Peter Hutterer
peter.hutterer at who-t.net
Mon Apr 25 23:54:55 UTC 2016
On Fri, Apr 22, 2016 at 10:58:16AM +0200, David Herrmann wrote:
> Hi
>
> On Thu, Apr 21, 2016 at 11:59 PM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > Disable the event code at the kernel level. When set, the kernel doesn't pass
> > that code on to userspace at all so we reduce the wakeups needed.
> >
> > Rather than setting the mask immediately on disabling/enabling, we wait until
> > the next libevdev_next_event() call and update the mask then. This reduces
> > ioctls if multiple event codes need to be disabled or enabled.
> >
> > This introduces a behavior change: previously, libevdev would update the
> > internal device state even for disabled event codes. Even though the event
> > wouldn't reach the client, a call to libevdev_get_event_value() would yield
> > the current kernel state. With this state libevdev itself does not see the
> > events and cannot update its state. libevdev_get_event_value() will thus
> > return the last known state only. The use-case of disabling a code but relying
> > on libevdev's state is discouraged though.
>
> I *really* like the idea of using EVIOCSMASK transparently in
> libevdev. Especially given the awkward overflow behavior, it should
> really be hidden in the library. Anyway, I think the main use-case for
> masking event-codes is a whitelist rather than a blacklist. So clients
> want to do things like: give me nothing but KEY_POWER. With the
> current libevdev API this is a bit awkward, as you need to iterate all
> KEYs, disable each one that *might* be present, ... I guess you see my
> point.
>
> Anyway, this is probably unrelated to this patch, as a simple addition
> of something like libevdev_disable_everything() would be enough.
tbh, I'd be worried about confusion with the libevdev_disable_event_type()
API call. and it's a two-liner to disable all codes
for (i = i; i < KEY_CNT; i++)
libevdev_disable_event_code(i)
That's quite expressive, possibly more so than a
libevdev_disable_event_code_all_for_type() or something.
so yeah, right now I'm not convinced we need another API here unless you
manage to come up with a good name :)
> However, what is the reason you don't allow masking entire TYPEs? So
> we already have libevdev_disable_event_type(), along
> libevdev_disable_event_code(). EVIOCSMASK allows disabling entire
> types by using EV_SYN (special behavior, just like for EVIOCGBIT).
> Right now, if you call libevdev_disable_event_type(), it will not make
> use of EVIOCSMASK.
hmm, this isn't immediately obvious I admit but it makes sense. I hadn't
done the type disabling yet simply because with the quirky mask behaviour I
wasn't 100% yet how to handle it the best way. I'll send a follow-up for
that if we're happy with the transparent behaviour.
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > libevdev/libevdev-int.h | 6 ++++++
> > libevdev/libevdev.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> > libevdev/libevdev.h | 5 +++++
> > 3 files changed, 60 insertions(+)
> >
> > diff --git a/libevdev/libevdev-int.h b/libevdev/libevdev-int.h
> > index 6c5b8b6..bced2e2 100644
> > --- a/libevdev/libevdev-int.h
> > +++ b/libevdev/libevdev-int.h
> > @@ -96,6 +96,12 @@ struct libevdev {
> > unsigned long key_values[NLONGS(KEY_CNT)];
> > unsigned long led_values[NLONGS(LED_CNT)];
> > unsigned long sw_values[NLONGS(SW_CNT)];
> > + /* A bit for each event type, if set we need to update the
> > + * EVIOCSMASK. EV_SYN is used as general flag that another one has
> > + * changed
> > + */
> > + unsigned long mask_update_needed[NLONGS(EV_CNT)];
> > +
> > struct input_absinfo abs_info[ABS_CNT];
> > int *mt_slot_vals; /* [num_slots * ABS_MT_CNT] */
> > int num_slots; /**< valid slots in mt_slot_vals */
> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> > index 149a0d7..194b17c 100644
> > --- a/libevdev/libevdev.c
> > +++ b/libevdev/libevdev.c
> > @@ -24,6 +24,7 @@
> > #include <errno.h>
> > #include <poll.h>
> > #include <stdlib.h>
> > +#include <stdint.h>
> > #include <string.h>
> > #include <limits.h>
> > #include <unistd.h>
> > @@ -1007,6 +1008,43 @@ sanitize_event(const struct libevdev *dev,
> > return EVENT_FILTER_NONE;
> > }
> >
> > +static inline int
> > +update_event_mask(struct libevdev *dev)
> > +{
> > + int rc = 0;
> > + int type;
> > +
> > + /* skip EV_SYN, it's the general "update needed" flag */
> > + for (type = 1; type < EV_CNT; type++) {
> > + unsigned long *m;
> > + int max;
> > + size_t sz;
> > + struct input_mask mask;
> > +
> > + if (!bit_is_set(dev->mask_update_needed, type))
> > + continue;
> > +
> > + max = type_to_mask(dev, type, &m, &sz);
> > + if (max == -1)
> > + continue;
> > +
> > + mask.type = type;
> > + mask.codes_size = sz;
> > + mask.codes_ptr = (uint64_t)m;
> > + rc = ioctl(dev->fd, EVIOCSMASK, &mask);
> > + if (rc == -1) {
> > + if (errno == EINVAL) /* unsupported by kernel */
> > + rc = 0;
> > + goto out;
>
> So we don't cache whether EVIOCSMASK is supported. Probably fine,
> since you're not supposed to change your masks at runtime at all, so
> we call it only once anyway. Just wanted to know whether that is
> intentional?
yes. there may be some use-case for disabling codes at runtime but I think
it's a niche case and not something we need to worry about until we see a
real requirement for it.
> Otherwise, I really like it! Thanks!
thanks!
Cheers,
Peter
> David
>
> > + }
> > + }
> > +
> > +out:
> > + memset(dev->mask_update_needed, 0, sizeof(dev->mask_update_needed));
> > +
> > + return rc == -1 ? -errno : 0;
> > +}
> > +
> > LIBEVDEV_EXPORT int
> > libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event *ev)
> > {
> > @@ -1028,6 +1066,9 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> > return -EINVAL;
> > }
> >
> > + if (bit_is_set(dev->mask_update_needed, EV_SYN))
> > + update_event_mask(dev);
> > +
> > if (flags & LIBEVDEV_READ_FLAG_SYNC) {
> > if (dev->sync_state == SYNC_NEEDED) {
> > rc = sync_state(dev);
> > @@ -1485,6 +1526,10 @@ libevdev_enable_event_code(struct libevdev *dev, unsigned int type,
> > return -1;
> >
> > set_bit(mask, code);
> > + if (type != EV_SYN) {
> > + set_bit(dev->mask_update_needed, type);
> > + set_bit(dev->mask_update_needed, EV_SYN);
> > + }
> >
> > if (type == EV_ABS) {
> > const struct input_absinfo *abs = data;
> > @@ -1512,6 +1557,10 @@ libevdev_disable_event_code(struct libevdev *dev, unsigned int type, unsigned in
> > return -1;
> >
> > clear_bit(mask, code);
> > + if (type != EV_SYN) {
> > + set_bit(dev->mask_update_needed, type);
> > + set_bit(dev->mask_update_needed, EV_SYN);
> > + }
> >
> > return 0;
> > }
> > diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> > index 856aeb9..a12a8d7 100644
> > --- a/libevdev/libevdev.h
> > +++ b/libevdev/libevdev.h
> > @@ -528,6 +528,8 @@ extern "C" {
> > * <dt>EVIOCREVOKE:</dt>
> > * <dd>currently not supported, see
> > * http://lists.freedesktop.org/archives/input-tools/2014-January/000688.html</dd>
> > + * <dt>EVIOCGMASK/EVIOCGSMASK:</dt>
> > + * <dd>supported, see libevdev_disable_event_code()
> > * </dl>
> > *
> > */
> > @@ -1858,6 +1860,9 @@ int libevdev_enable_event_code(struct libevdev *dev, unsigned int type, unsigned
> > * Disabling codes of type EV_SYN will not work. Don't shoot yourself in the
> > * foot. It hurts.
> > *
> > + * libevdev may transparently use the EVIOCSMASK ioctl if available. This
> > + * ioctl disables delivery of that event type in the kernel.
> > + *
> > * @note This function should only be invoked when the respective event type
> > * is in a neutral state. Otherwise, a caller may get unexpected event
> > * sequences. For example, disabling an event code while a touch is down may
> > --
> > 2.7.3
> >
> > _______________________________________________
> > Input-tools mailing list
> > Input-tools at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/input-tools
More information about the Input-tools
mailing list