linux-kernel - Re: [Intel-gfx] [RFC PATCH 3/3] i915: ignore lid open event when resuming

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] [day] [month] [year] [list]

Message-ID: <1359959205.2250.56.camel@rzhang1-mobl4>
Date:	Mon, 04 Feb 2013 14:26:45 +0800
From:	Zhang Rui <rui.zhang@...el.com>
To:	Daniel Vetter <daniel@...ll.ch>
Cc:	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	linux-acpi@...r.kernel.org, intel-gfx@...ts.freedesktop.org,
	rjw@...k.pl, lenb@...nel.org
Subject: Re: [Intel-gfx] [RFC PATCH 3/3] i915: ignore lid open event when
 resuming

On Tue, 2013-01-29 at 11:10 +0100, Daniel Vetter wrote:
> On Mon, Jan 28, 2013 at 06:06:38PM +0800, Zhang Rui wrote:
> > On Mon, 2013-01-28 at 09:31 +0100, Daniel Vetter wrote:
> > > On Mon, Jan 28, 2013 at 3:36 AM, Zhang Rui <rui.zhang@...el.com> wrote:
> > > >> Given that this essentially requires users to manually set this module
> > > >> option to make stuff work I don't like this.
> > > >>
> > > > sorry, I do not understand.
> > > > this patch just disables modeset_on_lid during suspend.
> > > 
> > > Pardon me, the misunderstanding is on my side - I've mixed up
> > > dev_priv->modeset_on_lid with the corresponding module option.
> > > 
> > > Is my understanding correct that only with the new SCI pm state this
> > > can happen, since that still allows acpi events to be processed (and
> > > so our lid_notifier being called?
> > > 
> > yep.
> > 
> > > >> I see a few possible options:
> > > >> - plug the races between all the different parts - I've never really
> > > >> understood why acpi sends us events before the resume code has
> > > >> completed ... If that's indeed intentional, we could delay the
> > > >> handling a bit until at least the i915 resume code completed.
> > > >
> > > > IMO, the real question is that, during a suspend/resume cycle, if we
> > > > need to take care of the lid open event or not.
> > > > To me, the answer is no, because when resuming from STR, i915 is not
> > > > aware of such an event, and the i915 resume code works well, right?
> > > > so I do not think it is a problem to ignore the lid event for another
> > > > lightweight suspend state, which is introduced in Patch 1/3 in this
> > > > series.
> > > >
> > > >> - Judging from the diff context you're not on the latest 3.8-rc
> > > >> codebase - we've applied a few fixes to this lid hack recently. Please
> > > >> retest on a kernel with
> > > >>
> > > > the patch is based on 3.8.0-rc3, which already includes the commit
> > > > below.
> > > > And yes, the problem still exists.
> > > 
> > > Ok, I think I see the issue now. But I suspect that we need some
> > > additional locking to make this solid, since currently
> > > dev_priv->modeset_on_lid updates can race with our lid notifier
> > > handler. Let me think about this a bit more.
> > 
> > hmm,
> > with this patch, intel_lid_notify() will return immediately if
> > modeset_on_lid is set to -1. So the only problem in my mind is that we
> > got a lid open event during i915_suspend().
> > 
> > Say,
> > lid is opened -> i915_lid_notify() is invoked (modeset_on_lid is 1 at
> > this time) -> i915_suspend() set modeset_on_lid to -1, just before
> > intel_modeset_setup_hw_state() being invoked in i915_lid_notify() ->
> > intel_modeset_setup_hw_state() breaks the system.
> > 
> > but my first question would be is this (lid open on suspend) possible in
> > real world?
> > If the answer is yes, then my second question is that, this problem
> > exists for STR as well because SCI is still valid at this time when
> > suspending to memory, have we seen such issues for STR before?
> > 
> > Anyway, if the current code does not break STR, this patch should be
> > enough for the new suspend state.
> > what do you think?
> 
> I think I have two wishlist changes for your patch ;-)
> 
> - Convert dev_priv->modeset_on_lid to an enum, so that we have descriptive
>   names instead of magic numbers.
> 
sure, will update in next version.

> - I think we can close all races by adding a new lid_notifier mutex (I
>   prefer a new lock instead of overloading an existing one with). If we
>   hold that in the suspend/resume paths just for changing modeset_on_lid
>   and also for the entire lid notifier callback (i.e. grab the lock before
>   first looking at modest_on_lid, only drop it once the modeset fixup has
>   been completed at the end of the function) we should be race-free in all
>   cases.
> 
>   Also, I think we should move the modeset_on_lid updates in the
>   suspend/resume paths to the very beginning/end.
> 
> Can you pls update your patch with these changes? Or do you see an
> additional race we need to plug somewhere?
> 
yep. will send out V2 soon.
thanks for your comments.

-rui

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