On Fri, 20 Jan 2012 18:24:16 -0800 Arve Hjønnevåg <[email protected]> wrote:
> Add an ioctl, EVIOCSSUSPENDBLOCK, to block suspend while the event
> queue is not empty. This allows userspace code to process input
> events while the device appears to be asleep.
>
> Signed-off-by: Arve Hjønnevåg <[email protected]>
This is exactly the sort of "feature creep" that I worried about in my reply
to Rafael's recent "autosleep" patches.
A particular issue here: This patch allows any process that can open an
input device to keep the device awake - by not reading an event that has
arrived (whether due to incompetence or malice).
So either we would need strict controls on who can open /dev/input/eventX,
or be happy that any process can disable suspend.
Or add some extra feature-creep to provide access control.
(or just keep this stuff out of the kernel and let a user-space daemon make
those decisions).
NeilBrown
> ---
> drivers/input/evdev.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/input.h | 3 ++
> 2 files changed, 66 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 76457d5..e212757 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -43,6 +43,7 @@ struct evdev_client {
> unsigned int tail;
> unsigned int packet_head; /* [future] position of the first element of next packet */
> spinlock_t buffer_lock; /* protects access to buffer, head and tail */
> + struct wakeup_source *wakeup_source;
> struct fasync_struct *fasync;
> struct evdev *evdev;
> struct list_head node;
> @@ -75,10 +76,14 @@ static void evdev_pass_event(struct evdev_client *client,
> client->buffer[client->tail].value = 0;
>
> client->packet_head = client->tail;
> + if (client->wakeup_source)
> + __pm_relax(client->wakeup_source);
> }
>
> if (event->type == EV_SYN && event->code == SYN_REPORT) {
> client->packet_head = client->head;
> + if (client->wakeup_source)
> + __pm_stay_awake(client->wakeup_source);
> kill_fasync(&client->fasync, SIGIO, POLL_IN);
> }
>
> @@ -255,6 +260,10 @@ static int evdev_release(struct inode *inode, struct file *file)
> mutex_unlock(&evdev->mutex);
>
> evdev_detach_client(evdev, client);
> + if (client->wakeup_source) {
> + __pm_relax(client->wakeup_source);
> + wakeup_source_unregister(client->wakeup_source);
> + }
> kfree(client);
>
> evdev_close_device(evdev);
> @@ -373,6 +382,9 @@ static int evdev_fetch_next_event(struct evdev_client *client,
> if (have_event) {
> *event = client->buffer[client->tail++];
> client->tail &= client->bufsize - 1;
> + if (client->wakeup_source &&
> + client->packet_head == client->tail)
> + __pm_relax(client->wakeup_source);
> }
>
> spin_unlock_irq(&client->buffer_lock);
> @@ -623,6 +635,48 @@ static int evdev_handle_set_keycode_v2(struct input_dev *dev, void __user *p)
> return input_set_keycode(dev, &ke);
> }
>
> +static int evdev_enable_suspend_block(struct evdev *evdev,
> + struct evdev_client *client)
> +{
> + struct wakeup_source *ws;
> + char name[28];
> +
> + if (client->wakeup_source)
> + return 0;
> +
> + snprintf(name, sizeof(name), "%s-%d",
> + dev_name(&evdev->dev), task_tgid_vnr(current));
> +
> + ws = wakeup_source_register(name);
> + if (!ws)
> + return -ENOMEM;
> +
> + spin_lock_irq(&client->buffer_lock);
> + client->wakeup_source = ws;
> + if (client->packet_head != client->tail)
> + __pm_stay_awake(client->wakeup_source);
> + spin_unlock_irq(&client->buffer_lock);
> + return 0;
> +}
> +
> +static int evdev_disable_suspend_block(struct evdev *evdev,
> + struct evdev_client *client)
> +{
> + struct wakeup_source *ws;
> +
> + spin_lock_irq(&client->buffer_lock);
> + ws = client->wakeup_source;
> + client->wakeup_source = NULL;
> + spin_unlock_irq(&client->buffer_lock);
> +
> + if (ws) {
> + __pm_relax(ws);
> + wakeup_source_unregister(ws);
> + }
> +
> + return 0;
> +}
> +
> static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> void __user *p, int compat_mode)
> {
> @@ -696,6 +750,15 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>
> case EVIOCSKEYCODE_V2:
> return evdev_handle_set_keycode_v2(dev, p);
> +
> + case EVIOCGSUSPENDBLOCK:
> + return put_user(!!client->wakeup_source, ip);
> +
> + case EVIOCSSUSPENDBLOCK:
> + if (p)
> + return evdev_enable_suspend_block(evdev, client);
> + else
> + return evdev_disable_suspend_block(evdev, client);
> }
>
> size = _IOC_SIZE(cmd);
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 3862e32..daf0177 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -129,6 +129,9 @@ struct input_keymap_entry {
>
> #define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */
>
> +#define EVIOCGSUSPENDBLOCK _IOR('E', 0x91, int) /* get suspend block enable */
> +#define EVIOCSSUSPENDBLOCK _IOW('E', 0x91, int) /* set suspend block enable */
> +
> /*
> * Device properties and quirks
> */
On Tuesday, February 14, 2012, NeilBrown wrote:
> On Fri, 20 Jan 2012 18:24:16 -0800 Arve Hjønnevåg <[email protected]> wrote:
>
> > Add an ioctl, EVIOCSSUSPENDBLOCK, to block suspend while the event
> > queue is not empty. This allows userspace code to process input
> > events while the device appears to be asleep.
> >
> > Signed-off-by: Arve Hjønnevåg <[email protected]>
>
> This is exactly the sort of "feature creep" that I worried about in my reply
> to Rafael's recent "autosleep" patches.
>
> A particular issue here: This patch allows any process that can open an
> input device to keep the device awake - by not reading an event that has
> arrived (whether due to incompetence or malice).
>
> So either we would need strict controls on who can open /dev/input/eventX,
> or be happy that any process can disable suspend.
> Or add some extra feature-creep to provide access control.
I actually think the approach almost correct, because if there are any events
coming from a wakeup device and there is any user space client interested in
them, the kernel should really stay awake until those events are removed from
the client's queue.
So, if they are not read, either we drop them entirely, or we don't suspend
_automatically_.
The problem I have with the $subject patch is that it doesn't check if the
device is a wakeup one and it adds ioctls allowing user space to use the event
queue of a non-wakeup device as a "wakeup source".
> (or just keep this stuff out of the kernel and let a user-space daemon make
> those decisions).
Which is never going to really work, IMHO.
Realistically, do you know of any distro, vendor, whoever, who tried to
actually do that in a released product (or even in a release candidate,
or milestone, or whatever different from a prototype running only on one's
personal desktop)? I don't.
Thanks,
Rafael
rafael j. wysocki wrote:
> On Tuesday, February 14, 2012, NeilBrown wrote:
>
> > (or just keep this stuff out of the kernel and let a user-space daemon make
> > those decisions).
>
> Which is never going to really work, IMHO.
>
> Realistically, do you know of any distro, vendor, whoever, who tried to
> actually do that in a released product (or even in a release candidate,
> or milestone, or whatever different from a prototype running only on one's
> personal desktop)? I don't.
well, depending on your decision of "that", there are something like
2.5 million OLPC XO laptops that do it. do they count? ;-)
we're still in the middle of converting our 2.6-era home-grown power
management mechanisms to the 3.0-era level, using the
.../power/wakeup[_count] and /sys/power/wakeup_count mechanisms.
(change comes slowly to shipping products.) but we do have a
user-level suspend manager.
to the real point of your question: no, i don't think it does what
you're talking about yet -- i.e., control by applications over whether
suspend should be permitted or not exists, but isn't nearly as
reliable or as foolproof as any of the mechanisms discussed here
recently.
paul
=---------------------
paul fox, [email protected]
On Thursday, February 16, 2012, Paul Fox wrote:
> rafael j. wysocki wrote:
> > On Tuesday, February 14, 2012, NeilBrown wrote:
> >
> > > (or just keep this stuff out of the kernel and let a user-space daemon make
> > > those decisions).
> >
> > Which is never going to really work, IMHO.
> >
> > Realistically, do you know of any distro, vendor, whoever, who tried to
> > actually do that in a released product (or even in a release candidate,
> > or milestone, or whatever different from a prototype running only on one's
> > personal desktop)? I don't.
>
> well, depending on your decision of "that", there are something like
> 2.5 million OLPC XO laptops that do it. do they count? ;-)
>
> we're still in the middle of converting our 2.6-era home-grown power
> management mechanisms to the 3.0-era level, using the
> .../power/wakeup[_count] and /sys/power/wakeup_count mechanisms.
> (change comes slowly to shipping products.) but we do have a
> user-level suspend manager.
>
> to the real point of your question: no, i don't think it does what
> you're talking about yet -- i.e., control by applications over whether
> suspend should be permitted or not exists, but isn't nearly as
> reliable or as foolproof as any of the mechanisms discussed here
> recently.
OK, cool!
I was wrong then, but good to hear that. :-)
Thanks,
Rafael