Re: [PATCH 2/5] vt: Fix up unregistration of vt drivers

[Daniel Vetter <daniel@ffwll.ch>]

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@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@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.cz>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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 = &registered_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