On Tue, Jun 16, 2009 at 12:19:26PM -0500, Jason Wessel wrote:
> Greg KH wrote:
> >> 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?
> >
>
> Yes other drivers could need to call the sysrq checks directly, but it
> is fine to convert it back to a static inline, there are no functions
> called there which are not already exported in the kernel's export
> function table (so we can still build external usb serial kernel
> modules).
>
> The original looked something like the following and was directly
> derived from the serial uart equivalent.
>
> static inline int
> usb_serial_handle_sysrq_char(struct usb_serial_port *port, unsigned int ch)
> {
> #ifdef CONFIG_SYSRQ
> 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;
> }
> #endif /* CONFIG_SYSRQ */
> return 0;
> }
>
>
> Alan correctly pointed out that we don't even want to look at each
> byte in the first place, if it is not a console device, so we
> definitely need that proposed change. The in-lining is a further
> optimization. If this is what we want, I will generate a new patch.
Ok, a new add-on patch sounds like the right way to go here, thanks for
sorting it all out.
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
