linux-kernel - Re: [PATCH] Revert "drm: Stop resetting connector state to unknown"

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]
Message-ID: <CAF6AEGsE2a1x7pQ38U9GdC6d+aSr=xVVyV6QTjHmhpST7ktcgg@mail.gmail.com>
Date:	Tue, 15 Dec 2015 13:42:33 -0500
From:	Rob Clark <robdclark@...il.com>
To:	Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:	Lyude <cpaul@...hat.com>, kuddel.mail@....de,
	Daniel Vetter <daniel.vetter@...ll.ch>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
	Julien Wajsberg <felash@...il.com>,
	Benjamin Tissoires <benjamin.tissoires@...hat.com>,
	Lennart Poettering <mzxreary@...inter.de>
Subject: Re: [PATCH] Revert "drm: Stop resetting connector state to unknown"

On Tue, Dec 15, 2015 at 1:33 PM, Ville Syrjälä
<ville.syrjala@...ux.intel.com> wrote:
> On Tue, Dec 15, 2015 at 01:23:06PM -0500, Rob Clark wrote:
>> On Tue, Dec 15, 2015 at 1:07 PM, Lyude <cpaul@...hat.com> wrote:
>> > This reverts commit 5677d67ae394 ("drm: Stop resetting connector state to
>> > unknown")
>> >
>> > Unfortunately, not resetting the connector status to unknown actually
>> > breaks reprobing on suspend/resume in i915, which is important to have
>> > working since it means a user docking their laptop in suspend won't have
>> > their monitors work after resume. This commit was originally pushed to fix
>> > a bug with systemd[1], however said bug has already been fixed in
>> > userspace.
>> >
>> > Since "unknown" is technically a valid option to return to userspace for a
>> > connector's status, I would think that this sort of behavior should
>> > probably be expected from userspace. Some good examples of this are the
>> > radeon driver reporting "unknown" for connectors that have done something
>> > wonky during a hotplug event (e.g. part of the initialization of the
>> > connector failed), and the omapdrm driver returns "unknown" for certain
>> > connector types by default.
>> >
>> > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=100641
>> >
>> > Signed-off-by: Lyude <cpaul@...hat.com>
>> > ---
>> >  drivers/gpu/drm/drm_crtc.c | 7 ++++---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> > index 24c5434..474e636 100644
>> > --- a/drivers/gpu/drm/drm_crtc.c
>> > +++ b/drivers/gpu/drm/drm_crtc.c
>> > @@ -5312,11 +5312,12 @@ void drm_mode_config_reset(struct drm_device *dev)
>> >                 if (encoder->funcs->reset)
>> >                         encoder->funcs->reset(encoder);
>> >
>> > -       mutex_lock(&dev->mode_config.mutex);
>> > -       drm_for_each_connector(connector, dev)
>> > +       list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>> > +               connector->status = connector_status_unknown;
>> > +
>> >                 if (connector->funcs->reset)
>> >                         connector->funcs->reset(connector);
>> > -       mutex_unlock(&dev->mode_config.mutex);
>> > +       }
>> >  }
>>
>> looks like git-revert might have been a bit over-ambitious and
>> clobbered a couple subsequent changes.. but that is easy enough to fix
>> once we figure out what the right thing to do is.
>>
>> Beyond that.. I'm not really sure how to apply the "do not break
>> userspace" rule here.. since prior to
>> c484f02d0f02fbbfc6decc945a69aae011041a27 userspace could see "unknown"
>> for certain hardware.  But after that commit it could start seeing
>> "unknown" for drivers/connectors that never would have returned
>> "unknown" before.  If userspace had a problem with "unknown", it
>> sounds like a userspace bug that was just unnoticed because no one
>> tested on the right hardware.
>>
>> But anyways, one idea to revert things to original behavior prior to
>> c484f02d0f02fbbfc6decc945a69aae011041a27 (so at least userspace
>> doesn't see 'unknown' for drivers/connectors that never used to report
>> 'unknown') would be to do something roughly like this in
>> status_show():
>>
>>   if (status == unknown)
>>      status = connector->funcs->detect(connector)
>>
>> So I could go with either just reverting this commit, or reverting
>> commit plus above change.  My $0.02 anyways..
>
> Hmm. Or maybe leave the states alone, and just fire off the uevent
> unconditionally and let userspace initiate the probe. That way we could
> skip all ->detect() calls during resume for a bit of extra speed.

That is, iirc, something that Lyude suggested when we discussed in
person (but I asked him to send revert patch just to start broader
discussion on what the right solution would be).  Depending on
userspace to reprobe seems like at least the wrong thing for fbcon..
which as much as I wish it would go away, as long as it is enabled in
kernel config, I don't think we can ignore.

BR,
-R

> Not 100% if that would be safe. Maybe something internal depends on the
> ->detect() having been called? DP stuff perhaps? Which makes me wonder
> how i915 copes with a DP monitor geting unplugged/change while
> suspended. I should probably try that.
>
> --
> Ville Syrjälä
> Intel OTC
--
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/