On Wednesday, July 12, 2017 12:40:45 PM Bartlomiej Zolnierkiewicz wrote: > On Thursday, July 06, 2017 02:57:32 PM Daniel Vetter wrote: > > 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@xxxxxxxxxxxxxxx> > > 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@xxxxxxxx> > > 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. > > > > Cc: Alan Cox <alan@xxxxxxxxxxxxxxxxxxx> > > Cc: Sergey Senozhatsky <sergey.senozhatsky.work@xxxxxxxxx> > > Cc: Linux Fbdev development list <linux-fbdev@xxxxxxxxxxxxxxx> > > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx> > > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> Patch queued for 4.14, thanks. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
