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

Message-ID: <20160923125248.GA23321@e106950-lin.cambridge.arm.com>
Date:   Fri, 23 Sep 2016 13:52:49 +0100
From:   Brian Starkey <brian.starkey@....com>
To:     Daniel Vetter <daniel@...ll.ch>
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 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.

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.

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.

Thanks,
Brian

>-Daniel
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>+41 (0) 79 365 57 48 - http://blog.ffwll.ch
>