* [PATCH v3 2/5] console: Drop double check for console_drivers being non-NULL
2020-01-27 11:47 [PATCH v3 1/5] console: Don't perform test for CON_BRL flag Andy Shevchenko
@ 2020-01-27 11:47 ` Andy Shevchenko
2020-01-29 13:24 ` Petr Mladek
2020-01-27 11:47 ` [PATCH v3 3/5] console: Use for_each_console() helper in unregister_console() Andy Shevchenko
` (3 subsequent siblings)
4 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-27 11:47 UTC (permalink / raw)
To: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
Cc: Andy Shevchenko
There is no need to explicitly check for console_drivers to be non-NULL
since for_each_console() does this.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: no changes
kernel/printk/printk.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fada22dc4ab6..51337ed426e0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1772,9 +1772,6 @@ static void call_console_drivers(const char *ext_text, size_t ext_len,
trace_console_rcuidle(text, len);
- if (!console_drivers)
- return;
-
for_each_console(con) {
if (exclusive_console && con != exclusive_console)
continue;
@@ -2653,18 +2650,17 @@ void register_console(struct console *newcon)
struct console_cmdline *c;
static bool has_preferred;
- if (console_drivers)
- for_each_console(bcon)
- if (WARN(bcon == newcon,
- "console '%s%d' already registered\n",
- bcon->name, bcon->index))
- return;
+ for_each_console(bcon) {
+ if (WARN(bcon == newcon, "console '%s%d' already registered\n",
+ bcon->name, bcon->index))
+ return;
+ }
/*
* before we register a new CON_BOOT console, make sure we don't
* already have a valid console
*/
- if (console_drivers && newcon->flags & CON_BOOT) {
+ if (newcon->flags & CON_BOOT) {
/* find the last or real console */
for_each_console(bcon) {
if (!(bcon->flags & CON_BOOT)) {
--
2.24.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 2/5] console: Drop double check for console_drivers being non-NULL
2020-01-27 11:47 ` [PATCH v3 2/5] console: Drop double check for console_drivers being non-NULL Andy Shevchenko
@ 2020-01-29 13:24 ` Petr Mladek
0 siblings, 0 replies; 28+ messages in thread
From: Petr Mladek @ 2020-01-29 13:24 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
On Mon 2020-01-27 13:47:16, Andy Shevchenko wrote:
> There is no need to explicitly check for console_drivers to be non-NULL
> since for_each_console() does this.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Nice catch.
Reviewed-by: Petr Mladek <pmladek@suse.com>
There is a candidate for another patch, see the note below.
> ---
> v3: no changes
> kernel/printk/printk.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index fada22dc4ab6..51337ed426e0 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2653,18 +2650,17 @@ void register_console(struct console *newcon)
> struct console_cmdline *c;
> static bool has_preferred;
>
> - if (console_drivers)
> - for_each_console(bcon)
> - if (WARN(bcon == newcon,
> - "console '%s%d' already registered\n",
> - bcon->name, bcon->index))
> - return;
> + for_each_console(bcon) {
> + if (WARN(bcon == newcon, "console '%s%d' already registered\n",
> + bcon->name, bcon->index))
> + return;
> + }
>
> /*
> * before we register a new CON_BOOT console, make sure we don't
> * already have a valid console
> */
> - if (console_drivers && newcon->flags & CON_BOOT) {
> + if (newcon->flags & CON_BOOT) {
> /* find the last or real console */
Note: This comment is misleading. I would just remove it.
> for_each_console(bcon) {
> if (!(bcon->flags & CON_BOOT)) {
Best Regards,
Petr
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 3/5] console: Use for_each_console() helper in unregister_console()
2020-01-27 11:47 [PATCH v3 1/5] console: Don't perform test for CON_BRL flag Andy Shevchenko
2020-01-27 11:47 ` [PATCH v3 2/5] console: Drop double check for console_drivers being non-NULL Andy Shevchenko
@ 2020-01-27 11:47 ` Andy Shevchenko
2020-01-29 14:11 ` Petr Mladek
2020-01-27 11:47 ` [PATCH v3 4/5] console: Avoid positive return code from unregister_console() Andy Shevchenko
` (2 subsequent siblings)
4 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-27 11:47 UTC (permalink / raw)
To: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
Cc: Andy Shevchenko
We have rather open coded single linked list manipulations where we may
simple use for_each_console() helper with properly set exit conditions.
Replace open coded single-linked list handling with for_each_console()
helper in use.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: no changes
kernel/printk/printk.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 51337ed426e0..d40a316908da 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2809,7 +2809,7 @@ EXPORT_SYMBOL(register_console);
int unregister_console(struct console *console)
{
- struct console *a, *b;
+ struct console *con;
int res;
pr_info("%sconsole [%s%d] disabled\n",
@@ -2825,11 +2825,10 @@ int unregister_console(struct console *console)
if (console_drivers == console) {
console_drivers=console->next;
res = 0;
- } else if (console_drivers) {
- for (a=console_drivers->next, b=console_drivers ;
- a; b=a, a=b->next) {
- if (a == console) {
- b->next = a->next;
+ } else {
+ for_each_console(con) {
+ if (con->next == console) {
+ con->next = console->next;
res = 0;
break;
}
--
2.24.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-27 11:47 [PATCH v3 1/5] console: Don't perform test for CON_BRL flag Andy Shevchenko
2020-01-27 11:47 ` [PATCH v3 2/5] console: Drop double check for console_drivers being non-NULL Andy Shevchenko
2020-01-27 11:47 ` [PATCH v3 3/5] console: Use for_each_console() helper in unregister_console() Andy Shevchenko
@ 2020-01-27 11:47 ` Andy Shevchenko
2020-01-28 4:43 ` Sergey Senozhatsky
2020-01-30 9:04 ` Petr Mladek
2020-01-27 11:47 ` [PATCH v3 5/5] console: Introduce ->exit() callback Andy Shevchenko
2020-01-29 12:29 ` [PATCH v3 1/5] console: Don't perform test for CON_BRL flag Petr Mladek
4 siblings, 2 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-27 11:47 UTC (permalink / raw)
To: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
Cc: Andy Shevchenko
There are two callers which use the returned code from unregister_console().
In some cases, i.e. successfully unregistered Braille console or when console
has not been enabled the return code is 1. This code is ambiguous and also
prevents callers to distinguish successful operation.
Replace this logic to return only negative error codes or 0 when console,
either enabled, disabled or Braille has been successfully unregistered.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v3: no changes
kernel/printk/printk.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d40a316908da..da6a9bdf76b6 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2817,10 +2817,12 @@ int unregister_console(struct console *console)
console->name, console->index);
res = _braille_unregister_console(console);
- if (res)
+ if (res < 0)
return res;
+ if (res > 0)
+ return 0;
- res = 1;
+ res = -ENODEV;
console_lock();
if (console_drivers == console) {
console_drivers=console->next;
@@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
if (!res && (console->flags & CON_EXTENDED))
nr_ext_console_drivers--;
+ if (res && !(console->flags & CON_ENABLED))
+ res = 0;
+
/*
* If this isn't the last console and it has CON_CONSDEV set, we
* need to set it on the next preferred console.
--
2.24.1
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-27 11:47 ` [PATCH v3 4/5] console: Avoid positive return code from unregister_console() Andy Shevchenko
@ 2020-01-28 4:43 ` Sergey Senozhatsky
2020-01-28 9:22 ` Andy Shevchenko
2020-01-30 9:04 ` Petr Mladek
1 sibling, 1 reply; 28+ messages in thread
From: Sergey Senozhatsky @ 2020-01-28 4:43 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
On (20/01/27 13:47), Andy Shevchenko wrote:
[..]
> res = _braille_unregister_console(console);
> - if (res)
> + if (res < 0)
> return res;
> + if (res > 0)
> + return 0;
>
> - res = 1;
> + res = -ENODEV;
> console_lock();
> if (console_drivers == console) {
> console_drivers=console->next;
> @@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
> if (!res && (console->flags & CON_EXTENDED))
> nr_ext_console_drivers--;
>
> + if (res && !(console->flags & CON_ENABLED))
> + res = 0;
Console is not on the console_drivers list. Why does !ENABLED case
require extra handling? What about the case when console is ENABLED
but still not registered?
I think that if the console is not on the list (was never registered)
then we can just bail out, without console_sysfs_notify(), etc. IOW,
if (res) {
console->flags &= ~CON_ENABLED; /* just in case */
console_unlock();
return res;
}
-ss
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-28 4:43 ` Sergey Senozhatsky
@ 2020-01-28 9:22 ` Andy Shevchenko
2020-01-28 9:25 ` Sergey Senozhatsky
2020-01-28 9:37 ` Sergey Senozhatsky
0 siblings, 2 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-28 9:22 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
On Tue, Jan 28, 2020 at 01:43:32PM +0900, Sergey Senozhatsky wrote:
> On (20/01/27 13:47), Andy Shevchenko wrote:
> [..]
> > res = _braille_unregister_console(console);
> > - if (res)
> > + if (res < 0)
> > return res;
> > + if (res > 0)
> > + return 0;
> >
> > - res = 1;
> > + res = -ENODEV;
> > console_lock();
> > if (console_drivers == console) {
> > console_drivers=console->next;
> > @@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
> > if (!res && (console->flags & CON_EXTENDED))
> > nr_ext_console_drivers--;
> >
> > + if (res && !(console->flags & CON_ENABLED))
> > + res = 0;
>
> Console is not on the console_drivers list. Why does !ENABLED case
> require extra handling?
It's mirroring (to some extend) the register_console() abort conditions.
> What about the case when console is ENABLED
> but still not registered?
What about when console is ENABLED and we call register_console()?
I think you can tell me what to do in these corner cases (however,
that's not the point of this series).
> I think that if the console is not on the list (was never registered)
> then we can just bail out, without console_sysfs_notify(), etc. IOW,
>
> if (res) {
> console->flags &= ~CON_ENABLED; /* just in case */
> console_unlock();
> return res;
> }
Perhaps. But see above. I would rather drop this condition for now
for sake of this series being to the point.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-28 9:22 ` Andy Shevchenko
@ 2020-01-28 9:25 ` Sergey Senozhatsky
2020-01-28 9:37 ` Sergey Senozhatsky
1 sibling, 0 replies; 28+ messages in thread
From: Sergey Senozhatsky @ 2020-01-28 9:25 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sergey Senozhatsky, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, linux-kernel
On (20/01/28 11:22), Andy Shevchenko wrote:
[..]
> > Console is not on the console_drivers list. Why does !ENABLED case
> > require extra handling?
>
> It's mirroring (to some extend) the register_console() abort conditions.
>
> > What about the case when console is ENABLED
> > but still not registered?
>
> What about when console is ENABLED and we call register_console()?
I think that ENABLED bit makes sense only when console is on the list.
Otherwise, I suspect, nothing will be able to access the console.
-ss
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-28 9:22 ` Andy Shevchenko
2020-01-28 9:25 ` Sergey Senozhatsky
@ 2020-01-28 9:37 ` Sergey Senozhatsky
2020-01-28 9:52 ` Andy Shevchenko
1 sibling, 1 reply; 28+ messages in thread
From: Sergey Senozhatsky @ 2020-01-28 9:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sergey Senozhatsky, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, linux-kernel
On (20/01/28 11:22), Andy Shevchenko wrote:
> On Tue, Jan 28, 2020 at 01:43:32PM +0900, Sergey Senozhatsky wrote:
> > On (20/01/27 13:47), Andy Shevchenko wrote:
> > [..]
> > > res = _braille_unregister_console(console);
> > > - if (res)
> > > + if (res < 0)
> > > return res;
> > > + if (res > 0)
> > > + return 0;
> > >
> > > - res = 1;
> > > + res = -ENODEV;
> > > console_lock();
> > > if (console_drivers == console) {
> > > console_drivers=console->next;
> > > @@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
> > > if (!res && (console->flags & CON_EXTENDED))
> > > nr_ext_console_drivers--;
> > >
> > > + if (res && !(console->flags & CON_ENABLED))
> > > + res = 0;
> >
> > Console is not on the console_drivers list. Why does !ENABLED case
> > require extra handling?
>
> It's mirroring (to some extend) the register_console() abort conditions.
Could you please explain?
I see the "newcon->flags & CON_ENABLED" error out path. I'm guessing,
that the expectation is that this is how we filter out consoles which
were not matched (there is that "newcon->flags |= CON_ENABLED" several
lines earlier.) So this looks like the assumption is that consoles don't
have CON_ENABLED bit set prior to register_console(), as far as I understand.
Well, look at these
...
drivers/net/netconsole.c: .flags = CON_ENABLED,
drivers/tty/ehv_bytechan.c: .flags = CON_PRINTBUFFER | CON_ENABLED,
drivers/tty/serial/mux.c: .flags = CON_ENABLED | CON_PRINTBUFFER,
...
-ss
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-28 9:37 ` Sergey Senozhatsky
@ 2020-01-28 9:52 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-28 9:52 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
On Tue, Jan 28, 2020 at 06:37:26PM +0900, Sergey Senozhatsky wrote:
> On (20/01/28 11:22), Andy Shevchenko wrote:
> > On Tue, Jan 28, 2020 at 01:43:32PM +0900, Sergey Senozhatsky wrote:
> > > On (20/01/27 13:47), Andy Shevchenko wrote:
> > > [..]
> > > > res = _braille_unregister_console(console);
> > > > - if (res)
> > > > + if (res < 0)
> > > > return res;
> > > > + if (res > 0)
> > > > + return 0;
> > > >
> > > > - res = 1;
> > > > + res = -ENODEV;
> > > > console_lock();
> > > > if (console_drivers == console) {
> > > > console_drivers=console->next;
> > > > @@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
> > > > if (!res && (console->flags & CON_EXTENDED))
> > > > nr_ext_console_drivers--;
> > > >
> > > > + if (res && !(console->flags & CON_ENABLED))
> > > > + res = 0;
> > >
> > > Console is not on the console_drivers list. Why does !ENABLED case
> > > require extra handling?
> >
> > It's mirroring (to some extend) the register_console() abort conditions.
>
> Could you please explain?
>
> I see the "newcon->flags & CON_ENABLED" error out path. I'm guessing,
> that the expectation is that this is how we filter out consoles which
> were not matched (there is that "newcon->flags |= CON_ENABLED" several
> lines earlier.) So this looks like the assumption is that consoles don't
> have CON_ENABLED bit set prior to register_console(), as far as I understand.
I put it to cover the case when register_console() fails (since it has no
return code caller is not able to say this anyhow) somebody may call
unregister_console() on it unconditionally (and I guess many do like this).
In such case we shouldn't return an error code.
> Well, look at these
> ...
> drivers/net/netconsole.c: .flags = CON_ENABLED,
> drivers/tty/ehv_bytechan.c: .flags = CON_PRINTBUFFER | CON_ENABLED,
> drivers/tty/serial/mux.c: .flags = CON_ENABLED | CON_PRINTBUFFER,
> ...
The code there (I meant register_console() and unregister_console() and
their usage) is quite twisted and probably abused, so, I have definitely
miss something.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-27 11:47 ` [PATCH v3 4/5] console: Avoid positive return code from unregister_console() Andy Shevchenko
2020-01-28 4:43 ` Sergey Senozhatsky
@ 2020-01-30 9:04 ` Petr Mladek
2020-01-30 9:58 ` Andy Shevchenko
1 sibling, 1 reply; 28+ messages in thread
From: Petr Mladek @ 2020-01-30 9:04 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
On Mon 2020-01-27 13:47:18, Andy Shevchenko wrote:
> There are two callers which use the returned code from unregister_console().
> In some cases, i.e. successfully unregistered Braille console or when console
> has not been enabled the return code is 1. This code is ambiguous and also
> prevents callers to distinguish successful operation.
>
> Replace this logic to return only negative error codes or 0 when console,
> either enabled, disabled or Braille has been successfully unregistered.
I am quite confused by the above message. It is probably because
the patched code is so confusing ;-)
I would start with something like:
<begin>
There are only two callers that use the returned code from
unregister_console():
+ unregister_early_console() in arch/m68k/kernel/early_printk.c
+ kgdb_unregister_nmi_console() in drivers/tty/serial/kgdb_nmi.c
They both expect to get "0" on success and a non-zero value on error.
</end>
The above is more or less clear. Now, the question is what behavior
is considered as success and what is failure.
I started thinking about this in a paranoid mode. The console
registration code is so tricky and it is easy to create
regression.
But I think that it is actually not much important. There are only
two callers that handle the return code:
+ The 1st one m68k is a late init call and the error code of
init calls is ignored.
+ The 2nd one in kdb code is not much important. I wonder if anyone
is actually using kdb. If I remember correctly then Linus
prosed to remove it completely during the discussion about
lockless printk at Plumbers 2019 and nobody was against.
In fact, the kdb code is probably wrong. tty_register_driver()
is called before register_console() in
kgdb_register_nmi_console() =>
kgdb_unregister_nmi_console() should probably call
tty_unregister_driver() even when unregister_console() fails.
unregister_console() is exported symbol but I doubt that the are
more users of the error code.
So, I think that we do not need to care about regressions.
But it is worth to define some resonable behavior, see
below.
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index d40a316908da..da6a9bdf76b6 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2817,10 +2817,12 @@ int unregister_console(struct console *console)
> console->name, console->index);
>
> res = _braille_unregister_console(console);
> - if (res)
> + if (res < 0)
> return res;
> + if (res > 0)
> + return 0;
Sigh, I wish that _braille_unregister_console() did not returned 1
on success but ...
I would describe this as a bugfix. unregister_console() should return
success (0) when _braille_unregister_console() succeeds.
>
> - res = 1;
> + res = -ENODEV;
I would describe this as using a regular "meaningful" error code.
> console_lock();
> if (console_drivers == console) {
> console_drivers=console->next;
> @@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
> if (!res && (console->flags & CON_EXTENDED))
> nr_ext_console_drivers--;
>
> + if (res && !(console->flags & CON_ENABLED))
> + res = 0;
I personally think that success or failure of unregister_console()
should not depend on the state of CON_ENABLED flag:
+ As it was discussed in the other thread. There are few consoles
that have set CON_ENABLED by default. unregister_console()
should not succeed when register_console() was not called
before.
+ This check would open a question if we should return error
when the console was in the list but CON_ENABLED was not set.
But consoles might be temporary disabled, see console_stop().
unregister_console() should succeed even when the console
was temporary stopped.
But I think that this is only theoretical discussion. IMHO, nobody
really depends on the return code in reality. Alternative solution
would be to make it symetric with register_console() and do not
return the error code at all.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-30 9:04 ` Petr Mladek
@ 2020-01-30 9:58 ` Andy Shevchenko
2020-01-30 12:22 ` Petr Mladek
0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-30 9:58 UTC (permalink / raw)
To: Petr Mladek; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
On Thu, Jan 30, 2020 at 10:04:29AM +0100, Petr Mladek wrote:
> On Mon 2020-01-27 13:47:18, Andy Shevchenko wrote:
> > There are two callers which use the returned code from unregister_console().
> > In some cases, i.e. successfully unregistered Braille console or when console
> > has not been enabled the return code is 1. This code is ambiguous and also
> > prevents callers to distinguish successful operation.
> >
> > Replace this logic to return only negative error codes or 0 when console,
> > either enabled, disabled or Braille has been successfully unregistered.
>
> I am quite confused by the above message. It is probably because
> the patched code is so confusing ;-)
True, and thanks for the elaboration. Some comments below, nevertheless.
> I would start with something like:
>
> <begin>
> There are only two callers that use the returned code from
> unregister_console():
>
> + unregister_early_console() in arch/m68k/kernel/early_printk.c
> + kgdb_unregister_nmi_console() in drivers/tty/serial/kgdb_nmi.c
>
> They both expect to get "0" on success and a non-zero value on error.
> </end>
I'll rewrite commit message.
> The above is more or less clear. Now, the question is what behavior
> is considered as success and what is failure.
>
> I started thinking about this in a paranoid mode. The console
> registration code is so tricky and it is easy to create
> regression.
>
> But I think that it is actually not much important. There are only
> two callers that handle the return code:
>
> + The 1st one m68k is a late init call and the error code of
> init calls is ignored.
That's not fully true. If you pass initcall_debug it will be helpful to see
what is failed and what is not.
> + The 2nd one in kdb code is not much important. I wonder if anyone
> is actually using kdb. If I remember correctly then Linus
> prosed to remove it completely during the discussion about
> lockless printk at Plumbers 2019 and nobody was against.
I agree with Linus, but It's not my area of expertise, for the scope of this
series I would rather ignore what it does with returned code and fix it later
if anybody complains (probably we won't see any complaint).
> In fact, the kdb code is probably wrong. tty_register_driver()
> is called before register_console() in
> kgdb_register_nmi_console() =>
>
> kgdb_unregister_nmi_console() should probably call
> tty_unregister_driver() even when unregister_console() fails.
>
> unregister_console() is exported symbol but I doubt that the are
> more users of the error code.
>
> So, I think that we do not need to care about regressions.
> But it is worth to define some resonable behavior, see
> below.
Agree.
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index d40a316908da..da6a9bdf76b6 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2817,10 +2817,12 @@ int unregister_console(struct console *console)
> > console->name, console->index);
> >
> > res = _braille_unregister_console(console);
> > - if (res)
> > + if (res < 0)
> > return res;
> > + if (res > 0)
> > + return 0;
>
> Sigh, I wish that _braille_unregister_console() did not returned 1
> on success but ...
>
> I would describe this as a bugfix. unregister_console() should return
> success (0) when _braille_unregister_console() succeeds.
You mean do a separate patch for it with Fixes tag?
> > - res = 1;
> > + res = -ENODEV;
>
> I would describe this as using a regular "meaningful" error code.
In the commit message? Will do!
> > console_lock();
> > if (console_drivers == console) {
> > console_drivers=console->next;
> > @@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
> > if (!res && (console->flags & CON_EXTENDED))
> > nr_ext_console_drivers--;
> >
> > + if (res && !(console->flags & CON_ENABLED))
> > + res = 0;
>
> I personally think that success or failure of unregister_console()
> should not depend on the state of CON_ENABLED flag:
>
> + As it was discussed in the other thread. There are few consoles
> that have set CON_ENABLED by default. unregister_console()
> should not succeed when register_console() was not called
> before.
>
> + This check would open a question if we should return error
> when the console was in the list but CON_ENABLED was not set.
> But consoles might be temporary disabled, see console_stop().
> unregister_console() should succeed even when the console
> was temporary stopped.
>
> But I think that this is only theoretical discussion. IMHO, nobody
> really depends on the return code in reality. Alternative solution
> would be to make it symetric with register_console() and do not
> return the error code at all.
Okay, I understand that for time being it's matter of how eloquent
the commit message will be. (And maybe some comments in the code?)
Is it correct?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-30 9:58 ` Andy Shevchenko
@ 2020-01-30 12:22 ` Petr Mladek
2020-01-30 13:13 ` Andy Shevchenko
0 siblings, 1 reply; 28+ messages in thread
From: Petr Mladek @ 2020-01-30 12:22 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
On Thu 2020-01-30 11:58:07, Andy Shevchenko wrote:
> On Thu, Jan 30, 2020 at 10:04:29AM +0100, Petr Mladek wrote:
> > On Mon 2020-01-27 13:47:18, Andy Shevchenko wrote:
> > > There are two callers which use the returned code from unregister_console().
> > > In some cases, i.e. successfully unregistered Braille console or when console
> > > has not been enabled the return code is 1. This code is ambiguous and also
> > > prevents callers to distinguish successful operation.
> > >
> > > Replace this logic to return only negative error codes or 0 when console,
> > > either enabled, disabled or Braille has been successfully unregistered.
> >
> > I am quite confused by the above message. It is probably because
> > the patched code is so confusing ;-)
>
> True, and thanks for the elaboration. Some comments below, nevertheless.
>
> > I would start with something like:
> >
> > <begin>
> > There are only two callers that use the returned code from
> > unregister_console():
> >
> > + unregister_early_console() in arch/m68k/kernel/early_printk.c
> > + kgdb_unregister_nmi_console() in drivers/tty/serial/kgdb_nmi.c
> >
> > They both expect to get "0" on success and a non-zero value on error.
> > </end>
>
> I'll rewrite commit message.
>
> > The above is more or less clear. Now, the question is what behavior
> > is considered as success and what is failure.
> >
> > I started thinking about this in a paranoid mode. The console
> > registration code is so tricky and it is easy to create
> > regression.
> >
> > But I think that it is actually not much important. There are only
> > two callers that handle the return code:
> >
> > + The 1st one m68k is a late init call and the error code of
> > init calls is ignored.
>
> That's not fully true. If you pass initcall_debug it will be helpful to see
> what is failed and what is not.
>
> > + The 2nd one in kdb code is not much important. I wonder if anyone
> > is actually using kdb. If I remember correctly then Linus
> > prosed to remove it completely during the discussion about
> > lockless printk at Plumbers 2019 and nobody was against.
>
> I agree with Linus, but It's not my area of expertise, for the scope of this
> series I would rather ignore what it does with returned code and fix it later
> if anybody complains (probably we won't see any complaint).
>
> > In fact, the kdb code is probably wrong. tty_register_driver()
> > is called before register_console() in
> > kgdb_register_nmi_console() =>
> >
> > kgdb_unregister_nmi_console() should probably call
> > tty_unregister_driver() even when unregister_console() fails.
> >
> > unregister_console() is exported symbol but I doubt that the are
> > more users of the error code.
> >
> > So, I think that we do not need to care about regressions.
> > But it is worth to define some resonable behavior, see
> > below.
>
> Agree.
>
> > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > > index d40a316908da..da6a9bdf76b6 100644
> > > --- a/kernel/printk/printk.c
> > > +++ b/kernel/printk/printk.c
> > > @@ -2817,10 +2817,12 @@ int unregister_console(struct console *console)
> > > console->name, console->index);
> > >
> > > res = _braille_unregister_console(console);
> > > - if (res)
> > > + if (res < 0)
> > > return res;
> > > + if (res > 0)
> > > + return 0;
> >
> > Sigh, I wish that _braille_unregister_console() did not returned 1
> > on success but ...
> >
> > I would describe this as a bugfix. unregister_console() should return
> > success (0) when _braille_unregister_console() succeeds.
>
> You mean do a separate patch for it with Fixes tag?
>
> > > - res = 1;
> > > + res = -ENODEV;
> >
> > I would describe this as using a regular "meaningful" error code.
>
> In the commit message? Will do!
>
> > > console_lock();
> > > if (console_drivers == console) {
> > > console_drivers=console->next;
> > > @@ -2838,6 +2840,9 @@ int unregister_console(struct console *console)
> > > if (!res && (console->flags & CON_EXTENDED))
> > > nr_ext_console_drivers--;
> > >
> > > + if (res && !(console->flags & CON_ENABLED))
> > > + res = 0;
> >
> > I personally think that success or failure of unregister_console()
> > should not depend on the state of CON_ENABLED flag:
> >
> > + As it was discussed in the other thread. There are few consoles
> > that have set CON_ENABLED by default. unregister_console()
> > should not succeed when register_console() was not called
> > before.
> >
> > + This check would open a question if we should return error
> > when the console was in the list but CON_ENABLED was not set.
> > But consoles might be temporary disabled, see console_stop().
> > unregister_console() should succeed even when the console
> > was temporary stopped.
> >
> > But I think that this is only theoretical discussion. IMHO, nobody
> > really depends on the return code in reality. Alternative solution
> > would be to make it symetric with register_console() and do not
> > return the error code at all.
>
> Okay, I understand that for time being it's matter of how eloquent
> the commit message will be. (And maybe some comments in the code?)
> Is it correct?
Good question.
Please, remove the last hunk if Sergey is not against it.
I think that the success/error should not depend on the state
of CON_ENABLED flag.
The other two changes might stay in the same patch. We just need
to make the commit message easier to understand. I would write
something like:
<begin>
There are only two callers that use the returned code from
unregister_console():
+ unregister_early_console() in arch/m68k/kernel/early_printk.c
+ kgdb_unregister_nmi_console() in drivers/tty/serial/kgdb_nmi.c
They both expect to get "0" on success and a non-zero value on error.
But the current behavior is confusing and buggy:
+ _braille_unregister_console() returns "1" on success
+ unregister_console() returns "1" on error
Fix and clean up the behavior:
+ Return success when _braille_unregister_console() succeeded.
+ Return a meaningful error code when the console was not
registered before.
</end>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 4/5] console: Avoid positive return code from unregister_console()
2020-01-30 12:22 ` Petr Mladek
@ 2020-01-30 13:13 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-30 13:13 UTC (permalink / raw)
To: Petr Mladek; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
On Thu, Jan 30, 2020 at 01:22:26PM +0100, Petr Mladek wrote:
> On Thu 2020-01-30 11:58:07, Andy Shevchenko wrote:
> > On Thu, Jan 30, 2020 at 10:04:29AM +0100, Petr Mladek wrote:
...
> > Okay, I understand that for time being it's matter of how eloquent
> > the commit message will be. (And maybe some comments in the code?)
> > Is it correct?
>
> Good question.
>
> Please, remove the last hunk if Sergey is not against it.
> I think that the success/error should not depend on the state
> of CON_ENABLED flag.
If I understood his last message correctly, he is exactly in favour of not
using it (and thus changing conditional for ->exit() callback to rely only
on res value).
> The other two changes might stay in the same patch. We just need
> to make the commit message easier to understand. I would write
> something like:
Thanks! Will do this way.
> <begin>
> There are only two callers that use the returned code from
> unregister_console():
>
> + unregister_early_console() in arch/m68k/kernel/early_printk.c
> + kgdb_unregister_nmi_console() in drivers/tty/serial/kgdb_nmi.c
>
> They both expect to get "0" on success and a non-zero value on error.
> But the current behavior is confusing and buggy:
>
> + _braille_unregister_console() returns "1" on success
> + unregister_console() returns "1" on error
>
> Fix and clean up the behavior:
>
> + Return success when _braille_unregister_console() succeeded.
> + Return a meaningful error code when the console was not
> registered before.
> </end>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-27 11:47 [PATCH v3 1/5] console: Don't perform test for CON_BRL flag Andy Shevchenko
` (2 preceding siblings ...)
2020-01-27 11:47 ` [PATCH v3 4/5] console: Avoid positive return code from unregister_console() Andy Shevchenko
@ 2020-01-27 11:47 ` Andy Shevchenko
2020-01-28 5:17 ` Sergey Senozhatsky
2020-01-30 9:09 ` Petr Mladek
2020-01-29 12:29 ` [PATCH v3 1/5] console: Don't perform test for CON_BRL flag Petr Mladek
4 siblings, 2 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-27 11:47 UTC (permalink / raw)
To: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
Cc: Andy Shevchenko
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 <andriy.shevchenko@linux.intel.com>
---
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-27 11:47 ` [PATCH v3 5/5] console: Introduce ->exit() callback Andy Shevchenko
@ 2020-01-28 5:17 ` Sergey Senozhatsky
2020-01-28 9:44 ` Andy Shevchenko
2020-01-30 9:09 ` Petr Mladek
1 sibling, 1 reply; 28+ messages in thread
From: Sergey Senozhatsky @ 2020-01-28 5:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-28 5:17 ` Sergey Senozhatsky
@ 2020-01-28 9:44 ` Andy Shevchenko
2020-01-29 13:41 ` Sergey Senozhatsky
0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-28 9:44 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-28 9:44 ` Andy Shevchenko
@ 2020-01-29 13:41 ` Sergey Senozhatsky
2020-01-29 14:25 ` Andy Shevchenko
0 siblings, 1 reply; 28+ messages in thread
From: Sergey Senozhatsky @ 2020-01-29 13:41 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sergey Senozhatsky, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-29 13:41 ` Sergey Senozhatsky
@ 2020-01-29 14:25 ` Andy Shevchenko
2020-01-29 15:12 ` Sergey Senozhatsky
2020-01-30 13:22 ` Petr Mladek
0 siblings, 2 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-29 14:25 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Sergey Senozhatsky, Petr Mladek, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-29 14:25 ` Andy Shevchenko
@ 2020-01-29 15:12 ` Sergey Senozhatsky
2020-01-29 16:50 ` Sergey Senozhatsky
2020-01-30 13:22 ` Petr Mladek
1 sibling, 1 reply; 28+ messages in thread
From: Sergey Senozhatsky @ 2020-01-29 15:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Petr Mladek,
Steven Rostedt, linux-kernel
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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-29 15:12 ` Sergey Senozhatsky
@ 2020-01-29 16:50 ` Sergey Senozhatsky
2020-01-30 13:14 ` Andy Shevchenko
0 siblings, 1 reply; 28+ messages in thread
From: Sergey Senozhatsky @ 2020-01-29 16:50 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sergey Senozhatsky, Petr Mladek, Steven Rostedt, linux-kernel,
Sergey Senozhatsky
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-29 16:50 ` Sergey Senozhatsky
@ 2020-01-30 13:14 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-30 13:14 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Sergey Senozhatsky, Petr Mladek, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-29 14:25 ` Andy Shevchenko
2020-01-29 15:12 ` Sergey Senozhatsky
@ 2020-01-30 13:22 ` Petr Mladek
2020-01-30 13:39 ` Andy Shevchenko
1 sibling, 1 reply; 28+ messages in thread
From: Petr Mladek @ 2020-01-30 13:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-30 13:22 ` Petr Mladek
@ 2020-01-30 13:39 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-30 13:39 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-27 11:47 ` [PATCH v3 5/5] console: Introduce ->exit() callback Andy Shevchenko
2020-01-28 5:17 ` Sergey Senozhatsky
@ 2020-01-30 9:09 ` Petr Mladek
2020-01-30 10:01 ` Andy Shevchenko
1 sibling, 1 reply; 28+ messages in thread
From: Petr Mladek @ 2020-01-30 9:09 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 5/5] console: Introduce ->exit() callback
2020-01-30 9:09 ` Petr Mladek
@ 2020-01-30 10:01 ` Andy Shevchenko
0 siblings, 0 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-01-30 10:01 UTC (permalink / raw)
To: Petr Mladek; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v3 1/5] console: Don't perform test for CON_BRL flag
2020-01-27 11:47 [PATCH v3 1/5] console: Don't perform test for CON_BRL flag Andy Shevchenko
` (3 preceding siblings ...)
2020-01-27 11:47 ` [PATCH v3 5/5] console: Introduce ->exit() callback Andy Shevchenko
@ 2020-01-29 12:29 ` Petr Mladek
4 siblings, 0 replies; 28+ messages in thread
From: Petr Mladek @ 2020-01-29 12:29 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
On Mon 2020-01-27 13:47:15, Andy Shevchenko wrote:
> We don't call braille_register_console() without CON_BRL flag set.
> And the _braille_unregister_console() already tests for console to have
> CON_BRL flag. No need to repeat this in braille_unregister_console().
>
> Drop the repetitive checks from Braille console driver.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
I wish the braille console support was added a cleaner way.
I am always confused what is the generic code and what is specific
for the only supported VisioBraille console. And braile()
functions called from _braile() wrappers just add to the confusion.
Anyway, I am fine with this patch.
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 28+ messages in thread