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

[PATCH v1 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.

Fixes: 68af43173d3f ("serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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 v1 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.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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

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

[Dmitry Safonov]

On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> It is useful to see on the serial console the magic sequence itself
> to enable SysRq without rummaging source code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

> ---
>  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);
>  
> 

Thanks,
          Dmitry

[PATCH v1 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.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 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

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

[Dmitry Safonov]

On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> 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.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

> ---
>  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)
> 

Thanks,
          Dmitry

[PATCH v1 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>
---
 drivers/tty/serial/serial_core.c | 22 ++++++++++------------
 include/linux/serial_core.h      |  3 +--
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e3f8e641e3a7..ee51cf87e409 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3286,22 +3286,20 @@ 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) {
-		spin_unlock_irqrestore(&port->lock, irqflags);
-		return;
+	if (port->has_sysrq) {
+		sysrq_ch = port->sysrq_ch;
+		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 v1 4/4] serial: core: Refactor uart_unlock_and_check_sysrq()

[Dmitry Safonov]

On 3/10/20 1:20 PM, Andy Shevchenko wrote:
[..]
> @@ -3286,22 +3286,20 @@ 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;

Probably, you could move it inside the condition it's used.
Though, I wonder why you decided to rearrange the code.
Otherwise, LGTM.

>  
> -	if (!port->has_sysrq) {
> -		spin_unlock_irqrestore(&port->lock, irqflags);
> -		return;
> +	if (port->has_sysrq) {
> +		sysrq_ch = port->sysrq_ch;
> +		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);

Thanks,
          Dmitry

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

[Andy Shevchenko]

On Tue, Mar 10, 2020 at 02:48:51PM +0000, Dmitry Safonov wrote:
> On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> [..]
> > @@ -3286,22 +3286,20 @@ 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;
> 
> Probably, you could move it inside the condition it's used.

I can do this.

> Though, I wonder why you decided to rearrange the code.

I hope commit message sheds a light on this. Main reason to (easily) see
how locks are being maintained.

> Otherwise, LGTM.

Thanks, I will send v2 when we get settlement on patch 1.

> > -	if (!port->has_sysrq) {
> > -		spin_unlock_irqrestore(&port->lock, irqflags);
> > -		return;
> > +	if (port->has_sysrq) {
> > +		sysrq_ch = port->sysrq_ch;
> > +		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);
> 
> Thanks,
>           Dmitry

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Safonov]

Hi Andy,

thanks for the patch,

On 3/10/20 1:20 PM, Andy Shevchenko wrote:
[..]> @@ -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;

Eh, I wanted to avoid the strlen() call in runtime for every time sysrq
is pressed. It's not very frequent moment surely, but..

Could you try

: int sysrq_toggle_seq_len = ARRAY_SIZE(sysrq_toggle_seq);
:
: if (sysrq_toggle_seq_len <= 1)
:     return false;
: /* ... */
: port->sysrq_seq++;
: if (port->sysrq_seq + 1 < sysrq_toggle_seq_len) {

if this will shut the warning instead?
BTW, is this gcc 10 you see the warning with?
I have gcc (GCC) 9.2.0 and I don't see a warning with/without the config
string.

Thanks,
          Dmitry

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

[Andy Shevchenko]

On Tue, Mar 10, 2020 at 02:38:48PM +0000, Dmitry Safonov wrote:
> On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> [..]> @@ -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;
> 
> Eh, I wanted to avoid the strlen() call in runtime for every time sysrq
> is pressed. It's not very frequent moment surely, but..

I really don't like ARRAY_SIZE() against plain strings.
This will use \0 inclusively and confuse the understanding the code.

> Could you try

I even can tell you w/o trying, that it will fix this (yes, it does),
but see above.

> : int sysrq_toggle_seq_len = ARRAY_SIZE(sysrq_toggle_seq);
> :
> : if (sysrq_toggle_seq_len <= 1)
> :     return false;
> : /* ... */
> : port->sysrq_seq++;
> : if (port->sysrq_seq + 1 < sysrq_toggle_seq_len) {
> 
> if this will shut the warning instead?
> BTW, is this gcc 10 you see the warning with?
> I have gcc (GCC) 9.2.0 and I don't see a warning with/without the config
> string.

gcc (Debian 9.2.1-30) 9.2.1 20200224

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Mar 10, 2020 at 04:57:06PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 10, 2020 at 02:38:48PM +0000, Dmitry Safonov wrote:
> > On 3/10/20 1:20 PM, Andy Shevchenko wrote:

...

> > BTW, is this gcc 10 you see the warning with?
> > I have gcc (GCC) 9.2.0 and I don't see a warning with/without the config
> > string.
> 
> gcc (Debian 9.2.1-30) 9.2.1 20200224

I think it will be even on older versions.
I usually run build with `make O=... W=1 C=1 CF=-D__CHECK_ENDIAN__ -j64`.

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Safonov]

Hi Andy,

On 3/10/20 2:57 PM, Andy Shevchenko wrote:
> On Tue, Mar 10, 2020 at 02:38:48PM +0000, Dmitry Safonov wrote:
>> On 3/10/20 1:20 PM, Andy Shevchenko wrote:
>> [..]> @@ -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;
>>
>> Eh, I wanted to avoid the strlen() call in runtime for every time sysrq
>> is pressed. It's not very frequent moment surely, but..
> 
> I really don't like ARRAY_SIZE() against plain strings.
> This will use \0 inclusively and confuse the understanding the code.

I still feel a bit awkward that handler will run strlen() for something
that could be done compile-time, but I won't insist.

Thanks again for looking into warning,
          Dmitry

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

[Andy Shevchenko]

On Tue, Mar 10, 2020 at 03:57:17PM +0000, Dmitry Safonov wrote:
> Hi Andy,
> 
> On 3/10/20 2:57 PM, Andy Shevchenko wrote:
> > On Tue, Mar 10, 2020 at 02:38:48PM +0000, Dmitry Safonov wrote:
> >> On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> >> [..]> @@ -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;
> >>
> >> Eh, I wanted to avoid the strlen() call in runtime for every time sysrq
> >> is pressed. It's not very frequent moment surely, but..
> > 
> > I really don't like ARRAY_SIZE() against plain strings.
> > This will use \0 inclusively and confuse the understanding the code.
> 
> I still feel a bit awkward that handler will run strlen() for something
> that could be done compile-time, but I won't insist.

Have you seen to the assembly?

For me it seems that GCC is clever enough to precalc length for constant
literals.

With default string (empty) there is no uart_sysrq_on() at all in the code.

With "pqr" I see this function and in particular:
     621:       be 03 00 00 00          mov    $0x3,%esi

With "pqrst"

     621:       be 05 00 00 00          mov    $0x5,%esi

(Note, I compiled entire series, that's why uart_sysrq_on() has strlen() in)

> Thanks again for looking into warning,

You are welcome.

-- 
With Best Regards,
Andy Shevchenko

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

[Dmitry Safonov]

On 3/10/20 4:48 PM, Andy Shevchenko wrote:
> 
> Have you seen to the assembly?
> 
> For me it seems that GCC is clever enough to precalc length for constant
> literals.
> 
> With default string (empty) there is no uart_sysrq_on() at all in the code.
> 
> With "pqr" I see this function and in particular:
>      621:       be 03 00 00 00          mov    $0x3,%esi
> 
> With "pqrst"
> 
>      621:       be 05 00 00 00          mov    $0x5,%esi
> 
> (Note, I compiled entire series, that's why uart_sysrq_on() has strlen() in)

Ah, that solves all worries I have.

Thanks again,
          Dmitry

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

[Dmitry Safonov]

On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> 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.
> 
> Fixes: 68af43173d3f ("serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

Thanks!

> ---
>  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;
>  	}
> 

-- 
          Dima