On Fri, Jun 06, 2014 at 09:24:35AM +0200, David Herrmann wrote: > Hi > > On Thu, Jun 5, 2014 at 4:58 PM, Daniel Vetter <daniel.vetter at ffwll.ch> > wrote: > > A bunch of issues: > > - We should not kick out the default console (which is tracked in > > conswitchp), so check for that. > > - Add better error codes so callers can differentiate between "something > > went wrong" and "your driver isn't registered already". i915 needs > > that so it doesn't fall over when reloading the driver and hence > > vga_con is already unregistered. > > - There's a mess with the driver flags: What we need to check for is > > that the driver isn't used any more, i.e. unbound completely (FLAG_INIT). > > And not whether it's the boot console or not (which is the only one > > which doesn't have FLAG_MODULE). Otherwise there's no way to kick > > out the boot console, which i915 wants to do to prevent havoc with > > vga_con interferring (which tends to hang machines). > > > > Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org> > > Cc: Jiri Slaby <jslaby at suse.cz> > > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch> > > --- > > drivers/tty/vt/vt.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c > > index ea600f482eeb..5077fe87324d 100644 > > --- a/drivers/tty/vt/vt.c > > +++ b/drivers/tty/vt/vt.c > > @@ -3573,17 +3573,20 @@ err: > > */ > > int do_unregister_con_driver(const struct consw *csw) > > { > > - int i, retval = -ENODEV; > > + int i; > > > > /* cannot unregister a bound driver */ > > if (con_is_bound(csw)) > > - goto err; > > + return -EBUSY; > > + > > + if (csw == conswitchp) > > + return -EINVAL; > > Ugh, that fix is correct, but I'd rather like to see > do_unbind_con_driver() do the right thing. It currently resets > conswitchp _only_ if the new fallback is unbound. Why not _always_ set > conswitchp to defcsw _iff_ conswitchp == csw there?
Ha, that's what I've thought, too. But do_unbind doesn't actually change conswitchp, it only restores it because apparently the vga_con->con_startup function is a real con and changes it behind everyones back for no good reason. Or at least that's what the comment claims. Note how defconsw != defcsw ... I've tried to follow around how conswitchp is actually used, but besides that it's used to select the boot console I'm not sure at all what's going hence. > This way, you _know_ here that if !con_is_bound(csw), then csw != conswitchp. Hence why I dropped this approach again (I've done it originally) and opted for the straightforward but albeit crude direct check. > > > > for (i = 0; i < MAX_NR_CON_DRIVER; i++) { > > struct con_driver *con_driver = ®istered_con_driver[i]; > > > > if (con_driver->con == csw && > > - con_driver->flag & CON_DRIVER_FLAG_MODULE) { > > + con_driver->flag & CON_DRIVER_FLAG_INIT) { > > That makes FLAG_MODULE almost a no-op except for ->unbind(). I wonder > why FLAG_MODULE exists, anyway. I've dug around in git history and it's less than useful. It was renamed from FLAG_BIND (which makes somewhat sense, since it roughly tracks whether a console is bound). But there's never been a justification for it, neither in the original patch nor in the one that renamed it. So I decided to not tempt fate and went with the small change here that I've understood somewhat (I've tried other, more invasive changes and failed). > Otherwise looks good. I'm really reluctant to do the right thing here since the code overall has very unclear semantics with conswitchp and FLAG_MODULE. Can I convince yout that the more direct approach here is the right one? Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch