[00/33] fbcon notifier begone!
mbox series

Message ID 20190524085354.27411-1-daniel.vetter@ffwll.ch
Headers show
Series
  • fbcon notifier begone!
Related show

Message

Daniel Vetter May 24, 2019, 8:53 a.m. UTC
Hi all,

I fixed the fbcon_exited mess that CI spotted (hopefully it works now, the
code is still rather brittle imo). Plus all the little nits 0day and
people found.

Maarten slapped an rb onto the entire pile, but I feel like enough has
changed that a 2nd look from him is warranted.

I also added a backlight patch on top, I think that nicely highlights how
the fb notifier is now only used for backlight notifications. Maybe we
could rename it as a follow-up to make that clear.

Oh and one rather badly looking thing: am200epd is abusing the notifier in
very interesting ways. I guess a proper fix would be to figure out where
the display boot memory reservation is some more direct way, instead of
listening to the fw fb driver. But that's definitely outside of my
knowledge, so I left it at a bunch of #ifdef and comments.

I think we also still need an ack from Greg KH for the vt and staging
bits.

As usual, comments, review and testing very much welcome.

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.

- 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.

Cheers, Daniel

Daniel Vetter (33):
  dummycon: Sprinkle locking checks
  fbdev: locking check for fb_set_suspend
  vt: might_sleep() annotation for do_blank_screen
  vt: More locking checks
  fbdev/sa1100fb: Remove dead code
  fbdev/cyber2000: Remove struct display
  fbdev/aty128fb: Remove dead code
  fbcon: s/struct display/struct fbcon_display/
  fbcon: Remove fbcon_has_exited
  fbcon: call fbcon_fb_(un)registered directly
  fbdev/sh_mobile: remove sh_mobile_lcdc_display_notify
  fbdev/omap: sysfs files can't disappear before the device is gone
  fbdev: sysfs files can't disappear before the device is gone
  staging/olpc: lock_fb_info can't fail
  fbdev/atyfb: lock_fb_info can't fail
  fbdev: lock_fb_info cannot fail
  fbcon: call fbcon_fb_bind directly
  fbdev: make unregister/unlink functions not fail
  fbdev: unify unlink_framebuffer paths
  fbdev/sh_mob: Remove fb notifier callback
  fbdev: directly call fbcon_suspended/resumed
  fbcon: Call fbcon_mode_deleted/new_modelist directly
  fbdev: Call fbcon_get_requirement directly
  Revert "backlight/fbcon: Add FB_EVENT_CONBLANK"
  fbmem: pull fbcon_fb_blanked out of fb_blank
  fbdev: remove FBINFO_MISC_USEREVENT around fb_blank
  fb: Flatten control flow in fb_set_var
  fbcon: replace FB_EVENT_MODE_CHANGE/_ALL with direct calls
  vgaswitcheroo: call fbcon_remap_all directly
  fbcon: Call con2fb_map functions directly
  fbcon: Document what I learned about fbcon locking
  staging/olpc_dcon: Add drm conversion to TODO
  backlight: simplify lcd notifier

 arch/arm/mach-pxa/am200epd.c                  |  13 +-
 drivers/gpu/vga/vga_switcheroo.c              |  11 +-
 drivers/media/pci/ivtv/ivtvfb.c               |   6 +-
 drivers/staging/fbtft/fbtft-core.c            |   4 +-
 drivers/staging/olpc_dcon/TODO                |   7 +
 drivers/staging/olpc_dcon/olpc_dcon.c         |   6 +-
 drivers/tty/vt/vt.c                           |  18 +
 drivers/video/backlight/backlight.c           |   2 +-
 drivers/video/backlight/lcd.c                 |  12 -
 drivers/video/console/dummycon.c              |   6 +
 drivers/video/fbdev/aty/aty128fb.c            |  64 ---
 drivers/video/fbdev/aty/atyfb_base.c          |   3 +-
 drivers/video/fbdev/core/fbcmap.c             |   6 +-
 drivers/video/fbdev/core/fbcon.c              | 311 ++++++--------
 drivers/video/fbdev/core/fbcon.h              |   6 +-
 drivers/video/fbdev/core/fbmem.c              | 399 +++++++-----------
 drivers/video/fbdev/core/fbsysfs.c            |  20 +-
 drivers/video/fbdev/cyber2000fb.c             |   1 -
 drivers/video/fbdev/neofb.c                   |   9 +-
 .../video/fbdev/omap2/omapfb/omapfb-sysfs.c   |  21 +-
 drivers/video/fbdev/sa1100fb.c                |  25 --
 drivers/video/fbdev/savage/savagefb_driver.c  |   9 +-
 drivers/video/fbdev/sh_mobile_lcdcfb.c        | 112 +----
 drivers/video/fbdev/sh_mobile_lcdcfb.h        |   5 -
 include/linux/console_struct.h                |   5 +-
 include/linux/fb.h                            |  45 +-
 include/linux/fbcon.h                         |  30 ++
 27 files changed, 394 insertions(+), 762 deletions(-)

Comments

Sam Ravnborg May 25, 2019, 5:19 p.m. UTC | #1
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.


> 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.

> - 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.

	Sam
Daniel Vetter May 27, 2019, 7:17 a.m. UTC | #2
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.

> > 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 May 27, 2019, 11:56 a.m. UTC | #3
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