linux-kernel - Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector registration

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: <20161108092552.6qnum32uhndnsdou@phenom.ffwll.local>
Date:   Tue, 8 Nov 2016 10:25:52 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     Daniel Vetter <daniel@...ll.ch>,
        Brian Starkey <brian.starkey@....com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm: tda998x: mali-dp: hdlcd: refactor connector
 registration

On Mon, Oct 31, 2016 at 08:58:43AM +0000, Russell King - ARM Linux wrote:
> On Mon, Oct 24, 2016 at 10:24:42PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 24, 2016 at 4:52 PM, Brian Starkey <brian.starkey@....com> wrote:
> > >>
> > >>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> > >>> b/drivers/gpu/drm/i2c/tda998x_drv.c
> > >>> index f4315bc..6e6fca2 100644
> > >>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> > >>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> > >>> @@ -1369,7 +1369,6 @@ const struct drm_connector_helper_funcs
> > >>> tda998x_connector_helper_funcs = {
> > >>>
> > >>>  static void tda998x_connector_destroy(struct drm_connector *connector)
> > >>>  {
> > >>> -       drm_connector_unregister(connector);
> > >>>         drm_connector_cleanup(connector);
> > >>>  }
> > >>>
> > >>> @@ -1441,16 +1440,10 @@ static int tda998x_bind(struct device *dev,
> > >>> struct device *master, void *data)
> > >>>         if (ret)
> > >>>                 goto err_connector;
> > >>>
> > >>> -       ret = drm_connector_register(&priv->connector);
> > >>> -       if (ret)
> > >>> -               goto err_sysfs;
> > >>> -
> > >>
> > >>
> > >> Instead of smashing all these patches into one, what about checking here
> > >> for midlayer driver set with:
> > >>
> > >>         /* register here for drivers still using midlayer load/unload */
> > >>         if (dev->driver->load)
> > >>                 drm_connector_register(connector),
> > >>
> > >> Similar in other places. That way we wouldn't need to switch the world in
> > >> one patch.
> > >
> > >
> > > I don't think that helps. If we do that in isolation (first), then
> > > mali-dp and hdlcd won't get their connectors registered because their
> > > bind order is:
> > >
> > >         drm_dev_register();
> > >         component_bind_all();
> > >
> > > If we change the mali-dp/hdlcd bind order first, then tda998x will
> > > explode on drm_connector_register() until it's patched to remove that.
> > >
> > > As I mentioned in my mail to Russell, the only way I can see to avoid
> > > patching all three drivers in one go is:
> > >  1) Add (probably open-coded) drm_connector_register_all() to the end
> > >     of bind in hdlcd and mali-dp
> > >  2) Patch tda998x to remove drm_connector_register()
> > >  3) Reorder hdlcd/mali-dp bind and remove the connector registration
> > >     added in 1)
> > >
> > > We can do that, but it's extra churn for the same result, and none of
> > > the 5 patches will really make sense in isolation anyway.
> > 
> > I thought there's also armada to take care of, which this patch would
> > break?
> 
> NO NO NO NO NO.  I've said this several times.  Let's try it again,
> and see if it sticks.
> 
> Because Armada has not been converted from a mid-layered driver, it
> is _IMMUNE_ from any patch removing the drm_connector_register() call
> in TDA998x.  It does _NOT_ break in any way.
> 
> Only those drivers which are de-mid-layered, and worked around the
> drm_connector_register() call inside TDA998x (eg, mali) break, because
> of the order in which they are _forced_ to call stuff.
> 
> In a de-mid-layered driver, with the drm_connector_register() call in
> place in TDA998x, drm_dev_register() _MUST_ be called prior to
> component_bind_all(), otherwise you get a WARN_ON() dump from the
> kobject code.  With the drm_connector_register() call removed,
> drm_dev_register() _MUST_ be called after component_bind_all() so that
> the connector is registered.
> 
> It's the de-mid-layered drivers which are the problem here, not the
> mid-layered ones like Armada.
> 
> > Maybe even another driver, so the hack would still be useful
> > for those other drivers. And it would have been useful if malidp/hdlcd
> > wouldn't have started out with the wrong init ordering ;-)
> 
> It's forced into the "wrong init ordering" due to the kobject WARN_ON.

Hm, I entirely missed that part of the troubles. Anyway, if you all agree
on a patch I certainly won't block it, feel free to merge through suitable
trees (or I can smash it into drm-misc if that's wanted).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch