linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/4] serial: core: Use string length for SysRq magic sequence
@ 2020-03-10 17:43 Andy Shevchenko
  2020-03-10 17:43 ` [PATCH v2 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Andy Shevchenko @ 2020-03-10 17:43 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial, Dmitry Safonov; +Cc: 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 2/4] serial: core: Print escaped SysRq Magic sequence if enabled
  2020-03-10 17:43 [PATCH v2 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
@ 2020-03-10 17:43 ` Andy Shevchenko
  2020-03-10 17:43 ` [PATCH v2 3/4] serial: core: Use uart_console() helper in SysRq code Andy Shevchenko
  2020-03-10 17:43 ` [PATCH v2 4/4] serial: core: Refactor uart_unlock_and_check_sysrq() Andy Shevchenko
  2 siblings, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2020-03-10 17:43 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial, Dmitry Safonov; +Cc: 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 3/4] serial: core: Use uart_console() helper in SysRq code
  2020-03-10 17:43 [PATCH v2 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
  2020-03-10 17:43 ` [PATCH v2 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
@ 2020-03-10 17:43 ` Andy Shevchenko
  2020-03-10 17:43 ` [PATCH v2 4/4] serial: core: Refactor uart_unlock_and_check_sysrq() Andy Shevchenko
  2 siblings, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2020-03-10 17:43 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial, Dmitry Safonov; +Cc: 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 4/4] serial: core: Refactor uart_unlock_and_check_sysrq()
  2020-03-10 17:43 [PATCH v2 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
  2020-03-10 17:43 ` [PATCH v2 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
  2020-03-10 17:43 ` [PATCH v2 3/4] serial: core: Use uart_console() helper in SysRq code Andy Shevchenko
@ 2020-03-10 17:43 ` Andy Shevchenko
  2020-03-10 17:45   ` Dmitry Safonov
  2 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2020-03-10 17:43 UTC (permalink / raw)
  To: Greg Kroah-Hartman, linux-serial, Dmitry Safonov; +Cc: 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 4/4] serial: core: Refactor uart_unlock_and_check_sysrq()
  2020-03-10 17:43 ` [PATCH v2 4/4] serial: core: Refactor uart_unlock_and_check_sysrq() Andy Shevchenko
@ 2020-03-10 17:45   ` Dmitry Safonov
  0 siblings, 0 replies; 5+ messages in thread
From: Dmitry Safonov @ 2020-03-10 17:45 UTC (permalink / raw)
  To: Andy Shevchenko, Greg Kroah-Hartman, linux-serial



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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-03-10 17:45 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 17:43 [PATCH v2 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
2020-03-10 17:43 ` [PATCH v2 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
2020-03-10 17:43 ` [PATCH v2 3/4] serial: core: Use uart_console() helper in SysRq code Andy Shevchenko
2020-03-10 17:43 ` [PATCH v2 4/4] serial: core: Refactor uart_unlock_and_check_sysrq() Andy Shevchenko
2020-03-10 17:45   ` Dmitry Safonov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).