Re: [RFC] [PATCH] Adding ctrl-o sysrq hack support to 8250 driver

[Russell King <rmk+lkml@arm.linux.org.uk>]

On Wed, Nov 30, 2005 at 04:47:14PM +0530, Sachin Sant wrote:
> The following patch will allow a user to use sysrq keys over a serial 
> console using the ctrl-o key sequence. This is similar to functionality 
> provided by the hvc console drivers on PPC boxes.

Several problems:
1. ^O is normally the "flush" control code, for discarding unsent output.
2. ^O might be inadvertantly received due to a connected PC trying to
   auto-detect connected devices.
3. see review below.

> diff -Naurp linux-2.6.14.3/drivers/serial/8250.c linux-2.6.14.3-new/drivers/serial/8250.c
> --- linux-2.6.14.3/drivers/serial/8250.c	2005-11-11 11:03:12.000000000 +0530
> +++ linux-2.6.14.3-new/drivers/serial/8250.c	2005-11-17 15:12:42.000000000 +0530
> @@ -1084,6 +1084,23 @@ receive_chars(struct uart_8250_port *up,
>  			 */
>  		}
>  		ch = serial_inp(up, UART_RX);
> +
> +#if defined(CONFIG_MAGIC_SYSRQ) && defined(CONFIG_SERIAL_CORE_CONSOLE)
> +		/* Handle the SysRq ^O Hack */
> +		if (ch == '\x0f') {
> +			up->port.sysrq = jiffies + HZ*5;

Why are you doing this and not using uart_handle_break()?

Do you realise that this enables sysrq support for _any_ 8250 serial
port, be it console or not?

> +			goto ignore_char; 
> +		}
> +		if (up->port.sysrq) {
> +			int swallow;
> +			spin_unlock(&up->port.lock);
> +			swallow = uart_handle_sysrq_char(&up->port, ch, regs);
> +			spin_lock(&up->port.lock);

We don't drop the lock when calling uart_handle_sysrq_char() further
down in this function.  Why is this needed?  Also, why is this needed
to be duplicated?

> +			if (swallow)
> +				goto ignore_char;
> +		}
> +#endif /* CONFIG_MAGIC_SYSRQ && CONFIG_SERIAL_CORE_CONSOLE */
> +
>  		flag = TTY_NORMAL;
>  		up->port.icount.rx++;
>  

Sorry, NACK.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core