On Tue, Jun 16, 2009 at 07:06:33AM -0500, Jason Wessel wrote:
> 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.
Ah, sorry, I thought it would be helpful for other drivers to be able to
call this. Inline vs. non-inline shouldn't be the issue here, it's an
overall "let's not touch every individual byte" issue :)
> 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?
No, I think other drivers still need to call this, right?
thanks,
greg k-h
--
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
