Re: [PATCH 19/21] fbcon: Maintain a private array of fb_info

[Sam Ravnborg]

Hi Daniel,

On Tue, Feb 08, 2022 at 03:03:28PM +0100, Daniel Vetter wrote:
> On Fri, Feb 04, 2022 at 09:15:40PM +0100, Sam Ravnborg wrote:
> > Hi Daniel,
> > 
> > On Mon, Jan 31, 2022 at 10:05:50PM +0100, Daniel Vetter wrote:
> > > Accessing the one in fbmem.c without taking the right locks is a bad
> > > idea. Instead maintain our own private copy, which is fully protected
> > > by console_lock() (like everything else in fbcon.c). That copy is
> > > serialized through fbcon_fb_registered/unregistered() calls.
> > 
> > I fail to see why we can make a private copy of registered_fb
> > just like that - are they not somehow shared between fbcon and fbmem.
> > So when fbmem updates it, then fbcon will use the entry or such?
> > 
> > I guess I am just ignorant of how registered_fb is used - but please
> > explain.
> 
> The private copy is protected under console_lock, and hence safe to access
> from fbcon.c code.
> 
> The main registered_fb array is protected by a different mutex, so we
> could indeed end up with hilarious corruption because the value is
> inconsistent while we try to access it (e.g. we check for !NULL, but later
> on gcc decides to reload the value and now it's suddenly become NULL and
> we blow up).
> 
> The two are synchronized by fbmem.c calling fbcon_register/unregister, so
> aside from the different locks if there's no race going on, they will
> always be identical.
IT was this part that I missed, and it is already spelled out in the
commit message.

> 
> Other option would be to roll out get_fb_info() to fbcon.c, but since
> fbcon.c is fully protected by console_lock that would add complexity in
> the code flow that we don't really need. And we'd have to wire fb_info
> through all call chains, since the right way to use get_fb_info is to look
> it up once and then only drop it when your callback has finished.
> 
> Since the current code just assume it's all protected by console_lock and
> we never drop that during a callback this would mean major surgery and
> essentially refactoring all of fbcon.c to only access the fbcon stuff
> through fb_info, i.e. to get rid of _all_ the global arrays we have more
> or less. I'm not volunteering for that (despite that really this would be
> the right thing to do if we'd have infinite engineering time).
> 
> Ack with that explainer added to the commit message?

I consider the current commit message fine - it helps when you actually
read it.

Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>