[PATCH v2 1/4] serial: core: Use string length for SysRq magic sequence

[PATCH v2 1/4] serial: core: Use string length for SysRq magic sequence

[Andy Shevchenko]

Compiler is not happy about using ARRAY_SIZE() in comparison to smaller type:

  CC      drivers/tty/serial/serial_core.o
.../serial_core.c: In function ‘uart_try_toggle_sysrq’:
.../serial_core.c:3222:24: warning: comparison is always false due to limited range of data type [-Wtype-limits]
 3222 |  if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
      |                        ^

Looking at the code it appears that there is an additional weirdness,
i.e. use ARRAY_SIZE() against simple string literal. Yes, the idea probably
was to allow '\0' in the sequence, but it's impractical: kernel configuration
won't accept it to begin with followed by a comment about '\0' before
comparison in question.

Drop all these by switching to strlen() and convert code accordingly.

Note, GCC seems clever enough to calculate string length at compile time.

Fixes: 68af43173d3f ("serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE")
Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: add note that GCC optimizes strlen() at run time, add tag
 drivers/tty/serial/serial_core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index aec98db45406..ec3b833e9f22 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3209,7 +3209,9 @@ static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
  */
 static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
 {
-	if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
+	int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
+
+	if (!sysrq_toggle_seq_len)
 		return false;
 
 	BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= U8_MAX);
@@ -3218,8 +3220,7 @@ static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
 		return false;
 	}
 
-	/* Without the last \0 */
-	if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
+	if (++port->sysrq_seq < sysrq_toggle_seq_len) {
 		port->sysrq = jiffies + SYSRQ_TIMEOUT;
 		return true;
 	}
-- 
2.25.1

[PATCH v2 2/4] serial: core: Print escaped SysRq Magic sequence if enabled

[Andy Shevchenko]

It is useful to see on the serial console the magic sequence itself
to enable SysRq without rummaging source code.

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: add tag
 drivers/tty/serial/serial_core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ec3b833e9f22..e3f2afc15ad4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3191,8 +3191,11 @@ static const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
 
 static void uart_sysrq_on(struct work_struct *w)
 {
+	int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
+
 	sysrq_toggle_support(1);
-	pr_info("SysRq is enabled by magic sequence on serial\n");
+	pr_info("SysRq is enabled by magic sequence '%*pE' on serial\n",
+		sysrq_toggle_seq_len, sysrq_toggle_seq);
 }
 static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
 
-- 
2.25.1

[PATCH v2 3/4] serial: core: Use uart_console() helper in SysRq code

[Andy Shevchenko]

Use uart_console() helper in SysRq code instead of open coded variant.
This eliminates the conditional entirely for SERIAL_CORE_CONSOLE=n case.
While here, refactor the conditional to be more compact.

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: add tag
 drivers/tty/serial/serial_core.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e3f2afc15ad4..e3f8e641e3a7 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3315,14 +3315,12 @@ int uart_handle_break(struct uart_port *port)
 	if (port->handle_break)
 		port->handle_break(port);
 
