[PATCH v2 1/5] console: Don't perform test for CON_BRL flag twice

[PATCH v2 1/5] console: Don't perform test for CON_BRL flag twice

[Andy Shevchenko]

The braille_unregister_console() already tests for console to have
CON_BRL flag. No need to repeat this in _braille_unregister_console().

However, we need to check for that flag at the beginning of the function
to avoid any regressions in the callers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 drivers/accessibility/braille/braille_console.c | 4 ++--
 kernel/printk/braille.c                         | 5 +----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
index 1339c586bf64..f0ffccfb6bb7 100644
--- a/drivers/accessibility/braille/braille_console.c
+++ b/drivers/accessibility/braille/braille_console.c
@@ -369,10 +369,10 @@ int braille_register_console(struct console *console, int index,
 
 int braille_unregister_console(struct console *console)
 {
-	if (braille_co != console)
-		return -EINVAL;
 	if (!(console->flags & CON_BRL))
 		return 0;
+	if (braille_co != console)
+		return -EINVAL;
 	unregister_keyboard_notifier(&keyboard_notifier_block);
 	unregister_vt_notifier(&vt_notifier_block);
 	braille_co = NULL;
diff --git a/kernel/printk/braille.c b/kernel/printk/braille.c
index 17a9591e54ff..2ec42173890f 100644
--- a/kernel/printk/braille.c
+++ b/kernel/printk/braille.c
@@ -51,8 +51,5 @@ _braille_register_console(struct console *console, struct console_cmdline *c)
 int
 _braille_unregister_console(struct console *console)
 {
-	if (console->flags & CON_BRL)
-		return braille_unregister_console(console);
-
-	return 0;
+	return braille_unregister_console(console);
 }
-- 
2.24.1

[PATCH v2 2/5] 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>
---
v2: mark it as a console patch in the Subject line
 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 v2 3/5] 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>
---
v2: new patch
 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 v2 4/5] console: Avoid positive return code from unregister_console()

[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>
---
v2: new patch
 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

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

[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>
---
v2: new patch
 include/linux/console.h | 1 +
 kernel/printk/printk.c  | 3 +++
 2 files changed, 4 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..0319bb698d05 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2850,6 +2850,9 @@ int unregister_console(struct console *console)
 	if (console_drivers != NULL && console->flags & CON_CONSDEV)
 		console_drivers->flags |= CON_CONSDEV;
 
+	if (console->exit)
+		console->exit(console);
+
 	console->flags &= ~CON_ENABLED;
 	console_unlock();
 	console_sysfs_notify();
-- 
2.24.1

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

[Sergey Senozhatsky]

On (20/01/24 17:57), 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..0319bb698d05 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2850,6 +2850,9 @@ int unregister_console(struct console *console)
>  	if (console_drivers != NULL && console->flags & CON_CONSDEV)
>  		console_drivers->flags |= CON_CONSDEV;
>
> +	if (console->exit)
> +		console->exit(console);
> +
>  	console->flags &= ~CON_ENABLED;
>  	console_unlock();
>  	console_sysfs_notify();

Do you need to call ->exit() under console_lock?

	-ss

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

[Andy Shevchenko]

On Mon, Jan 27, 2020 at 11:22:20AM +0900, Sergey Senozhatsky wrote:
> On (20/01/24 17:57), 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..0319bb698d05 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2850,6 +2850,9 @@ int unregister_console(struct console *console)
> >  	if (console_drivers != NULL && console->flags & CON_CONSDEV)
> >  		console_drivers->flags |= CON_CONSDEV;
> >
> > +	if (console->exit)
> > +		console->exit(console);
> > +
> >  	console->flags &= ~CON_ENABLED;
> >  	console_unlock();
> >  	console_sysfs_notify();
> 
> Do you need to call ->exit() under console_lock?

I think we don't need that lock to be held.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/5] console: Don't perform test for CON_BRL flag twice

[Sergey Senozhatsky]

On (20/01/24 17:57), Andy Shevchenko wrote:
[..]
> +++ b/drivers/accessibility/braille/braille_console.c
> @@ -369,10 +369,10 @@ int braille_register_console(struct console *console, int index,
>  
>  int braille_unregister_console(struct console *console)
>  {
> -	if (braille_co != console)
> -		return -EINVAL;
>  	if (!(console->flags & CON_BRL))
>  		return 0;
> +	if (braille_co != console)
> +		return -EINVAL;
>  	unregister_keyboard_notifier(&keyboard_notifier_block);
>  	unregister_vt_notifier(&vt_notifier_block);
>  	braille_co = NULL;
> diff --git a/kernel/printk/braille.c b/kernel/printk/braille.c
> index 17a9591e54ff..2ec42173890f 100644
> --- a/kernel/printk/braille.c
> +++ b/kernel/printk/braille.c
> @@ -51,8 +51,5 @@ _braille_register_console(struct console *console, struct console_cmdline *c)
>  int
>  _braille_unregister_console(struct console *console)
>  {
> -	if (console->flags & CON_BRL)
> -		return braille_unregister_console(console);
> -
> -	return 0;
> +	return braille_unregister_console(console);
>  }

Hmm, I don't know. This moves sort of important code from common upper
layer down to particular driver implementation. Should there be another
driver/super-braille.c it must test CON_BRL flag as well.
Because printk invokes _braille_unregister_console() unconditionally,
and _braille_unregister_console() unconditionally calls into the driver.

I guess we can remove CON_BRL from braille_unregister_console() instead,
because printk tests it.

	-ss

Re: [PATCH v2 1/5] console: Don't perform test for CON_BRL flag twice

[Andy Shevchenko]

On Mon, Jan 27, 2020 at 11:41:14AM +0900, Sergey Senozhatsky wrote:
> On (20/01/24 17:57), Andy Shevchenko wrote:
> [..]
> > +++ b/drivers/accessibility/braille/braille_console.c
> > @@ -369,10 +369,10 @@ int braille_register_console(struct console *console, int index,
> >  
> >  int braille_unregister_console(struct console *console)
> >  {
> > -	if (braille_co != console)
> > -		return -EINVAL;
> >  	if (!(console->flags & CON_BRL))
> >  		return 0;
> > +	if (braille_co != console)
> > +		return -EINVAL;
> >  	unregister_keyboard_notifier(&keyboard_notifier_block);
> >  	unregister_vt_notifier(&vt_notifier_block);
> >  	braille_co = NULL;
> > diff --git a/kernel/printk/braille.c b/kernel/printk/braille.c
> > index 17a9591e54ff..2ec42173890f 100644
> > --- a/kernel/printk/braille.c
> > +++ b/kernel/printk/braille.c
> > @@ -51,8 +51,5 @@ _braille_register_console(struct console *console, struct console_cmdline *c)
> >  int
> >  _braille_unregister_console(struct console *console)
> >  {
> > -	if (console->flags & CON_BRL)
> > -		return braille_unregister_console(console);
> > -
> > -	return 0;
> > +	return braille_unregister_console(console);
> >  }
> 
> Hmm, I don't know. This moves sort of important code from common upper
> layer down to particular driver implementation. Should there be another
> driver/super-braille.c it must test CON_BRL flag as well.
> Because printk invokes _braille_unregister_console() unconditionally,
> and _braille_unregister_console() unconditionally calls into the driver.
> 
> I guess we can remove CON_BRL from braille_unregister_console() instead,
> because printk tests it.

Let me do that way, thanks!

-- 
With Best Regards,
Andy Shevchenko