[PATCH 0/2] more usb sysrq improvements

[PATCH 0/2] more usb sysrq improvements

[Jason Wessel]

This patch series is to add sysrq support for the pl2303 usb device
and to stop sysrq processing on non-console usb serial ports.

Thanks,
Jason.


The short log and git info is below, which is against the 2.6.30
development tree with the previous 4 accepted usb patches in the
gregkh queue.

The following changes since commit a425a638c858fd10370b573bde81df3ba500e271:
are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/jwessel/linux-2.6-kgdb.git for_gregkh_2

Jason Wessel (2):
      pl2303 usb_serial: implement sysrq handling on break
      usb_serial: only allow sysrq on a console port

 drivers/usb/serial/generic.c |    2 +-
 drivers/usb/serial/pl2303.c  |    5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

[PATCH 1/2] pl2303 usb_serial: implement sysrq handling on break

[Jason Wessel]

Add callbacks to process the sysrq when using a pl2303 usb device as a
console.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/usb/serial/pl2303.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index 751a533..c5a8745 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -946,6 +946,8 @@ static void pl2303_update_line_status(struct usb_serial_port *port,
 	spin_lock_irqsave(&priv->lock, flags);
 	priv->line_status = data[status_idx];
 	spin_unlock_irqrestore(&priv->lock, flags);
+	if (priv->line_status & UART_BREAK_ERROR)
+		usb_serial_handle_break(port);
 	wake_up_interruptible(&priv->delta_msr_wait);
 }
 
@@ -1056,7 +1058,8 @@ static void pl2303_read_bulk_callback(struct urb *urb)
 		if (line_status & UART_OVERRUN_ERROR)
 			tty_insert_flip_char(tty, 0, TTY_OVERRUN);
 		for (i = 0; i < urb->actual_length; ++i)
-			tty_insert_flip_char(tty, data[i], tty_flag);
+			if (!usb_serial_handle_sysrq_char(port, data[i]))
+				tty_insert_flip_char(tty, data[i], tty_flag);
 		tty_flip_buffer_push(tty);
 	}
 	tty_kref_put(tty);
-- 
1.6.3.1.9.g95405b

[PATCH 2/2] usb_serial: only allow sysrq on a console port

[Jason Wessel]

The only time a sysrq should get processed is if the attached device
is a console.  This is intended to protect sysrq execution on a host
connected with a terminal program.

Here is the problem scenario:

host A <-- rs232 link --> host B

Host A is using mincom and a usb pl2303 device to connect to host b
which is a linux system with a usb pl2303 device acting as the serial
console.  When host B is rebooted the pl2303 emits random junk
characters on reset.  These character sequences contain serial break
signals most of the time and when translated to a sysrq have caused
host A to get random processes killed, reboots or power down.