-	if (port->has_sysrq) {
-		if (port->cons && port->cons->index == port->line) {
-			if (!port->sysrq) {
-				port->sysrq = jiffies + SYSRQ_TIMEOUT;
-				return 1;
-			}
-			port->sysrq = 0;
+	if (port->has_sysrq && uart_console(port)) {
+		if (!port->sysrq) {
+			port->sysrq = jiffies + SYSRQ_TIMEOUT;
+			return 1;
 		}
+		port->sysrq = 0;
 	}
 
 	if (port->flags & UPF_SAK)
-- 
2.25.1

[PATCH v2 4/4] serial: core: Refactor uart_unlock_and_check_sysrq()

[Andy Shevchenko]

Refactor uart_unlock_and_check_sysrq() to:

  - explicitly show that we release a port lock which makes
    static analyzers happy:

CHECK   drivers/tty/serial/serial_core.c
.../serial_core.c:3290:17: warning: context imbalance in 'uart_unlock_and_check_sysrq' - unexpected unlock

  - use flags instead of irqflags to avoid confusion with IRQ flags

  - provide one return point

  - be more compact

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: move sysrq_ch declaration inside branch (Dmitry)
 drivers/tty/serial/serial_core.c | 23 ++++++++++-------------
 include/linux/serial_core.h      |  3 +--
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e3f8e641e3a7..203697465afc 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3286,22 +3286,19 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
 }
 EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
 
-void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags)
+__releases(&port->lock)
 {
-	int sysrq_ch;
+	if (port->has_sysrq) {
+		int sysrq_ch = port->sysrq_ch;
 
-	if (!port->has_sysrq) {
-		spin_unlock_irqrestore(&port->lock, irqflags);
-		return;
+		port->sysrq_ch = 0;
+		spin_unlock_irqrestore(&port->lock, flags);
+		if (sysrq_ch)
+			handle_sysrq(sysrq_ch);
+	} else {
+		spin_unlock_irqrestore(&port->lock, flags);
 	}
-
-	sysrq_ch = port->sysrq_ch;
-	port->sysrq_ch = 0;
-
-	spin_unlock_irqrestore(&port->lock, irqflags);
-
-	if (sysrq_ch)
-		handle_sysrq(sysrq_ch);
 }
 EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 7a025042e0bb..cab896c60e35 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -447,8 +447,7 @@ extern void uart_insert_char(struct uart_port *port, unsigned int status,
 
 extern int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch);
 extern int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch);
-extern void uart_unlock_and_check_sysrq(struct uart_port *port,
-					unsigned long irqflags);
+extern void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags);
 extern int uart_handle_break(struct uart_port *port);
 
 /*
-- 
2.25.1

Re: [PATCH v2 4/4] serial: core: Refactor uart_unlock_and_check_sysrq()

[Dmitry Safonov]

On 3/10/20 5:43 PM, Andy Shevchenko wrote:
> Refactor uart_unlock_and_check_sysrq() to:
> 
>   - explicitly show that we release a port lock which makes
>     static analyzers happy:
> 
> CHECK   drivers/tty/serial/serial_core.c
> .../serial_core.c:3290:17: warning: context imbalance in 'uart_unlock_and_check_sysrq' - unexpected unlock
> 
>   - use flags instead of irqflags to avoid confusion with IRQ flags
> 
>   - provide one return point
> 
>   - be more compact
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks!

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>

> ---
> v2: move sysrq_ch declaration inside branch (Dmitry)
>  drivers/tty/serial/serial_core.c | 23 ++++++++++-------------
>  include/linux/serial_core.h      |  3 +--
>  2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index e3f8e641e3a7..203697465afc 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3286,22 +3286,19 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
>  }
>  EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
>  
> -void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
> +void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags)
> +__releases(&port->lock)
>  {
> -	int sysrq_ch;
> +	if (port->has_sysrq) {
> +		int sysrq_ch = port->sysrq_ch;
>  
> -	if (!port->has_sysrq) {
> -		spin_unlock_irqrestore(&port->lock, irqflags);
> -		return;
> +		port->sysrq_ch = 0;
> +		spin_unlock_irqrestore(&port->lock, flags);
> +		if (sysrq_ch)
> +			handle_sysrq(sysrq_ch);
> +	} else {
> +		spin_unlock_irqrestore(&port->lock, flags);
>  	}
> -
> -	sysrq_ch = port->sysrq_ch;
> -	port->sysrq_ch = 0;
> -
> -	spin_unlock_irqrestore(&port->lock, irqflags);
> -
> -	if (sysrq_ch)
> -		handle_sysrq(sysrq_ch);
>  }
>  EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
>  
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 7a025042e0bb..cab896c60e35 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -447,8 +447,7 @@ extern void uart_insert_char(struct uart_port *port, unsigned int status,
>  
>  extern int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch);
>  extern int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch);
> -extern void uart_unlock_and_check_sysrq(struct uart_port *port,
> -					unsigned long irqflags);
> +extern void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags);
>  extern int uart_handle_break(struct uart_port *port);
>  
>  /*
> 

-- 
          Dima