Suspend block api (version 4)

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 <[email protected]>

---
 drivers/input/evdev.c |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

drivers/input/evdev.c +13 -0

index 2ee6c7a..66e0d16 100644
@@ -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')

include/linux/input.h +0 -0

index 7ed2251..b2d93b4 100644

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

Quoted 36 lines

Signed-off-by Arve Hjønnevåg <[email protected]>

---

 drivers/input/evdev.c |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

drivers/input/evdev.c +13 -0

index 2ee6c7a..66e0d16 100644
@@ -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;
+

drivers/input/evdev.c +0 -0

 	default:
 		if (_IOC_TYPE(cmd) != 'E')

include/linux/input.h struct input_absinfo { · line 83

Quoted 7 lines

include/linux/input.h +0 -0

index 7ed2251..b2d93b4 100644
@@ -82,6 +82,9 @@ struct input_absinfo {
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release 

include/linux/input.h struct input_absinfo { · line 84

Quoted 2 lines

include/linux/input.h +1 -0

+#define EVIOCGSUSPENDBLOCK	_IOR('E', 0x91, int)			/* get 

Quoted 1 line

enable */ +#define EVIOCSSUSPENDBLOCK _IOW('E', 0x91, int) /*

Quoted 6 lines

suspend block enable */ +

/*

* Event types
*/

thanks,
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code *** --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

Quoted 36 lines

Signed-off-by Arve Hjønnevåg <[email protected]>

---

 drivers/input/evdev.c |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

drivers/input/evdev.c +13 -0

index 2ee6c7a..66e0d16 100644
@@ -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;
+

drivers/input/evdev.c +0 -0

 	default:
 		if (_IOC_TYPE(cmd) != 'E')

include/linux/input.h struct input_absinfo { · line 83

Quoted 7 lines

include/linux/input.h +0 -0

index 7ed2251..b2d93b4 100644
@@ -82,6 +82,9 @@ struct input_absinfo {
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release 

include/linux/input.h struct input_absinfo { · line 84

Quoted 2 lines

include/linux/input.h +1 -0

+#define EVIOCGSUSPENDBLOCK	_IOR('E', 0x91, int)			/* get 

Quoted 1 line

enable */ +#define EVIOCSSUSPENDBLOCK _IOW('E', 0x91, int) /*

Quoted 6 lines

suspend block enable */ +

/*

* Event types
*/

thanks,
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code *** --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 5 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

Quoted 2 lines

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

I do not see the reason for it to be in the kernel still. Have a process that listens to all input devices (or subset of them), once events stop coming initiate suspend.

Quoted 5 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

Quoted 2 lines

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

I do not see the reason for it to be in the kernel still. Have a process that listens to all input devices (or subset of them), once events stop coming initiate suspend.

Quoted 9 lines · Dmitry Torokhov

Quoted 7 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

Quoted 4 lines

I do not see the reason for it to be in the kernel still. Have a process that listens to all input devices (or subset of them), once events stop coming initiate suspend.

I think the document added by the first patch explains this. The solution you propose above will ignore a wakeup key pressed right after user space decides to initiate suspend.

Quoted 9 lines · Dmitry Torokhov

Quoted 7 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

Quoted 4 lines

I do not see the reason for it to be in the kernel still. Have a process that listens to all input devices (or subset of them), once events stop coming initiate suspend.

I think the document added by the first patch explains this. The solution you propose above will ignore a wakeup key pressed right after user space decides to initiate suspend.

Quoted 8 lines · Arve Hjønnevåg

Quoted 6 lines · Dmitry Torokhov

Quoted 5 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

I do not see the reason for it to be in the kernel still. Have a process that listens to all input devices (or subset of them), once events stop coming initiate suspend.

Quoted 3 lines

I think the document added by the first patch explains this. The solution you propose above will ignore a wakeup key pressed right after user space decides to initiate suspend.

Is there some reason why this feature needs to be enabled by an ioctl? Why not make this suspend blocker permanently enabled?

From all of the interfaces that could be used for this purpose ioctl appears to

Quoted 8 lines · Arve Hjønnevåg

Quoted 6 lines · Dmitry Torokhov

Quoted 5 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

I do not see the reason for it to be in the kernel still. Have a process that listens to all input devices (or subset of them), once events stop coming initiate suspend.

Quoted 3 lines

I think the document added by the first patch explains this. The solution you propose above will ignore a wakeup key pressed right after user space decides to initiate suspend.

Is there some reason why this feature needs to be enabled by an ioctl? Why not make this suspend blocker permanently enabled?

Quoted 9 lines · Alan Stern

Quoted 7 lines · Arve Hjønnevåg

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

Quoted 2 lines

Is there some reason why this feature needs to be enabled by an ioctl? Why not make this suspend blocker permanently enabled?

The ioctl is there so that user space can use suspend blockers, which is needed because only user space know that some activities are going to continue and therefore the system should not be suspended (like playing music "in the background").

Quoted 9 lines · Alan Stern

Quoted 7 lines · Arve Hjønnevåg

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

Quoted 2 lines

Is there some reason why this feature needs to be enabled by an ioctl? Why not make this suspend blocker permanently enabled?

The ioctl is there so that user space can use suspend blockers, which is needed because only user space know that some activities are going to continue and therefore the system should not be suspended (like playing music "in the background").

Quoted 11 lines · Rafael J. Wysocki

Quoted 9 lines · Alan Stern

Quoted 7 lines · Arve Hjønnevåg

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

Quoted 7 lines

Quoted 2 lines

Is there some reason why this feature needs to be enabled by an ioctl?  Why not make this suspend blocker permanently enabled?

The ioctl is there so that user space can use suspend blockers, which is needed because only user space know that some activities are going to continue and therefore the system should not be suspended (like playing music "in the background").

No, you're thinking of a different ioctl: SUSPEND_BLOCKER_IOCTL_BLOCK. This one (EVIOCSSUSPENDBLOCK) is present _only_ to enable one specific suspend blocker, which is activated when the input event queue is non-empty.  I don't see any reason why it shouldn't be enabled all the time.

Quoted 11 lines · Rafael J. Wysocki

Quoted 9 lines · Alan Stern

Quoted 7 lines · Arve Hjønnevåg

Quoted 5 lines · Dmitry Torokhov

Quoted 4 lines · Randy Dunlap

Quoted 3 lines · On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

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.

Quoted 7 lines

Quoted 2 lines

Is there some reason why this feature needs to be enabled by an ioctl?  Why not make this suspend blocker permanently enabled?

The ioctl is there so that user space can use suspend blockers, which is needed because only user space know that some activities are going to continue and therefore the system should not be suspended (like playing music "in the background").

No, you're thinking of a different ioctl: SUSPEND_BLOCKER_IOCTL_BLOCK. This one (EVIOCSSUSPENDBLOCK) is present _only_ to enable one specific suspend blocker, which is activated when the input event queue is non-empty.  I don't see any reason why it shouldn't be enabled all the time.

Quoted 4 lines

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.

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

Quoted 4 lines

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.

All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

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 <[email protected]>

---
 drivers/input/evdev.c |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

drivers/input/evdev.c +13 -0

index 2ee6c7a..66e0d16 100644
@@ -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')

include/linux/input.h +0 -0

index 7ed2251..b2d93b4 100644

Allow work to be queued that will block suspend while it is pending or executing. To get the same functionality in the calling code often requires a separate suspend_blocker for pending and executing work, or additional state and locking.

Allow work to be queued that will block suspend while it is pending or executing. To get the same functionality in the calling code often requires a separate suspend_blocker for pending and executing work, or additional state and locking.

Quoted 4 lines · Arve Hjønnevåg

Allow work to be queued that will block suspend while it is pending or executing. To get the same functionality in the calling code often requires a separate suspend_blocker for pending and executing work, or additional state and locking.

Hmm... I think this can be implemented as pure wrapper around workqueue instead of injecting a flag and code into workqueue core. Adding @fn field to suspend_blocking_work struct and using a custom work function to call it and then invoke suspend_unblock() should be enough, right? Oh, dedicated queue functions will be needed too. I don't think it's wise to meddle with workqueue core code for this.

Quoted 4 lines · Arve Hjønnevåg

Allow work to be queued that will block suspend while it is pending or executing. To get the same functionality in the calling code often requires a separate suspend_blocker for pending and executing work, or additional state and locking.

Hmm... I think this can be implemented as pure wrapper around workqueue instead of injecting a flag and code into workqueue core. Adding @fn field to suspend_blocking_work struct and using a custom work function to call it and then invoke suspend_unblock() should be enough, right? Oh, dedicated queue functions will be needed too. I don't think it's wise to meddle with workqueue core code for this.

Quoted 7 lines · Tejun Heo

Quoted 4 lines · Arve Hjønnevåg

Allow work to be queued that will block suspend while it is pending or executing. To get the same functionality in the calling code often requires a separate suspend_blocker for pending and executing work, or additional state and locking.

Quoted 6 lines

Hmm... I think this can be implemented as pure wrapper around workqueue instead of injecting a flag and code into workqueue core. Adding @fn field to suspend_blocking_work struct and using a custom work function to call it and then invoke suspend_unblock() should be enough, right?  Oh, dedicated queue functions will be needed too.  I don't think it's wise to meddle with workqueue core code for this.

Completely agreed. The patch adds very "strange" hacks into workqueue code to solve the very specific problems.

Besides, the patch doesn't look right. suspend_unblock() can be called twice if you use cancel_work(). Perhaps this is not a problem, I dunno.

WORK_STRUCT_SUSPEND_BLOCKING needs to ensure that cpu_workqueue_struct has a proper alignment.

The unblock code in run_workqueue() is racy,
it can unblock after the work was queued on another CPU, cwq->lock can't help.

Quoted 7 lines · Tejun Heo

Quoted 4 lines · Arve Hjønnevåg

Allow work to be queued that will block suspend while it is pending or executing. To get the same functionality in the calling code often requires a separate suspend_blocker for pending and executing work, or additional state and locking.

Quoted 6 lines

Hmm... I think this can be implemented as pure wrapper around workqueue instead of injecting a flag and code into workqueue core. Adding @fn field to suspend_blocking_work struct and using a custom work function to call it and then invoke suspend_unblock() should be enough, right?  Oh, dedicated queue functions will be needed too.  I don't think it's wise to meddle with workqueue core code for this.

Completely agreed. The patch adds very "strange" hacks into workqueue code to solve the very specific problems.

Besides, the patch doesn't look right. suspend_unblock() can be called twice if you use cancel_work(). Perhaps this is not a problem, I dunno.

WORK_STRUCT_SUSPEND_BLOCKING needs to ensure that cpu_workqueue_struct has a proper alignment.

The unblock code in run_workqueue() is racy,
it can unblock after the work was queued on another CPU, cwq->lock can't help.

Quoted 15 lines · Tejun Heo

Quoted 13 lines · Arve Hjønnevåg

Quoted 4 lines

Allow work to be queued that will block suspend while it is pending or executing. To get the same functionality in the calling code often requires a separate suspend_blocker for pending and executing work, or additional state and locking.

Hmm... I think this can be implemented as pure wrapper around workqueue instead of injecting a flag and code into workqueue core. Adding @fn field to suspend_blocking_work struct and using a custom work function to call it and then invoke suspend_unblock() should be enough, right?  Oh, dedicated queue functions will be needed too.  I don't think it's wise to meddle with workqueue core code for this.

Quoted 3 lines

Completely agreed. The patch adds very "strange" hacks into workqueue code to solve the very specific problems.

I want the suspend blocker active when the work is pending or running. I did not see a way to do this on top of the workqueue api without adding additional locking.

Quoted 15 lines · Tejun Heo

Quoted 13 lines · Arve Hjønnevåg

Quoted 4 lines

Allow work to be queued that will block suspend while it is pending or executing. To get the same functionality in the calling code often requires a separate suspend_blocker for pending and executing work, or additional state and locking.

Hmm... I think this can be implemented as pure wrapper around workqueue instead of injecting a flag and code into workqueue core. Adding @fn field to suspend_blocking_work struct and using a custom work function to call it and then invoke suspend_unblock() should be enough, right?  Oh, dedicated queue functions will be needed too.  I don't think it's wise to meddle with workqueue core code for this.

Quoted 3 lines

Completely agreed. The patch adds very "strange" hacks into workqueue code to solve the very specific problems.

I want the suspend blocker active when the work is pending or running. I did not see a way to do this on top of the workqueue api without adding additional locking.

I want the suspend blocker active when the work is pending or running. I did not see a way to do this on top of the workqueue api without adding additional locking.

If the work is both queued and starts running on another workqueue between "get_wq_data(work) == cwq" and "!work_pending(work)", then suspend_unblock will be called when it shouldn't. It should work fine if I change to it check pending first though, since it cannot move back to the current workqueue without locking cwq->lock first.

Or are you talking about the race when the callback is running on multiple (cpu) workqueues at the same time. In that case the suspend blocker is released when the callback returns from the last workqueue is was queued on, not when all the callbacks have returned. On that note, is it ever safe to use flush_work and cancel_work_sync for work queues on anything other than a single single threaded workqueue?

I want the suspend blocker active when the work is pending or running. I did not see a way to do this on top of the workqueue api without adding additional locking.

If the work is both queued and starts running on another workqueue between "get_wq_data(work) == cwq" and "!work_pending(work)", then suspend_unblock will be called when it shouldn't. It should work fine if I change to it check pending first though, since it cannot move back to the current workqueue without locking cwq->lock first.

Or are you talking about the race when the callback is running on multiple (cpu) workqueues at the same time. In that case the suspend blocker is released when the callback returns from the last workqueue is was queued on, not when all the callbacks have returned. On that note, is it ever safe to use flush_work and cancel_work_sync for work queues on anything other than a single single threaded workqueue?

Hello,

Quoted 3 lines · Arve Hjønnevåg

I want the suspend blocker active when the work is pending or running. I did not see a way to do this on top of the workqueue api without adding additional locking.

Well, then add additional locking.  suspend_blocker is far away from being a hot path and there's nothing wrong with additional locking as long as everything is wrapped inside proper APIs.  Adding stuff to the core code for things as specific as this is much worse.

Quoted 5 lines

If the work is both queued and starts running on another workqueue between "get_wq_data(work) == cwq" and "!work_pending(work)", then suspend_unblock will be called when it shouldn't. It should work fine if I change to it check pending first though, since it cannot move back to the current workqueue without locking cwq->lock first.

The code is more fundamentally broken.  Once work->func has started executing, the work pointer can't be dereferenced as work callback is allowed to free or re-purpose the work data structure and in the same manner you can't check for pending status after execution has completed.

Quoted 6 lines

Or are you talking about the race when the callback is running on multiple (cpu) workqueues at the same time. In that case the suspend blocker is released when the callback returns from the last workqueue is was queued on, not when all the callbacks have returned. On that note, is it ever safe to use flush_work and cancel_work_sync for work queues on anything other than a single single threaded workqueue?

flush_work() is guaranteed only to flush from the last queued cpu but cancel_work_sync() will guarantee that no cpu is executing or holding onto the work.  So, yeah, as long as the limitations of flush_work() is understood, it's safe.

Going back to the original subject, just add simplistic outside locking in suspend_blocker_work API (or whatever other name you prefer).  That will be much cleaner and safer.  Let's think about moving them into workqueue implementation proper after the number of the API users grows to hundreds.

Hello,

Quoted 3 lines · Arve Hjønnevåg

I want the suspend blocker active when the work is pending or running. I did not see a way to do this on top of the workqueue api without adding additional locking.

Well, then add additional locking.  suspend_blocker is far away from being a hot path and there's nothing wrong with additional locking as long as everything is wrapped inside proper APIs.  Adding stuff to the core code for things as specific as this is much worse.

Quoted 5 lines

If the work is both queued and starts running on another workqueue between "get_wq_data(work) == cwq" and "!work_pending(work)", then suspend_unblock will be called when it shouldn't. It should work fine if I change to it check pending first though, since it cannot move back to the current workqueue without locking cwq->lock first.

The code is more fundamentally broken.  Once work->func has started executing, the work pointer can't be dereferenced as work callback is allowed to free or re-purpose the work data structure and in the same manner you can't check for pending status after execution has completed.

Quoted 6 lines

Or are you talking about the race when the callback is running on multiple (cpu) workqueues at the same time. In that case the suspend blocker is released when the callback returns from the last workqueue is was queued on, not when all the callbacks have returned. On that note, is it ever safe to use flush_work and cancel_work_sync for work queues on anything other than a single single threaded workqueue?

flush_work() is guaranteed only to flush from the last queued cpu but cancel_work_sync() will guarantee that no cpu is executing or holding onto the work.  So, yeah, as long as the limitations of flush_work() is understood, it's safe.

Going back to the original subject, just add simplistic outside locking in suspend_blocker_work API (or whatever other name you prefer).  That will be much cleaner and safer.  Let's think about moving them into workqueue implementation proper after the number of the API users grows to hundreds.

OK, I'll try to do this. Do you want it in a separate header file as well?

I only touch the work structure after the callback has returned for suspend blocking work, which does not allow that.

Quoted 3 lines · Oleg Nesterov

The unblock code in run_workqueue() is racy,
it can unblock after the work was queued on another CPU, cwq->lock can't help.

If the work is both queued and starts running on another workqueue between "get_wq_data(work) == cwq" and "!work_pending(work)", then suspend_unblock will be called when it shouldn't. It should work fine if I change to it check pending first though, since it cannot move back to the current workqueue without locking cwq->lock first.

Quoted 3 lines · Oleg Nesterov

The unblock code in run_workqueue() is racy,
it can unblock after the work was queued on another CPU, cwq->lock can't help.

If the work is both queued and starts running on another workqueue between "get_wq_data(work) == cwq" and "!work_pending(work)", then suspend_unblock will be called when it shouldn't. It should work fine if I change to it check pending first though, since it cannot move back to the current workqueue without locking cwq->lock first.

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

Signed-off-by Arve Hjønnevåg <[email protected]>

---
 kernel/power/suspend_blocker.c |   44 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 42 insertions(+), 2 deletions(-)

kernel/power/suspend_blocker.c +33 -2

index 868e8f1..047e910 100644
@@ -17,6 +17,7 @@
 #include <linux/rtc.h>
 #include <linux/suspend.h>
 #include <linux/suspend_blocker.h>
+#include <linux/debugfs.h>
 #include "power.h"
 
 enum {

@@ -41,6 +42,7 @@ struct workqueue_struct *suspend_work_queue;
 struct suspend_blocker main_suspend_blocker;
 static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
 static bool enable_suspend_blockers;
+static struct dentry *suspend_blocker_stats_dentry;
 
 #define pr_info_time(fmt, args...) \
 	do { \

@@ -54,6 +56,21 @@ static bool enable_suspend_blockers;
 			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
 	} while (0);
 
+static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
+{
+	unsigned long irqflags;
+	struct suspend_blocker *blocker;
+
+	seq_puts(m, "name\tactive\n");
+	spin_lock_irqsave(&list_lock, irqflags);
+	list_for_each_entry(blocker, &inactive_blockers, link)
+		seq_printf(m, "\"%s\"\t0\n", blocker->name);
+	list_for_each_entry(blocker, &active_blockers, link)
+		seq_printf(m, "\"%s\"\t1\n", blocker->name);
+	spin_unlock_irqrestore(&list_lock, irqflags);
+	return 0;
+}
+
 static void print_active_blockers_locked(void)
 {
 	struct suspend_blocker *blocker;

@@ -101,8 +118,8 @@ static DECLARE_WORK(suspend_work, suspend_worker);
 /**
  * suspend_blocker_init() - Initialize a suspend blocker
  * @blocker:	The suspend blocker to initialize.
- * @name:	The name of the suspend blocker to show in debug messages.
- *
+ * @name:	The name of the suspend blocker to show in debug messages and
+ *		/sys/kernel/debug/suspend_blockers.
  * The suspend blocker struct and name must not be freed before calling
  * suspend_blocker_destroy.
  */

@@ -240,6 +257,19 @@ int request_suspend_state(suspend_state_t state)
 	return 0;
 }
 
+static int suspend_blocker_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, suspend_blocker_stats_show, NULL);
+}
+
+static const struct file_operations suspend_blocker_stats_fops = {
+	.owner = THIS_MODULE,
+	.open = suspend_blocker_stats_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
 static int __init suspend_block_init(void)
 {
 	suspend_work_queue = create_singlethread_workqueue("suspend");

@@ -251,4 +281,14 @@ static int __init suspend_block_init(void)
 	return 0;
 }
 
+static int __init suspend_block_postcore_init(void)

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

Signed-off-by Arve Hjønnevåg <[email protected]>

---
 kernel/power/suspend_blocker.c |   44 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 42 insertions(+), 2 deletions(-)

kernel/power/suspend_blocker.c +33 -2

index 868e8f1..047e910 100644
@@ -17,6 +17,7 @@
 #include <linux/rtc.h>
 #include <linux/suspend.h>
 #include <linux/suspend_blocker.h>
+#include <linux/debugfs.h>
 #include "power.h"
 
 enum {

@@ -41,6 +42,7 @@ struct workqueue_struct *suspend_work_queue;
 struct suspend_blocker main_suspend_blocker;
 static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
 static bool enable_suspend_blockers;
+static struct dentry *suspend_blocker_stats_dentry;
 
 #define pr_info_time(fmt, args...) \
 	do { \

@@ -54,6 +56,21 @@ static bool enable_suspend_blockers;
 			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
 	} while (0);
 
+static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
+{
+	unsigned long irqflags;
+	struct suspend_blocker *blocker;
+
+	seq_puts(m, "name\tactive\n");
+	spin_lock_irqsave(&list_lock, irqflags);
+	list_for_each_entry(blocker, &inactive_blockers, link)
+		seq_printf(m, "\"%s\"\t0\n", blocker->name);
+	list_for_each_entry(blocker, &active_blockers, link)
+		seq_printf(m, "\"%s\"\t1\n", blocker->name);
+	spin_unlock_irqrestore(&list_lock, irqflags);
+	return 0;
+}
+
 static void print_active_blockers_locked(void)
 {
 	struct suspend_blocker *blocker;

@@ -101,8 +118,8 @@ static DECLARE_WORK(suspend_work, suspend_worker);
 /**
  * suspend_blocker_init() - Initialize a suspend blocker
  * @blocker:	The suspend blocker to initialize.
- * @name:	The name of the suspend blocker to show in debug messages.
- *
+ * @name:	The name of the suspend blocker to show in debug messages and
+ *		/sys/kernel/debug/suspend_blockers.
  * The suspend blocker struct and name must not be freed before calling
  * suspend_blocker_destroy.
  */

@@ -240,6 +257,19 @@ int request_suspend_state(suspend_state_t state)
 	return 0;
 }
 
+static int suspend_blocker_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, suspend_blocker_stats_show, NULL);
+}
+
+static const struct file_operations suspend_blocker_stats_fops = {
+	.owner = THIS_MODULE,
+	.open = suspend_blocker_stats_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
 static int __init suspend_block_init(void)
 {
 	suspend_work_queue = create_singlethread_workqueue("suspend");

@@ -251,4 +281,14 @@ static int __init suspend_block_init(void)
 	return 0;
 }
 
+static int __init suspend_block_postcore_init(void)

Quoted 3 lines

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

Quoted 3 lines

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

2010/4/23 Randy Dunlap <[email protected]>:

Quoted 6 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hjønnevåg wrote:

Quoted 3 lines

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

Quoted 2 lines

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

Where should it be? It is not allowed in sysfs, and procfs does not appear to be open to new files.

Quoted 7 lines

2010/4/23 Randy Dunlap <[email protected]>:

Quoted 6 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hjønnevåg wrote:

Quoted 3 lines

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

Quoted 5 lines

Quoted 2 lines

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

Where should it be? It is not allowed in sysfs, and procfs does not appear to be open to new files.

Why is it not allowed in sysfs?  due to more than one value per file

or something else?

I don't think that procfs is closed to new files, but you may have to justify why it should be used.

Quoted 7 lines

2010/4/23 Randy Dunlap <[email protected]>:

Quoted 6 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hjønnevåg wrote:

Quoted 3 lines

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

Quoted 5 lines

Quoted 2 lines

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

Where should it be? It is not allowed in sysfs, and procfs does not appear to be open to new files.

Why is it not allowed in sysfs?  due to more than one value per file

or something else?

I don't think that procfs is closed to new files, but you may have to justify why it should be used.

Quoted 2 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

Quoted 6 lines · Randy Dunlap

Quoted 2 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

Quoted 2 lines

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it. I have no objection to it going there.

Quoted 6 lines · Randy Dunlap

Quoted 2 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

Quoted 2 lines

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it. I have no objection to it going there.

Quoted 3 lines

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it. I have no objection to it going there.

I have no objection if it really is debug info, but I'm not convinced of that yet.

Quoted 3 lines

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it. I have no objection to it going there.

I have no objection if it really is debug info, but I'm not convinced of that yet.

Quoted 6 lines · Randy Dunlap

Quoted 3 lines

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it. I have no objection to it going there.

I have no objection if it really is debug info, but I'm not convinced of that yet.

Well, I'll note right now we have a somewhat annoying gap. If you need to export multiple values such that they are consistent with each other, what's the choice? /proc, where some (but not all) kernel developers will say, "eeeeeeviilllll". /sys is explicitly for single value per files only. And then we have /debugfs, where some pendants are kvetching about whether something is "really" debug information.

One of the things that we sometimes have to tell people who are trying to navigate the maze of upstream submission is that sometimes you need to know who to ignore, and that sometimes rules are guidelines (despite pendants who will NACK based on rules like, "/proc, eeeeewwww", or "/debugfs must only strictly be for debug information".

Telling embedded developers who only want to submit their driver that they must create a whole new pseudo-filesystem just to export a single file that in older, simpler times, would have just been thrown into /proc is really not fair, and is precisely the sort of thing that may cause them to say, "f*ck it, these is one too many flaming hoops to jump through". If we throw up too many barriers, in the long run it's not actually doing Linux a service.

Sure, we need to make sure is code doesn't become a future burden, but does a new file in /proc or something that might not _really_ be debug information showing up in /debugfs really such a terrible thing in terms of making the kernel less maintainable in the future?

Quoted 6 lines · Randy Dunlap

Quoted 3 lines

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it. I have no objection to it going there.

I have no objection if it really is debug info, but I'm not convinced of that yet.

Well, I'll note right now we have a somewhat annoying gap. If you need to export multiple values such that they are consistent with each other, what's the choice? /proc, where some (but not all) kernel developers will say, "eeeeeeviilllll". /sys is explicitly for single value per files only. And then we have /debugfs, where some pendants are kvetching about whether something is "really" debug information.

One of the things that we sometimes have to tell people who are trying to navigate the maze of upstream submission is that sometimes you need to know who to ignore, and that sometimes rules are guidelines (despite pendants who will NACK based on rules like, "/proc, eeeeewwww", or "/debugfs must only strictly be for debug information".

Telling embedded developers who only want to submit their driver that they must create a whole new pseudo-filesystem just to export a single file that in older, simpler times, would have just been thrown into /proc is really not fair, and is precisely the sort of thing that may cause them to say, "f*ck it, these is one too many flaming hoops to jump through". If we throw up too many barriers, in the long run it's not actually doing Linux a service.

Sure, we need to make sure is code doesn't become a future burden, but does a new file in /proc or something that might not _really_ be debug information showing up in /debugfs really such a terrible thing in terms of making the kernel less maintainable in the future?

Yeah, I think that it should be in procfs. It's not strictly closed to new files. (IOW, I'm sure that we can find a bunch of recent files added to procfs.)

Yeah, I think that it should be in procfs. It's not strictly closed to new files. (IOW, I'm sure that we can find a bunch of recent files added to procfs.)

Quoted 3 lines · Randy Dunlap

Yeah, I think that it should be in procfs. It's not strictly closed to new files. (IOW, I'm sure that we can find a bunch of recent files added to procfs.)

That's reasonable (I think the whole /proc is evil crusade is really dumb) --- but at the same time, remember how frustrating it is for the poor embedded developer who doesn't know who to ignore and what "rules" to ignore. They were told months ago /proc is evil, and so they moved it to /debugfs, precisely because it was billed as "it has no rules". For all I know some helpful community developer may have even suggested that to them.

It is extremely frustrating to embedded developers when they get conflicting advice, especially in this case, when it was *in* /proc in the first place. I'm not sure what to do about this --- my approach is to sometimes say, "f*ck it, that's stupid advice", and ship it to Linus, who tends to be *much* less of a pendant than most of the people who review code --- but that's because I know what I can ignore. (I seriously doubt Linus cares much about whether it ends up the file ends up /debugfs or /proc, and would take the code either way.) But for someone who doesn't understand when you can do this, and who tries to follow every single piece of criticism they get, the end result can often be a huge amount fo wasted time and frustration.

It would be nice if we could get better at this, since I'm sure this is not the only time when embedded code submissions have gotten what the submitting developers might consider to be a runaround....

(And just to be clear, I'm not criticising your commends; my personal preference is slightly in favor of /proc, but more than anythign else, I consider it a very minor point. I just want to point out that they _started_ with the file in /proc and changed it to /debugfs based on someone NACK'ing their patch, with a "/proc, eeeeewwww" comment. Sometimes I think some code reviewers get too much of a sense of power trip by thinking they can NACK other people's code over their own pet peeves.... instead of approaching it from a somewhat more collegial point of view trying to make the code better. Present company excepted, of course. :-)

Quoted 3 lines · Randy Dunlap

Yeah, I think that it should be in procfs. It's not strictly closed to new files. (IOW, I'm sure that we can find a bunch of recent files added to procfs.)

That's reasonable (I think the whole /proc is evil crusade is really dumb) --- but at the same time, remember how frustrating it is for the poor embedded developer who doesn't know who to ignore and what "rules" to ignore. They were told months ago /proc is evil, and so they moved it to /debugfs, precisely because it was billed as "it has no rules". For all I know some helpful community developer may have even suggested that to them.

It is extremely frustrating to embedded developers when they get conflicting advice, especially in this case, when it was *in* /proc in the first place. I'm not sure what to do about this --- my approach is to sometimes say, "f*ck it, that's stupid advice", and ship it to Linus, who tends to be *much* less of a pendant than most of the people who review code --- but that's because I know what I can ignore. (I seriously doubt Linus cares much about whether it ends up the file ends up /debugfs or /proc, and would take the code either way.) But for someone who doesn't understand when you can do this, and who tries to follow every single piece of criticism they get, the end result can often be a huge amount fo wasted time and frustration.

It would be nice if we could get better at this, since I'm sure this is not the only time when embedded code submissions have gotten what the submitting developers might consider to be a runaround....

(And just to be clear, I'm not criticising your commends; my personal preference is slightly in favor of /proc, but more than anythign else, I consider it a very minor point. I just want to point out that they _started_ with the file in /proc and changed it to /debugfs based on someone NACK'ing their patch, with a "/proc, eeeeewwww" comment. Sometimes I think some code reviewers get too much of a sense of power trip by thinking they can NACK other people's code over their own pet peeves.... instead of approaching it from a somewhat more collegial point of view trying to make the code better. Present company excepted, of course. :-)

Quoted 6 lines

Well, I'll note right now we have a somewhat annoying gap. If you need to export multiple values such that they are consistent with each other, what's the choice? /proc, where some (but not all) kernel developers will say, "eeeeeeviilllll". /sys is explicitly for single value per files only. And then we have /debugfs, where some pendants are kvetching about whether something is "really" debug information.

First of all, I am not a pendant.

Quoted 6 lines

Well, I'll note right now we have a somewhat annoying gap. If you need to export multiple values such that they are consistent with each other, what's the choice? /proc, where some (but not all) kernel developers will say, "eeeeeeviilllll". /sys is explicitly for single value per files only. And then we have /debugfs, where some pendants are kvetching about whether something is "really" debug information.

First of all, I am not a pendant.

Quoted 10 lines · Greg KH

Quoted 8 lines · Randy Dunlap

Quoted 2 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

Quoted 3 lines

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it.  I have no objection to it going there.

I have no objection if it really is debug info, but I'm not convinced of that yet.

Quoted 10 lines · Greg KH

Quoted 8 lines · Randy Dunlap

Quoted 2 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

Quoted 3 lines

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it.  I have no objection to it going there.

I have no objection if it really is debug info, but I'm not convinced of that yet.

Quoted 15 lines · Randy Dunlap

Quoted 13 lines · Greg KH

Quoted 8 lines · Randy Dunlap

Quoted 2 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it. I have no objection to it going there.

Quoted 2 lines

I have no objection if it really is debug info, but I'm not convinced of that yet.

Our most common use of this information is to capture it in bugreports (along with process lists, system/radio logs, memory stats, etc) as one additional piece of data used to diagnose a misbehaving device (most commonly to answer the "why does the battery seem to be draining too quickly?" question).

Quoted 15 lines · Randy Dunlap

Quoted 13 lines · Greg KH

Quoted 8 lines · Randy Dunlap

Quoted 2 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

It's debug-like information, and has more than one value per file, so debugfs seems like the proper place for it. I have no objection to it going there.

Quoted 2 lines

I have no objection if it really is debug info, but I'm not convinced of that yet.

Our most common use of this information is to capture it in bugreports (along with process lists, system/radio logs, memory stats, etc) as one additional piece of data used to diagnose a misbehaving device (most commonly to answer the "why does the battery seem to be draining too quickly?" question).

Quoted 2 lines · On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

This looks more like user interface information than it does debug information. If that's correct, it shouldn't be in debugfs.

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hjønnevåg <[email protected]>

---
 Documentation/power/suspend-blockers.txt |   17 ++++
 include/linux/suspend_block_dev.h        |   25 ++++++
 kernel/power/Kconfig                     |    9 ++
 kernel/power/Makefile                    |    1 +
 kernel/power/user_suspend_blocker.c      |  128 ++++++++++++++++++++++++++++++
 5 files changed, 180 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/suspend_block_dev.h
 create mode 100644 kernel/power/user_suspend_blocker.c

Documentation/power/suspend-blockers.txt +0 -0

index 1c48514..877bd8c 100644

Documentation/power/suspend-blockers.txt +4 -0

+
+To destroy the suspend_blocker, close the device:
+    close(fd);
+

kernel/power/user_suspend_blocker.c new file +20 -0

new file mode 100644
index 0000000..a9be6f4
@@ -0,0 +1,128 @@
+/* kernel/power/user_suspend_block.c
+ *
+ * Copyright (C) 2009-2010 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hjønnevåg <[email protected]>

---
 Documentation/power/suspend-blockers.txt |   17 ++++
 include/linux/suspend_block_dev.h        |   25 ++++++
 kernel/power/Kconfig                     |    9 ++
 kernel/power/Makefile                    |    1 +
 kernel/power/user_suspend_blocker.c      |  128 ++++++++++++++++++++++++++++++
 5 files changed, 180 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/suspend_block_dev.h
 create mode 100644 kernel/power/user_suspend_blocker.c

Documentation/power/suspend-blockers.txt +0 -0

index 1c48514..877bd8c 100644

Documentation/power/suspend-blockers.txt +4 -0

+
+To destroy the suspend_blocker, close the device:
+    close(fd);
+

kernel/power/user_suspend_blocker.c new file +20 -0

new file mode 100644
index 0000000..a9be6f4
@@ -0,0 +1,128 @@
+/* kernel/power/user_suspend_block.c
+ *
+ * Copyright (C) 2009-2010 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 4 lines

Documentation/power/suspend-blockers.txt +4 -0

+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

Why not initialize the user suspend blocker struct by default and then allow each BLOCK to specify the name? Also, my guess is it's not really a name so much as a description of why we're blocking suspend, right?

Should the kernel reject empty strings or strings composed only of "non-printing" characters?

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 110

Quoted 6 lines

Documentation/power/suspend-blockers.txt +6 -0

+
+To activate a suspend_blocker call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To unblock call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);

lsof will show which tasks hold the device open but not which ones are blocking suspend. If merely keeping the device open corresponded to blocking suspend then this would be obvious and no ioctls would be necessary -- just write() the name/description.

Do you block/unblock often enough that frequent open/close of the device are a problem?

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+static DEFINE_MUTEX(ioctl_lock);

nit: Usually locks protect data -- not functions.

Couldn't this be part of the user_suspend_blocker struct? That would allow the description/name to change as described above.

kernel/power/user_suspend_blocker.c line 35

Quoted 6 lines

kernel/power/user_suspend_blocker.c new file +5 -0

+
+struct user_suspend_blocker {
+     struct suspend_blocker  blocker;
+     char                    name[0];
+};

<snip>

Cheers,
       -Matt Helsley

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 4 lines

Documentation/power/suspend-blockers.txt +4 -0

+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

Why not initialize the user suspend blocker struct by default and then allow each BLOCK to specify the name? Also, my guess is it's not really a name so much as a description of why we're blocking suspend, right?

Should the kernel reject empty strings or strings composed only of "non-printing" characters?

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 110

Quoted 6 lines

Documentation/power/suspend-blockers.txt +6 -0

+
+To activate a suspend_blocker call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To unblock call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);

lsof will show which tasks hold the device open but not which ones are blocking suspend. If merely keeping the device open corresponded to blocking suspend then this would be obvious and no ioctls would be necessary -- just write() the name/description.

Do you block/unblock often enough that frequent open/close of the device are a problem?

Review

Quoted 2 lines

(quoted diff) +2 -0

+
+static DEFINE_MUTEX(ioctl_lock);

nit: Usually locks protect data -- not functions.

Couldn't this be part of the user_suspend_blocker struct? That would allow the description/name to change as described above.

kernel/power/user_suspend_blocker.c line 35

Quoted 6 lines

kernel/power/user_suspend_blocker.c new file +5 -0

+
+struct user_suspend_blocker {
+     struct suspend_blocker  blocker;
+     char                    name[0];
+};

<snip>

Cheers,
       -Matt Helsley

Should the kernel reject empty strings or strings composed only of "non-printing" characters?

Should the kernel reject empty strings or strings composed only of "non-printing" characters?

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Hi!

Quoted 13 lines

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Documentation/power/suspend-blockers.txt +0 -0

@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 7 lines

Documentation/power/suspend-blockers.txt +7 -0

+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

This seems like very wrong idea -- it uses different ioctl number for each length AFAICT.

Hi!

Quoted 13 lines

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Documentation/power/suspend-blockers.txt +0 -0

@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 7 lines

Documentation/power/suspend-blockers.txt +7 -0

+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

This seems like very wrong idea -- it uses different ioctl number for each length AFAICT.

Quoted 15 lines · Pavel Machek

Hi!

Quoted 13 lines

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Documentation/power/suspend-blockers.txt +0 -0

@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))

 else
    suspend_block(&state->suspend_blocker);

Quoted 11 lines

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 7 lines

Documentation/power/suspend-blockers.txt +7 -0

+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

This seems like very wrong idea -- it uses different ioctl number for each length AFAICT.

How about specifying the name by an ordinary write() call instead of by an ioctl()?

Quoted 15 lines · Pavel Machek

Hi!

Quoted 13 lines

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Documentation/power/suspend-blockers.txt +0 -0

@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))

 else
    suspend_block(&state->suspend_blocker);

Quoted 11 lines

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 7 lines

Documentation/power/suspend-blockers.txt +7 -0

+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

This seems like very wrong idea -- it uses different ioctl number for each length AFAICT.

How about specifying the name by an ordinary write() call instead of by an ioctl()?

Quoted 29 lines · Alan Stern

Quoted 26 lines · Pavel Machek

Hi!

Quoted 13 lines

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Documentation/power/suspend-blockers.txt +0 -0

@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))

 else
    suspend_block(&state->suspend_blocker);

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 7 lines

Documentation/power/suspend-blockers.txt +7 -0

+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

This seems like very wrong idea -- it uses different ioctl number for each length AFAICT.

Quoted 3 lines

How about specifying the name by an ordinary write() call instead of by an ioctl()?

I prefer using ioctls. We have three operations at the moment. Init, block and unblock. If we do init with write but block and unblock using ioctls, it would be pretty strange. Specifying a command and

argument in a string to write is more complicated to parse than using ioctls.

Quoted 29 lines · Alan Stern

Quoted 26 lines · Pavel Machek

Hi!

Quoted 13 lines

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Documentation/power/suspend-blockers.txt +0 -0

@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))

 else
    suspend_block(&state->suspend_blocker);

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 7 lines

Documentation/power/suspend-blockers.txt +7 -0

+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

This seems like very wrong idea -- it uses different ioctl number for each length AFAICT.

Quoted 3 lines

How about specifying the name by an ordinary write() call instead of by an ioctl()?

I prefer using ioctls. We have three operations at the moment. Init, block and unblock. If we do init with write but block and unblock using ioctls, it would be pretty strange. Specifying a command and

argument in a string to write is more complicated to parse than using ioctls.

Quoted 34 lines · Arve Hj?nnev?g

Quoted 32 lines · Alan Stern

Quoted 15 lines · Pavel Machek

Hi!

Quoted 13 lines

Add a misc device, "suspend_blocker", that allows user-space processes to block auto suspend. The device has ioctls to create a suspend_blocker, and to block and unblock suspend. To delete the suspend_blocker, close the device.

Signed-off-by Arve Hj??nnev??g <[email protected]>

Documentation/power/suspend-blockers.txt +0 -0

@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))

 else
    suspend_block(&state->suspend_blocker);

Quoted 11 lines

Documentation/power/suspend-blockers.txt if (list_empty(&state->pending_work)) · line 104

Quoted 7 lines

Documentation/power/suspend-blockers.txt +7 -0

+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

This seems like very wrong idea -- it uses different ioctl number for each length AFAICT.

How about specifying the name by an ordinary write() call instead of by an ioctl()?

Quoted 3 lines

I prefer using ioctls. We have three operations at the moment. Init, block and unblock. If we do init with write but block and unblock using ioctls, it would be pretty strange. Specifying a command and

Why would it be "strange"?

Quoted 2 lines

argument in a string to write is more complicated to parse than using ioctls.

More complicated to parse?