[Intel-gfx] [PATCH 00/33] fbcon notifier begone!
Daniel Vetter
daniel.vetter at ffwll.ch
Mon May 20 08:21:43 UTC 2019
Hi all,
This patch series removes the fbcon notifier. It also contains the
beginnings of trying to understand how locking in this area works.
The super short summary of locking rules is that everything in fbcon needs
to be protected by the monolithic console_lock, including the setup code.
So not really an option to pull the kms modeset code out from underneath
that, at least not without rewriting half the console layer. Or I'm not
yet seeing clear enough.
The other side is that fbdev userspace access vs. fbcon locking is totally
broken.
For context of this series I'm just going to quote the commit message that
started this all:
commit 6104c37094e729f3d4ce65797002112735d49cd1
Author: Daniel Vetter <daniel.vetter at ffwll.ch>
Date: Tue Aug 1 17:32:07 2017 +0200
fbcon: Make fbcon a built-time depency for fbdev
There's a bunch of folks who're trying to make printk less
contended and faster, but there's a problem: printk uses the
console_lock, and the console lock has become the BKL for all things
fbdev/fbcon, which in turn pulled in half the drm subsystem under that
lock. That's awkward.
There reasons for that is probably just a historical accident:
- fbcon is a runtime option of fbdev, i.e. at runtime you can pick
whether your fbdev driver instances are used as kernel consoles.
Unfortunately this wasn't implemented with some module option, but
through some module loading magic: As long as you don't load
fbcon.ko, there's no fbdev console support, but loading it (in any
order wrt fbdev drivers) will create console instances for all fbdev
drivers.
- This was implemented through a notifier chain. fbcon.ko enumerates
all fbdev instances at load time and also registers itself as
listener in the fbdev notifier. The fbdev core tries to register new
fbdev instances with fbcon using the notifier.
- On top of that the modifier chain is also used at runtime by the
fbdev subsystem to e.g. control backlights for panels.
- The problem is that the notifier puts a mutex locking context
between fbdev and fbcon, which mixes up the locking contexts for
both the runtime usage and the register time usage to notify fbcon.
And at runtime fbcon (through the fbdev core) might call into the
notifier from a printk critical section while console_lock is held.
- This means console_lock must be an outer lock for the entire fbdev
subsystem, which also means it must be acquired when registering a
new framebuffer driver as the outermost lock since we might call
into fbcon (through the notifier) which would result in a locking
inversion if fbcon would acquire the console_lock from its notifier
callback (which it needs to register the console).
- console_lock can be held anywhere, since printk can be called
anywhere, and through the above story, plus drm/kms being an fbdev
driver, we pull in a shocking amount of locking hiercharchy
underneath the console_lock. Which makes cleaning up printk really
hard (not even splitting console_lock into an rwsem is all that
useful due to this).
There's various ways to address this, but the cleanest would be to
make fbcon a compile-time option, where fbdev directly calls the fbcon
register functions from register_framebuffer, or dummy static inline
versions if fbcon is disabled. Maybe augmented with a runtime knob to
disable fbcon, if that's needed (for debugging perhaps).
But this could break some users who rely on the magic "loading
fbcon.ko enables/disables fbdev framebuffers at runtime" thing, even
if that's unlikely. Hence we must be careful:
1. Create a compile-time dependency between fbcon and fbdev in the
least minimal way. This is what this patch does.
2. Wait at least 1 year to give possible users time to scream about
how we broke their setup. Unlikely, since all distros make fbcon
compile-in, and embedded platforms only compile stuff they know they
need anyway. But still.
3. Convert the notifier to direct functions calls, with dummy static
inlines if fbcon is disabled. We'll still need the fb notifier for the
other uses (like backlights), but we can probably move it into the fb
core (atm it must be built-into vmlinux).
4. Push console_lock down the call-chain, until it is down in
console_register again.
5. Finally start to clean up and rework the printk/console locking.
For context of this saga see
commit 50e244cc793d511b86adea24972f3a7264cae114
Author: Alan Cox <alan at linux.intel.com>
Date: Fri Jan 25 10:28:15 2013 +1000
fb: rework locking to fix lock ordering on takeover
plus the pile of commits on top that tried to make this all work
without terminally upsetting lockdep. We've uncovered all this when
console_lock lockdep annotations where added in
commit daee779718a319ff9f83e1ba3339334ac650bb22
Author: Daniel Vetter <daniel.vetter at ffwll.ch>
Date: Sat Sep 22 19:52:11 2012 +0200
console: implement lockdep support for console_lock
On the patch itself:
- Switch CONFIG_FRAMEBUFFER_CONSOLE to be a boolean, using the overall
CONFIG_FB tristate to decided whether it should be a module or
built-in.
- At first I thought I could force the build depency with just a dummy
symbol that fbcon.ko exports and fb.ko uses. But that leads to a
module depency cycle (it works fine when built-in).
Since this tight binding is the entire goal the simplest solution is
to move all the fbcon modules (and there's a bunch of optinal
source-files which are each modules of their own, for no good
reason) into the overall fb.ko core module. That's a bit more than
what I would have liked to do in this patch, but oh well.
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
Cc: Alan Cox <alan at lxorguk.ukuu.org.uk>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work at gmail.com>
Cc: Steven Rostedt <rostedt at goodmis.org>
Reviewed-by: Sean Paul <seanpaul at chromium.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie at samsung.com>
Comments, review and testing much appreciated, as always.
Wrt long-term plans: I'm not sure where this will lead just yet, or
whether it'll lead anywhere at all. But I think removing the notifier
indirection is definitely a step into the right direction.
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_framebufer 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"
fbcon: directly call fbcon_fb_blanked
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
drivers/gpu/vga/vga_switcheroo.c | 11 +-
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 | 2 -
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 | 303 ++++++--------
drivers/video/fbdev/core/fbcon.h | 6 +-
drivers/video/fbdev/core/fbmem.c | 374 ++++++------------
drivers/video/fbdev/core/fbsysfs.c | 20 +-
drivers/video/fbdev/cyber2000fb.c | 1 -
.../video/fbdev/omap2/omapfb/omapfb-sysfs.c | 21 +-
drivers/video/fbdev/sa1100fb.c | 25 --
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 | 40 +-
include/linux/fbcon.h | 30 ++
22 files changed, 342 insertions(+), 725 deletions(-)
--
2.20.1
More information about the Intel-gfx
mailing list