On Fri, 29 May 2009 13:34:17 -0500
Jason Wessel <[email protected]> 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 ?
On Mon 1 Jun 2009 09:35, Alan Cox pondered:
> On Fri, 29 May 2009 13:34:17 -0500
> Jason Wessel <[email protected]> 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;
}
> 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
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.
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
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)...