apm emulation driver broken ?

apm emulation driver broken ?

[Franck Bui-Huu]

Rafael,

Looking at commit:

	831441862956fffa17b9801db37e6ea1650b0f69
	Freezer: make kernel threads nonfreezable by default

it seems that you broke the apm emulation driver.

You removed PF_NOFREEZE flag setting in apm_ioctl(), which is
definitely not part of the apm kernel daemon but instead is called by
user space proccesses...

I'm just reading this code for the first time so I can be wrong but it
looks like it's not going to work anymore.

Could you confirm ?

		Franck

Re: apm emulation driver broken ?

[Rafael J. Wysocki]

Hi,

On Friday, 16 of November 2007, Franck Bui-Huu wrote:
> Rafael,
> 
> Looking at commit:
> 
> 	831441862956fffa17b9801db37e6ea1650b0f69
> 	Freezer: make kernel threads nonfreezable by default
> 
> it seems that you broke the apm emulation driver.
> 
> You removed PF_NOFREEZE flag setting in apm_ioctl(), which is
> definitely not part of the apm kernel daemon but instead is called by
> user space proccesses...

Yes ...

> I'm just reading this code for the first time so I can be wrong but it
> looks like it's not going to work anymore.
> 
> Could you confirm ?

Well, no, AFAICS.

The freezer doesn't regard the current task as freezable.

Greetings,
Rafael

Re: apm emulation driver broken ?

[Franck Bui-Huu]

On Nov 16, 2007 5:20 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> The freezer doesn't regard the current task as freezable.
>


Hmm, I don't get your point.

If I understood this driver correctly, several processes can be
waiting for a suspend event by reading /dev/apm_bios, apmd (the _user_
space daemon) can be one of them.

Then another process asks to suspend the system by calling 'apm -s',
which results in a apm_ioctl() call. This process will basically
execute:

	err = queue_suspend_event(APM_USER_SUSPEND, as);
	flags = current->flags;
	wait_event_interruptible(apm_suspend_waitqueue,
			as->suspend_state == SUSPEND_DONE);

It's basically waiting for the waiters to ack the event. But it won't
be the process that is going to suspend the system, right ?

So now all waiting processes are waken up and need to acknolwedge the
event for the system to actually suspend. So they need to call
apm_ioctl(). They'll basically do:

	flags = current->flags;
	wait_event(apm_suspend_waitqueue,
			as->suspend_state == SUSPEND_DONE);

Except for the last acknowledging process which will do instead:

	apm_suspend();

It's a call to pm_suspend().

So you can see that the process which initiates the suspend, the one
that calls 'apm -s', is not the current process but is going to be
waken up by the fake signal sent by freeze_task().

One of the consequence I can see is at this time 'as->result' won't be
setup, so the return value of apm_ioctl() may be wrong.

As I said, I'm not familiar with this code, so please correct me if
I'm wrong.

BTW, how does try_to_freeze_tasks() deal with user land thread waiting
in the UNINTERRUPTIBLE state ?

Thanks.

               Franck

Re: apm emulation driver broken ?

[Rafael J. Wysocki]

On Friday, 16 of November 2007, Franck Bui-Huu wrote:
> On Nov 16, 2007 5:20 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > The freezer doesn't regard the current task as freezable.
> >
> 
> 
> Hmm, I don't get your point.
> 
> If I understood this driver correctly, several processes can be
> waiting for a suspend event by reading /dev/apm_bios, apmd (the _user_
> space daemon) can be one of them.
> 
> Then another process asks to suspend the system by calling 'apm -s',
> which results in a apm_ioctl() call. This process will basically
> execute:
> 
> 	err = queue_suspend_event(APM_USER_SUSPEND, as);
> 	flags = current->flags;
> 	wait_event_interruptible(apm_suspend_waitqueue,
> 			as->suspend_state == SUSPEND_DONE);
> 
> It's basically waiting for the waiters to ack the event. But it won't
> be the process that is going to suspend the system, right ?
> 
> So now all waiting processes are waken up and need to acknolwedge the
> event for the system to actually suspend. So they need to call
> apm_ioctl(). They'll basically do:
> 
> 	flags = current->flags;
> 	wait_event(apm_suspend_waitqueue,
> 			as->suspend_state == SUSPEND_DONE);
> 
> Except for the last acknowledging process which will do instead:
> 
> 	apm_suspend();
> 
> It's a call to pm_suspend().
> 
> So you can see that the process which initiates the suspend, the one
> that calls 'apm -s', is not the current process but is going to be
> waken up by the fake signal sent by freeze_task().
> 
> One of the consequence I can see is at this time 'as->result' won't be
> setup, so the return value of apm_ioctl() may be wrong.

Ah, that.  Yes, I see your point.

