[PATCH 7/8] Input: Block suspend while event queue is not empty.

[PATCH 7/8] Input: Block suspend while event queue is not empty.

[Arve Hjønnevåg]

Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will 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 |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 2ee6c7a..66e0d16 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -20,6 +20,7 @@
 #include <linux/input.h>
 #include <linux/major.h>
 #include <linux/device.h>
+#include <linux/suspend_blocker.h>
 #include "input-compat.h"
 
 struct evdev {
@@ -43,6 +44,8 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	struct suspend_blocker suspend_blocker;
+	bool use_suspend_blocker;
 };
 
 static struct evdev *evdev_table[EVDEV_MINORS];
@@ -55,6 +58,8 @@ static void evdev_pass_event(struct evdev_client *client,
 	 * Interrupts are disabled, just acquire the lock
 	 */
 	spin_lock(&client->buffer_lock);
+	if (client->use_suspend_blocker)
+		suspend_block(&client->suspend_blocker);
 	client->buffer[client->head++] = *event;
 	client->head &= EVDEV_BUFFER_SIZE - 1;
 	spin_unlock(&client->buffer_lock);
@@ -234,6 +239,8 @@ static int evdev_release(struct inode *inode, struct file *file)
 	mutex_unlock(&evdev->mutex);
 
 	evdev_detach_client(evdev, client);
+	if (client->use_suspend_blocker)
+		suspend_blocker_destroy(&client->suspend_blocker);
 	kfree(client);
 
 	evdev_close_device(evdev);
@@ -335,6 +342,8 @@ static int evdev_fetch_next_event(struct evdev_client *client,
 	if (have_event) {
 		*event = client->buffer[client->tail++];
 		client->tail &= EVDEV_BUFFER_SIZE - 1;
+		if (client->use_suspend_blocker && client->head == client->tail)
+			suspend_unblock(&client->suspend_blocker);
 	}
 
 	spin_unlock_irq(&client->buffer_lock);
@@ -585,6 +594,19 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCGSUSPENDBLOCK:
+		return put_user(client->use_suspend_blocker, ip);
+
+	case EVIOCSSUSPENDBLOCK:
+		spin_lock_irq(&client->buffer_lock);
+		if (!client->use_suspend_blocker && p)
+			suspend_blocker_init(&client->suspend_blocker, "evdev");
+		else if (client->use_suspend_blocker && !p)
+			suspend_blocker_destroy(&client->suspend_blocker);
+		client->use_suspend_blocker = !!p;
+		spin_unlock_irq(&client->buffer_lock);
+		return 0;
+
 	default:
 
 		if (_IOC_TYPE(cmd) != 'E')
diff --git a/include/linux/input.h b/include/linux/input.h
index 7ed2251..b2d93b4 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -82,6 +82,9 @@ struct input_absinfo {
 
 #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 */
+
 /*
  * Event types
  */
-- 
1.6.5.1

_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

[PATCH 7/8] Input: Block suspend while event queue is not empty.

[Arve Hjønnevåg]

Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will 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 |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 2ee6c7a..66e0d16 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -20,6 +20,7 @@
 #include <linux/input.h>
 #include <linux/major.h>
 #include <linux/device.h>
+#include <linux/suspend_blocker.h>
 #include "input-compat.h"
 
 struct evdev {
@@ -43,6 +44,8 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	struct suspend_blocker suspend_blocker;
+	bool use_suspend_blocker;
 };
 
 static struct evdev *evdev_table[EVDEV_MINORS];
@@ -55,6 +58,8 @@ static void evdev_pass_event(struct evdev_client *client,
 	 * Interrupts are disabled, just acquire the lock
 	 */
 	spin_lock(&client->buffer_lock);
+	if (client->use_suspend_blocker)
+		suspend_block(&client->suspend_blocker);
 	client->buffer[client->head++] = *event;
 	client->head &= EVDEV_BUFFER_SIZE - 1;
 	spin_unlock(&client->buffer_lock);
@@ -234,6 +239,8 @@ static int evdev_release(struct inode *inode, struct file *file)
 	mutex_unlock(&evdev->mutex);
 
 	evdev_detach_client(evdev, client);
+	if (client->use_suspend_blocker)
+		suspend_blocker_destroy(&client->suspend_blocker);
 	kfree(client);
 
 	evdev_close_device(evdev);
@@ -335,6 +342,8 @@ static int evdev_fetch_next_event(struct evdev_client *client,
 	if (have_event) {
 		*event = client->buffer[client->tail++];
 		client->tail &= EVDEV_BUFFER_SIZE - 1;
+		if (client->use_suspend_blocker && client->head == client->tail)
+			suspend_unblock(&client->suspend_blocker);
 	}
 
 	spin_unlock_irq(&client->buffer_lock);
@@ -585,6 +594,19 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCGSUSPENDBLOCK:
+		return put_user(client->use_suspend_blocker, ip);
+
+	case EVIOCSSUSPENDBLOCK:
+		spin_lock_irq(&client->buffer_lock);
+		if (!client->use_suspend_blocker && p)
+			suspend_blocker_init(&client->suspend_blocker, "evdev");
+		else if (client->use_suspend_blocker && !p)
+			suspend_blocker_destroy(&client->suspend_blocker);
+		client->use_suspend_blocker = !!p;
+		spin_unlock_irq(&client->buffer_lock);
+		return 0;
+
 	default:
 
 		if (_IOC_TYPE(cmd) != 'E')
diff --git a/include/linux/input.h b/include/linux/input.h
index 7ed2251..b2d93b4 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -82,6 +82,9 @@ struct input_absinfo {
 
 #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 */
+
 /*
  * Event types
  */
-- 
1.6.5.1

[PATCH 7/8] Input: Block suspend while event queue is not empty.

[Arve Hjønnevåg]

Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will 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 |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 2ee6c7a..bff2247 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -20,6 +20,7 @@
 #include <linux/input.h>
 #include <linux/major.h>
 #include <linux/device.h>
+#include <linux/suspend.h>
 #include "input-compat.h"
 
 struct evdev {
@@ -43,6 +44,8 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	struct suspend_blocker suspend_blocker;
+	bool use_suspend_blocker;
 };
 
 static struct evdev *evdev_table[EVDEV_MINORS];
@@ -55,6 +58,8 @@ static void evdev_pass_event(struct evdev_client *client,
 	 * Interrupts are disabled, just acquire the lock
 	 */
 	spin_lock(&client->buffer_lock);
+	if (client->use_suspend_blocker)
+		suspend_block(&client->suspend_blocker);
 	client->buffer[client->head++] = *event;
 	client->head &= EVDEV_BUFFER_SIZE - 1;
 	spin_unlock(&client->buffer_lock);
@@ -234,6 +239,8 @@ static int evdev_release(struct inode *inode, struct file *file)
 	mutex_unlock(&evdev->mutex);
 
 	evdev_detach_client(evdev, client);
+	if (client->use_suspend_blocker)
+		suspend_blocker_unregister(&client->suspend_blocker);
 	kfree(client);
 
 	evdev_close_device(evdev);
@@ -335,6 +342,8 @@ static int evdev_fetch_next_event(struct evdev_client *client,
 	if (have_event) {
 		*event = client->buffer[client->tail++];
 		client->tail &= EVDEV_BUFFER_SIZE - 1;
+		if (client->use_suspend_blocker && client->head == client->tail)
+			suspend_unblock(&client->suspend_blocker);
 	}
 
 	spin_unlock_irq(&client->buffer_lock);
@@ -585,6 +594,19 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCGSUSPENDBLOCK:
+		return put_user(client->use_suspend_blocker, ip);
+
+	case EVIOCSSUSPENDBLOCK:
+		spin_lock_irq(&client->buffer_lock);
+		if (!client->use_suspend_blocker && p)
+			suspend_blocker_init(&client->suspend_blocker, "evdev");
+		else if (client->use_suspend_blocker && !p)
+			suspend_blocker_unregister(&client->suspend_blocker);
+		client->use_suspend_blocker = !!p;
+		spin_unlock_irq(&client->buffer_lock);
+		return 0;
+
 	default:
 
 		if (_IOC_TYPE(cmd) != 'E')
diff --git a/include/linux/input.h b/include/linux/input.h
index 7ed2251..b2d93b4 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -82,6 +82,9 @@ struct input_absinfo {
 
 #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 */
+
 /*
  * Event types
  */
-- 
1.6.5.1

_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

Re: [PATCH 0/8] Suspend block api (version 6)

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 963 bytes --]


Hello,

On Mon, 3 May 2010, Arve Hjønnevåg wrote:

> No, suspend blockers are mostly used to ensure wakeup events are not
> ignored, and to ensure tasks triggered by these wakeup events
> complete.

Standard Linux systems don't need these, because the scheduler just keeps 
the system running as long as there is work to be done.

Suspend-blocks are only needed because patch 1's opportunistic suspend 
governor tries to suspend the system even when the scheduler indicates 
that there is work to be done.  That decision requires all kinds of hacks 
throughout the codebase [1][2].


- Paul


1. Paul Walmsley E-mail to the linux-pm mailing list, dated Fri, 14
   May 2010 00:27:56 -0600:
   http://permalink.gmane.org/gmane.linux.power-management.general/18658

2. Paul Walmsley E-mail to the linux-pm mailing list, dated Fri, 14
   May 2010 00:13:50 -0600:
   http://permalink.gmane.org/gmane.linux.power-management.general/18657


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/8] Suspend block api (version 6)

[Arve Hjønnevåg]

On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
>
> Hello,
>
> On Mon, 3 May 2010, Arve Hjønnevåg wrote:
>
>> No, suspend blockers are mostly used to ensure wakeup events are not
>> ignored, and to ensure tasks triggered by these wakeup events
>> complete.
>
> Standard Linux systems don't need these,

If you don't want to lose wakeup events they do. Standard Linux
systems support suspend, but since they usually don't have a lot of
wakeup events you don't run into a lot of problems.

> because the scheduler just keeps
> the system running as long as there is work to be done.
>

That is only true if you never use suspend.

> Suspend-blocks are only needed because patch 1's opportunistic suspend
> governor tries to suspend the system even when the scheduler indicates
> that there is work to be done.  That decision requires all kinds of hacks
> throughout the codebase [1][2].
>
>
> - Paul
>
>
> 1. Paul Walmsley E-mail to the linux-pm mailing list, dated Fri, 14
>   May 2010 00:27:56 -0600:
>   http://permalink.gmane.org/gmane.linux.power-management.general/18658
>
> 2. Paul Walmsley E-mail to the linux-pm mailing list, dated Fri, 14
>   May 2010 00:13:50 -0600:
>   http://permalink.gmane.org/gmane.linux.power-management.general/18657
>
>


-- 
Arve Hjønnevåg

Re: [PATCH 0/8] Suspend block api (version 6)

[Alan Stern]

On Fri, 14 May 2010, Arve Hjønnevåg wrote:

> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
> >
> > Hello,
> >
> > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
> >
> >> No, suspend blockers are mostly used to ensure wakeup events are not
> >> ignored, and to ensure tasks triggered by these wakeup events
> >> complete.
> >
> > Standard Linux systems don't need these,
> 
> If you don't want to lose wakeup events they do. Standard Linux
> systems support suspend, but since they usually don't have a lot of
> wakeup events you don't run into a lot of problems.

The primary client for opportunistic suspend and suspend blockers
appears to be embedded systems.  These systems are typically capable of
powering the CPU down and up again with low latency, and they typically
have very aggressive runtime PM support capable of powering down each
device when it's not in active use.  Given this ability, it does seem
that opportunistic suspend and suspend blockers might be unnecessary.

I'd like to explore this avenue a little farther.  In particular, what 
is the issue involving loss of wakeup events?  Can you describe this in 
more detail?

> > because the scheduler just keeps
> > the system running as long as there is work to be done.
> >
> 
> That is only true if you never use suspend.

Why would you want to use system suspend if runtime PM can do
everything you need?

Sure, I can see that an ACPI-based system needs something more.  But 
that's not the real issue here.

Alan Stern

Re: [PATCH 0/8] Suspend block api (version 6)

[Arve Hjønnevåg]

On Fri, May 14, 2010 at 7:25 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 14 May 2010, Arve Hjønnevåg wrote:
>
>> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> >
>> > Hello,
>> >
>> > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
>> >
>> >> No, suspend blockers are mostly used to ensure wakeup events are not
>> >> ignored, and to ensure tasks triggered by these wakeup events
>> >> complete.
>> >
>> > Standard Linux systems don't need these,
>>
>> If you don't want to lose wakeup events they do. Standard Linux
>> systems support suspend, but since they usually don't have a lot of
>> wakeup events you don't run into a lot of problems.
>
> The primary client for opportunistic suspend and suspend blockers
> appears to be embedded systems.  These systems are typically capable of
> powering the CPU down and up again with low latency, and they typically
> have very aggressive runtime PM support capable of powering down each
> device when it's not in active use.  Given this ability, it does seem
> that opportunistic suspend and suspend blockers might be unnecessary.
>

With the current kernel and user-space code we run, we get
significantly better battery life on the msm plaform using suspend
compared to entering the same power state from idle. On some devices
it takes only five wakeup events per second to cut the best case
battery life in half.

> I'd like to explore this avenue a little farther.  In particular, what
> is the issue involving loss of wakeup events?  Can you describe this in
> more detail?
>

On the desktop systems I have used I only wake the up the system by
pressing a button/key or with an rtc alarm. Losing a button or key
wakeup event is not usually a problem on a desktop since the user will
press it again. Losing an alarm however could be a problem and this
can be avoided by using opportunistic suspend and suspend blockers.

>> > because the scheduler just keeps
>> > the system running as long as there is work to be done.
>> >
>>
>> That is only true if you never use suspend.
>
> Why would you want to use system suspend if runtime PM can do
> everything you need?
>
Because it stops threads that wakeup every second to check if they
have work to do (this includes standard kernel threads), and it
prevents bad apps that never go idle from completely destroying our
battery life.

> Sure, I can see that an ACPI-based system needs something more.  But
> that's not the real issue here.
>

The system we started with entered a much lower power state from
suspend than idle so we needed wakelocks to get more than 24 hours
battery life. We kept wakelocks when we moved to the msm platform
since it reduces our power consumption.

-- 
Arve Hjønnevåg

Re: [PATCH 0/8] Suspend block api (version 6)

[Alan Stern]

On Fri, 14 May 2010, Arve Hjønnevåg wrote:

> > I'd like to explore this avenue a little farther.  In particular, what
> > is the issue involving loss of wakeup events?  Can you describe this in
> > more detail?
> >
> 
> On the desktop systems I have used I only wake the up the system by
> pressing a button/key or with an rtc alarm. Losing a button or key
> wakeup event is not usually a problem on a desktop since the user will
> press it again. Losing an alarm however could be a problem and this
> can be avoided by using opportunistic suspend and suspend blockers.

How can runtime PM combined with CPUidle cause an alarm to be lost?

> > Why would you want to use system suspend if runtime PM can do
> > everything you need?
> >
> Because it stops threads that wakeup every second to check if they
> have work to do (this includes standard kernel threads), and it
> prevents bad apps that never go idle from completely destroying our
> battery life.

Ah, the ill-behaved apps problem.  I think everybody agrees that they 
are hard to deal with.

The kernel-threads problem might better be addressed by fixing those
threads than by adding a new API.

> > Sure, I can see that an ACPI-based system needs something more.  But
> > that's not the real issue here.
> >
> 
> The system we started with entered a much lower power state from
> suspend than idle so we needed wakelocks to get more than 24 hours
> battery life. We kept wakelocks when we moved to the msm platform
> since it reduces our power consumption.

Is it generally true among embedded systems nowadays that idle is 
capable of reaching essentially the same power states as suspend?

Alan Stern

Re: [PATCH 0/8] Suspend block api (version 6)

[Arve Hjønnevåg]

On Sat, May 15, 2010 at 2:25 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 14 May 2010, Arve Hjønnevåg wrote:
>
>> > I'd like to explore this avenue a little farther.  In particular, what
>> > is the issue involving loss of wakeup events?  Can you describe this in
>> > more detail?
>> >
>>
>> On the desktop systems I have used I only wake the up the system by
>> pressing a button/key or with an rtc alarm. Losing a button or key
>> wakeup event is not usually a problem on a desktop since the user will
>> press it again. Losing an alarm however could be a problem and this
>> can be avoided by using opportunistic suspend and suspend blockers.
>
> How can runtime PM combined with CPUidle cause an alarm to be lost?
>
It doesn't. My desktop systems only gets to a low power state (<3W)
from suspend.

>> > Why would you want to use system suspend if runtime PM can do
>> > everything you need?
>> >
>> Because it stops threads that wakeup every second to check if they
>> have work to do (this includes standard kernel threads), and it
>> prevents bad apps that never go idle from completely destroying our
>> battery life.
>
> Ah, the ill-behaved apps problem.  I think everybody agrees that they
> are hard to deal with.
>
> The kernel-threads problem might better be addressed by fixing those
> threads than by adding a new API.
>
>> > Sure, I can see that an ACPI-based system needs something more.  But
>> > that's not the real issue here.
>> >
>>
>> The system we started with entered a much lower power state from
>> suspend than idle so we needed wakelocks to get more than 24 hours
>> battery life. We kept wakelocks when we moved to the msm platform
>> since it reduces our power consumption.
>
> Is it generally true among embedded systems nowadays that idle is
> capable of reaching essentially the same power states as suspend?

Embedded system in general, no, but all the recent SOCs I've seen that
target phones do.

-- 
Arve Hjønnevåg

Re: [PATCH 0/8] Suspend block api (version 6)

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1404 bytes --]

Hello Arve,

On Fri, 14 May 2010, Arve Hjønnevåg wrote:

> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
> >
> > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
> >
> >> No, suspend blockers are mostly used to ensure wakeup events are not
> >> ignored, and to ensure tasks triggered by these wakeup events
> >> complete.
> >
> > Standard Linux systems don't need these,
> 
> If you don't want to lose wakeup events they do.  Standard Linux systems 
> support suspend, but since they usually don't have a lot of wakeup 
> events you don't run into a lot of problems.

Sorry, I don't follow.  What causes wakeup events to be lost?  Is it the 
current opportunistic suspend governor?  On OMAP Linux systems, as far as 
I know, we don't lose any wakeup events.

> > because the scheduler just keeps the system running as long as there 
> > is work to be done.
> 
> That is only true if you never use suspend.

If, instead of the current Android opportunistic suspend governor, the 
system entered suspend from pm_idle(), wouldn't that keep the system 
running as long as there is work to done?

As far as I can see, it's the current Android opportunistic suspend 
governor design in patch 1 that causes the system to enter suspend even 
when there is work to be done, since it will try to suspend even when the 
system is out of the idle loop.


- Paul

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/8] Suspend block api (version 6)

[Arve Hjønnevåg]

On Mon, May 17, 2010 at 7:26 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Hello Arve,
>
> On Fri, 14 May 2010, Arve Hjønnevåg wrote:
>
>> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> >
>> > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
>> >
>> >> No, suspend blockers are mostly used to ensure wakeup events are not
>> >> ignored, and to ensure tasks triggered by these wakeup events
>> >> complete.
>> >
>> > Standard Linux systems don't need these,
>>
>> If you don't want to lose wakeup events they do.  Standard Linux systems
>> support suspend, but since they usually don't have a lot of wakeup
>> events you don't run into a lot of problems.
>
> Sorry, I don't follow.  What causes wakeup events to be lost?  Is it the
> current opportunistic suspend governor?  On OMAP Linux systems, as far as
> I know, we don't lose any wakeup events.
>
Have you used suspend?

>> > because the scheduler just keeps the system running as long as there
>> > is work to be done.
>>
>> That is only true if you never use suspend.
>
> If, instead of the current Android opportunistic suspend governor, the
> system entered suspend from pm_idle(), wouldn't that keep the system
> running as long as there is work to done?
>
How do you know if the work being done while suspending is work that
is needed to suspend or work that should abort suspend? When should
the system wake up?

> As far as I can see, it's the current Android opportunistic suspend
> governor design in patch 1 that causes the system to enter suspend even
> when there is work to be done, since it will try to suspend even when the
> system is out of the idle loop.
>

It does not matter how you enter suspend. Without opportunistic
suspend, once you tell the kernel that you want to suspend, you cannot
abort. If a wakeup event occurs at this point, it will probably not be
processed until the system wakes up for another wakeup event.

-- 
Arve Hjønnevåg

Re: [PATCH 0/8] Suspend block api (version 6)

[Henrik Rydberg]

Arve Hjønnevåg wrote:
> It does not matter how you enter suspend. Without opportunistic
> suspend, once you tell the kernel that you want to suspend, you cannot
> abort. If a wakeup event occurs at this point, it will probably not be
> processed until the system wakes up for another wakeup event.
> 

Since it seems to be at the very core of the argument, how long time does it
take to suspend your system? Is it something that could be improved upon?

Henrik

Re: [PATCH 0/8] Suspend block api (version 6)

[Rafael J. Wysocki]

On Tuesday 18 May 2010, Henrik Rydberg wrote:
> Arve Hjønnevåg wrote:
> > It does not matter how you enter suspend. Without opportunistic
> > suspend, once you tell the kernel that you want to suspend, you cannot
> > abort. If a wakeup event occurs at this point, it will probably not be
> > processed until the system wakes up for another wakeup event.
> > 
> 
> Since it seems to be at the very core of the argument, how long time does it
> take to suspend your system? Is it something that could be improved upon?

It's not a matter of time.  It's a matter of lost keystrokes and such.

Rafael

Re: [PATCH 0/8] Suspend block api (version 6)

[David Brownell]

> > > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
> > >
> > >> No, suspend blockers are mostly used to
> ensure wakeup events are not
> > >> ignored, and to ensure tasks triggered by
> these wakeup events
> > >> complete.
> > >
> > > Standard Linux systems don't need these,
> > 
> > If you don't want to lose wakeup events they do. 
> Standard Linux systems 
> > support suspend, but since they usually don't have a
> lot of wakeup 
> > events you don't run into a lot of problems.

Yes and no...  stick a bunch of USB hubs and devices
on the system, and you may have a lot of wake events.
Keyboard, mouse, network adapter, and so on.

 Way back when I made USB remote wakeup work, ISTR running
into a version of the issue you're raising.  Briefly, the
wake event could be queued in hardware, but there were also
various unavoidable races .... with ambiguity about just when
particular events should be recognized.  If the system entered
sleep before the wake event(s) triggered, or the relevant devices
suspended first, then things were clear; otherwise, not.  On
first blush, I'd expect every hardware wake mechanism would have
the same issues; the fact that USB wakes are mediated via khubd
is not a huge difference.

[PATCH 7/8] Input: Block suspend while event queue is not empty.

[Arve Hjønnevåg]

Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will 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 |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 2ee6c7a..bff2247 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -20,6 +20,7 @@
 #include <linux/input.h>
 #include <linux/major.h>
 #include <linux/device.h>
+#include <linux/suspend.h>
 #include "input-compat.h"
 
 struct evdev {
@@ -43,6 +44,8 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	struct suspend_blocker suspend_blocker;
+	bool use_suspend_blocker;
 };
 
 static struct evdev *evdev_table[EVDEV_MINORS];
@@ -55,6 +58,8 @@ static void evdev_pass_event(struct evdev_client *client,
 	 * Interrupts are disabled, just acquire the lock
 	 */
 	spin_lock(&client->buffer_lock);
+	if (client->use_suspend_blocker)
+		suspend_block(&client->suspend_blocker);
 	client->buffer[client->head++] = *event;
 	client->head &= EVDEV_BUFFER_SIZE - 1;
 	spin_unlock(&client->buffer_lock);
@@ -234,6 +239,8 @@ static int evdev_release(struct inode *inode, struct file *file)
 	mutex_unlock(&evdev->mutex);
 
 	evdev_detach_client(evdev, client);
+	if (client->use_suspend_blocker)
+		suspend_blocker_unregister(&client->suspend_blocker);
 	kfree(client);
 
 	evdev_close_device(evdev);
@@ -335,6 +342,8 @@ static int evdev_fetch_next_event(struct evdev_client *client,
 	if (have_event) {
 		*event = client->buffer[client->tail++];
 		client->tail &= EVDEV_BUFFER_SIZE - 1;
+		if (client->use_suspend_blocker && client->head == client->tail)
+			suspend_unblock(&client->suspend_blocker);
 	}
 
 	spin_unlock_irq(&client->buffer_lock);
@@ -585,6 +594,19 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCGSUSPENDBLOCK:
+		return put_user(client->use_suspend_blocker, ip);
+
+	case EVIOCSSUSPENDBLOCK:
+		spin_lock_irq(&client->buffer_lock);
+		if (!client->use_suspend_blocker && p)
+			suspend_blocker_init(&client->suspend_blocker, "evdev");
+		else if (client->use_suspend_blocker && !p)
+			suspend_blocker_unregister(&client->suspend_blocker);
+		client->use_suspend_blocker = !!p;
+		spin_unlock_irq(&client->buffer_lock);
+		return 0;
+
 	default:
 
 		if (_IOC_TYPE(cmd) != 'E')
diff --git a/include/linux/input.h b/include/linux/input.h
index 7ed2251..b2d93b4 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -82,6 +82,9 @@ struct input_absinfo {
 
 #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 */
+
 /*
  * Event types
  */
-- 
1.6.5.1

Re: [PATCH 0/8] Suspend block api (version 6)

[Paul Walmsley]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 10357 bytes --]

Hello Arve,

I regret the delay in responding.

On Mon, 17 May 2010, Arve Hjønnevåg wrote:

> On Mon, May 17, 2010 at 7:26 PM, Paul Walmsley <paul@pwsan.com> wrote:
> > On Fri, 14 May 2010, Arve Hjønnevåg wrote:
> >> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
> >> > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
> >> >
> >> >> No, suspend blockers are mostly used to ensure wakeup events are not
> >> >> ignored, and to ensure tasks triggered by these wakeup events
> >> >> complete.
> >> >
> >> > Standard Linux systems don't need these,
> >>
> >> If you don't want to lose wakeup events they do.  Standard Linux systems
> >> support suspend, but since they usually don't have a lot of wakeup
> >> events you don't run into a lot of problems.
> >
> > Sorry, I don't follow.  What causes wakeup events to be lost?  Is it the
> > current opportunistic suspend governor?  On OMAP Linux systems, as far as
> > I know, we don't lose any wakeup events.
> >
> Have you used suspend?

I see.  You are referring to wakeup event loss/delay caused by the full 
system suspend process (e.g., kernel/power/suspend.c:enter_state() 
onwards[1]), correct?  This message addresses this at the end of the mail.

> >> > because the scheduler just keeps the system running as long as there
> >> > is work to be done.
> >>
> >> That is only true if you never use suspend.
> >
> > If, instead of the current Android opportunistic suspend governor, the
> > system entered suspend from pm_idle(), wouldn't that keep the system
> > running as long as there is work to done?
> >
> How do you know if the work being done while suspending is work that
> is needed to suspend or work that should abort suspend?

Consider a suspending system in which all "untrusted" processes have been 
placed into TASK_INTERRUPTIBLE state and have had their timers 
disabled.[2] The only remaining work to do on the system is work that 
needs to be done as part of the suspend process, or work that is part of a 
"trusted" process or the kernel.  If the suspend needs to be aborted due 
to the arrival of an event, this can be handled as part of the standard 
suspend process (described below).

> When should the system wake up?

I suspect that I am misunderstanding your question, but the system should 
wake up the same way it does now: when a wakeup interrupt arrives (either 
from the next scheduled timer expiration, or from some external device).

> > As far as I can see, it's the current Android opportunistic suspend
> > governor design in patch 1 that causes the system to enter suspend even
> > when there is work to be done, since it will try to suspend even when the
> > system is out of the idle loop.
> 
> It does not matter how you enter suspend. Without opportunistic suspend, 
> once you tell the kernel that you want to suspend, you cannot abort.

Any driver should be able to abort suspend by returning an error from its 
struct device_driver/struct dev_pm_ops suspend function.  Consider this 
partial callgraph for full system suspend, from [3], [4]:

   kernel/power/suspend.c:   enter_state() ->
   kernel/power/suspend.c:    suspend_devices_and_enter() ->
drivers/base/power/main.c:     dpm_suspend_start() ->
drivers/base/power/main.c:      dpm_suspend() ->
drivers/base/power/main.c:       device_suspend() ->
drivers/base/power/main.c:        __device_suspend() ->
drivers/base/power/main.c:         pm_op() ->
drivers/base/power/main.c:          ops->suspend()

If ops->suspend() returns an error, it's ultimately passed back to 
suspend_devices_and_enter(), which aborts the suspend[5].

In this context, the main change that should be necessary is for 
driver/subsystem suspend functions to return an error when there are 
events pending.  For example, as an alternative to the Android series' 
evdev.c patch[6], the evdev.c suspend function could return an error such 
as -EBUSY if events are currently queued (example below).

Aborting suspend in the event of driver activity seems like the right 
thing to do in a system that attempts to enter full system suspend while 
idle.  But it is not the current expected Linux behavior, which also seems 
useful.  Any abort-suspend-if-busy behavior should be configurable.  One 
way would be to add a sysfs file for each driver/subsystem: perhaps 
something like 'abort_suspend_if_busy'.  The default value would be zero 
to preserve the current behavior.  Systems that wish to use opportunistic 
suspend could write a one to some or all of those files.

An untested, purely illustrative example, based on evdev.c, is at the end 
of this message.  It's not meant to be functional; it's just meant to 
demonstrate the basic idea in code form.

...

Like suspend-blockers, adding abort_suspend_if_busy support would require 
changes throughout many drivers and subsystems.  However, unlike 
suspend-blockers, the above abort_suspend_if_busy approach would not 
introduce any new APIs[7], nor would it change the default suspend 
behavior of the system.

As an aside, it seems that Android shouldn't currently need this if it's 
possible to pause "untrusted" processes and timers[8], since current 
shipping Android hardware can enter the same system low power state both 
with and without full system suspend[9].  But this could be useful for 
non-Android systems that still wish to use an idle-loop based, 
opportunistic full system suspend.


regards,

- Paul


1. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l258

2. Section B4 of Paul Walmsley E-mail to the linux-pm mailing list, 
   dated Mon May 17 19:39:44 CDT 2010:
   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025663.html

3. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l258

4. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/base/power/main.c;h=941fcb87e52a12765df2aaa2e2ab21267693af9f;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l1062

5. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l207

6. Arve Hjønnevåg E-mail to the linux-pm mailing list, dated Fri May 21 
   15:46:54 PDT 2010:
   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025747.html

7. Paul Walmsley E-mail to the linux-pm mailing list, dated Thu May 13 
   23:13:50 PDT 2010:
   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025524.html

8. Paul Walmsley E-mail to the linux-pm mailing list, dated Mon May 17 
   18:39:44 PDT 2010:
   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025663.html  

9. Arve Hjønnevåg E-mail to the linux-pm mailing list, dated Mon, May 17 
   20:06:33 PDT 2010:   
   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025668.html



--------------------------------------------------------------------------------------

evdev.c abort_suspend_if_idle experiment:

---
 drivers/input/evdev.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 2ee6c7a..23eaad1 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -20,8 +20,16 @@
 #include <linux/input.h>
 #include <linux/major.h>
 #include <linux/device.h>
+#include <linux/pm.h>
 #include "input-compat.h"
 
+/*
+ * abort_suspend_if_busy: set to 1 to prevent suspend as long as there
+ * is an event in the queue; set to 0 to allow suspend at any time.
+ * XXX Intended to be read from sysfs or the input subsystem.
+ */
+static int abort_suspend_if_busy;
+
 struct evdev {
 	int exist;
 	int open;
@@ -118,6 +126,36 @@ static int evdev_flush(struct file *file, fl_owner_t id)
 	return retval;
 }
 
+static int evdev_suspend(struct device *dev)
+{
+	struct evdev *evdev = container_of(dev, struct evdev, dev);
+	struct evdev_client *client;
+	int ret = 0;
+
+	if (!abort_suspend_if_busy)
+		return 0;
+
+	rcu_read_lock();
+
+	client = rcu_dereference(evdev->grab);
+	if (client) {
+		if (client->head != client->tail)
+			ret = -EBUSY;
+	} else {
+		list_for_each_entry_rcu(client, &evdev->client_list, node) {
+			if (client->head != client->tail) {
+				ret = -EBUSY;
+				break;
+			}
+		}
+	}
+
+	rcu_read_unlock();
+
+	return ret;
+}
+
+
 static void evdev_free(struct device *dev)
 {
 	struct evdev *evdev = container_of(dev, struct evdev, dev);
@@ -787,6 +825,21 @@ static void evdev_cleanup(struct evdev *evdev)
 	}
 }
 
+static const struct dev_pm_ops evdev_pm_ops = {
+	.suspend	= &evdev_suspend,
+};
+
+static char *evdev_devnode(struct device *dev, mode_t *mode)
+{
+	return kasprintf(GFP_KERNEL, "input/%s", dev_name(dev));
+}
+
+static struct class evdev_class = {
+	.name		= "evdev",
+	.devnode	= evdev_devnode,
+	.pm		= &evdev_pm_ops,
+};
+
 /*
  * Create new evdev device. Note that input core serializes calls
  * to connect and disconnect so we don't need to lock evdev_table here.
@@ -826,7 +879,7 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
 	evdev->handle.private = evdev;
 
 	evdev->dev.devt = MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + minor);
-	evdev->dev.class = &input_class;
+	evdev->dev.class = &evdev_class;
 	evdev->dev.parent = &dev->dev;
 	evdev->dev.release = evdev_free;
 	device_initialize(&evdev->dev);
@@ -883,12 +936,21 @@ static struct input_handler evdev_handler = {
 
 static int __init evdev_init(void)
 {
+	int err;
+
+	err = class_register(&input_class);
+	if (err) {
+		printk(KERN_ERR "evdev: unable to register evdev class\n");
+		return err;
+	}
+
 	return input_register_handler(&evdev_handler);
 }
 
 static void __exit evdev_exit(void)
 {
 	input_unregister_handler(&evdev_handler);
+	class_unregister(&evdev_class);
 }
 
 module_init(evdev_init);
-- 
1.7.1.GIT

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/8] Suspend block api (version 6)

[Dmitry Torokhov]

On Fri, May 14, 2010 at 03:18:13PM -0700, Arve Hjønnevåg wrote:
> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
> >
> > Hello,
> >
> > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
> >
> >> No, suspend blockers are mostly used to ensure wakeup events are not
> >> ignored, and to ensure tasks triggered by these wakeup events
> >> complete.
> >
> > Standard Linux systems don't need these,
> 
> If you don't want to lose wakeup events they do. Standard Linux
> systems support suspend, but since they usually don't have a lot of
> wakeup events you don't run into a lot of problems.
> 

What I do not quite understand is how exactly suspend blocks save us
from missing wakeup events. Consider case when all events have been
processed and system starts entering the sleep: we are in a half-sleep,
half awake state, with some devices already put to sleep and some just
now being processed. If user presses a key while physical input device
is being suspended, what will happen? Do you expect interrupt routine
abort suspend process? Initiate resume if suspend of the device has
completed?

Also, let's say device is suspended and is configured as a wakeup source
while the rest of the system is still awake... If I press a key I do not
think that it will necessarily abort suspend process. Or will it?

BTW, If you are concerned about events that already "left" physical
device but has not reached userspace yet - maybe instead of suspend
blockers we should make sure that all drivers throughout the chain
implement suspend/resume and refuse suspending if their queues are not
empty.  In input land that would mean extending suspend routine in
input_dev and adding one to evdev.

Thanks.

-- 
Dmitry

Re: [PATCH 0/8] Suspend block api (version 6)

[Alan Stern]

On Tue, 25 May 2010, Dmitry Torokhov wrote:

> What I do not quite understand is how exactly suspend blocks save us
> from missing wakeup events. Consider case when all events have been
> processed and system starts entering the sleep: we are in a half-sleep,
> half awake state, with some devices already put to sleep and some just
> now being processed. If user presses a key while physical input device
> is being suspended, what will happen? Do you expect interrupt routine
> abort suspend process? Initiate resume if suspend of the device has
> completed?

This depends on several factors, such as how the input device drivers
are written and how the hardware works, as well as the details of the 
timing.  Here are a few possibilities:

	The keypress is not handled but stays in the hardware as a
	wakeup event.  The system goes through its entire suspend
	procedure, and then the pending wakeup event immediately
	cases it to wake up.  Then the keypress gets handled
	normally.

	Or the keypress is treated as a normal IRQ and handled by
	the hardware.  The input subsystem puts the keystroke on
	the event queue (all done while in_interrupt), where it
	languishes because userspace is frozen and can't read it.
	The system goes to sleep and stays that way until something
	else wakes it up.  (This is a type-3 failure as described
	in my earlier email.)

	Or the keypress is handled normally and the input subsystem 
	interprets it as a runtime resume request, telling the PM core 
	to queue a resume call.  But the PM core's runtime workqueue 
	thread has already been frozen, and so the event remains on the 
	work queue until the system wakes up on its own.  (This is a
	type-2 failure.)

	Or we fix the PM core, so that when the resume call is added
	to the workqueue it realizes the suspend-in-progress needs
	to be aborted.  The system goes back to a fully-awake state
	and the keystroke is handled normally.

> Also, let's say device is suspended and is configured as a wakeup source
> while the rest of the system is still awake... If I press a key I do not
> think that it will necessarily abort suspend process. Or will it?

Since the keyboard is a wakeup device, it certainly should abort the
suspend.  Or at least it should wake the system up as soon as the
system finishes going to sleep.  At the moment, though, I'm not sure
that it does behave either way.

> BTW, If you are concerned about events that already "left" physical
> device but has not reached userspace yet - maybe instead of suspend
> blockers we should make sure that all drivers throughout the chain
> implement suspend/resume and refuse suspending if their queues are not
> empty.  In input land that would mean extending suspend routine in
> input_dev and adding one to evdev.

That's not the only problem.  We also have to consider events that have
reached userspace but not yet been fully processed.  The user thread
handling the event needs some way to prevent the system from suspending
until it is all done.

Alan Stern

Re: [PATCH 0/8] Suspend block api (version 6)

[Dmitry Torokhov]

On Tue, May 25, 2010 at 02:25:08PM -0400, Alan Stern wrote:
> On Tue, 25 May 2010, Dmitry Torokhov wrote:
> 
> > BTW, If you are concerned about events that already "left" physical
> > device but has not reached userspace yet - maybe instead of suspend
> > blockers we should make sure that all drivers throughout the chain
> > implement suspend/resume and refuse suspending if their queues are not
> > empty.  In input land that would mean extending suspend routine in
> > input_dev and adding one to evdev.
> 
> That's not the only problem.  We also have to consider events that have
> reached userspace but not yet been fully processed.  The user thread
> handling the event needs some way to prevent the system from suspending
> until it is all done.
> 

It is a problem if kernel initiated suspend transition on its own. I
believe that it is userspace responsibility to initiate sustend (and
make sure that needs of userspace, including processing of certain
events, are served beforehand).

-- 
Dmitry

Re: [PATCH 0/8] Suspend block api (version 6)

[Arve Hjønnevåg]

On Tue, May 25, 2010 at 11:33 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, May 25, 2010 at 02:25:08PM -0400, Alan Stern wrote:
>> On Tue, 25 May 2010, Dmitry Torokhov wrote:
>>
>> > BTW, If you are concerned about events that already "left" physical
>> > device but has not reached userspace yet - maybe instead of suspend
>> > blockers we should make sure that all drivers throughout the chain
>> > implement suspend/resume and refuse suspending if their queues are not
>> > empty.  In input land that would mean extending suspend routine in
>> > input_dev and adding one to evdev.
>>
>> That's not the only problem.  We also have to consider events that have
>> reached userspace but not yet been fully processed.  The user thread
>> handling the event needs some way to prevent the system from suspending
>> until it is all done.
>>
>
> It is a problem if kernel initiated suspend transition on its own. I
> believe that it is userspace responsibility to initiate sustend (and
> make sure that needs of userspace, including processing of certain
> events, are served beforehand).

That only works if a single user-space thread initiates suspend and
consumes all wakeup events (or if the user-space code that initiates
suspend implements suspend blockers), and even then the kernel would
still need to block suspend in same places as it does if the kernel
initiates suspend. If a wakeup event happens right after user-space
initiates suspend, that suspend must be aborted. By only initiating
suspend from user-space, you force this to be a polling operation.

-- 
Arve Hjønnevåg

Re: [PATCH 0/8] Suspend block api (version 6)

[Dmitry Torokhov]

On Tue, May 25, 2010 at 03:05:41PM -0700, Arve Hjønnevåg wrote:
> On Tue, May 25, 2010 at 11:33 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Tue, May 25, 2010 at 02:25:08PM -0400, Alan Stern wrote:
> >> On Tue, 25 May 2010, Dmitry Torokhov wrote:
> >>
> >> > BTW, If you are concerned about events that already "left" physical
> >> > device but has not reached userspace yet - maybe instead of suspend
> >> > blockers we should make sure that all drivers throughout the chain
> >> > implement suspend/resume and refuse suspending if their queues are not
> >> > empty.  In input land that would mean extending suspend routine in
> >> > input_dev and adding one to evdev.
> >>
> >> That's not the only problem.  We also have to consider events that have
> >> reached userspace but not yet been fully processed.  The user thread
> >> handling the event needs some way to prevent the system from suspending
> >> until it is all done.
> >>
> >
> > It is a problem if kernel initiated suspend transition on its own. I
> > believe that it is userspace responsibility to initiate sustend (and
> > make sure that needs of userspace, including processing of certain
> > events, are served beforehand).
> 
> That only works if a single user-space thread initiates suspend

Yes, there should be a single entity controlling suspend.

> and
> consumes all wakeup events (or if the user-space code that initiates
> suspend implements suspend blockers),

Yes.

> and even then the kernel would
> still need to block suspend in same places as it does if the kernel
> initiates suspend. If a wakeup event happens right after user-space
> initiates suspend, that suspend must be aborted.

This is true regardless of whether we use suspend blockers or not. If,
after initiating suspend, we detect wake up event that event shoudl be
acted upon and suspend should be ignored.

> By only initiating
> suspend from user-space, you force this to be a polling operation.
>

I am not sure what you mean by this.

-- 
Dmitry

Re: [PATCH 0/8] Suspend block api (version 6)

[Arve Hjønnevåg]

On Tue, May 25, 2010 at 2:41 AM, Paul Walmsley <paul@pwsan.com> wrote:
> Hello Arve,
>
> I regret the delay in responding.
>
> On Mon, 17 May 2010, Arve Hjønnevåg wrote:
>
>> On Mon, May 17, 2010 at 7:26 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> > On Fri, 14 May 2010, Arve Hjønnevåg wrote:
>> >> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> >> > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
>> >> >
>> >> >> No, suspend blockers are mostly used to ensure wakeup events are not
>> >> >> ignored, and to ensure tasks triggered by these wakeup events
>> >> >> complete.
>> >> >
>> >> > Standard Linux systems don't need these,
>> >>
>> >> If you don't want to lose wakeup events they do.  Standard Linux systems
>> >> support suspend, but since they usually don't have a lot of wakeup
>> >> events you don't run into a lot of problems.
>> >
>> > Sorry, I don't follow.  What causes wakeup events to be lost?  Is it the
>> > current opportunistic suspend governor?  On OMAP Linux systems, as far as
>> > I know, we don't lose any wakeup events.
>> >
>> Have you used suspend?
>
> I see.  You are referring to wakeup event loss/delay caused by the full
> system suspend process (e.g., kernel/power/suspend.c:enter_state()
> onwards[1]), correct?  This message addresses this at the end of the mail.
>
>> >> > because the scheduler just keeps the system running as long as there
>> >> > is work to be done.
>> >>
>> >> That is only true if you never use suspend.
>> >
>> > If, instead of the current Android opportunistic suspend governor, the
>> > system entered suspend from pm_idle(), wouldn't that keep the system
>> > running as long as there is work to done?
>> >
>> How do you know if the work being done while suspending is work that
>> is needed to suspend or work that should abort suspend?
>
> Consider a suspending system in which all "untrusted" processes have been
> placed into TASK_INTERRUPTIBLE state and have had their timers
> disabled.[2] The only remaining work to do on the system is work that
> needs to be done as part of the suspend process, or work that is part of a
> "trusted" process or the kernel.  If the suspend needs to be aborted due
> to the arrival of an event, this can be handled as part of the standard
> suspend process (described below).
>

That means modifying all kernel code that would use suspend blockers
to abort suspend by returning an error from suspend instead. How is
this better than using suspend blockers?

>> When should the system wake up?
>
> I suspect that I am misunderstanding your question, but the system should
> wake up the same way it does now: when a wakeup interrupt arrives (either
> from the next scheduled timer expiration, or from some external device).
>

That is not how the system wakes up from suspend now. The monotonic
clock stops, and timers are ignored.

>> > As far as I can see, it's the current Android opportunistic suspend
>> > governor design in patch 1 that causes the system to enter suspend even
>> > when there is work to be done, since it will try to suspend even when the
>> > system is out of the idle loop.
>>
>> It does not matter how you enter suspend. Without opportunistic suspend,
>> once you tell the kernel that you want to suspend, you cannot abort.
>
> Any driver should be able to abort suspend by returning an error from its
> struct device_driver/struct dev_pm_ops suspend function.  Consider this

Yes a driver can abort, but the user space code that initiated suspend cannot.

> partial callgraph for full system suspend, from [3], [4]:
>
>   kernel/power/suspend.c:   enter_state() ->
>   kernel/power/suspend.c:    suspend_devices_and_enter() ->
> drivers/base/power/main.c:     dpm_suspend_start() ->
> drivers/base/power/main.c:      dpm_suspend() ->
> drivers/base/power/main.c:       device_suspend() ->
> drivers/base/power/main.c:        __device_suspend() ->
> drivers/base/power/main.c:         pm_op() ->
> drivers/base/power/main.c:          ops->suspend()
>
> If ops->suspend() returns an error, it's ultimately passed back to
> suspend_devices_and_enter(), which aborts the suspend[5].
>
> In this context, the main change that should be necessary is for
> driver/subsystem suspend functions to return an error when there are
> events pending.  For example, as an alternative to the Android series'
> evdev.c patch[6], the evdev.c suspend function could return an error such
> as -EBUSY if events are currently queued (example below).
>
> Aborting suspend in the event of driver activity seems like the right
> thing to do in a system that attempts to enter full system suspend while
> idle.  But it is not the current expected Linux behavior, which also seems
> useful.  Any abort-suspend-if-busy behavior should be configurable.  One
> way would be to add a sysfs file for each driver/subsystem: perhaps
> something like 'abort_suspend_if_busy'.  The default value would be zero
> to preserve the current behavior.  Systems that wish to use opportunistic
> suspend could write a one to some or all of those files.
>
> An untested, purely illustrative example, based on evdev.c, is at the end
> of this message.  It's not meant to be functional; it's just meant to
> demonstrate the basic idea in code form.
>

Do you think this is simpler than using a suspend blocker?

> ...
>
> Like suspend-blockers, adding abort_suspend_if_busy support would require
> changes throughout many drivers and subsystems.  However, unlike
> suspend-blockers, the above abort_suspend_if_busy approach would not
> introduce any new APIs[7], nor would it change the default suspend
> behavior of the system.
>

Opportunistic suspend does not change the default behaviour either.
Your proposal requires that all the drivers that would block suspend
using a suspend blocker, now block suspend without the help of the
suspend blocker api.

> As an aside, it seems that Android shouldn't currently need this if it's
> possible to pause "untrusted" processes and timers[8], since current

Your proposal to "pause"processes by putting them in
TASK_INTERRUPTIBLE state and stop their timers does not work. It is
effectivly the same ase freezing them, since if you "paused" it while
it was busy, a wakeup event cannot unpause it.

> shipping Android hardware can enter the same system low power state both
> with and without full system suspend[9].  But this could be useful for
> non-Android systems that still wish to use an idle-loop based,
> opportunistic full system suspend.
>
>
> regards,
>
> - Paul
>
>
> 1. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l258
>
> 2. Section B4 of Paul Walmsley E-mail to the linux-pm mailing list,
>   dated Mon May 17 19:39:44 CDT 2010:
>   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025663.html
>
> 3. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l258
>
> 4. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/base/power/main.c;h=941fcb87e52a12765df2aaa2e2ab21267693af9f;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l1062
>
> 5. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l207
>
> 6. Arve Hjønnevåg E-mail to the linux-pm mailing list, dated Fri May 21
>   15:46:54 PDT 2010:
>   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025747.html
>
> 7. Paul Walmsley E-mail to the linux-pm mailing list, dated Thu May 13
>   23:13:50 PDT 2010:
>   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025524.html
>
> 8. Paul Walmsley E-mail to the linux-pm mailing list, dated Mon May 17
>   18:39:44 PDT 2010:
>   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025663.html
>
> 9. Arve Hjønnevåg E-mail to the linux-pm mailing list, dated Mon, May 17
>   20:06:33 PDT 2010:
>   https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025668.html
>
>
>
> --------------------------------------------------------------------------------------
>
> evdev.c abort_suspend_if_idle experiment:
>
> ---
>  drivers/input/evdev.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 63 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 2ee6c7a..23eaad1 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -20,8 +20,16 @@
>  #include <linux/input.h>
>  #include <linux/major.h>
>  #include <linux/device.h>
> +#include <linux/pm.h>
>  #include "input-compat.h"
>
> +/*
> + * abort_suspend_if_busy: set to 1 to prevent suspend as long as there
> + * is an event in the queue; set to 0 to allow suspend at any time.
> + * XXX Intended to be read from sysfs or the input subsystem.
> + */
> +static int abort_suspend_if_busy;
> +
>  struct evdev {
>        int exist;
>        int open;
> @@ -118,6 +126,36 @@ static int evdev_flush(struct file *file, fl_owner_t id)
>        return retval;
>  }
>
> +static int evdev_suspend(struct device *dev)
> +{
> +       struct evdev *evdev = container_of(dev, struct evdev, dev);
> +       struct evdev_client *client;
> +       int ret = 0;
> +
> +       if (!abort_suspend_if_busy)
> +               return 0;
> +
> +       rcu_read_lock();
> +
> +       client = rcu_dereference(evdev->grab);
> +       if (client) {
> +               if (client->head != client->tail)
> +                       ret = -EBUSY;
> +       } else {
> +               list_for_each_entry_rcu(client, &evdev->client_list, node) {
> +                       if (client->head != client->tail) {
> +                               ret = -EBUSY;
> +                               break;
> +                       }
> +               }
> +       }
> +
> +       rcu_read_unlock();
> +
> +       return ret;
> +}
> +
> +
>  static void evdev_free(struct device *dev)
>  {
>        struct evdev *evdev = container_of(dev, struct evdev, dev);
> @@ -787,6 +825,21 @@ static void evdev_cleanup(struct evdev *evdev)
>        }
>  }
>
> +static const struct dev_pm_ops evdev_pm_ops = {
> +       .suspend        = &evdev_suspend,
> +};
> +
> +static char *evdev_devnode(struct device *dev, mode_t *mode)
> +{
> +       return kasprintf(GFP_KERNEL, "input/%s", dev_name(dev));
> +}
> +
> +static struct class evdev_class = {
> +       .name           = "evdev",
> +       .devnode        = evdev_devnode,
> +       .pm             = &evdev_pm_ops,
> +};
> +
>  /*
>  * Create new evdev device. Note that input core serializes calls
>  * to connect and disconnect so we don't need to lock evdev_table here.
> @@ -826,7 +879,7 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
>        evdev->handle.private = evdev;
>
>        evdev->dev.devt = MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + minor);
> -       evdev->dev.class = &input_class;
> +       evdev->dev.class = &evdev_class;
>        evdev->dev.parent = &dev->dev;
>        evdev->dev.release = evdev_free;
>        device_initialize(&evdev->dev);
> @@ -883,12 +936,21 @@ static struct input_handler evdev_handler = {
>
>  static int __init evdev_init(void)
>  {
> +       int err;
> +
> +       err = class_register(&input_class);
> +       if (err) {
> +               printk(KERN_ERR "evdev: unable to register evdev class\n");
> +               return err;
> +       }
> +
>        return input_register_handler(&evdev_handler);
>  }
>
>  static void __exit evdev_exit(void)
>  {
>        input_unregister_handler(&evdev_handler);
> +       class_unregister(&evdev_class);
>  }
>
>  module_init(evdev_init);
> --
> 1.7.1.GIT



-- 
Arve Hjønnevåg

Re: [PATCH 0/8] Suspend block api (version 6)

[Linus WALLEIJ]

[Arve wrote]
> On Tue, May 25, 2010 at 2:41 AM, Paul Walmsley <paul@pwsan.com> wrote:
> > On Mon, 17 May 2010, Arve Hjønnevåg wrote:
> >>
> >> How do you know if the work being done while suspending is work that
> >> is needed to suspend or work that should abort suspend?
> >
> > Consider a suspending system in which all "untrusted" processes have been
> > placed into TASK_INTERRUPTIBLE state and have had their timers
> > disabled.[2] The only remaining work to do on the system is work that
> > needs to be done as part of the suspend process, or work that is part of
> > a
> > "trusted" process or the kernel.  If the suspend needs to be aborted due
> > to the arrival of an event, this can be handled as part of the standard
> > suspend process (described below).
> 
> That means modifying all kernel code that would use suspend blockers
> to abort suspend by returning an error from suspend instead. How is
> this better than using suspend blockers?

It will mean that the same API is used for blocking suspend in
regular suspend/resume, runtime PM runtime_suspend/runtime_resume
and now also suspend blockers, right? So only one code path for
drivers that want to support both, no?

That'll be a bit more elegant for drivers that want to support
both runtime PM and suspend blockers I believe.

Overall as a driver writer I'm quite confused at this point, if
I have a driver X, how is that code going to look if it is going
to support both runtime PM *and* suspend blockers? I don't say
it has to support both mechanisms in parallell, I'm even fine
with putting it in #ifdef:s, but how will the source code look?
A real-world example on a simple driver out of the Motorola Droid
git or so would help a lot.

It's quite relevant since e.g. all OMAP drivers will need this
modification to be used with Android-like systems, and at
ST-Ericsson we will need both runtime PM and suspend blockers for
our drivers as well. It will as a consequence affect the drivers
for all ARM reference boards since we use the drivers for ARM
PrimeCells. etc...

Now if this mechanism is going in, my main interest is that there
is some clearly cut way and design pattern for supporting both
runtime PM and suspend blocks.

Yours,
Linus Walleij

Re: [PATCH 0/8] Suspend block api (version 6)

[Alan Stern]

On Wed, 26 May 2010, Linus WALLEIJ wrote:

> Overall as a driver writer I'm quite confused at this point, if
> I have a driver X, how is that code going to look if it is going
> to support both runtime PM *and* suspend blockers? I don't say
> it has to support both mechanisms in parallell, I'm even fine
> with putting it in #ifdef:s, but how will the source code look?
> A real-world example on a simple driver out of the Motorola Droid
> git or so would help a lot.
> 
> It's quite relevant since e.g. all OMAP drivers will need this
> modification to be used with Android-like systems, and at
> ST-Ericsson we will need both runtime PM and suspend blockers for
> our drivers as well. It will as a consequence affect the drivers
> for all ARM reference boards since we use the drivers for ARM
> PrimeCells. etc...
> 
> Now if this mechanism is going in, my main interest is that there
> is some clearly cut way and design pattern for supporting both
> runtime PM and suspend blocks.

It's not difficult.  You just have to decide what situations should 
block opportunistic suspend.  For example, let's say that opportunistic 
suspend should be blocked when your event queue is non-empty.

Then in the routine that adds new events to the queue, while still
holding the spinlock that protects your queue, you enable your suspend
blocker.  Likewise, in the routine that takes events off the queue and
sends them to userspace, while holding the spinlock you test whether
the queue has become empty; if it has you disable your suspend blocker.

That's all.  No other changes are needed (except to create and destroy 
the suspend blocker, of course).  No other interaction with suspend, 
resume, or runtime PM.

Alan Stern

Re: [PATCH 0/8] Suspend block api (version 6)

[Linus WALLEIJ]

[Alan Stern]
> On Wed, 26 May 2010, Linus WALLEIJ wrote:
> (...)
> > Now if this mechanism is going in, my main interest is that there
> > is some clearly cut way and design pattern for supporting both
> > runtime PM and suspend blocks.
> 
> It's not difficult.  You just have to decide what situations should
> block opportunistic suspend.  For example, let's say that opportunistic
> suspend should be blocked when your event queue is non-empty.
> 
> Then in the routine that adds new events to the queue, while still
> holding the spinlock that protects your queue, you enable your suspend
> blocker.  Likewise, in the routine that takes events off the queue and
> sends them to userspace, while holding the spinlock you test whether
> the queue has become empty; if it has you disable your suspend blocker.
> 
> That's all.  No other changes are needed (except to create and destroy
> the suspend blocker, of course).  No other interaction with suspend,
> resume, or runtime PM.

OK I understand it will be rather OK to do this. For example I have
drivers/spi/amba-pl022.c which is quite clearly taking elements
off a queue and transmitting them.

Currently, as we do not have suspend blockers, we try to stop the
queue in suspend() and if that fails we return something negative
so that the suspend() is blocked by the negative return code.

Maybe the behaviour for an SPI bus should rather be to return
-EBUSY as long as there are events in the queue. I don't quite
know frankly.

(We haven't added runtime PM yet, it will likely be the same
thing.)

With suspend blockers we take a suspend blocker when we add
elements to the queue and release it when the queue is empty.

But with suspend blockers the need for suspend() to check if
the queue can be stopped or return -EBUSY if there are elements
in it is not necessary: suspend() simply won't be called
since a suspend blocker is taken. There is no way for
suspend() to stop a running queue like we do today if using
suspend blockers.

This means that driver writers only targeting configurations
where suspend blockers are used will probably not care about
handling suspend() calls by trying to stop the queue or
checking if there are things in it, because there will never
be anything there.

So while it is easy for me to have the driver work under both
suspend blockers and runtime PM or common suspend(), the
two configurations actually have different semantics of the
suspend() call: in the suspend blockers case you don't need
to check anything, just suspend, and in  the runtime PM
case you have to check that you can actually suspend.

Is this analysis correct?

If yes then OK, it's not totally elegant but if that is
where we have to go, I can live with it. There will likely
be people who implement for only one or the other semantic
behaviour, but we have worse things to cope with already.

Yours,
Linus Walleij

Re: [PATCH 0/8] Suspend block api (version 6)

[Florian Mickler]

On Thu, 27 May 2010 09:46:51 +0200
Linus WALLEIJ <linus.walleij@stericsson.com> wrote:


> If yes then OK, it's not totally elegant but if that is
> where we have to go, I can live with it. There will likely
> be people who implement for only one or the other semantic
> behaviour, but we have worse things to cope with already.

Alan Cox suggested, that this kind of explicit requirement definition
might be necessary for all drivers anyway in the long run.

That way, the semantic differences between those two cases would
vanish. 

Cheers,
Flo

Re: [PATCH 0/8] Suspend block api (version 6)

[Arve Hjønnevåg]

On Thu, May 27, 2010 at 12:46 AM, Linus WALLEIJ
<linus.walleij@stericsson.com> wrote:
> [Alan Stern]
>> On Wed, 26 May 2010, Linus WALLEIJ wrote:
>> (...)
>> > Now if this mechanism is going in, my main interest is that there
>> > is some clearly cut way and design pattern for supporting both
>> > runtime PM and suspend blocks.
>>
>> It's not difficult.  You just have to decide what situations should
>> block opportunistic suspend.  For example, let's say that opportunistic
>> suspend should be blocked when your event queue is non-empty.
>>
>> Then in the routine that adds new events to the queue, while still
>> holding the spinlock that protects your queue, you enable your suspend
>> blocker.  Likewise, in the routine that takes events off the queue and
>> sends them to userspace, while holding the spinlock you test whether
>> the queue has become empty; if it has you disable your suspend blocker.
>>
>> That's all.  No other changes are needed (except to create and destroy
>> the suspend blocker, of course).  No other interaction with suspend,
>> resume, or runtime PM.
>
> OK I understand it will be rather OK to do this. For example I have
> drivers/spi/amba-pl022.c which is quite clearly taking elements
> off a queue and transmitting them.
>
> Currently, as we do not have suspend blockers, we try to stop the
> queue in suspend() and if that fails we return something negative
> so that the suspend() is blocked by the negative return code.
>
> Maybe the behaviour for an SPI bus should rather be to return
> -EBUSY as long as there are events in the queue. I don't quite
> know frankly.
>
> (We haven't added runtime PM yet, it will likely be the same
> thing.)
>
> With suspend blockers we take a suspend blocker when we add
> elements to the queue and release it when the queue is empty.
>
> But with suspend blockers the need for suspend() to check if
> the queue can be stopped or return -EBUSY if there are elements
> in it is not necessary: suspend() simply won't be called
> since a suspend blocker is taken. There is no way for
> suspend() to stop a running queue like we do today if using
> suspend blockers.
>
> This means that driver writers only targeting configurations
> where suspend blockers are used will probably not care about
> handling suspend() calls by trying to stop the queue or
> checking if there are things in it, because there will never
> be anything there.
>
> So while it is easy for me to have the driver work under both
> suspend blockers and runtime PM or common suspend(), the
> two configurations actually have different semantics of the
> suspend() call: in the suspend blockers case you don't need
> to check anything, just suspend, and in  the runtime PM
> case you have to check that you can actually suspend.
>
> Is this analysis correct?
>

Mostly, yes. With the current suspend blocker implementation, if you
only start blocking suspend from a user-space thread, or a freezable
kernel thread, then your suspend hook will never be called while you
block suspend unless you use forced suspend. If you for instance start
blocking suspend in response to an interrupt on the other hand, there
is no guarantee that drivers you depend on have not already been
suspended or that they will not suspend after you block suspend, so
you may have to implement a suspend hook to manually abort suspend
even when using suspend blockers. You can add a check if your suspend
blocker is active within your critical section in your suspend hook.
Our alarm driver does this.

> If yes then OK, it's not totally elegant but if that is
> where we have to go, I can live with it. There will likely
> be people who implement for only one or the other semantic
> behaviour, but we have worse things to cope with already.
>

Yes, but you can support both forced suspend and opportunistic suspend
in the same driver. Supporting both is harder if the suspend blocker
api is not in the mainline kernel.

-- 
Arve Hjønnevåg

Re: [PATCH 0/8] Suspend block api (version 6)

[Alan Stern]

On Thu, 27 May 2010, Linus WALLEIJ wrote:

> OK I understand it will be rather OK to do this. For example I have
> drivers/spi/amba-pl022.c which is quite clearly taking elements
> off a queue and transmitting them.
> 
> Currently, as we do not have suspend blockers, we try to stop the
> queue in suspend() and if that fails we return something negative
> so that the suspend() is blocked by the negative return code.

How can it fail?  Even if the queue can't be stopped immediately, can't
the suspend routine block for a few milliseconds until the queue can be 
stopped?

> Maybe the behaviour for an SPI bus should rather be to return
> -EBUSY as long as there are events in the queue. I don't quite
> know frankly.

No.  It should stop the queue.

Think of it this way: Somebody has just closed the lid of their 
laptop and thrown the computer into a backpack.  You don't want the 
machine to fail to suspend merely because an SPI queue wasn't empty.

> (We haven't added runtime PM yet, it will likely be the same
> thing.)

Runtime PM is very different.  It is supposed to take effect only when 
the device is idle.  So failing if the queue is non-empty makes sense.

> With suspend blockers we take a suspend blocker when we add
> elements to the queue and release it when the queue is empty.
> 
> But with suspend blockers the need for suspend() to check if
> the queue can be stopped or return -EBUSY if there are elements
> in it is not necessary: suspend() simply won't be called
> since a suspend blocker is taken.

No, that's completely wrong.  The user can tell the computer to suspend
at any time, whether or not any suspend blockers are enabled.  Think of
the laptop-in-the-backpack case.

> There is no way for
> suspend() to stop a running queue like we do today if using
> suspend blockers.

I don't know what you mean by this, but it doesn't sound right.

> This means that driver writers only targeting configurations
> where suspend blockers are used will probably not care about
> handling suspend() calls by trying to stop the queue or
> checking if there are things in it, because there will never
> be anything there.

You also have to handle races.  What happens if your suspend routine is 
called, and at that moment an interrupt occurs, causing the driver to 
add an entry to the queue?

You're trying to over-simplify this, probably because you're not 
thinking about it in the right way.  To put it bluntly, suspend 
blockers should not affect with your suspend/resume routines at all.

> So while it is easy for me to have the driver work under both
> suspend blockers and runtime PM or common suspend(), the
> two configurations actually have different semantics of the
> suspend() call: in the suspend blockers case you don't need
> to check anything, just suspend, and in  the runtime PM
> case you have to check that you can actually suspend.

The suspend-blockers case is exactly the same as the normal suspend 
case.  The runtime PM case does require extra checking.

> Is this analysis correct?

No, it's mostly wrong.

Alan Stern

Re: [PATCH 0/8] Suspend block api (version 6)

[Linus WALLEIJ]

[Alan]
> On Thu, 27 May 2010, Linus WALLEIJ wrote:
> 
> > OK I understand it will be rather OK to do this. For example I have
> > drivers/spi/amba-pl022.c which is quite clearly taking elements
> > off a queue and transmitting them.
> >
> > Currently, as we do not have suspend blockers, we try to stop the
> > queue in suspend() and if that fails we return something negative
> > so that the suspend() is blocked by the negative return code.
> 
> How can it fail?  Even if the queue can't be stopped immediately, can't
> the suspend routine block for a few milliseconds until the queue can be
> stopped?
> 
> > Maybe the behaviour for an SPI bus should rather be to return
> > -EBUSY as long as there are events in the queue. I don't quite
> > know frankly.
> 
> No.  It should stop the queue.
> 
> Think of it this way: Somebody has just closed the lid of their
> laptop and thrown the computer into a backpack.  You don't want the
> machine to fail to suspend merely because an SPI queue wasn't empty.

Not quite! On the other end of that SPI link is our power
regulators. (I'm not making this up, they are really there.) The events
in the queue may be the register writes that actually put the system
to low-power sleep.

But the apropriate action may likely be to wait for this queue to become
empty before returning 0 from suspend().

Yours,
Linus Walleij