It is true that in this setup with this patch host B might still have
the same problem as host A if you reboot host A.  In most cases host A
is a development host which seldom gets rebooted, and you could turn
off sysrq temporarily on host B if you need to reboot host A.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 drivers/usb/serial/generic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index df56ed3..4797402 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -529,7 +529,7 @@ void usb_serial_generic_unthrottle(struct tty_struct *tty)
 
 int usb_serial_handle_sysrq_char(struct usb_serial_port *port, unsigned int ch)
 {
-	if (port->sysrq) {
+	if (port->sysrq && port->console) {
 		if (ch && time_before(jiffies, port->sysrq)) {
 			handle_sysrq(ch, tty_port_tty_get(&port->port));
 			port->sysrq = 0;
-- 
1.6.3.1.9.g95405b

Re: [PATCH 2/2] usb_serial: only allow sysrq on a console port

[Alan Cox]

On Fri, 29 May 2009 13:34:17 -0500
Jason Wessel <jason.wessel@windriver.com> wrote:

> The only time a sysrq should get processed is if the attached device
> is a console.  This is intended to protect sysrq execution on a host
> connected with a terminal program.

This doesn't seem to match any tree I can find ?

Re: [PATCH 2/2] usb_serial: only allow sysrq on a console port

[Robin Getz]

On Mon 1 Jun 2009 09:35, Alan Cox pondered:
> On Fri, 29 May 2009 13:34:17 -0500
> Jason Wessel <jason.wessel@windriver.com> wrote:
> 
> > The only time a sysrq should get processed is if the attached device
> > is a console.  This is intended to protect sysrq execution on a host
> > connected with a terminal program.
> 
> This doesn't seem to match any tree I can find ?

Alan:

If Jason's patch is necessary () - should this be fixed up for standard
UARTs too?

Make sure that only serial console (not _any_ serial port) responds to 
sysrq (or should something else be ensuring that this isn't set when 
the port !console? (I didn't see anything in serial_core.c?)

---

diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 23d2fb0..f8ab858 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -474,7 +474,7 @@ static inline int
 uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 {
 #ifdef SUPPORT_SYSRQ
-       if (port->sysrq) {
+       if (port->sysrq && port->console) {
                if (ch && time_before(jiffies, port->sysrq)) {
                        handle_sysrq(ch, port->info->port.tty);
                        port->sysrq = 0;

----------

That brings up the next question...

The above patch would sync the (seemlying duplicated) code between
drivers/usb/serial/generic.c and include/linux/serial_core.h

Greg?

drivers/usb/serial/generic.c
int usb_serial_handle_sysrq_char(struct usb_serial_port *port, unsigned int ch)
{
        if (port->sysrq && port->console) {
                if (ch && time_before(jiffies, port->sysrq)) {
                        handle_sysrq(ch, tty_port_tty_get(&port->port));
                        port->sysrq = 0;
                        return 1;
                }
                port->sysrq = 0;
        }
        return 0;
}

include/linux/serial_core.h
static inline int
uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
{
#ifdef SUPPORT_SYSRQ
        if (port->sysrq) {
                if (ch && time_before(jiffies, port->sysrq)) {
                        handle_sysrq(ch, port->info->port.tty);
                        port->sysrq = 0;
                        return 1;
                }
                port->sysrq = 0;
        }
#endif
        return 0;
}

Re: [PATCH 2/2] usb_serial: only allow sysrq on a console port

[Alan Cox]

> If Jason's patch is necessary () - should this be fixed up for standard
> UARTs too?

I think so yes, although I'd not realised it wasn't protected currently ?
> 
> Make sure that only serial console (not _any_ serial port) responds to 
> sysrq (or should something else be ensuring that this isn't set when 
> the port !console? (I didn't see anything in serial_core.c?)

will check

> The above patch would sync the (seemlying duplicated) code between
> drivers/usb/serial/generic.c and include/linux/serial_core.h

There is a lot of near duplicate code like this. That is one reason for
adding struct tty_port. In theory both could be collapsed into

	int tty_port_handle_sysrq(struct tty_port *port, unsigned int ch)
	{
	}

at this point as both USB and serial layer UARTs now have a port object.
That would just need port->sysrq collapsing into the tty_port.
port->console sort of already is.

> Greg?
> 
> drivers/usb/serial/generic.c
> int usb_serial_handle_sysrq_char(struct usb_serial_port *port, unsigned int ch)
> {
>         if (port->sysrq && port->console) {
>                 if (ch && time_before(jiffies, port->sysrq)) {
>                         handle_sysrq(ch, tty_port_tty_get(&port->port));
>                         port->sysrq = 0;
>                         return 1;

That also looks wrong - tty_port_tty_get takes a tty kref. I will check
that Monday and look at collapsing these int one as you note.

Alan

Re: [PATCH 2/2] usb_serial: only allow sysrq on a console port

[Robin Getz]

On Sun 5 Jul 2009 14:17, Alan Cox pondered:
> > If Jason's patch is necessary () - should this be fixed up for
> > standard UARTs too?
> 
> I think so yes, although I'd not realised it wasn't protected currently
> ?

Hmm - try as I may - I can't get this to fail - so it must be protected 
somewhere....

Ahh---

It is in include/linux/serial_core.h:uart_handle_break() - never checked the 
header for the magic before I bugged you... sorry about that...

> > The above patch would sync the (seemlying duplicated) code between
> > drivers/usb/serial/generic.c and include/linux/serial_core.h
> 
> There is a lot of near duplicate code like this. That is one reason for
> adding struct tty_port. In theory both could be collapsed into
> 
> 	int tty_port_handle_sysrq(struct tty_port *port, unsigned int ch)
> 	{
> 	}
> 
> at this point as both USB and serial layer UARTs now have a port object.
> That would just need port->sysrq collapsing into the tty_port.
> port->console sort of already is.

It appears that the usb serial doesn't handle breaks like serial_core does. (I 
don't see any support for SAK in usb_serial either?)

Maybe _that_ is the real problem that Jason is trying to work around???


> > drivers/usb/serial/generic.c
> > int usb_serial_handle_sysrq_char(struct usb_serial_port *port,
> unsigned int ch)
> > {
> >         if (port->sysrq && port->console) {
> >                 if (ch && time_before(jiffies, port->sysrq)) {
> >                         handle_sysrq(ch,
> tty_port_tty_get(&port->port));
> >                         port->sysrq = 0;
> >                         return 1;
> 
> That also looks wrong - tty_port_tty_get takes a tty kref. I will check
> that Monday and look at collapsing these int one as you note.

Re: [PATCH 2/2] usb_serial: only allow sysrq on a console port

[Mike Frysinger]

On Mon, Jul 6, 2009 at 00:38, Robin Getz wrote:
> On Sun 5 Jul 2009 14:17, Alan Cox pondered:
>> > If Jason's patch is necessary () - should this be fixed up for
>> > standard UARTs too?
>>
>> I think so yes, although I'd not realised it wasn't protected currently
>> ?
>
> Hmm - try as I may - I can't get this to fail - so it must be protected
> somewhere....
>
> Ahh---
>
> It is in include/linux/serial_core.h:uart_handle_break() - never checked the
> header for the magic before I bugged you... sorry about that...
>
>> > The above patch would sync the (seemlying duplicated) code between
>> > drivers/usb/serial/generic.c and include/linux/serial_core.h
>>
>> There is a lot of near duplicate code like this. That is one reason for
>> adding struct tty_port. In theory both could be collapsed into
>>
>>       int tty_port_handle_sysrq(struct tty_port *port, unsigned int ch)
>>       {
>>       }
>>
>> at this point as both USB and serial layer UARTs now have a port object.
>> That would just need port->sysrq collapsing into the tty_port.
>> port->console sort of already is.
>
> It appears that the usb serial doesn't handle breaks like serial_core does. (I
> don't see any support for SAK in usb_serial either?)
>
> Maybe _that_ is the real problem that Jason is trying to work around???

perhaps, but what Jason proposed originally sounds pretty sane.  if i
enable sysrq support on my desktop, i dont want some development board
being able to send a sysrq request back over my serial port and
rebooting my desktop.
-mike

Re: [PATCH 2/2] usb_serial: only allow sysrq on a console port

[Robin Getz]

On Mon 6 Jul 2009 00:56, Mike Frysinger pondered:
> On Mon, Jul 6, 2009 at 00:38, Robin Getz wrote:
> > It appears that the usb serial doesn't handle breaks like serial_core
> > does. (I  don't see any support for SAK in usb_serial either?)
> >
> > Maybe _that_ is the real problem that Jason is trying to work around???
> 
> perhaps, but what Jason proposed originally sounds pretty sane.

Which is why Greg added it to the USB tree, and it is in 2.6.31-rc2 :)

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=568d422e9cf52b7b26d2e026ae1617971f62b560

> if i 
> enable sysrq support on my desktop, i dont want some development board
> being able to send a sysrq request back over my serial port and
> rebooting my desktop.

Fixing something in the wrong place -- while noble -- is still wrong :)

However -- with the way that usb serial is structured - I didn't see a better 
place right now either.

The point being that is would be nice to make usb serial look more like other 
serial devices - so they can share mode code - and things like this (that 
have been in serial_core.h since -pre-git history (2005) wouldn't just be 
getting added to the usb serial infrastructure now)...