Re: [PATCH] Input: add support for polling to input devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 13. 08. 19 16:04, Benjamin Tissoires wrote:
On Mon, Aug 12, 2019 at 7:11 PM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:

Hi Benjamin,

On Mon, Aug 12, 2019 at 06:50:38PM +0200, Benjamin Tissoires wrote:
Hi Dmitry,

On Fri, Aug 9, 2019 at 7:35 PM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:

Separating "normal" and "polled" input devices was a mistake, as often we
want to allow the very same device work on both interrupt-driven and
polled mode, depending on the board on which the device is used.

This introduces new APIs:

- input_setup_polling
- input_set_poll_interval
- input_set_min_poll_interval
- input_set_max_poll_interval

These new APIs allow switching an input device into polled mode with sysfs
attributes matching drivers using input_polled_dev APIs that will be
eventually removed.

Are you sure that using sysfs is the correct way here?
I would think using generic properties that can be overwritten by the
DSDT or by the device tree would make things easier from a driver
point of view.

Couple of points: I wanted it to be compatible with input-polldev.c so
the sysfs attributes must match (so that we can convert existing drivers
and zap input-polldev).

Oh, I missed that. Good point.

I also am not sure if polling parameters are
property of board, or it is either fundamental hardware limitation or
simply desired system behavior.

I think it's a combination of everything: sometimes the board misses
the capability to not do IRQs for that device, and using properties
would be better here: you can define them where you need (board,
platform or device level), and have a working platfrom from the kernel
description entirely.
However, it doesn't solve the issue of input-polldev, so maybe
properties should be added on top of this sysfs.

If Rob is OK with adding device
properties I'd be fine adding them as a followup, otherwise we can have
udev adjust the behavior as needed for given box shortly after boot.

Fair enough.



Also, checkpatch complains about a few octal permissions that are
preferred over symbolic ones, and there is a possible 'out of memory'
nessage at drivers/input/input-poller.c:75.

Yes, there is. It is there so we would know what device we were trying
to set up when OOM happened. You can probable decipher the driver from
the stack trace, but figuring particular device instance is harder.

Could you add a comment there explaining this choice? I have a feeling
you'll have to refuse a few patches of people running coccinelle
scripts and be too happy to send a kernel patch.

Other than that:
Acked-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>


Hi Dmitry,

what is the status of this patch? Are we still waiting for Rob to
comment on the device properties or is this ready to land?

Little bit OT question: what tree/branch do you use to apply patches?
According to the mailing list you recently applied some patches but
I can not find them here [1].

Thank you,
Michal

[1] https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/


Tested-by: Michal Vokáč <michal.vokac@xxxxxxxxx>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
---
  drivers/input/Makefile       |   2 +-
  drivers/input/input-poller.c | 208 +++++++++++++++++++++++++++++++++++
  drivers/input/input-poller.h |  18 +++
  drivers/input/input.c        |  36 ++++--
  include/linux/input.h        |  12 ++
  5 files changed, 268 insertions(+), 8 deletions(-)

diff --git a/drivers/input/Makefile b/drivers/input/Makefile
index 40de6a7be641..e35650930371 100644
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -6,7 +6,7 @@
  # Each configuration option enables a list of files.

  obj-$(CONFIG_INPUT)            += input-core.o
-input-core-y := input.o input-compat.o input-mt.o ff-core.o
+input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o

  obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
  obj-$(CONFIG_INPUT_POLLDEV)    += input-polldev.o
diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
new file mode 100644
index 000000000000..e041adb04f5a
--- /dev/null
+++ b/drivers/input/input-poller.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Support for polling mode for input devices.
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/jiffies.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+#include "input-poller.h"
+
+struct input_dev_poller {
+       void (*poll)(struct input_dev *dev);
+
+       unsigned int poll_interval; /* msec */
+       unsigned int poll_interval_max; /* msec */
+       unsigned int poll_interval_min; /* msec */
+
+       struct input_dev *input;
+       struct delayed_work work;
+};
+
+static void input_dev_poller_queue_work(struct input_dev_poller *poller)
+{
+       unsigned long delay;
+
+       delay = msecs_to_jiffies(poller->poll_interval);
+       if (delay >= HZ)
+               delay = round_jiffies_relative(delay);
+
+       queue_delayed_work(system_freezable_wq, &poller->work, delay);
+}
+
+static void input_dev_poller_work(struct work_struct *work)
+{
+       struct input_dev_poller *poller =
+               container_of(work, struct input_dev_poller, work.work);
+
+       poller->poll(poller->input);
+       input_dev_poller_queue_work(poller);
+}
+
+void input_dev_poller_finalize(struct input_dev_poller *poller)
+{
+       if (!poller->poll_interval)
+               poller->poll_interval = 500;
+       if (!poller->poll_interval_max)
+               poller->poll_interval_max = poller->poll_interval;
+}
+
+void input_dev_poller_start(struct input_dev_poller *poller)
+{
+       /* Only start polling if polling is enabled */
+       if (poller->poll_interval > 0) {
+               poller->poll(poller->input);
+               input_dev_poller_queue_work(poller);
+       }
+}
+
+void input_dev_poller_stop(struct input_dev_poller *poller)
+{
+       cancel_delayed_work_sync(&poller->work);
+}
+
+int input_setup_polling(struct input_dev *dev,
+                       void (*poll_fn)(struct input_dev *dev))
+{
+       struct input_dev_poller *poller;
+
+       poller = kzalloc(sizeof(*poller), GFP_KERNEL);
+       if (!poller) {
+               dev_err(dev->dev.parent ?: &dev->dev,
+                       "%s: unable to allocate poller structure\n", __func__);
+               return -ENOMEM;
+       }
+
+       INIT_DELAYED_WORK(&poller->work, input_dev_poller_work);
+       poller->input = dev;
+       poller->poll = poll_fn;
+
+       dev->poller = poller;
+       return 0;
+}
+EXPORT_SYMBOL(input_setup_polling);
+
+static bool input_dev_ensure_poller(struct input_dev *dev)
+{
+       if (!dev->poller) {
+               dev_err(dev->dev.parent ?: &dev->dev,
+                       "poller structure has not been set up\n");
+               return false;
+       }
+
+       return true;
+}
+
+void input_set_poll_interval(struct input_dev *dev, unsigned int interval)
+{
+       if (input_dev_ensure_poller(dev))
+               dev->poller->poll_interval = interval;
+}
+EXPORT_SYMBOL(input_set_poll_interval);
+
+void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval)
+{
+       if (input_dev_ensure_poller(dev))
+               dev->poller->poll_interval_min = interval;
+}
+EXPORT_SYMBOL(input_set_min_poll_interval);
+
+void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval)
+{
+       if (input_dev_ensure_poller(dev))
+               dev->poller->poll_interval_max = interval;
+}
+EXPORT_SYMBOL(input_set_max_poll_interval);
+
+/* SYSFS interface */
+
+static ssize_t input_dev_get_poll_interval(struct device *dev,
+                                          struct device_attribute *attr,
+                                          char *buf)
+{
+       struct input_dev *input = to_input_dev(dev);
+
+       return sprintf(buf, "%d\n", input->poller->poll_interval);
+}
+
+static ssize_t input_dev_set_poll_interval(struct device *dev,
+                                          struct device_attribute *attr,
+                                          const char *buf, size_t count)
+{
+       struct input_dev *input = to_input_dev(dev);
+       struct input_dev_poller *poller = input->poller;
+       unsigned int interval;
+       int err;
+
+       err = kstrtouint(buf, 0, &interval);
+       if (err)
+               return err;
+
+       if (interval < poller->poll_interval_min)
+               return -EINVAL;
+
+       if (interval > poller->poll_interval_max)
+               return -EINVAL;
+
+       mutex_lock(&input->mutex);
+
+       poller->poll_interval = interval;
+
+       if (input->users) {
+               cancel_delayed_work_sync(&poller->work);
+               if (poller->poll_interval > 0)
+                       input_dev_poller_queue_work(poller);
+       }
+
+       mutex_unlock(&input->mutex);
+
+       return count;
+}
+
+static DEVICE_ATTR(poll, S_IRUGO | S_IWUSR,
+                  input_dev_get_poll_interval, input_dev_set_poll_interval);
+
+static ssize_t input_dev_get_poll_max(struct device *dev,
+                                     struct device_attribute *attr, char *buf)
+{
+       struct input_dev *input = to_input_dev(dev);
+
+       return sprintf(buf, "%d\n", input->poller->poll_interval_max);
+}
+
+static DEVICE_ATTR(max, S_IRUGO, input_dev_get_poll_max, NULL);
+
+static ssize_t input_dev_get_poll_min(struct device *dev,
+                                    struct device_attribute *attr, char *buf)
+{
+       struct input_dev *input = to_input_dev(dev);
+
+       return sprintf(buf, "%d\n", input->poller->poll_interval_min);
+}
+
+static DEVICE_ATTR(min, S_IRUGO, input_dev_get_poll_min, NULL);
+
+static umode_t input_poller_attrs_visible(struct kobject *kobj,
+                                         struct attribute *attr, int n)
+{
+       struct device *dev = kobj_to_dev(kobj);
+       struct input_dev *input = to_input_dev(dev);
+
+       return input->poller ? attr->mode : 0;
+}
+
+static struct attribute *input_poller_attrs[] = {
+       &dev_attr_poll.attr,
+       &dev_attr_max.attr,
+       &dev_attr_min.attr,
+       NULL
+};
+
+struct attribute_group input_poller_attribute_group = {
+       .is_visible     = input_poller_attrs_visible,
+       .attrs          = input_poller_attrs,
+};
diff --git a/drivers/input/input-poller.h b/drivers/input/input-poller.h
new file mode 100644
index 000000000000..e3fca0be1d32
--- /dev/null
+++ b/drivers/input/input-poller.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _INPUT_POLLER_H
+#define _INPUT_POLLER_H
+
+/*
+ * Support for polling mode for input devices.
+ */
+#include <linux/sysfs.h>
+
+struct input_dev_poller;
+
+void input_dev_poller_finalize(struct input_dev_poller *poller);
+void input_dev_poller_start(struct input_dev_poller *poller);
+void input_dev_poller_stop(struct input_dev_poller *poller);
+
+extern struct attribute_group input_poller_attribute_group;
+
+#endif /* _INPUT_POLLER_H */
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 7494a0dede79..c08aa3596144 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -24,6 +24,7 @@
  #include <linux/mutex.h>
  #include <linux/rcupdate.h>
  #include "input-compat.h"
