Re: [Intel-gfx] [PATCH 2/5] vt: Fix up unregistration of vt drivers
On Fri, Jun 06, 2014 at 08:51:31AM -0700, Greg Kroah-Hartman wrote:
> On Fri, Jun 06, 2014 at 11:40:50AM +0200, Daniel Vetter wrote:
> > On Fri, Jun 06, 2014 at 10:47:25AM +0200, David Herrmann wrote:
> > > Hi
> > >
> > > On Fri, Jun 6, 2014 at 9:56 AM, Daniel Vetter wrote:
> > > > 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
> > > >> 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
> > > >> > Cc: Jiri Slaby
> > > >> > Signed-off-by: Daniel Vetter
> > > >> > ---
> > > >> > 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.
> > >
> > > I missed that line:
> > > const struct consw *defconsw = conswitchp;
> > >
> > > Now that I looked at it again, this looks _really_ wrong. If
> > > conswitchp is vgacon and startup fails, we keep it set to vgacon..
> > > ugh, weird. But ok, that's up to the people who wrote that.
> > >
> > > Btw., conswitchp is used for visual_init(), that is, every new VT
> > > allocation gets assigned to conswitchp. Therefore, it must be a valid
> > > driver. (VTs are allocated and deallocated by agetty/systemd after a
> > > session is started/ended).
> >
> > Ah, I missed where conswitchp was used in the maze, was getting a bit hazy
> > ;-)
> >
> > Rules for changing/updateing conswitchp are indeed a bit strange, which is
> > why I really don't want to touch this too much.
> >
> > > >> 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
Re: [Intel-gfx] [PATCH 2/5] vt: Fix up unregistration of vt drivers
On Fri, Jun 06, 2014 at 11:40:50AM +0200, Daniel Vetter wrote:
> On Fri, Jun 06, 2014 at 10:47:25AM +0200, David Herrmann wrote:
> > Hi
> >
> > On Fri, Jun 6, 2014 at 9:56 AM, Daniel Vetter wrote:
> > > 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
> > >> 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
> > >> > Cc: Jiri Slaby
> > >> > Signed-off-by: Daniel Vetter
> > >> > ---
> > >> > 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.
> >
> > I missed that line:
> > const struct consw *defconsw = conswitchp;
> >
> > Now that I looked at it again, this looks _really_ wrong. If
> > conswitchp is vgacon and startup fails, we keep it set to vgacon..
> > ugh, weird. But ok, that's up to the people who wrote that.
> >
> > Btw., conswitchp is used for visual_init(), that is, every new VT
> > allocation gets assigned to conswitchp. Therefore, it must be a valid
> > driver. (VTs are allocated and deallocated by agetty/systemd after a
> > session is started/ended).
>
> Ah, I missed where conswitchp was used in the maze, was getting a bit hazy
> ;-)
>
> Rules for changing/updateing conswitchp are indeed a bit strange, which is
> why I really don't want to touch this too much.
>
> > >> 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
Re: [Intel-gfx] [PATCH 2/5] vt: Fix up unregistration of vt drivers
On Fri, Jun 06, 2014 at 10:47:25AM +0200, David Herrmann wrote:
> Hi
>
> On Fri, Jun 6, 2014 at 9:56 AM, Daniel Vetter wrote:
> > 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
> >> 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
> >> > Cc: Jiri Slaby
> >> > Signed-off-by: Daniel Vetter
> >> > ---
> >> > 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.
>
> I missed that line:
> const struct consw *defconsw = conswitchp;
>
> Now that I looked at it again, this looks _really_ wrong. If
> conswitchp is vgacon and startup fails, we keep it set to vgacon..
> ugh, weird. But ok, that's up to the people who wrote that.
>
> Btw., conswitchp is used for visual_init(), that is, every new VT
> allocation gets assigned to conswitchp. Therefore, it must be a valid
> driver. (VTs are allocated and deallocated by agetty/systemd after a
> session is started/ended).
Ah, I missed where conswitchp was used in the maze, was getting a bit hazy
;-)
Rules for changing/updateing conswitchp are indeed a bit strange, which is
why I really don't want to touch this too much.
> >> 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?
>
> Yeah, patch looks good.
>
> Reviewed-by: David Herrmann
Thanks for the review.
Greg, are you ok with these 3
Re: [Intel-gfx] [PATCH 2/5] vt: Fix up unregistration of vt drivers
Hi
On Fri, Jun 6, 2014 at 9:56 AM, Daniel Vetter wrote:
> 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 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
>> > Cc: Jiri Slaby
>> > Signed-off-by: Daniel Vetter
>> > ---
>> > 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.
I missed that line:
const struct consw *defconsw = conswitchp;
Now that I looked at it again, this looks _really_ wrong. If
conswitchp is vgacon and startup fails, we keep it set to vgacon..
ugh, weird. But ok, that's up to the people who wrote that.
Btw., conswitchp is used for visual_init(), that is, every new VT
allocation gets assigned to conswitchp. Therefore, it must be a valid
driver. (VTs are allocated and deallocated by agetty/systemd after a
session is started/ended).
>> 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?
Yeah, patch looks good.
Reviewed-by: David Herrmann
Thanks
David
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] vt: Fix up unregistration of vt drivers
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 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
> > Cc: Jiri Slaby
> > Signed-off-by: Daniel Vetter
> > ---
> > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] vt: Fix up unregistration of vt drivers
Hi
On Thu, Jun 5, 2014 at 4:58 PM, Daniel Vetter 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
> Cc: Jiri Slaby
> Signed-off-by: Daniel Vetter
> ---
> 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?
This way, you _know_ here that if !con_is_bound(csw), then csw != conswitchp.
>
> 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.
Otherwise looks good.
Thanks
David
> vtconsole_deinit_device(con_driver);
> device_destroy(vtconsole_class,
>MKDEV(0, con_driver->node));
> @@ -3594,12 +3597,11 @@ int do_unregister_con_driver(const struct consw *csw)
> con_driver->flag = 0;
> con_driver->first = 0;
> con_driver->last = 0;
> - retval = 0;
> - break;
> + return 0;
> }
> }
> -err:
> - return retval;
> +
> + return -ENODEV;
> }
> EXPORT_SYMBOL_GPL(do_unregister_con_driver);
>
> --
> 1.8.1.4
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
