diff mbox series

[printk,v2,21/38] serial: kgdboc: use srcu console list iterator

Message ID 20221019145600.1282823-22-john.ogness@linutronix.de
State New
Headers
Series reduce console_lock scope |

Commit Message

John Ogness Oct. 19, 2022, 2:55 p.m. UTC 
  Use srcu console list iteration for safe console list traversal.

Note that configure_kgdboc() still requires the console_lock in
order to ensure that no console is in its write() callback when
its direct() callback is called. Add comments to clarify this.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

Comments

Greg KH Oct. 19, 2022, 4:02 p.m. UTC | #1 
On Wed, Oct 19, 2022 at 05:01:43PM +0206, John Ogness wrote:
> Use srcu console list iteration for safe console list traversal.
> 
> Note that configure_kgdboc() still requires the console_lock in
> order to ensure that no console is in its write() callback when
> its direct() callback is called. Add comments to clarify this.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Petr Mladek Oct. 21, 2022, 3:09 p.m. UTC | #2 
On Wed 2022-10-19 17:01:43, John Ogness wrote:
> Use srcu console list iteration for safe console list traversal.
>
> Note that configure_kgdboc() still requires the console_lock in
> order to ensure that no console is in its write() callback when
> its direct() callback is called. Add comments to clarify this.

s/direct()/device()/

Do you know about such requirements or is it just
a conservative approach, please?

I ask because the comment in the code says "may assume".


Anyway, this would deserve a comment why the SRCU list iteration is
needed even when console_lock() is needed as well.

The reason is that further patches are going to synchronize
console_list manipulation with another lock and console_lock()
will be used only to serialize accessing con->write() callbacks.

Best Regards,
Petr

> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index b17aa7e49894..e9d3f8c6e3dc 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -171,6 +171,7 @@ static int configure_kgdboc(void)
>  	int err = -ENODEV;
>  	char *cptr = config;
>  	struct console *cons;
> +	int cookie;
>  
>  	if (!strlen(config) || isspace(config[0])) {
>  		err = 0;
> @@ -193,8 +194,14 @@ static int configure_kgdboc(void)
>  	if (!p)
>  		goto noconfig;
>  
> +	/*
> +	 * Stop console printing because the device() callback may
> +	 * assume the console is not within its write() callback.
> +	 */
>  	console_lock();
> -	for_each_console(cons) {
> +
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(cons) {
>  		int idx;
>  		if (cons->device && cons->device(cons, &idx) == p &&
>  		    idx == tty_line) {
> @@ -202,6 +209,8 @@ static int configure_kgdboc(void)
>  			break;
>  		}
>  	}
> +	console_srcu_read_unlock(cookie);
> +
>  	console_unlock();
>  
>  	kgdb_tty_driver = p;
> @@ -451,6 +460,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  {
>  	struct console *con;
>  	static bool already_warned;
> +	int cookie;
>  
>  	if (already_warned)
>  		return;
> @@ -463,9 +473,14 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  	 * serial drivers might be OK with this, print a warning once per
>  	 * boot if we detect this case.
>  	 */
> -	for_each_console(con)
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con) {
>  		if (con == kgdboc_earlycon_io_ops.cons)
> -			return;
> +			break;
> +	}
> +	console_srcu_read_unlock(cookie);
> +	if (con)
> +		return;
>  
>  	already_warned = true;
>  	pr_warn("kgdboc_earlycon is still using bootconsole\n");
> -- 
> 2.30.2
Doug Anderson Oct. 25, 2022, 12:33 a.m. UTC | #3 
Hi,

On Fri, Oct 21, 2022 at 8:09 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2022-10-19 17:01:43, John Ogness wrote:
> > Use srcu console list iteration for safe console list traversal.
> >
> > Note that configure_kgdboc() still requires the console_lock in
> > order to ensure that no console is in its write() callback when
> > its direct() callback is called. Add comments to clarify this.
>
> s/direct()/device()/
>
> Do you know about such requirements or is it just
> a conservative approach, please?
>
> I ask because the comment in the code says "may assume".
>
>
> Anyway, this would deserve a comment why the SRCU list iteration is
> needed even when console_lock() is needed as well.
>
> The reason is that further patches are going to synchronize
> console_list manipulation with another lock and console_lock()
> will be used only to serialize accessing con->write() callbacks.

I had the same concern. I'll note that at the end of the series the
documentation for console_lock() still says:

 * Acquires a lock which guarantees that the caller has
 * exclusive access to the console system.

That seems to imply (at least to me) that if you're holding
console_lock() there's no need to hold the SRCU lock.

-Doug
diff mbox series

Patch

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index b17aa7e49894..e9d3f8c6e3dc 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -171,6 +171,7 @@  static int configure_kgdboc(void)
 	int err = -ENODEV;
 	char *cptr = config;
 	struct console *cons;
+	int cookie;
 
 	if (!strlen(config) || isspace(config[0])) {
 		err = 0;
@@ -193,8 +194,14 @@  static int configure_kgdboc(void)
 	if (!p)
 		goto noconfig;
 
+	/*
+	 * Stop console printing because the device() callback may
+	 * assume the console is not within its write() callback.
+	 */
 	console_lock();
-	for_each_console(cons) {
+
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(cons) {
 		int idx;
 		if (cons->device && cons->device(cons, &idx) == p &&
 		    idx == tty_line) {
@@ -202,6 +209,8 @@  static int configure_kgdboc(void)
 			break;
 		}
 	}
+	console_srcu_read_unlock(cookie);
+
 	console_unlock();
 
 	kgdb_tty_driver = p;
@@ -451,6 +460,7 @@  static void kgdboc_earlycon_pre_exp_handler(void)
 {
 	struct console *con;
 	static bool already_warned;
+	int cookie;
 
 	if (already_warned)
 		return;
@@ -463,9 +473,14 @@  static void kgdboc_earlycon_pre_exp_handler(void)
 	 * serial drivers might be OK with this, print a warning once per
 	 * boot if we detect this case.
 	 */
-	for_each_console(con)
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(con) {
 		if (con == kgdboc_earlycon_io_ops.cons)
-			return;
+			break;
+	}
+	console_srcu_read_unlock(cookie);
+	if (con)
+		return;
 
 	already_warned = true;
 	pr_warn("kgdboc_earlycon is still using bootconsole\n");