[PATCH v4 1/6] console: Don't perform test for CON_BRL flag

[PATCH v4 1/6] console: Don't perform test for CON_BRL flag

[Andy Shevchenko]

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>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
v4: Add tag (Petr)
 drivers/accessibility/braille/braille_console.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
index 1339c586bf64..a8f7c278b691 100644
--- a/drivers/accessibility/braille/braille_console.c
+++ b/drivers/accessibility/braille/braille_console.c
@@ -347,8 +347,6 @@ int braille_register_console(struct console *console, int index,
 {
 	int ret;
 
-	if (!(console->flags & CON_BRL))
-		return 0;
 	if (!console_options)
 		/* Only support VisioBraille for now */
 		console_options = "57600o8";
@@ -371,8 +369,6 @@ int braille_unregister_console(struct console *console)
 {
 	if (braille_co != console)
 		return -EINVAL;
-	if (!(console->flags & CON_BRL))
-		return 0;
 	unregister_keyboard_notifier(&keyboard_notifier_block);
 	unregister_vt_notifier(&vt_notifier_block);
 	braille_co = NULL;
-- 
2.24.1

[PATCH v4 2/6] console: Drop double check for console_drivers being non-NULL

[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>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
v4: Add tag (Petr)
 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

[PATCH v4 3/6] console: Use for_each_console() helper in unregister_console()

[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>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
v4: Add tag (Petr)
 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

[PATCH v4 4/6] console: Avoid positive return code from unregister_console()

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v4: Drop return code mangling (Sergey, Petr), update commit message (Petr)
 kernel/printk/printk.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d40a316908da..932345e6cd71 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;
-- 
2.24.1

[PATCH v4 5/6] console: Introduce ->exit() callback

[Andy Shevchenko]

Some consoles might require special operations on unregistering.
For instance, serial console, when registered in the kernel,
keeps power on for entire time, until it gets unregistered.
Example of use:

	->setup(console):
		pm_runtime_get(...);

	->exit(console):
		pm_runtime_put(...);

For such cases to have a balance we would provide ->exit() callback.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v4: Don't rely on CON_ENABLED at all (Sergey, Petr), update commit message (Petr)
 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 932345e6cd71..0117d4d92a8e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2850,6 +2850,10 @@ int unregister_console(struct console *console)
 	console->flags &= ~CON_ENABLED;
 	console_unlock();
 	console_sysfs_notify();
+
+	if (!res && console->exit)
+		console->exit(console);
+
 	return res;
 }
 EXPORT_SYMBOL(unregister_console);
-- 
2.24.1

Re: [PATCH v4 5/6] console: Introduce ->exit() callback

[Sergey Senozhatsky]

On (20/01/30 17:25), Andy Shevchenko wrote:
[..]
> 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 *);

Should console ->exit() be able to return an error code?

>  	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 932345e6cd71..0117d4d92a8e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2850,6 +2850,10 @@ int unregister_console(struct console *console)
>  	console->flags &= ~CON_ENABLED;
>  	console_unlock();
>  	console_sysfs_notify();
> +
> +	if (!res && console->exit)
> +		console->exit(console);
> +
>  	return res;
>  }

I would probably push it a bit further (I posted this snippet in another
thread). If console is not on the list then there is nothing for us to do
and sysfs notify is pointless.

---

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 0117d4d92a8e..7116e421210b 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2837,7 +2837,13 @@ int unregister_console(struct console *console)
 		}
 	}
 
