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

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

[Eran Liberty]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

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)

Liberty

Signed-off-by: Eran Liberty <liberty@extricom.org>
---



[-- Attachment #2: sysrq.patch --]
[-- Type: text/x-patch, Size: 1068 bytes --]

Index: include/linux/serial_core.h
===================================================================
--- include/linux/serial_core.h	(revision 119)
+++ include/linux/serial_core.h	(revision 120)
@@ -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;
 }
 
 /**

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

[Andrew Morton]

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

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

[Eran Liberty]

Andrew Morton wrote:
> 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.
>   
So  should I resubmit or have you done then nasty work for me (thanks :) )?
>   
>> @@ -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?
>   

HZ/50 is totally OK by me.


-- Liberty