Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190524085354.27411-1-daniel.vetter@ffwll.ch>
 <20190525171928.GA13526@ravnborg.org> <20190527071729.GM21222@phenom.ffwll.local>
In-Reply-To: <20190527071729.GM21222@phenom.ffwll.local>
From:   Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Mon, 27 May 2019 13:56:48 +0200
Message-ID: <CAKMK7uEikzt0VRk-9nAScw1ppoOUs4Ay_L4KqHRD5FJBJAWmMQ@mail.gmail.com>
Subject: Re: [PATCH 00/33] fbcon notifier begone!
To:     Sam Ravnborg <sam@ravnborg.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
        DRI Development <dri-devel@lists.freedesktop.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, May 27, 2019 at 9:17 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Sat, May 25, 2019 at 07:19:28PM +0200, Sam Ravnborg wrote:
> > Hi Daniel.
> >
> > Good work, nice cleanup all over.
> >
> > A few comments to a few patches - not something that warrant a
> > new series to be posted as long as it is fixed before the patches are
> > applied.
>
> Hm yeah good idea, I'll add that to the next version.

Actually I thought a bit more about the locking story, and it's a bit
worse than my simple plan here. I think better to just have that
floating around, and not make it look like it's an easy simple
cleanup.

The case I forgot about is that any places that has a printk can
recurse through the console_lock, which means we need a lot more
try_lock than I originally thought we'd need.
-Daniel

>
> > > btw for future plans: I think this is tricky enough (it's old code and all
> > > that) that we should let this soak for 2-3 kernel releases. I think the
> > > following would be nice subsequent cleanup/fixes:
> > >
> > > - push the console lock completely from fbmem.c to fbcon.c. I think we're
> > >   mostly there with prep, but needs to pondering of corner cases.
> > I wonder - should this code consistently use __acquire() etc so we could
> > get a little static analysis help for the locking?
> >
> > I have not played with this for several years and I do not know the
> > maturity of this today.
>
> Ime these sparse annotations are pretty useless because too inflexible. I
> tend to use runtime locking checks based on lockdep. Those are more
> accurate, and work even when the place the lock is taken is very far away
> from where you're checking, without having to annoate all functions in
> between. We need that for something like console_lock which is a very big
> lock. Only downside is that only paths you hit at runtime are checked.
>
> > > - move fbcon.c from using indices for tracking fb_info (and accessing
> > >   registered_fbs without proper locking all the time) to real fb_info
> > >   pointers with the right amount of refcounting. Mostly motivated by the
> > >   fun I had trying to simplify fbcon_exit().
> > >
> > > - make sure that fbcon call lock/unlock_fb when it calls fbmem.c
> > >   functions, and sprinkle assert_lockdep_held around in fbmem.c. This
> > >   needs the console_lock cleanups first.
> > >
> > > But I think that's material for maybe next year or so.
> > Or maybe after next kernel release.
> > Could we put this nice plan into todo.rst or similar so we do not have
> > to hunt it down by asking google?
> >
> > For the whole series you can add my:
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> >
> > Some parts are reviewed as "this looks entirely correct", other parts
> > I would claim that I actually understood.
> > And after having spend some hours on this a r-b seems in order.
>
> Thanks, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch


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