Re: apm emulation driver broken ?

[Rafael J. Wysocki]

On Monday, 19 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > On Sunday, 18 of November 2007, Franck Bui-Huu wrote:
> >> Rafael J. Wysocki wrote:
> >> See the call to wait_even() made by apm_ioctl(). If any processes
> >> run this, it will prevent the system to suspend...
> > 
> > True, but does it actually happen in practice?
> > 
> 
> when several processes are waiting for a suspend event.
> 
> > 
> > At this point the second branch of the "if (as->suspend_state == SUSPEND_READ)"
> > can be fixed by replacing wait_event_interruptible() with
> > wait_event_freezable(), 
> 
> yes
> 
> > but the fix for the first branch depends on whether or
> > not the wait_event() is really necessary.
> 
> As I said I don't know. It's probably time to put some people
> on CC but don't know who though.

OK, never mind.  I think the patch below is the right fix.


---
From: Rafael J. Wysocki <rjw@xxxxxxx>

The APM emulation is currently broken as a result of commit
831441862956fffa17b9801db37e6ea1650b0f69
"Freezer: make kernel threads nonfreezable by default"
that removed the PF_NOFREEZE annotations from apm_ioctl() without adding
the appropriate freezer hooks.  Fix it and remove the unnecessary variable flags
from apm_ioctl().

Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx>
---
 drivers/char/apm-emulation.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/char/apm-emulation.c
===================================================================
--- linux-2.6.orig/drivers/char/apm-emulation.c
+++ linux-2.6/drivers/char/apm-emulation.c
@@ -295,7 +295,6 @@ static int
 apm_ioctl(struct inode * inode, struct file *filp, u_int cmd, u_long arg)
 {
 	struct apm_user *as = filp->private_data;
-	unsigned long flags;
 	int err = -EINVAL;
 
 	if (!as->suser || !as->writer)
@@ -331,10 +330,16 @@ apm_ioctl(struct inode * inode, struct f
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
 			 */
-			flags = current->flags;
+			freezer_do_not_count();
 
 			wait_event(apm_suspend_waitqueue,
 				   as->suspend_state == SUSPEND_DONE);
+
+			/*
+			 * Since we are waiting until the suspend is done, the
+			 * try_to_freeze() in freezer_count() will not trigger
+			 */
+			freezer_count();
 		} else {
 			as->suspend_state = SUSPEND_WAIT;
 			mutex_unlock(&state_lock);
@@ -362,14 +367,10 @@ apm_ioctl(struct inode * inode, struct f
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
 			 */
-			flags = current->flags;
-
-			wait_event_interruptible(apm_suspend_waitqueue,
+			wait_event_freezable(apm_suspend_waitqueue,
 					 as->suspend_state == SUSPEND_DONE);
 		}
 
-		current->flags = flags;
-
 		mutex_lock(&state_lock);
 		err = as->suspend_result;
 		as->suspend_state = SUSPEND_NONE;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/