However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
a good idea.

I'd probably use wait_event_freezable() (defined in include/linux/freezer.h)
for that.

> As I said, I'm not familiar with this code, so please correct me if
> I'm wrong.

No, you're not wrong and I have overlooked the problem.

> BTW, how does try_to_freeze_tasks() deal with user land thread waiting
> in the UNINTERRUPTIBLE state ?

It tries to send them fake signals and waits for them to freeze.  If they don't
freeze within the timeout, it fails and clears their TIF_FREEZE bits.

Greetings,
Rafael

Re: apm emulation driver broken ?

[Franck Bui-Huu]

Rafael J. Wysocki wrote:
> However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
> a good idea.
> 

Indeed but...

> I'd probably use wait_event_freezable() (defined in
> include/linux/freezer.h) for that.

...I would just revert this bits from now to make sure this driver
work again for v2.6.24.
 
> It tries to send them fake signals and waits for them to freeze.  If
> they don't freeze within the timeout, it fails and clears their
> TIF_FREEZE bits.

But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
only. Looking at signal_wake_up(), it basically do:

	wake_up_state(t, TASK_INTERRUPTIBLE);

What am I missing ?

		Franck

Re: apm emulation driver broken ?

[Rafael J. Wysocki]

On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
> > a good idea.
> > 
> 
> Indeed but...
> 
> > I'd probably use wait_event_freezable() (defined in
> > include/linux/freezer.h) for that.
> 
> ...I would just revert this bits from now to make sure this driver
> work again for v2.6.24.

I'd prefer not to.

The PF_NOFREEZE was not present in 2.6.23 already and I wouldn't like to
reintroduce it now.

Why do you think that using wait_event_freezable() would not work, BTW?

> > It tries to send them fake signals and waits for them to freeze.  If
> > they don't freeze within the timeout, it fails and clears their
> > TIF_FREEZE bits.
> 
> But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
> only. Looking at signal_wake_up(), it basically do:
> 
> 	wake_up_state(t, TASK_INTERRUPTIBLE);
> 
> What am I missing ?

Nothing. :-)

I didn't remember the change that made the freezer use TASK_INTERRUPTIBLE
explicitly in there (should have looked at the current code before replying).

Greetings,
Rafael

Re: apm emulation driver broken ?

[Rafael J. Wysocki]

On Saturday, 17 of November 2007, Rafael J. Wysocki wrote:
> On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> > Rafael J. Wysocki wrote:
> > > However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
> > > a good idea.
> > > 
> > 
> > Indeed but...
> > 
> > > I'd probably use wait_event_freezable() (defined in
> > > include/linux/freezer.h) for that.
> > 
> > ...I would just revert this bits from now to make sure this driver
> > work again for v2.6.24.
> 
> I'd prefer not to.
> 
> The PF_NOFREEZE was not present in 2.6.23 already and I wouldn't like to
> reintroduce it now.
> 
> Why do you think that using wait_event_freezable() would not work, BTW?
> 
> > > It tries to send them fake signals and waits for them to freeze.  If
> > > they don't freeze within the timeout, it fails and clears their
> > > TIF_FREEZE bits.
> > 
> > But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
> > only. Looking at signal_wake_up(), it basically do:
> > 
> > 	wake_up_state(t, TASK_INTERRUPTIBLE);
> > 
> > What am I missing ?
> 
> Nothing. :-)
> 
> I didn't remember the change that made the freezer use TASK_INTERRUPTIBLE
> explicitly in there (should have looked at the current code before replying).

Actually, not even that one.  You're right anyway.

Below is a patch that IMO should fix the issue with apm_ioctl().

Greetings,
Rafael


---
From: Rafael J. Wysocki <rjw@sisk.pl>

The code in apm_ioctl() allows user space tasks waiting for a suspend to
complete to be woken up prematurely as a result of the thawing of tasks carried
out by the freezer.  Fix it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/char/apm-emulation.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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
@@ -364,7 +364,7 @@ apm_ioctl(struct inode * inode, struct f
 			 */
 			flags = current->flags;
 
-			wait_event_interruptible(apm_suspend_waitqueue,
+			wait_event_freezable(apm_suspend_waitqueue,
 					 as->suspend_state == SUSPEND_DONE);
 		}
 

Re: apm emulation driver broken ?

[Franck Bui-Huu]

Rafael J. Wysocki wrote:
> On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
>> Rafael J. Wysocki wrote:
>>> However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
>>> a good idea.
>>>
>> Indeed but...
>>
>>> I'd probably use wait_event_freezable() (defined in
>>> include/linux/freezer.h) for that.
>> ...I would just revert this bits from now to make sure this driver
>> work again for v2.6.24.
> 
> I'd prefer not to.
> 
> The PF_NOFREEZE was not present in 2.6.23 already and I wouldn't like to
> reintroduce it now.
> 
> Why do you think that using wait_event_freezable() would not work, BTW?
> 

