[PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[David VomLehn]

When handling output of a newline character in O_ONLCR mode, the n_tty.c
do_output_char() function uses the struct tty_operations function write_room()
to determine whether it is possible to write two characters. It uses
tty_put_char() for each character which, for the USB generic serial driver,
translates into a write() for each character. For the USB generic serial
driver the value returned by write_room() only applies to the next write().
A second write() done in quick succession will fail because the write URB
buffer is still busy from the first write(). In this case, it results in the
printing of a carriage return without the following line feed.

This patch combines the two write() calls into a single, two-character, write().

Signed-off-by: David VomLehn
---
 drivers/char/n_tty.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/n_tty.c b/drivers/char/n_tty.c
index 973be2f..bf06c61 100644
--- a/drivers/char/n_tty.c
+++ b/drivers/char/n_tty.c
@@ -297,12 +297,12 @@ static int do_output_char(unsigned char c, struct tty_struct *tty, int space)
 		if (O_ONLRET(tty))
 			tty->column = 0;
 		if (O_ONLCR(tty)) {
-			if (space < 2)
+			static const char crlf[] = {'\r', '\n'};
+			if (space < ARRAY_SIZE(crlf))
 				return -1;
 			tty->canon_column = tty->column = 0;
-			tty_put_char(tty, '\r');
-			tty_put_char(tty, c);
-			return 2;
+			tty->ops->write(tty, crlf, ARRAY_SIZE(crlf));
+			return ARRAY_SIZE(crlf);
 		}
 		tty->canon_column = tty->column;
 		break;

Re: [PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[Greg KH]

On Thu, Aug 13, 2009 at 10:54:30AM -0700, David VomLehn wrote:
> When handling output of a newline character in O_ONLCR mode, the n_tty.c
> do_output_char() function uses the struct tty_operations function write_room()
> to determine whether it is possible to write two characters. It uses
> tty_put_char() for each character which, for the USB generic serial driver,
> translates into a write() for each character. For the USB generic serial
> driver the value returned by write_room() only applies to the next write().
> A second write() done in quick succession will fail because the write URB
> buffer is still busy from the first write(). In this case, it results in the
> printing of a carriage return without the following line feed.

How about fixing the usb-serial generic driver to properly handle stuff
like this instead?  It should be using a fifo and not the stupid method
it currently is.

Anyway, what device are you using that uses the usb-serial generic
driver that you are seeing this problem with?

thanks,

greg k-h

Re: [PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[David VomLehn]

On Thu, Aug 13, 2009 at 11:15:21AM -0700, Greg KH wrote:
> On Thu, Aug 13, 2009 at 10:54:30AM -0700, David VomLehn wrote:
> > When handling output of a newline character in O_ONLCR mode, the n_tty.c
> > do_output_char() function uses the struct tty_operations function write_room()
> > to determine whether it is possible to write two characters. It uses
> > tty_put_char() for each character which, for the USB generic serial driver,
> > translates into a write() for each character. For the USB generic serial
> > driver the value returned by write_room() only applies to the next write().
> > A second write() done in quick succession will fail because the write URB
> > buffer is still busy from the first write(). In this case, it results in the
> > printing of a carriage return without the following line feed.
> 
> How about fixing the usb-serial generic driver to properly handle stuff
> like this instead?  It should be using a fifo and not the stupid method
> it currently is.

Good question. Right now, there are a number of USB serial devices that don't
use the generic USB serial framework, but they generally seem to use the
write_urb_busy flag the same way the generic code does. This makes it easier
to mix and match generic and non-generic code. Going to a FIFO approach and
doing away with the write_urb_busy flag seemed like a lot of change for very
little benefit. In addition, I would argue that doing one two-character write()
of two one-character writes() is a more logical way to output two characters,
anyway.

I have another motivation for maintaining the write_urb_busy flag as part of
the standard USB generic serial environment: I'm finishing up an RFC patchset
that provides for a polled USB console. This way you won't need any of the
buffering some of the USB serial drivers use, for which overruns can still
occur and, which may very well not have written the output by the time they
return. In other words USB consoles will have the same synchronous output
behavior as dumb UART consoles.  I have it working with EHCI and am looking
at OHCI.

> Anyway, what device are you using that uses the usb-serial generic
> driver that you are seeing this problem with?

I'm using a cp210x work-alike.

> greg k-h

David VL

Re: [PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[Alan Cox]

> Good question. Right now, there are a number of USB serial devices that don't
> use the generic USB serial framework, but they generally seem to use the
> write_urb_busy flag the same way the generic code does. This makes it easier

They all suck, they all misimplement the interfaces. The proper way to do
this is to use a FIFO (kfifo makes it tiny amounts of code) and then
either fire off a new URB whenever one completes or if you are
constrained on urb counts but have over one implement Nagle on it.

> to mix and match generic and non-generic code. Going to a FIFO approach and
> doing away with the write_urb_busy flag seemed like a lot of change for very
> little benefit. In addition, I would argue that doing one two-character write()
> of two one-character writes() is a more logical way to output two characters,
> anyway.

The tty layer requires you can provide some buffering and that your
write_room function doesn't reduce by more than the bytes offered without
them being written. The sane way to do that is to use the kfifo API.

The ONLCR stuff is just a corner case example of where the USB serial
code breaks so the real bug wants fixing.

Re: [PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[Alan Cox]

> do_output_char() function uses the struct tty_operations function write_room()
> to determine whether it is possible to write two characters. It uses
> tty_put_char() for each character which, for the USB generic serial driver,
> translates into a write() for each character. For the USB generic serial
> driver the value returned by write_room() only applies to the next write().

The USB serial driver is broken, fix it instead. It's not even hard to
fix.

> +			tty->ops->write(tty, crlf, ARRAY_SIZE(crlf));
> +			return ARRAY_SIZE(crlf);

Emphatic NAK. Someone needs to actually fix the broken USB drivers for
once. This is just one example of where the device will go kersplat if its
broken, others include things like losing flow control.
 

Re: [PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[David VomLehn]

On Thu, Aug 13, 2009 at 08:50:18PM +0100, Alan Cox wrote:
> > do_output_char() function uses the struct tty_operations function write_room()
> > to determine whether it is possible to write two characters. It uses
> > tty_put_char() for each character which, for the USB generic serial driver,
> > translates into a write() for each character. For the USB generic serial
> > driver the value returned by write_room() only applies to the next write().
> 
> The USB serial driver is broken, fix it instead. It's not even hard to
> fix.
> 
> > +			tty->ops->write(tty, crlf, ARRAY_SIZE(crlf));
> > +			return ARRAY_SIZE(crlf);
> 
> Emphatic NAK. Someone needs to actually fix the broken USB drivers for
> once. This is just one example of where the device will go kersplat if its
> broken, others include things like losing flow control.

Works for me; please consider the patch withdrawn. I'll take a crack at fixing
the generic serial driver the right way; if other drivers are doing mix-and-match
with per-device code and generic code, they could get broken. I have no way to
test for this, so I'm hoping others will help out.

David VL

Re: [PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[Alan Cox]

> Works for me; please consider the patch withdrawn. I'll take a crack at fixing
> the generic serial driver the right way; if other drivers are doing mix-and-match
> with per-device code and generic code, they could get broken. I have no way to
> test for this, so I'm hoping others will help out.

I'll be happy to help test some of them and review the code. It was
always on the todo list to switch then to kfifo_*. 

Alan

Re: [PATCH] Combine two one-character CR-LF writes into one two-character write for O_ONLCR

[Greg KH]

On Thu, Aug 13, 2009 at 12:55:54PM -0700, David VomLehn wrote:
> On Thu, Aug 13, 2009 at 08:50:18PM +0100, Alan Cox wrote:
> > > do_output_char() function uses the struct tty_operations function write_room()
> > > to determine whether it is possible to write two characters. It uses
> > > tty_put_char() for each character which, for the USB generic serial driver,
> > > translates into a write() for each character. For the USB generic serial
> > > driver the value returned by write_room() only applies to the next write().
> > 
> > The USB serial driver is broken, fix it instead. It's not even hard to
> > fix.
> > 
> > > +			tty->ops->write(tty, crlf, ARRAY_SIZE(crlf));
> > > +			return ARRAY_SIZE(crlf);
> > 
> > Emphatic NAK. Someone needs to actually fix the broken USB drivers for
> > once. This is just one example of where the device will go kersplat if its
> > broken, others include things like losing flow control.
> 
> Works for me; please consider the patch withdrawn. I'll take a crack at fixing
> the generic serial driver the right way; if other drivers are doing mix-and-match
> with per-device code and generic code, they could get broken. I have no way to
> test for this, so I'm hoping others will help out.

I have a whole box of different usb-serial devices, and can test just
about everything out.

thanks,

greg k-h