[PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Arve Hjønnevåg]

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 <arve@android.com>
---
 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
  */
-- 
1.7.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Rafael J. Wysocki]

Hi,

On Saturday, January 21, 2012, Arve Hjønnevåg 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.

I have two questions to start with, if you don't mind:

(1) Does Android user space use an analogous interface right now or is it
    a prototype?

(2) What exactly are the use cases for it (ie. what problem does it address
    that cannot be addressed in a different way)?

Rafael

> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
>  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
>   */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Arve Hjønnevåg]

2012/2/11 Rafael J. Wysocki <rjw@sisk.pl>:
> Hi,
>
> On Saturday, January 21, 2012, Arve Hjønnevåg 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.
>
> I have two questions to start with, if you don't mind:
>
> (1) Does Android user space use an analogous interface right now or is it
>    a prototype?
>

Yes (for the next version), but with a wakelock based implementation.

> (2) What exactly are the use cases for it (ie. what problem does it address
>    that cannot be addressed in a different way)?
>

The reason for adding an ioctl versus the old android version with
used a wakelock for all input events, is that not all input events are
wakeup events. Specifically some sensors generate input events at such
a high rate that they prevent suspend while the sensor is on even
though the driver has a suspend hook that turns the sensor off.

If you are asking why input events need to block suspend at all, this
was the example used in the suspend blocker documentation:
- The Keypad driver gets an interrupt. It then calls suspend_block on the
  keypad-scan suspend_blocker and starts scanning the keypad matrix.
- The keypad-scan code detects a key change and reports it to the input-event
  driver.
- The input-event driver sees the key change, enqueues an event, and calls
  suspend_block on the input-event-queue suspend_blocker.
- The keypad-scan code detects that no keys are held and calls suspend_unblock
  on the keypad-scan suspend_blocker.
- The user-space input-event thread returns from select/poll, calls
  suspend_block on the process-input-events suspend_blocker and then calls read
  on the input-event device.
- The input-event driver dequeues the key-event and, since the queue is now
  empty, it calls suspend_unblock on the input-event-queue suspend_blocker.
- The user-space input-event thread returns from read. If it determines that
  the key should be ignored, it calls suspend_unblock on the
  process_input_events suspend_blocker and then calls select or poll. The
  system will automatically suspend again, since now no suspend blockers are
  active.


-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Rafael J. Wysocki]

Hi,

On Tuesday, February 14, 2012, Arve Hjønnevåg wrote:
> 2012/2/11 Rafael J. Wysocki <rjw@sisk.pl>:
> > Hi,
> >
> > On Saturday, January 21, 2012, Arve Hjønnevåg 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.
> >
> > I have two questions to start with, if you don't mind:
> >
> > (1) Does Android user space use an analogous interface right now or is it
> >    a prototype?
> >
> 
> Yes (for the next version), but with a wakelock based implementation.

That's exactly what I wanted to know. :-)

> > (2) What exactly are the use cases for it (ie. what problem does it address
> >    that cannot be addressed in a different way)?
> >
> 
> The reason for adding an ioctl versus the old android version with
> used a wakelock for all input events, is that not all input events are
> wakeup events. Specifically some sensors generate input events at such
> a high rate that they prevent suspend while the sensor is on even
> though the driver has a suspend hook that turns the sensor off.
> 
> If you are asking why input events need to block suspend at all, this
> was the example used in the suspend blocker documentation:
> - The Keypad driver gets an interrupt. It then calls suspend_block on the
>   keypad-scan suspend_blocker and starts scanning the keypad matrix.
> - The keypad-scan code detects a key change and reports it to the input-event
>   driver.
> - The input-event driver sees the key change, enqueues an event, and calls
>   suspend_block on the input-event-queue suspend_blocker.
> - The keypad-scan code detects that no keys are held and calls suspend_unblock
>   on the keypad-scan suspend_blocker.
> - The user-space input-event thread returns from select/poll, calls
>   suspend_block on the process-input-events suspend_blocker and then calls read
>   on the input-event device.
> - The input-event driver dequeues the key-event and, since the queue is now
>   empty, it calls suspend_unblock on the input-event-queue suspend_blocker.
> - The user-space input-event thread returns from read. If it determines that
>   the key should be ignored, it calls suspend_unblock on the
>   process_input_events suspend_blocker and then calls select or poll. The
>   system will automatically suspend again, since now no suspend blockers are
>   active.

That looks reasonable to me.

Still, I have one issue with adding those ioctls.  Namely, wouldn't it be
simpler to treat all events coming from wakeup devices as wakeup events?

With the new ioctls user space can "mark" a queue of events coming out of
a device that's not a wakeup one as a "wakeup source", which doesn't seem to
be correct.

Conversely, with those ioctls user space can effectively turn events coming
out of a wakeup device into non-wakeup ones, which doesn't seem to be correct
either.

So, I think evdev client queue's wakeup source (or wakelock) should stay active
if there are any events in the queue and the underlying device is a wakeup one,
i.e. device_may_wakeup() returns true for it.  Then, we won't need the new
ioctls at all and user space will still be able to control which events are to
be regarded as wakeup ones by changing the wakeup settings of the devices that
generate them.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Arve Hjønnevåg]

