Message-ID: <AANLkTimISbs4X2YR_Pal9A80mpkNyCy8laWBcOgR2Cl7@mail.gmail.com>
Date:	Wed, 2 Jun 2010 02:12:10 -0700
From:	Arve Hjønnevåg <arve@...roid.com>
To:	Neil Brown <neilb@...e.de>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Alan Stern <stern@...land.harvard.edu>,
	Felipe Balbi <felipe.balbi@...ia.com>,
	Peter Zijlstra <peterz@...radead.org>,
	"Paul@...p1.linux-foundation.org" <Paul@...p1.linux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Florian Mickler <florian@...kler.org>,
	Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
	Linux PM <linux-pm@...ts.linux-foundation.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	James Bottomley <James.Bottomley@...e.de>
Subject: Re: [PATCH] - race-free suspend. Was: Re: [linux-pm] [PATCH 0/8] 
	Suspend block api (version 8)

2010/6/2 Neil Brown <neilb@...e.de>:
> On Wed, 2 Jun 2010 00:05:14 -0700
> Arve Hjønnevåg <arve@...roid.com> wrote:
>
>> On Tue, Jun 1, 2010 at 10:32 PM, Neil Brown <neilb@...e.de> wrote:
>> > On Tue, 1 Jun 2010 12:50:01 +0200 (CEST)
>> > Thomas Gleixner <tglx@...utronix.de> wrote:
>> >
>> >> On Tue, 1 Jun 2010, Neil Brown wrote:
>> >> >
>> >> > I think you have acknowledged that there is a race with suspend - thanks.
>> >> > Next step was "can it be closed".
>> >> > You seem to suggest that it can, but you describe it as a "work around"
>> >> > rather than a "bug fix"...
>> >> >
>> >> > Do you agree that the race is a "bug", and therefore it is appropriate to
>> >> > "fix" it assuming an acceptable fix can be found (which I think it can)?
>> >>
>> >> If we can fix it, yes we definitely should do and not work around it.
>> >>
>> >> Thanks,
>> >>
>> >>       tglx
>> >
>> > OK.
>> > Here is my suggestion.
>> >
>> > While I think this patch would actually work, and hope the ugly aspects are
>> > reasonably balanced by the simplicity, I present it primarily as a base for
>> > improvement.
>> > The important part is to present how drivers and user-space can co-operate
>> > to avoid losing wake-events.  The details of what happens in the kernel are
>> > certainly up for discussion (as is everything else really of course).
>> >
>> > The user-space suspend daemon avoids losing wake-events by using
>> > fcntl(F_OWNER) to ensure it gets a signal whenever any important wake-event
>> > is ready to be read by user-space.  This may involve:
>> >  - the one daemon processing all wake events
>>
>> Wake up events are not all processed by one daemon.
>
> Not with your current user-space code, no.  Are you saying that you are not
> open to any significant change in the Android user-space code?  That would
> make the situation a lot harder to resolve.
>

Some wakeup events like the an incoming phone may be handled by a
vendor supplied daemon that I do not have the source code for. And, no
I'm not open to a change that would require all wakeup events to go to
a single process.

>>
>> >  - Both the suspend daemon and the main event handling daemon opening any
>> >    given device that delivers wake events (this should work with input
>> >    events ... unless grabbing is needed)
>>
>> Not all wakeup events are broadcast like input events so they cannot
>> be read by both daemons. Not that this really matters, since reading
>> the event from the suspend daemon does not mean that it has been
>> delivered to and processed by the other daemon.
>
> There would still need to be some sort of communication between the the
> suspend daemon on any event daemon to ensure that the events had been
> processed.  This could be very light weight interaction.  The point though is
> that with this patch it becomes possible to avoid races.  Possible is better
> than impossible.
>

We already have a solution. I don't think rejecting our solution but
merging a worse solution should be the goal.

>>
>> >  - The event handling daemon giving the suspend-daemon's pid as F_OWNER, and
>> >    using poll/select to get the events itself.
>>
>> I don't like the idea of using signals for this. Without the hack Alan
>> Stern suggested, you will temporarily block suspend if the wakeup
>> event happened before the suspend daemon thread made it to the kernel,
>> but abort suspend if it happened right after.
>
> I'm not sure why that difference matters.  But I'm also not sure that it is
> true.
> When any wakeup event happen, a signal will be delivered to the suspend
> daemon.
> This will interrupt a pending suspend request, or a sleep, or whatever else
> the daemon is doing.
> It can then go back to waiting for a good time to suspend, and then try to
> suspend again.
>

This is inferior to the solution that is in the android kernel and the
suspend blocker patchset. Both suspend as soon as possible and do not
require signal handlers that modify the argument to your kernel call.

>
>>
>> >
>> > When 'mem' is written to /sys/power/state, suspend_prepare waits in an
>> > interruptible wait until any wake-event that might have been initiated before
>> > the suspend was request, has had a chance to be queued for user-space and
>> > trigger kill_fasync.
>>
>> And what happens if you are not waiting when this happens?
>
> I'm not sure I understand the question.  Could you explain it please?
>

If the thread is not already in the kernel how does your signal
handler abort suspend.

> Either the initial event happens late enough to abort/resume the suspend, or
> the signal happens early enough to abort the suspend, or alert the daemon not
> to do a suspend.  Either way we don't get stuck in suspend.
>
>
>>
>> > Currently this wait is a configurable time after the last wake-event was
>> > initiated.  This is hackish, but simple and probably adequate.
>>
>> Waiting after a wake event is the same as suspend_block_timeout. This
>> is useful when passing events through layers of code that does no
>> block suspend, but we should strive to avoid it. Other people had much
>> stronger objections to this, which is why it is not included in the
>> last suspend blocker patchset.
>
> Absolutely agree.  The idea of of waiting was just a simple way to present
> code that actually could work.  There are doubtlessly better ways and I
> assume they have been implemented in the suspend-blocker code.
> We just need some way to wait for the suspend-block count to reach zero, with
> some confidence that this amount of time is limited.
>
> (though to be honest ... the incredible simplicity of waiting a little while
> is very compelling.... :-))
>

Sure, but forcing that as the only way to prevent suspend is taking to too far.

>>
>> It also does not work for drivers that need to block suspend for more
>> than a few seconds. For instance the gpio keypad matrix driver needs
>> to block suspend while keys are pressed so it can scan the keypad.
>
> I cannot imagine why it would take multiple seconds to scan a keypad.
> Can you explain that?
>
> Do you mean while keys are held pressed?

Yes.

>  Maybe you don't get a wake-up event
> on key-release?

We should.

>  In that case your user-space daemon could block suspend
> while there are any pressed keys....  confused.
>

The user-space daemon should not need to know which keys are in a
matrix. We also have other drivers that need to block suspend. For
instance, some devices need to block suspend while connected to a USB
host.

>
> Thanks for the review,
>
> NeilBrown
>
>



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

Powered by blists - more mailing lists