-	if (!res && (console->flags & CON_EXTENDED))
+	if (res) {
+		console->flags &= ~CON_ENABLED;
+		console_unlock();
+		return res;
+	}
+
+	if (console->flags & CON_EXTENDED)
 		nr_ext_console_drivers--;
 
 	/*
@@ -2851,7 +2857,7 @@ int unregister_console(struct console *console)
 	console_unlock();
 	console_sysfs_notify();
 
-	if (!res && console->exit)
+	if (console->exit)
 		console->exit(console);
 
 	return res;

Re: [PATCH v4 5/6] console: Introduce ->exit() callback

[Andy Shevchenko]

On Fri, Jan 31, 2020 at 10:31:54AM +0900, Sergey Senozhatsky wrote:
> On (20/01/30 17:25), Andy Shevchenko wrote:
> [..]
> > 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 *);
> 

> Should console ->exit() be able to return an error code?

Let's do it!

> >  	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 932345e6cd71..0117d4d92a8e 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2850,6 +2850,10 @@ int unregister_console(struct console *console)
> >  	console->flags &= ~CON_ENABLED;
> >  	console_unlock();
> >  	console_sysfs_notify();
> > +
> > +	if (!res && console->exit)
> > +		console->exit(console);
> > +
> >  	return res;
> >  }
> 
> I would probably push it a bit further (I posted this snippet in another
> thread). If console is not on the list then there is nothing for us to do
> and sysfs notify is pointless.

I didn't see post in the other thread, but I suppose that this snipped is
for patch 4 in the series, correct?

> 
> ---
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 0117d4d92a8e..7116e421210b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2837,7 +2837,13 @@ int unregister_console(struct console *console)
>  		}
>  	}
>  
> -	if (!res && (console->flags & CON_EXTENDED))
> +	if (res) {
> +		console->flags &= ~CON_ENABLED;
> +		console_unlock();
> +		return res;
> +	}
> +
> +	if (console->flags & CON_EXTENDED)
>  		nr_ext_console_drivers--;
>  
>  	/*
> @@ -2851,7 +2857,7 @@ int unregister_console(struct console *console)
>  	console_unlock();
>  	console_sysfs_notify();
>  
> -	if (!res && console->exit)
> +	if (console->exit)

>  		console->exit(console);
>  
>  	return res;

...then something like

		return console->exit(console);

	return 0;

...or...

		res = console->exit(console);

Which one is preferable?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 5/6] console: Introduce ->exit() callback

[Sergey Senozhatsky]

On (20/01/31 13:27), Andy Shevchenko wrote:
[..]
> > 
> > I would probably push it a bit further (I posted this snippet in another
> > thread). If console is not on the list then there is nothing for us to do
> > and sysfs notify is pointless.
> 
> I didn't see post in the other thread, but I suppose that this snipped is
> for patch 4 in the series, correct?

No worries! Yes, for v4.

[..]
> > -	if (!res && console->exit)
> > +	if (console->exit)
> 
> >  		console->exit(console);
> >  
> >  	return res;
> 
> ...then something like
> 
> 		return console->exit(console);
> 
> 	return 0;
> 
> ...or...
> 
> 		res = console->exit(console);
> 
> Which one is preferable?

I don't really have preferences here, so up to you guys.

	-ss

Re: [PATCH v4 5/6] console: Introduce ->exit() callback

[Andy Shevchenko]

On Sat, Feb 01, 2020 at 10:08:04AM +0900, Sergey Senozhatsky wrote:
> On (20/01/31 13:27), Andy Shevchenko wrote:

> > > I would probably push it a bit further (I posted this snippet in another
> > > thread). If console is not on the list then there is nothing for us to do
> > > and sysfs notify is pointless.
> > 
> > I didn't see post in the other thread, but I suppose that this snipped is
> > for patch 4 in the series, correct?
> 
> No worries! Yes, for v4.

I guess it was v5 be mentioned above, nevertheless, just sent v5.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 6/6] console: Drop misleading comment

[Andy Shevchenko]

	/* find the last or real console */

This comment is misleading. The purpose of the loop is to check
if we are trying to register boot console when one had been
registered before.

Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v4: new patch
 kernel/printk/printk.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 0117d4d92a8e..e5388c8a88c6 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2661,7 +2661,6 @@ void register_console(struct console *newcon)
 	 * already have a valid console
 	 */
 	if (newcon->flags & CON_BOOT) {
-		/* find the last or real console */
 		for_each_console(bcon) {
 			if (!(bcon->flags & CON_BOOT)) {
 				pr_info("Too late to register bootconsole %s%d\n",
-- 
2.24.1