Alan Cox wrote:
> NAK this patch.
>
>> + for (i = 0; i < urb->actual_length; i++, ch++) {
>> + if (!usb_serial_handle_sysrq_char(port, *ch))
>> + tty_insert_flip_char(tty, *ch, TTY_NORMAL);
>
> Sorry but this is unacceptable. Some of the USB 3G modems and similar are
> running bytes through the USB serial layer at about 3MBits and this is an
> obscene pile of function calls thrown directly into the fast path.
>
> Matching a byte pattern also has security impacts because I can send that
> byte pattern and you currently have to recompile to turn the sysrq stuff
> around.
>
> At the very least before this patch is acceptable you need to inline the
> port->sysrq check
>
> And in future please Cc me on tty layer patches Jason (if this one was
> Cc'd and never made it here I apologise).
>
In the original patch I sent, this was inlined. Greg KH made the
change and added the exported function for other drivers to call. The
original patch used the same methodology that the uart drivers used
with the serial headers.
I agree that these function calls don't belong in the fast path unless
the device is acting as a system console, which is the only place a
sysrq should be processed in the first place. The handle break
function was already changed in a later patch in the series to only
fire its function calls if the device is acting as a system console.
Would you find the revised patch acceptable Alan, where the
functions are only called when the device is registered as a system
console? This also restores the multi-byte behavior when not a system
console which is definitely seems appropriate.
Specifically we are talking about the section:
if (port->console) {
for (i = 0; i < urb->actual_length; i++, ch++) {
if (!usb_serial_handle_sysrq_char(port, *ch))
tty_insert_flip_char(tty, *ch, TTY_NORMAL);
}
tty_flip_buffer_push(tty);
} else if (urb->actual_length) {
i = tty_buffer_request_room(tty, urb->actual_length);
if (i) {
tty_insert_flip_string(tty, urb->transfer_buffer, i);
tty_flip_buffer_push(tty);
}
}
Also should I submit a new patch which converts the
usb_serial_handle_sysrq_char() back to an inline?
Thanks,
Jason.
From: Jason Wessel <jason.wessel@xxxxxxxxxxxxx>
Subject: USB: serial: usb_debug,usb_generic_serial: implement sysrq and serial break
To: greg@xxxxxxxxx
Cc: stern@xxxxxxxxxxxxxxxxxxx, Jason Wessel <jason.wessel@xxxxxxxxxxxxx>
The usb_debug driver was modified to implement serial break handling
by using a "magic" data packet comprised of the sequence:
0x00 0xff 0x01 0xfe 0x00 0xfe 0x01 0xff
When the tty layer requests a serial break the usb_debug driver sends
the magic packet. On the receiving side the magic packet is thrown
away or a sysrq is activated depending on what kernel .config options
have been set.
The generic serial driver was modified as well as the usb serial
headers to generically implement sysrq processing in the same way the
non usb uart based drivers implement the sysrq handling. This will
allow other usb serial devices to implement sysrq handling as desired.
The new usb serial functions are named similarly and implemented
similarly to the uart functions as follows:
usb_serial_handle_break <-> uart_handle_break
usb_serial_handle_sysrq_char <-> uart_handle_sysrq_char
Signed-off-by: Jason Wessel <jason.wessel@xxxxxxxxxxxxx>
---
drivers/usb/serial/generic.c | 60 ++++++++++++++++++++++++++++++++++-------
drivers/usb/serial/usb_debug.c | 39 ++++++++++++++++++++++++++
include/linux/usb/serial.h | 8 +++++
3 files changed, 97 insertions(+), 10 deletions(-)
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -339,6 +339,7 @@ int usb_serial_generic_write(struct tty_
/* no bulk out, so return 0 bytes written */
return 0;
}
+EXPORT_SYMBOL_GPL(usb_serial_generic_write);
int usb_serial_generic_write_room(struct tty_struct *tty)
{
@@ -351,7 +352,9 @@ int usb_serial_generic_write_room(struct
spin_lock_irqsave(&port->lock, flags);
if (serial->type->max_in_flight_urbs) {
if (port->urbs_in_flight < serial->type->max_in_flight_urbs)
- room = port->bulk_out_size;
+ room = port->bulk_out_size *
+ (serial->type->max_in_flight_urbs -
+ port->urbs_in_flight);
} else if (serial->num_bulk_out && !(port->write_urb_busy)) {
room = port->bulk_out_size;
}
@@ -385,7 +388,8 @@ int usb_serial_generic_chars_in_buffer(s
}
-static void resubmit_read_urb(struct usb_serial_port *port, gfp_t mem_flags)
+void usb_serial_generic_resubmit_read_urb(struct usb_serial_port *port,
+ gfp_t mem_flags)
{
struct urb *urb = port->read_urb;
struct usb_serial *serial = port->serial;
@@ -406,25 +410,36 @@ static void resubmit_read_urb(struct usb
"%s - failed resubmitting read urb, error %d\n",
__func__, result);
}
+EXPORT_SYMBOL_GPL(usb_serial_generic_resubmit_read_urb);
/* Push data to tty layer and resubmit the bulk read URB */
static void flush_and_resubmit_read_urb(struct usb_serial_port *port)
{
struct urb *urb = port->read_urb;
struct tty_struct *tty = tty_port_tty_get(&port->port);
- int room;
+ char *ch = (char *)urb->transfer_buffer;
+ int i;
+
+ if (!tty)
+ goto done;
/* Push data to tty */
- if (tty && urb->actual_length) {
- room = tty_buffer_request_room(tty, urb->actual_length);
- if (room) {
- tty_insert_flip_string(tty, urb->transfer_buffer, room);
+ if (port->console) {
+ for (i = 0; i < urb->actual_length; i++, ch++) {
+ if (!usb_serial_handle_sysrq_char(port, *ch))
+ tty_insert_flip_char(tty, *ch, TTY_NORMAL);
+ }
+ tty_flip_buffer_push(tty);
+ } else if (urb->actual_length) {
+ i = tty_buffer_request_room(tty, urb->actual_length);
+ if (i) {
+ tty_insert_flip_string(tty, urb->transfer_buffer, i);
tty_flip_buffer_push(tty);
}
}
tty_kref_put(tty);
-
- resubmit_read_urb(port, GFP_ATOMIC);
+done:
+ usb_serial_generic_resubmit_read_urb(port, GFP_ATOMIC);
}
void usb_serial_generic_read_bulk_callback(struct urb *urb)
@@ -515,10 +530,35 @@ void usb_serial_generic_unthrottle(struc
if (was_throttled) {
/* Resume reading from device */
- resubmit_read_urb(port, GFP_KERNEL);
+ usb_serial_generic_resubmit_read_urb(port, GFP_KERNEL);
}
}
+int usb_serial_handle_sysrq_char(struct usb_serial_port *port, unsigned int ch)
+{
+ if (port->sysrq) {
+ 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;
+}
+EXPORT_SYMBOL_GPL(usb_serial_handle_sysrq_char);
+
+int usb_serial_handle_break(struct usb_serial_port *port)
+{
+ if (!port->sysrq) {
+ port->sysrq = jiffies + HZ*5;
+ return 1;
+ }
+ port->sysrq = 0;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(usb_serial_handle_break);
+
void usb_serial_generic_shutdown(struct usb_serial *serial)
{
int i;
--- a/drivers/usb/serial/usb_debug.c
+++ b/drivers/usb/serial/usb_debug.c
@@ -17,6 +17,17 @@
#define URB_DEBUG_MAX_IN_FLIGHT_URBS 4000
#define USB_DEBUG_MAX_PACKET_SIZE 8
+#define USB_DEBUG_BRK_SIZE 8
+static char USB_DEBUG_BRK[USB_DEBUG_BRK_SIZE] = {
+ 0x00,
+ 0xff,
+ 0x01,
+ 0xfe,
+ 0x00,
+ 0xfe,
+ 0x01,
+ 0xff,
+};
static struct usb_device_id id_table [] = {
{ USB_DEVICE(0x0525, 0x127a) },
@@ -39,6 +50,32 @@ static int usb_debug_open(struct tty_str
return usb_serial_generic_open(tty, port, filp);
}
+/* This HW really does not support a serial break, so one will be
+ * emulated when ever the break state is set to true.
+ */
+static void usb_debug_break_ctl(struct tty_struct *tty, int break_state)
+{
+ struct usb_serial_port *port = tty->driver_data;
+ if (!break_state)
+ return;
+ usb_serial_generic_write(tty, port, USB_DEBUG_BRK, USB_DEBUG_BRK_SIZE);
+}
+
+static void usb_debug_read_bulk_callback(struct urb *urb)
+{
+ struct usb_serial_port *port = urb->context;
+
+ if (urb->actual_length == USB_DEBUG_BRK_SIZE &&
+ memcmp(urb->transfer_buffer, USB_DEBUG_BRK,
+ USB_DEBUG_BRK_SIZE) == 0) {
+ usb_serial_handle_break(port);
+ usb_serial_generic_resubmit_read_urb(port, GFP_ATOMIC);
+ return;
+ }
+
+ usb_serial_generic_read_bulk_callback(urb);
+}
+
static struct usb_serial_driver debug_device = {
.driver = {
.owner = THIS_MODULE,
@@ -48,6 +85,8 @@ static struct usb_serial_driver debug_de
.num_ports = 1,
.open = usb_debug_open,
.max_in_flight_urbs = URB_DEBUG_MAX_IN_FLIGHT_URBS,
+ .break_ctl = usb_debug_break_ctl,
+ .read_bulk_callback = usb_debug_read_bulk_callback,
};
static int __init debug_init(void)
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -15,6 +15,7 @@
#include <linux/kref.h>
#include <linux/mutex.h>
+#include <linux/sysrq.h>
#define SERIAL_TTY_MAJOR 188 /* Nice legal number now */
#define SERIAL_TTY_MINORS 254 /* loads of devices :) */
@@ -99,6 +100,7 @@ struct usb_serial_port {
char throttled;
char throttle_req;
char console;
+ unsigned long sysrq; /* sysrq timeout */
struct device dev;
};
#define to_usb_serial_port(d) container_of(d, struct usb_serial_port, dev)
@@ -301,6 +303,12 @@ extern void usb_serial_generic_unthrottl
extern void usb_serial_generic_shutdown(struct usb_serial *serial);
extern int usb_serial_generic_register(int debug);
extern void usb_serial_generic_deregister(void);
+extern void usb_serial_generic_resubmit_read_urb(struct usb_serial_port *port,
+ gfp_t mem_flags);
+extern int usb_serial_handle_sysrq_char(struct usb_serial_port *port,
+ unsigned int ch);
+extern int usb_serial_handle_break(struct usb_serial_port *port);
+
extern int usb_serial_bus_register(struct usb_serial_driver *device);
extern void usb_serial_bus_deregister(struct usb_serial_driver *device);
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
