Re: [PATCH 2.6.26] SERIAL DRIVER: Handle Multiple consecutive sysrq from the serial

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 15 Sep 2008 19:30:05 +0300
Eran Liberty <liberty@extricom.com> wrote:

> Dear Penguins,
> 
> Let me start of by saying my particular hardware must be buggy in some 
> way. When I issue a sysrq (Ctrl A+ F from minicom) I get a lot of sysrq 
> triggers.
> 
> I have worked around the problem and I think this workaround is a viable 
> patch even for platforms which do not exhibit this peculiar behavior.
> 
> upon getting numerous interrupts which request sysrq the function 
> uart_handle_break in include/linux/serial_core.h is hit multiple times.
> The current code which looks like this:
> 
> static inline int uart_handle_break(struct uart_port *port)
>  {
>         struct uart_info *info = port->info;
>  #ifdef SUPPORT_SYSRQ
>         if (port->cons && port->cons->index == port->line) {
>                if (!port->sysrq) {
>                        port->sysrq = jiffies + HZ*5;
>                        return 1;
>                }
>                port->sysrq = 0;
>         }
>  #endif
>         if (port->flags & UPF_SAK)
>                 do_SAK(info->tty);
>        return 0;
> }
> 
> Will basicly toggle port->sysrq between a timeout value and zero. If you 
> are lucky this penguin rullet will stop on timeout and the next 
> character hit will trigger the sysrq in the function 
> "uart_handle_sysrq_char". But if you are not so lucky the last sysrq 
> interupt will toggle port->sysrq to zero and the next char hit will be 
> ignored (not trigger sysrq).
> 
> The suggested patch will do the next few things:
> 
> 1. "port->sysrq" is now the time when the last sysrq was triggered and 
> not the timeout for the the next char
> 2. Stamped "port->sysrq" every time there is a sysrq rather then toggled 
> it up and down.
> 3. Always continue to consider UPF_SAK.
> 4. "port->sysrq" is toggled back to zero only in uart_handle_break() and 
> only if the a char has been accepted after the sysrq timeout (5 sec)
> 5. uart_handle_break() will ignore extra chars received in super human 
> speed after the last sysrq (0.01 sec)
> 

yes, that could be irritating.

> Index: include/linux/serial_core.h
> ===================================================================
> --- include/linux/serial_core.h	(revision 119)
> +++ include/linux/serial_core.h	(revision 120)

We prefer patches in `patch -p1' form, please.

Even after fixing that, none of it applied, so I typed it in again.

> @@ -447,8 +447,8 @@
>  uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
>  {
>  #ifdef SUPPORT_SYSRQ
> -	if (port->sysrq) {
> -		if (ch && time_before(jiffies, port->sysrq)) {
> +	if (port->sysrq && time_after(jiffies, port->sysrq + (unsigned long)(HZ*0.01))) {
> +		if (ch && time_before(jiffies, port->sysrq + HZ*5)) {
>  			handle_sysrq(ch, port->info ? port->info->tty : NULL);
>  			port->sysrq = 0;
>  			return 1;
> @@ -467,19 +467,17 @@
>   */
>  static inline int uart_handle_break(struct uart_port *port)
>  {
> +	int ret = 0;
>  	struct uart_info *info = port->info;
>  #ifdef SUPPORT_SYSRQ
>  	if (port->cons && port->cons->index == port->line) {
> -		if (!port->sysrq) {
> -			port->sysrq = jiffies + HZ*5;
> -			return 1;
> -		}
> -		port->sysrq = 0;
> +		port->sysrq = jiffies;
> +		ret = 1;
>  	}
>  #endif
>  	if (port->flags & UPF_SAK)
>  		do_SAK(info->tty);
> -	return 0;
> +	return ret;
>  }

The 0.01 is a big no-no.  Sometimes gcc like to go into stupid mode and
starts doing floating point stuff.

A suitable fix would be to use HZ/100.  But that assumes that HZ is
always >= 100.  That's a pretty good assumption, and various parts of
the kernel will explode if HZ is set too small.  However it's always
good to ensure that someone else's stuff will explode before yours
does, so how about we make it HZ/50?  Will that still work OK for you?

--- a/include/linux/serial_core.h~serial-driver-handle-multiple-consecutive-sysrq-from-the-serial-fix
+++ a/include/linux/serial_core.h
@@ -444,8 +444,7 @@ static inline int
 uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 {
 #ifdef SUPPORT_SYSRQ
-	if (port->sysrq && time_after(jiffies, port->sysrq +
-					(unsigned long)(HZ*0.01))) {
+	if (port->sysrq && time_after(jiffies, port->sysrq + HZ / 50)) {
 		if (ch && time_before(jiffies, port->sysrq + HZ*5)) {
 			handle_sysrq(ch, port->info ? port->info->port.tty : NULL);
 			port->sysrq = 0;
_