linux-kernel - Re: [PATCH] drm/i2c: tda998x: don't register the connector

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: <20160925203825.GO20761@phenom.ffwll.local>
Date:   Sun, 25 Sep 2016 22:38:25 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Brian Starkey <brian.starkey@....com>
Cc:     Liviu Dudau <Liviu.Dudau@....com>,
        Sean Paul <seanpaul@...omium.org>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] drm/i2c: tda998x: don't register the connector

On Fri, Sep 23, 2016 at 03:42:52PM +0100, Brian Starkey wrote:
> On Fri, Sep 23, 2016 at 03:13:15PM +0200, Daniel Vetter wrote:
> > On Fri, Sep 23, 2016 at 01:52:49PM +0100, Brian Starkey wrote:
> > > On Fri, Sep 23, 2016 at 12:58:46PM +0200, Daniel Vetter wrote:
> > > > On Fri, Sep 23, 2016 at 11:34 AM, Liviu Dudau <Liviu.Dudau@....com> wrote:
> > > > > rmmod-ing the hdlcd module generates a WARN() splat as the vsync is still
> > > > > enabled, but we never got the call to turn off the CRTC. Brian is still
> > > > > tracking through the fbdev emulation to figure out the cause for that.
> > > >
> > > > fbdev emulation doesn't do that for you. If you need/want to shut down
> > > > all the crtcs on driver unload, you need to do that yourself. There's
> > > > atomic helpers to do that for you that for you.
> > > 
> > > The problem is a sort-of circular dependency between ->lastclose (at
> > > least the common implementation of it), unregister and disabling
> > > fbdev.
> > > 
> > > I want to move drm_dev_unregister() to be the first thing we do at
> > > rmmod-time. However we need to disable fbdev first, otherwise
> > > ->lastclose restores the fbdev mode, guaranteeing that vsync is turned
> > > on for drm_vsync_cleanup() to then WARN on.
> > > 
> > > There's a slightly different (perceived) problem - the one that Liviu
> > > mentions - that drm_fbdev_cma_fini() doesn't disable the CRTC anyway.
> > > You say it's not the fbdev helpers' responsibility to teardown their
> > > modeset, but regardless I have nowhere to disable the CRTC if I want
> > > to do drm_dev_unregister() first; and if the CRTC isn't disabled
> > > there's always a chance of hitting the same vsync WARN even without
> > > fbdev.
> > 
> > Just disable all crtc in a suitable place (after drm_dev_unregister,
> > before you tear down fbdev).
> 
> I think this is predicated on first removing the drm_vblank_cleanup
> call.
> 
> > > 
> > > We *could* add an ->unload and disable everything there, but as that's
> > > deprecated I'm guessing there should be another way.
> > > Perhaps we should track ->firstopen/->lastclose pairs so we can detect
> > > that ->lastclose is being called from unregister and use it to
> > > disable everything in that case.
> > 
> > Hm, maybe we should simply not call ->lastclose for kms drivers. That is
> > kinda only a hack for ums/dri1 drivers.
> > 
> 
> To be clear (and in response to Russell's question) - you mean
> only the call to ->lastclose in drm_dev_unregister, not in general?

Yup. But I also don't think it's needed, since drm_vblanke_cleanup seems
to be the root-cause offender.

> > But even with that gone you might still unload while fbdev is enabled,
> > so
> > this won't fix it all.
> > 
> 
> Yeah it will be tidier, but I don't think it actually fixes anything.
> 
> > > drm_vblank_cleanup() seems to have been carried over to unregister
> > > from drm_put_dev(), but drm_dev_register() doesn't call
> > > drm_vblank_init() so it seems a little strange to have it there.
> > > I can see other drivers I'd expect to hit the same WARN but I don't
> > > have HW to test it on.
> > 
> > Oops. That call to drm_vblank_cleanup() really shouldn't be in there. We
> > should push it into all callers instead I think.
> 
> OK so two things to do - remove drm_vblank_cleanup() from
> drm_dev_unregister(), and then do the teardown like so:
> 
> 	drm_dev_unregister();
> 	drm_crtc_force_disable_all(); // or atomic equivalent
> 	fbdev_teardown();
> 	...
> 
> Seems good to me. Are there any ordering constraints you're aware of
> for drm_vblank_cleanup()? Or you think just putting it after
> drm_dev_unregister() should be OK?

Since that's where it's currently is, mechanically pushing it right after
it for each caller should be fine. Driver maintainers can then move it to
an even more suitable place on their own ;-)

Also placing drm_vblank_cleanup in drm_dev_put() makes it even more
obvious that drivers really shouldn't use that function any more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch