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: <20151215183339.GO4437@intel.com>
Date:	Tue, 15 Dec 2015 20:33:39 +0200
From:	Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:	Rob Clark <robdclark@...il.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 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.

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/