From: Daniel Vetter <daniel@ffwll.ch> To: David Herrmann <dh.herrmann@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>, Jiri Slaby <jslaby@suse.cz>, LKML <linux-kernel@vger.kernel.org>, DRI Development <dri-devel@lists.freedesktop.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Subject: Re: [PATCH 2/5] vt: Fix up unregistration of vt drivers Date: Fri, 6 Jun 2014 09:56:55 +0200 [thread overview] Message-ID: <20140606075655.GC7416@phenom.ffwll.local> (raw) In-Reply-To: <CANq1E4QpST-ZQbhm8FS-xuyda0dnYAFjasqFCspTPKdzJ6zW6w@mail.gmail.com> 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 = ®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
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: David Herrmann <dh.herrmann@gmail.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>, LKML <linux-kernel@vger.kernel.org>, DRI Development <dri-devel@lists.freedesktop.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jslaby@suse.cz> Subject: Re: [PATCH 2/5] vt: Fix up unregistration of vt drivers Date: Fri, 6 Jun 2014 09:56:55 +0200 [thread overview] Message-ID: <20140606075655.GC7416@phenom.ffwll.local> (raw) In-Reply-To: <CANq1E4QpST-ZQbhm8FS-xuyda0dnYAFjasqFCspTPKdzJ6zW6w@mail.gmail.com> 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 = ®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
next prev parent reply other threads:[~2014-06-06 7:57 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-06-05 14:58 [PATCH 1/5] vt: Fix replacement console check when unbinding Daniel Vetter 2014-06-05 14:58 ` Daniel Vetter 2014-06-05 14:58 ` [PATCH 2/5] vt: Fix up unregistration of vt drivers Daniel Vetter 2014-06-05 14:58 ` Daniel Vetter 2014-06-06 7:24 ` David Herrmann 2014-06-06 7:24 ` David Herrmann 2014-06-06 7:56 ` Daniel Vetter [this message] 2014-06-06 7:56 ` Daniel Vetter 2014-06-06 8:47 ` David Herrmann 2014-06-06 9:40 ` Daniel Vetter 2014-06-06 9:40 ` Daniel Vetter 2014-06-06 15:51 ` Greg Kroah-Hartman 2014-06-06 20:21 ` [Intel-gfx] " Daniel Vetter 2014-06-06 20:21 ` Daniel Vetter 2014-06-05 14:58 ` [PATCH 3/5] vt: Don't ignore unbind errors in vt_unbind Daniel Vetter 2014-06-05 14:58 ` Daniel Vetter 2014-06-06 7:13 ` David Herrmann 2014-06-06 7:13 ` David Herrmann 2014-06-05 14:58 ` [PATCH 4/5] drm/i915: Fixup global gtt cleanup Daniel Vetter 2014-06-05 14:58 ` Daniel Vetter 2014-06-06 13:22 ` Imre Deak 2014-06-06 13:22 ` Imre Deak 2014-06-05 14:58 ` [PATCH 5/5] drm/i915: Kick out vga console Daniel Vetter 2014-06-05 14:58 ` Daniel Vetter 2014-06-05 14:58 ` Daniel Vetter 2014-06-06 7:28 ` David Herrmann 2014-06-06 7:28 ` David Herrmann 2014-06-06 7:28 ` David Herrmann 2014-06-06 7:47 ` Daniel Vetter 2014-06-06 7:47 ` Daniel Vetter 2014-06-06 7:47 ` Daniel Vetter 2014-06-06 15:20 ` Daniel Vetter 2014-06-06 15:20 ` Daniel Vetter 2014-06-06 15:20 ` Daniel Vetter 2014-06-09 13:22 ` Tomi Valkeinen 2014-06-09 13:22 ` Tomi Valkeinen 2014-06-09 13:22 ` Tomi Valkeinen 2014-06-28 19:28 ` Ed Tomlinson 2014-06-29 3:55 ` Ed Tomlinson 2014-06-29 3:55 ` Ed Tomlinson 2014-06-30 6:59 ` Chris Wilson 2014-06-30 6:59 ` Chris Wilson 2014-06-30 6:59 ` Chris Wilson 2014-06-30 8:19 ` David Herrmann 2014-06-30 8:19 ` David Herrmann 2014-07-01 13:51 ` Ed Tomlinson 2014-07-01 13:51 ` Ed Tomlinson 2014-07-01 13:51 ` Ed Tomlinson 2014-07-07 8:48 ` [Intel-gfx] " Daniel Vetter 2014-07-07 8:48 ` Daniel Vetter 2014-07-07 8:48 ` [Intel-gfx] " Daniel Vetter 2014-07-07 10:45 ` Ed Tomlinson 2014-07-07 10:45 ` Ed Tomlinson 2014-07-07 10:45 ` [Intel-gfx] " Ed Tomlinson 2014-07-07 12:26 ` Daniel Vetter 2014-07-07 12:26 ` Daniel Vetter 2014-07-07 12:26 ` Daniel Vetter 2014-07-08 2:53 ` Ed Tomlinson 2014-07-08 2:53 ` Ed Tomlinson 2014-07-08 2:53 ` [Intel-gfx] " Ed Tomlinson 2014-07-08 8:10 ` Daniel Vetter 2014-07-08 8:10 ` Daniel Vetter 2014-07-08 8:10 ` [Intel-gfx] " Daniel Vetter 2014-07-07 10:59 ` Ed Tomlinson 2014-07-07 10:59 ` Ed Tomlinson 2014-07-07 10:59 ` [Intel-gfx] " Ed Tomlinson 2014-06-06 7:16 ` [PATCH 1/5] vt: Fix replacement console check when unbinding David Herrmann 2014-06-06 7:16 ` David Herrmann 2014-06-06 7:49 ` Daniel Vetter 2014-06-06 7:49 ` Daniel Vetter 2014-06-06 9:43 ` [PATCH] " Daniel Vetter 2014-06-06 9:43 ` Daniel Vetter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140606075655.GC7416@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=daniel.vetter@ffwll.ch \ --cc=dh.herrmann@gmail.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=gregkh@linuxfoundation.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=jslaby@suse.cz \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.