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

[Eran Liberty]

Andrew Morton wrote:
> On Mon, 15 Sep 2008 19:30:05 +0300
> Eran Liberty <liberty@xxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/