2012/2/15 Rafael J. Wysocki <rjw@sisk.pl>:
...
> Still, I have one issue with adding those ioctls.  Namely, wouldn't it be
> simpler to treat all events coming from wakeup devices as wakeup events?
>

User-space needs to block suspend between select/poll and read for
this to work, so an explicit call to enable this is useful.

> With the new ioctls user space can "mark" a queue of events coming out of
> a device that's not a wakeup one as a "wakeup source", which doesn't seem to
> be correct.
>

OK, but I don't think this is a big problem.

> Conversely, with those ioctls user space can effectively turn events coming
> out of a wakeup device into non-wakeup ones, which doesn't seem to be correct
> either.
>

I don't agree with this. There can be multiple readers on an input
device. Only the reader that is responsible for acting on the wakeup
event should call this ioctl.

> So, I think evdev client queue's wakeup source (or wakelock) should stay active
> if there are any events in the queue and the underlying device is a wakeup one,

I don't want additional readers of the device to prevent suspend.

> i.e. device_may_wakeup() returns true for it.  Then, we won't need the new
> ioctls at all and user space will still be able to control which events are to
> be regarded as wakeup ones by changing the wakeup settings of the devices that
> generate them.
>

I don't think this is currently hooked up for most of the devices we
have. If we do add a dynamic wakeup settings I would prefer to have an
ioctl to set which events on a device should be enabled for wakeup (or
just enabled) instead of switching the entire drive. That way we could
turn off unneeded rows and columns in a key matrix.

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Rafael J. Wysocki]

On Thursday, February 16, 2012, Arve Hjønnevåg wrote:
> 2012/2/15 Rafael J. Wysocki <rjw@sisk.pl>:
> ...
> > Still, I have one issue with adding those ioctls.  Namely, wouldn't it be
> > simpler to treat all events coming from wakeup devices as wakeup events?
> >
> 
> User-space needs to block suspend between select/poll and read for
> this to work, so an explicit call to enable this is useful.
> 
> > With the new ioctls user space can "mark" a queue of events coming out of
> > a device that's not a wakeup one as a "wakeup source", which doesn't seem to
> > be correct.
> >
> 
> OK, but I don't think this is a big problem.
> 
> > Conversely, with those ioctls user space can effectively turn events coming
> > out of a wakeup device into non-wakeup ones, which doesn't seem to be correct
> > either.
> >
> 
> I don't agree with this. There can be multiple readers on an input
> device. Only the reader that is responsible for acting on the wakeup
> event should call this ioctl.

Ah, OK.  So you envision the new ioctls as the way of saying "I'm the one
who's going to take care of those wakeup events"?  If that's the case, may
I suggest changing the names?

> > So, I think evdev client queue's wakeup source (or wakelock) should stay active
> > if there are any events in the queue and the underlying device is a wakeup one,
> 
> I don't want additional readers of the device to prevent suspend.

OK

> > i.e. device_may_wakeup() returns true for it.  Then, we won't need the new
> > ioctls at all and user space will still be able to control which events are to
> > be regarded as wakeup ones by changing the wakeup settings of the devices that
> > generate them.
> >
> 
> I don't think this is currently hooked up for most of the devices we
> have. If we do add a dynamic wakeup settings I would prefer to have an
> ioctl to set which events on a device should be enabled for wakeup (or
> just enabled) instead of switching the entire drive. That way we could
> turn off unneeded rows and columns in a key matrix.

Can you actually select what keys may wake up the system from sleep
(I mean "real sleep", when the whole system is entirely off except for wakeup
devices)?

Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On Thu, Feb 16, 2012 at 11:31:06PM +0100, Rafael J. Wysocki wrote:

> Can you actually select what keys may wake up the system from sleep
> (I mean "real sleep", when the whole system is entirely off except for wakeup
> devices)?

Yes, that's often possible.  In suspend mode you can get a two stage
process where the wakeup itself is triggered by simply looking for any
electrical activity on a set of pins.  This generates a wakeup and an
interrupt for the keypad which then powers up the actual keypad circuit
and looks to see what the keypress was.  The Samsung keypad driver in
mainline does this in runtime PM, while no key is pressed the keypad
controller is in suspend mode.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5231 bytes --]

On Fri, 20 Jan 2012 18:24:16 -0800 Arve Hjønnevåg <arve@android.com> 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 <arve@android.com>

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
>   */


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Rafael J. Wysocki]

On Tuesday, February 14, 2012, NeilBrown wrote:
> On Fri, 20 Jan 2012 18:24:16 -0800 Arve Hjønnevåg <arve@android.com> 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 <arve@android.com>
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Paul Fox]

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, pgf@laptop.org

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Rafael J. Wysocki]

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

Re: [PATCH] Input: Add ioctl to block suspend while event queue is not empty.

[Rafael J. Wysocki]

Hi,

On Saturday, January 21, 2012, Arve Hjønnevåg 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 <arve@android.com>

Minor nit below.

> ---
>  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);
>  	}

Since __pm_relax(), __pm_stay_awake() and wakeup_source_unregister() all
check if the argument is not NULL, you can simply drop the
client->wakeup_source checks.

Also with the fix I've just posted:

http://marc.info/?l=linux-pm&m=132934445508717&w=4

I think it won't be necessary to call __pm_relax() before
wakeup_source_unregister() any more.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html