+#include "input-poller.h"

  MODULE_AUTHOR("Vojtech Pavlik <vojtech@xxxxxxx>");
  MODULE_DESCRIPTION("Input core");
@@ -603,20 +604,31 @@ int input_open_device(struct input_handle *handle)

         handle->open++;

-       if (!dev->users++ && dev->open)
-               retval = dev->open(dev);
+       if (dev->users++) {
+               /*
+                * Device is already opened, so we can exit immediately and
+                * report success.
+                */
+               goto out;
+       }

-       if (retval) {
-               dev->users--;
-               if (!--handle->open) {
+       if (dev->open) {
+               retval = dev->open(dev);
+               if (retval) {
+                       dev->users--;
+                       handle->open--;
                         /*
                          * Make sure we are not delivering any more events
                          * through this handle
                          */
                         synchronize_rcu();
+                       goto out;
                 }
         }

+       if (dev->poller)
+               input_dev_poller_start(dev->poller);
+
   out:
         mutex_unlock(&dev->mutex);
         return retval;
@@ -655,8 +667,13 @@ void input_close_device(struct input_handle *handle)

         __input_release_device(handle);

-       if (!--dev->users && dev->close)
-               dev->close(dev);
+       if (!--dev->users) {
+               if (dev->poller)
+                       input_dev_poller_stop(dev->poller);
+
+               if (dev->close)
+                       dev->close(dev);
+       }

         if (!--handle->open) {
                 /*
@@ -1502,6 +1519,7 @@ static const struct attribute_group *input_dev_attr_groups[] = {
         &input_dev_attr_group,
         &input_dev_id_attr_group,
         &input_dev_caps_attr_group,
+       &input_poller_attribute_group,
         NULL
  };

@@ -1511,6 +1529,7 @@ static void input_dev_release(struct device *device)

         input_ff_destroy(dev);
         input_mt_destroy_slots(dev);
+       kfree(dev->poller);
         kfree(dev->absinfo);
         kfree(dev->vals);
         kfree(dev);
@@ -2175,6 +2194,9 @@ int input_register_device(struct input_dev *dev)
         if (!dev->setkeycode)
                 dev->setkeycode = input_default_setkeycode;

+       if (dev->poller)
+               input_dev_poller_finalize(dev->poller);
+
         error = device_add(&dev->dev);
         if (error)
                 goto err_free_vals;
diff --git a/include/linux/input.h b/include/linux/input.h
index e95a439d8bd5..94f277cd806a 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -21,6 +21,8 @@
  #include <linux/timer.h>
  #include <linux/mod_devicetable.h>

+struct input_dev_poller;
+
  /**
   * struct input_value - input value representation
   * @type: type of value (EV_KEY, EV_ABS, etc)
@@ -71,6 +73,8 @@ enum input_clock_type {
   *     not sleep
   * @ff: force feedback structure associated with the device if device
   *     supports force feedback effects
+ * @poller: poller structure associated with the device if device is
+ *     set up to use polling mode
   * @repeat_key: stores key code of the last key pressed; used to implement
   *     software autorepeat
   * @timer: timer for software autorepeat
@@ -156,6 +160,8 @@ struct input_dev {

         struct ff_device *ff;

+       struct input_dev_poller *poller;
+
         unsigned int repeat_key;
         struct timer_list timer;

@@ -372,6 +378,12 @@ void input_unregister_device(struct input_dev *);

  void input_reset_device(struct input_dev *);

+int input_setup_polling(struct input_dev *dev,
+                       void (*poll_fn)(struct input_dev *dev));
+void input_set_poll_interval(struct input_dev *dev, unsigned int interval);
+void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval);
+void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval);
+
  int __must_check input_register_handler(struct input_handler *);
  void input_unregister_handler(struct input_handler *);

--
2.23.0.rc1.153.gdeed80330f-goog


--
Dmitry

--
Dmitry




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux