Some consoles might require special operations on unregistering. For example,
serial console, when registered in the kernel, keeps power on for entire time,
until it gets unregistered. For such cases to have a balance we would provide
->exit() callback.
Signed-off-by: Andy Shevchenko <[email protected]>
---
v3: move callback out from console_lock (Sergey)
include/linux/console.h | 1 +
kernel/printk/printk.c | 4 ++++
2 files changed, 5 insertions(+)
diff --git a/include/linux/console.h b/include/linux/console.h
index f33016b3a401..54759ad0c36b 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -148,6 +148,7 @@ struct console {
struct tty_driver *(*device)(struct console *, int *);
void (*unblank)(void);
int (*setup)(struct console *, char *);
+ void (*exit)(struct console *);
int (*match)(struct console *, char *name, int idx, char *options);
short flags;
short index;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index da6a9bdf76b6..6ca03d199132 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2853,6 +2853,10 @@ int unregister_console(struct console *console)
console->flags &= ~CON_ENABLED;
console_unlock();
console_sysfs_notify();
+
+ if (console->exit)
+ console->exit(console);
+
return res;
}
EXPORT_SYMBOL(unregister_console);
--
2.24.1
On (20/01/27 13:47), Andy Shevchenko wrote:
[..]
> +++ b/include/linux/console.h
> @@ -148,6 +148,7 @@ struct console {
> struct tty_driver *(*device)(struct console *, int *);
> void (*unblank)(void);
> int (*setup)(struct console *, char *);
> + void (*exit)(struct console *);
> int (*match)(struct console *, char *name, int idx, char *options);
> short flags;
> short index;
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index da6a9bdf76b6..6ca03d199132 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2853,6 +2853,10 @@ int unregister_console(struct console *console)
> console->flags &= ~CON_ENABLED;
> console_unlock();
> console_sysfs_notify();
> +
> + if (console->exit)
> + console->exit(console);
> +
If the console was not registered (hence not enabled) is it still required
to call ->exit()? Is there a requirement that ->exit() should handle such
cases?
-ss
On Tue, Jan 28, 2020 at 02:17:11PM +0900, Sergey Senozhatsky wrote:
> On (20/01/27 13:47), Andy Shevchenko wrote:
...
> > @@ -2853,6 +2853,10 @@ int unregister_console(struct console *console)
> > console->flags &= ~CON_ENABLED;
> > console_unlock();
> > console_sysfs_notify();
> > +
> > + if (console->exit)
> > + console->exit(console);
> > +
>
> If the console was not registered (hence not enabled) is it still required
> to call ->exit()? Is there a requirement that ->exit() should handle such
> cases?
This is a good point. The ->exit() purpose is to keep balance for whatever
happened at ->setup().
But ->setup() is being called either when we have has_preferred == false or
when we got no matching we call it for all such consoles, till it returns an
error (can you elaborate the logic behind it?).
In both cases we will get the console to have CON_ENABLED flag set.
--
With Best Regards,
Andy Shevchenko
On (20/01/28 11:44), Andy Shevchenko wrote:
> > If the console was not registered (hence not enabled) is it still required
> > to call ->exit()? Is there a requirement that ->exit() should handle such
> > cases?
>
> This is a good point. The ->exit() purpose is to keep balance for whatever
> happened at ->setup().
>
> But ->setup() is being called either when we have has_preferred == false or
> when we got no matching we call it for all such consoles, till it returns an
> error (can you elaborate the logic behind it?).
->match() does alias matching and ->setup(). If alias matching failed,
exact name match takes place. We don't call ->setup() for all consoles,
but only for those that have exact name match:
if (strcmp(c->name, newcon->name) != 0)
continue;
As to why we don't stop sooner in that loop - I need to to do some
archaeology. We need to have CON_CONSDEV at proper place, which is
IIRC the last matching console.
Pretty much every time we tried to change the logic we ended up
reverting the changes.
> In both cases we will get the console to have CON_ENABLED flag set.
And there are sneaky consoles that have CON_ENABLED before we even
register them.
-ss
On Wed, Jan 29, 2020 at 10:41:41PM +0900, Sergey Senozhatsky wrote:
> On (20/01/28 11:44), Andy Shevchenko wrote:
> > > If the console was not registered (hence not enabled) is it still required
> > > to call ->exit()? Is there a requirement that ->exit() should handle such
> > > cases?
> >
> > This is a good point. The ->exit() purpose is to keep balance for whatever
> > happened at ->setup().
> >
> > But ->setup() is being called either when we have has_preferred == false or
> > when we got no matching we call it for all such consoles, till it returns an
> > error (can you elaborate the logic behind it?).
>
> ->match() does alias matching and ->setup(). If alias matching failed,
> exact name match takes place. We don't call ->setup() for all consoles,
> but only for those that have exact name match:
>
> if (strcmp(c->name, newcon->name) != 0)
> continue;
>
> As to why we don't stop sooner in that loop - I need to to do some
> archaeology. We need to have CON_CONSDEV at proper place, which is
> IIRC the last matching console.
>
> Pretty much every time we tried to change the logic we ended up
> reverting the changes.
I understand. Seems the ->setup() has to be idempotent. We can tell the same
for ->exit() in some comment.
Can you describe, btw, struct console in kernel doc format?
It will be very helpful!
> > In both cases we will get the console to have CON_ENABLED flag set.
>
> And there are sneaky consoles that have CON_ENABLED before we even
> register them.
So, taking into consideration my comment to the previous patch, what would be
suggested guard here?
For a starter something like this?
if ((console->flags & CON_ENABLED) && console->exit)
console->exit(console);
--
With Best Regards,
Andy Shevchenko
On (20/01/29 16:25), Andy Shevchenko wrote:
> I understand. Seems the ->setup() has to be idempotent. We can tell the same
> for ->exit() in some comment.
>
> Can you describe, btw, struct console in kernel doc format?
> It will be very helpful!
We probably need some documentation.
> > > In both cases we will get the console to have CON_ENABLED flag set.
> >
> > And there are sneaky consoles that have CON_ENABLED before we even
> > register them.
>
> So, taking into consideration my comment to the previous patch, what would be
> suggested guard here?
>
> For a starter something like this?
>
> if ((console->flags & CON_ENABLED) && console->exit)
> console->exit(console);
This will work if we also add something like this
---
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a30ff46c0081..01ced6f38776 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2739,6 +2739,8 @@ void register_console(struct console *newcon)
}
}
+ newcon->flags &= ~CON_ENABLED;
+
if (console_drivers && console_drivers->flags & CON_BOOT)
bcon = console_drivers;
---
I don't know why some consoles set CON_ENABLED statically.
E.g. drivers/tty/serial/mux.c
static struct console mux_console = {
.name = "ttyB",
.write = mux_console_write,
.device = uart_console_device,
.setup = mux_console_setup,
.flags = CON_ENABLED | CON_PRINTBUFFER,
.index = 0,
.data = &mux_driver,
};
No idea...
Such consoles still will have CON_ENABLED even if registration failed
and we never ->setup() them.
-ss
On (20/01/30 00:12), Sergey Senozhatsky wrote:
> On (20/01/29 16:25), Andy Shevchenko wrote:
> > I understand. Seems the ->setup() has to be idempotent. We can tell the same
> > for ->exit() in some comment.
> >
> > Can you describe, btw, struct console in kernel doc format?
> > It will be very helpful!
>
> We probably need some documentation.
>
> > > > In both cases we will get the console to have CON_ENABLED flag set.
> > >
> > > And there are sneaky consoles that have CON_ENABLED before we even
> > > register them.
> >
> > So, taking into consideration my comment to the previous patch, what would be
> > suggested guard here?
> >
> > For a starter something like this?
> >
> > if ((console->flags & CON_ENABLED) && console->exit)
> > console->exit(console);
>
> This will work if we also add something like this
No, wait... This will not work, console can be suspended, yet
still registered. I think the only criteria is "the console is
on the list".
-ss
On Mon 2020-01-27 13:47:19, Andy Shevchenko wrote:
> Some consoles might require special operations on unregistering. For example,
> serial console, when registered in the kernel, keeps power on for entire time,
> until it gets unregistered. For such cases to have a balance we would provide
> ->exit() callback.
Is there any plan to use this callback, please?
The console init, setup, registration code needs a clean up,
definitely. If you plan some rework, I would like to understand
the bigger picture before we start adding new callbacks.
Best Regards,
Petr
On Thu, Jan 30, 2020 at 10:09:17AM +0100, Petr Mladek wrote:
> On Mon 2020-01-27 13:47:19, Andy Shevchenko wrote:
> > Some consoles might require special operations on unregistering. For example,
> > serial console, when registered in the kernel, keeps power on for entire time,
> > until it gets unregistered. For such cases to have a balance we would provide
> > ->exit() callback.
>
> Is there any plan to use this callback, please?
>
> The console init, setup, registration code needs a clean up,
> definitely. If you plan some rework, I would like to understand
> the bigger picture before we start adding new callbacks.
Yes, as mentioned in the commit message I would like to use it for balancing
runtime PM reference counters in the UART code later on.
It will look like:
->setup():
pm_runtime_get(...);
->exit():
pm_runtime_put(...);
The current operations have no needs to be undone.
--
With Best Regards,
Andy Shevchenko
On Thu, Jan 30, 2020 at 01:50:53AM +0900, Sergey Senozhatsky wrote:
> On (20/01/30 00:12), Sergey Senozhatsky wrote:
> > On (20/01/29 16:25), Andy Shevchenko wrote:
> > > I understand. Seems the ->setup() has to be idempotent. We can tell the same
> > > for ->exit() in some comment.
> > >
> > > Can you describe, btw, struct console in kernel doc format?
> > > It will be very helpful!
> >
> > We probably need some documentation.
> >
> > > > > In both cases we will get the console to have CON_ENABLED flag set.
> > > >
> > > > And there are sneaky consoles that have CON_ENABLED before we even
> > > > register them.
> > >
> > > So, taking into consideration my comment to the previous patch, what would be
> > > suggested guard here?
> > >
> > > For a starter something like this?
> > >
> > > if ((console->flags & CON_ENABLED) && console->exit)
> > > console->exit(console);
> >
> > This will work if we also add something like this
>
> No, wait... This will not work, console can be suspended, yet
> still registered. I think the only criteria is "the console is
> on the list".
Okay, I guess we need to drop CON_ENABLE bits from this patch and
rely on the res value instead.
--
With Best Regards,
Andy Shevchenko
On Wed 2020-01-29 16:25:58, Andy Shevchenko wrote:
> On Wed, Jan 29, 2020 at 10:41:41PM +0900, Sergey Senozhatsky wrote:
> > On (20/01/28 11:44), Andy Shevchenko wrote:
> > > > If the console was not registered (hence not enabled) is it still required
> > > > to call ->exit()? Is there a requirement that ->exit() should handle such
> > > > cases?
> > >
> > > This is a good point. The ->exit() purpose is to keep balance for whatever
> > > happened at ->setup().
> > >
> > > But ->setup() is being called either when we have has_preferred == false or
> > > when we got no matching we call it for all such consoles, till it returns an
> > > error (can you elaborate the logic behind it?).
> >
> > ->match() does alias matching and ->setup(). If alias matching failed,
> > exact name match takes place. We don't call ->setup() for all consoles,
> > but only for those that have exact name match:
> >
> > if (strcmp(c->name, newcon->name) != 0)
> > continue;
> >
> > As to why we don't stop sooner in that loop - I need to to do some
> > archaeology. We need to have CON_CONSDEV at proper place, which is
> > IIRC the last matching console.
> >
> > Pretty much every time we tried to change the logic we ended up
> > reverting the changes.
>
> I understand. Seems the ->setup() has to be idempotent. We can tell the same
> for ->exit() in some comment.
I believe that ->setup() can succeesfully be called only once.
It is tricky like hell:
1st piece:
if (!has_preferred || bcon || !console_drivers)
has_preferred = preferred_console >= 0;
note:
+ "has_preferred" is updated here only when it was not "true" before.
+ "has_preferred" is set to "true" here only when "preferred_console"
is set in __add_preferred_console()
2nd piece:
+ __add_preferred_console() is called for console defined on
the command line. "preferred_console" points to the console
defined by the last "console=" parameter.
3rd piece:
+ "has_preferred" is set to "true" later in register_console() when
a console with tty binding gets enabled.
4th piece:
+ The code:
/*
* See if we want to use this console driver. If we
* didn't select a console we take the first one
* that registers here.
*/
if (!has_preferred)
... try to enable the given console
The comment is a bit unclear. The code is used as a fallback
when no console was defined on the command line.
Note that "has_preferred" is always true when "preferred_console"
was defined via command line, see 2nd piece above.
By other words:
+ The fallback code (4th piece) is called only when
"preferred_console" was not defined on the command line.
+ The cycle below matches the given console only when
it was defined on the command line.
As a result, I believe that ->setup() could never be called
in both paths for the same console. Especially I think that
fallback code should not be used when the console was defined on
the command line.
I am not 100% sure but I am ready to risk this. Anyway, I think
that many ->setup() callbacks are not ready to be successfully
called twice.
(Sigh, I have started to clean up this code two years ago.
But I have never found time to finish the patchset. It is
such a huge mess.)
> Can you describe, btw, struct console in kernel doc format?
> It will be very helpful!
>
> > > In both cases we will get the console to have CON_ENABLED flag set.
> >
> > And there are sneaky consoles that have CON_ENABLED before we even
> > register them.
>
> So, taking into consideration my comment to the previous patch, what would be
> suggested guard here?
>
> For a starter something like this?
>
> if ((console->flags & CON_ENABLED) && console->exit)
> console->exit(console);
I would do:
if (!res && console->exit)
console->exit(console);
I mean. I would call ->exit() only when console->setup() succeeded in
register_console(). In this case, the console was later added to
the console_drivers list.
Best Regards,
Petr
On Thu, Jan 30, 2020 at 02:22:46PM +0100, Petr Mladek wrote:
> On Wed 2020-01-29 16:25:58, Andy Shevchenko wrote:
> > On Wed, Jan 29, 2020 at 10:41:41PM +0900, Sergey Senozhatsky wrote:
> > > On (20/01/28 11:44), Andy Shevchenko wrote:
...
> > > > > If the console was not registered (hence not enabled) is it still required
> > > > > to call ->exit()? Is there a requirement that ->exit() should handle such
> > > > > cases?
> > > >
> > > > This is a good point. The ->exit() purpose is to keep balance for whatever
> > > > happened at ->setup().
> > > >
> > > > But ->setup() is being called either when we have has_preferred == false or
> > > > when we got no matching we call it for all such consoles, till it returns an
> > > > error (can you elaborate the logic behind it?).
> > >
> > > ->match() does alias matching and ->setup(). If alias matching failed,
> > > exact name match takes place. We don't call ->setup() for all consoles,
> > > but only for those that have exact name match:
> > >
> > > if (strcmp(c->name, newcon->name) != 0)
> > > continue;
> > >
> > > As to why we don't stop sooner in that loop - I need to to do some
> > > archaeology. We need to have CON_CONSDEV at proper place, which is
> > > IIRC the last matching console.
> > >
> > > Pretty much every time we tried to change the logic we ended up
> > > reverting the changes.
> >
> > I understand. Seems the ->setup() has to be idempotent. We can tell the same
> > for ->exit() in some comment.
>
> I believe that ->setup() can succeesfully be called only once.
> It is tricky like hell:
Indeed. I think this code is highly starving for comments.
> 1st piece:
>
> if (!has_preferred || bcon || !console_drivers)
> has_preferred = preferred_console >= 0;
>
> note:
>
> + "has_preferred" is updated here only when it was not "true" before.
> + "has_preferred" is set to "true" here only when "preferred_console"
> is set in __add_preferred_console()
>
> 2nd piece:
>
> + __add_preferred_console() is called for console defined on
> the command line. "preferred_console" points to the console
> defined by the last "console=" parameter.
>
> 3rd piece:
>
> + "has_preferred" is set to "true" later in register_console() when
> a console with tty binding gets enabled.
>
> 4th piece:
>
> + The code:
>
> /*
> * See if we want to use this console driver. If we
> * didn't select a console we take the first one
> * that registers here.
> */
> if (!has_preferred)
> ... try to enable the given console
>
> The comment is a bit unclear. The code is used as a fallback
> when no console was defined on the command line.
>
> Note that "has_preferred" is always true when "preferred_console"
> was defined via command line, see 2nd piece above.
>
>
> By other words:
>
> + The fallback code (4th piece) is called only when
> "preferred_console" was not defined on the command line.
>
> + The cycle below matches the given console only when
> it was defined on the command line.
>
>
> As a result, I believe that ->setup() could never be called
> in both paths for the same console. Especially I think that
> fallback code should not be used when the console was defined on
> the command line.
>
> I am not 100% sure but I am ready to risk this. Anyway, I think
> that many ->setup() callbacks are not ready to be successfully
> called twice.
>
> (Sigh, I have started to clean up this code two years ago.
> But I have never found time to finish the patchset. It is
> such a huge mess.)
Thanks for the elaboration in such details!
> > Can you describe, btw, struct console in kernel doc format?
> > It will be very helpful!
> >
> > > > In both cases we will get the console to have CON_ENABLED flag set.
> > >
> > > And there are sneaky consoles that have CON_ENABLED before we even
> > > register them.
> >
> > So, taking into consideration my comment to the previous patch, what would be
> > suggested guard here?
> >
> > For a starter something like this?
> >
> > if ((console->flags & CON_ENABLED) && console->exit)
> > console->exit(console);
>
> I would do:
>
> if (!res && console->exit)
> console->exit(console);
>
> I mean. I would call ->exit() only when console->setup() succeeded in
> register_console(). In this case, the console was later added to
> the console_drivers list.
Yes, that is exactly what I meant in previous mails to you.
--
With Best Regards,
Andy Shevchenko