Date:   Tue, 20 Aug 2019 13:56:53 +0200
From:   Michal Vokáč <michal.vokac@...ft.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Rob Herring <robh@...nel.org>
Subject: Re: [PATCH] Input: add support for polling to input devices

On 13. 08. 19 16:04, Benjamin Tissoires wrote:
> On Mon, Aug 12, 2019 at 7:11 PM Dmitry Torokhov
> <dmitry.torokhov@...il.com> 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@...il.com> 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@...hat.com>
> 

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@...ft.com>
>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
>>>> ---
>>>>   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@...e.cz>");
>>>>   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

Powered by blists - more mailing lists