On Mon, May 27, 2019 at 9:17 AM Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxx> > > > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
