linux-kernel - Re: [PATCH] - race-free suspend. Was: Re: [linux-pm] [PATCH 0/8] Suspend block api (version 8)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Jun 2010 13:26:48 +1000
From:	Neil Brown <neilb@...e.de>
To:	Arve Hjønnevåg <arve@...roid.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Florian Mickler <florian@...kler.org>,
	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>,
	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)

On Wed, 2 Jun 2010 19:44:59 -0700
Arve Hjønnevåg <arve@...roid.com> wrote:

> On Wed, Jun 2, 2010 at 4:32 PM, Dmitry Torokhov
> <dmitry.torokhov@...il.com> wrote:
> > On Wed, Jun 02, 2010 at 09:05:21PM +0200, Florian Mickler wrote:
> >> On Wed, 2 Jun 2010 21:02:24 +1000
> >> Neil Brown <neilb@...e.de> wrote:
> >> >
> >> > And this decision (to block suspend) really needs to be made in the driver,
> >> > not in userspace?
> >>
> >> Well, it fits. The requirement is a direct consequence of the intimate
> >> knowledge the driver has about the driven devices.
> >
> > That is not really true. A driver does have intimate knowledge of the
> > device, however it does not necessarily have an idea about the data read
> > from the device. Consider the gpio_matrix driver: Arve says that it has
> > to continue scanning matrix once first interrupt arrvies. But it really
> > depends on what key has been pressed - if user pressed KEY_SUSPEND or
> > KEY_POWER it cmight be better if we did not wait for key release but
> > initiated the action right away. The decision on how system reacts to a
> > key press does not belong to the driver but really to userspace.
> 
> If we just suspend while the keypad scan is active, a second press of
> KEY_POWER would not be able wake the device up. The gpio keypad matrix
> driver has two operating modes. No keys are pressed: all the outputs
> are active so a key press will generate an interrupt in one of the
> inputs. Keys are pressed: Activate a single output at a time so we can
> determine which keys are pressed. The second mode is entered when the
> driver gets an interrupt in the first mode. The first mode is entered
> if the scan detected that no keys are pressed. The driver could be
> modified to stop the scan on suspend and forcibly put the device back
> in no-keys-pressed state, but pressing additional keys connected to
> the same input can no longer generate any events (the input does not
> change).
> 

Thanks for the detailed explanation.  That helps a lot.

I see that if follows that performing a suspend while keys are pressed would
not be good, and keys could be pressed for arbitrarily long periods.
It does not follow that the keypad driver should therefore explicitly disable
suspend.  It could simply inform user-space that the keypad is in the
alternate scan mode, so user-space can choose not to enter suspend in at that
time (i.e. policy in user-space).

I can see also how one might want to express this behaviour as a PM_QOS
constraint (now that I have read a bit about PM_QOS), but I cannot see that
you would need to.  As the keypad driver is polling during this mode, it
would presumably set a timer that would fire in the near future, and the very
existence of this timer (with a duration shorter than the S3 latency) would
be enough to ensure S3 wasn't automatically entered by PM.
At most you might use set_timer_slack to give a slightly higher upper bound
to the timeout.

So if we take the suspend-from-idle approach, this driver needs no
annotation, and if we take the 'fix the suspend/wake-event race' then this
driver needs no special treatment - just enough to close the race, and some
user-space policy support.

i.e. it doesn't seem to be a special case.

NeilBrown
--
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/