I've never claimed this. I just said it may be safer to revert the
changes for v2.6.24 and improve the current code for next releases.

>>> It tries to send them fake signals and waits for them to freeze.  If
>>> they don't freeze within the timeout, it fails and clears their
>>> TIF_FREEZE bits.
>> But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
>> only. Looking at signal_wake_up(), it basically do:
>>
>> 	wake_up_state(t, TASK_INTERRUPTIBLE);
>>
>> What am I missing ?
> 
> Nothing. :-)
> 
> I didn't remember the change that made the freezer use TASK_INTERRUPTIBLE
> explicitly in there (should have looked at the current code before replying).
> 

ok so now we agreed on this point, can we assert that a user
land thread waiting for an event in an UNINTERRUPTIBLE state
will prevent a suspend to happen ?

		Franck

Re: apm emulation driver broken ?

[Rafael J. Wysocki]

On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> >> Rafael J. Wysocki wrote:
> >>> However, using PF_NOFREEZE to prevent this from happening doesn't seem to be
> >>> a good idea.
> >>>
> >> Indeed but...
> >>
> >>> I'd probably use wait_event_freezable() (defined in
> >>> include/linux/freezer.h) for that.
> >> ...I would just revert this bits from now to make sure this driver
> >> work again for v2.6.24.
> > 
> > I'd prefer not to.
> > 
> > The PF_NOFREEZE was not present in 2.6.23 already and I wouldn't like to
> > reintroduce it now.
> > 
> > Why do you think that using wait_event_freezable() would not work, BTW?
> > 
> 
> I've never claimed this. I just said it may be safer to revert the
> changes for v2.6.24 and improve the current code for next releases.
> 
> >>> It tries to send them fake signals and waits for them to freeze.  If
> >>> they don't freeze within the timeout, it fails and clears their
> >>> TIF_FREEZE bits.
> >> But send_fake_signal() seems to wake up task in INTERRUPTIBLE state
> >> only. Looking at signal_wake_up(), it basically do:
> >>
> >> 	wake_up_state(t, TASK_INTERRUPTIBLE);
> >>
> >> What am I missing ?
> > 
> > Nothing. :-)
> > 
> > I didn't remember the change that made the freezer use TASK_INTERRUPTIBLE
> > explicitly in there (should have looked at the current code before replying).
> > 
> 
> ok so now we agreed on this point, can we assert that a user
> land thread waiting for an event in an UNINTERRUPTIBLE state
> will prevent a suspend to happen ?

Yes.

Rafael

Re: apm emulation driver broken ?

[Franck Bui-Huu]

Rafael J. Wysocki wrote:
> On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
>> ok so now we agreed on this point, can we assert that a user
>> land thread waiting for an event in an UNINTERRUPTIBLE state
>> will prevent a suspend to happen ?
> 
> Yes.
> 

So this driver seems really broken and actually I'm wondering if
it's used by anyone...

See the call to wait_even() made by apm_ioctl(). If any processes
run this, it will prevent the system to suspend...

And no, I don't know why call wait_event() is called.

		Franck

Re: apm emulation driver broken ?

[Rafael J. Wysocki]

On Sunday, 18 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > On Saturday, 17 of November 2007, Franck Bui-Huu wrote:
> >> ok so now we agreed on this point, can we assert that a user
> >> land thread waiting for an event in an UNINTERRUPTIBLE state
> >> will prevent a suspend to happen ?
> > 
> > Yes.
> > 
> 
> So this driver seems really broken and actually I'm wondering if
> it's used by anyone...

Well, it doesn't seem so.

> 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?

> And no, I don't know why call wait_event() is called.

I hope somebody knows. :-)

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(), but the fix for the first branch depends on whether or
not the wait_event() is really necessary.

If it can be replaced with an interruptible sleep, we can use
wait_event_freezable() in this case too.  Otherwise, the only woking fix would
be to reintroduce the PF_NOFREEZE in there.

Honestly, I'm leaning towards replacing wait_event() in apm_ioctl() with
wait_event_freezable() and seeing what happens ...

Greetings,
Rafael

Re: apm emulation driver broken ?

[Franck Bui-Huu]

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.

> 
> If it can be replaced with an interruptible sleep, we can use
> wait_event_freezable() in this case too.  Otherwise, the only woking fix would
> be to reintroduce the PF_NOFREEZE in there.

BTW, why not raising PF_NOFREEZE in wait_event(), so thread sleeping
in UNINTERRUPTIBLE state won't prevent suspend to happen ?


		Franck

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@sisk.pl>

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@sisk.pl>
---
 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;