[BUG/PATCH] : bug in tty_default_put_char()

[BUG/PATCH] : bug in tty_default_put_char()

[Jean Tourrilhes]

	Hi,

	Bug : tty_default_put_char() doesn't check the return value of
tty->driver.write(). However, the later may fail if buffers are full.

	Solution : It's not obvious what should be done. The attached
patch is certainly wrong, but gives you an idea of what the problem
is.

	Long story :
	User weant to do PPP over IrCOMM. "chat" opens ircomm device
in non blocking mode and write char by char. I suspect that it's using
the above call, but can't verify (because it doesn't happen to me). As
IrCOMM has not finished its initialisation (the open was
non-blocking), it refuses the write and returns 0.
	Character dropped, user unhappy, bugs me about it.
	I'll try to workaround that in IrCOMM.

	Regards,

	Jean

-----------------------------------------------
diff -u -p linux/drivers/char/tty_io.t1.c linux/drivers/char/tty_io.c
--- linux/drivers/char/tty_io.t1.c      Mon Aug 26 10:55:33 2002
+++ linux/drivers/char/tty_io.c Mon Aug 26 10:58:34 2002
@@ -2021,7 +2021,11 @@ static void initialize_tty_struct(struct
  */
 void tty_default_put_char(struct tty_struct *tty, unsigned char ch)
 {
-       tty->driver.write(tty, 0, &ch, 1);
+       int ret = tty->driver.write(tty, 0, &ch, 1);
+       /* This might fail if the lower layer is already full - Jean II */
+       if (ret == 0)
+               printk(KERN_WARNING "Warning: dev (%s) put_char failed\n",
+                      kdevname(tty->device));
 }
 
 /*

Re: [BUG/PATCH] : bug in tty_default_put_char()

[Jean Tourrilhes]

On Mon, Aug 26, 2002 at 07:57:04PM +0100, Alan Cox wrote:
> On Mon, 2002-08-26 at 19:07, Jean Tourrilhes wrote:
> > 	Hi,
> > 
> > 	Bug : tty_default_put_char() doesn't check the return value of
> > tty->driver.write(). However, the later may fail if buffers are full.
> > 
> > 	Solution : It's not obvious what should be done. The attached
> > patch is certainly wrong, but gives you an idea of what the problem
> > is.
> 
> Make it an int and return the tty->driver.write value. We know that
> since its void nobody is currently checking the error code, and now they
> can where it matters

	Just check drivers/char/n_tty.c for every occurence of
put_char() and be scared. The problem is to find a practical solution.
	For myself, I've added some clever workaround in IrCOMM to
accept data before full setup.

	Regards,

	Jean

Re: [BUG/PATCH] : bug in tty_default_put_char()

[Alan Cox]

On Mon, 2002-08-26 at 19:59, Jean Tourrilhes wrote:
> 	Just check drivers/char/n_tty.c for every occurence of
> put_char() and be scared. The problem is to find a practical solution.
> 	For myself, I've added some clever workaround in IrCOMM to
> accept data before full setup.

Sure making it return the right errors doesnt fix anything, but it
allows you to fix some of it bit by bit. 

Re: [BUG/PATCH] : bug in tty_default_put_char()

[Russell King]

On Mon, Aug 26, 2002 at 08:07:27PM +0100, Alan Cox wrote:
> On Mon, 2002-08-26 at 19:59, Jean Tourrilhes wrote:
> > 	Just check drivers/char/n_tty.c for every occurence of
> > put_char() and be scared. The problem is to find a practical solution.
> > 	For myself, I've added some clever workaround in IrCOMM to
> > accept data before full setup.
> 
> Sure making it return the right errors doesnt fix anything, but it
> allows you to fix some of it bit by bit. 

put_char() is not allowed to fail since the caller should have already
checked for buffer space via the write_room() method.

All places look adequately protected in n_tty.c, so I'm not currently
sure how Jean's users are seeing this condition; I'd need to have a
BUG() showing the call trace of such an event happening.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [BUG/PATCH] : bug in tty_default_put_char()

[Russell King]

On Mon, Aug 26, 2002 at 11:07:49AM -0700, Jean Tourrilhes wrote:
> 	Bug : tty_default_put_char() doesn't check the return value of
> tty->driver.write(). However, the later may fail if buffers are full.

Hmm.

> 	Solution : It's not obvious what should be done. The attached
> patch is certainly wrong, but gives you an idea of what the problem
> is.

Every invocation of the put_char() method should be preceded by a check
to ensure that there is sufficient space in the drivers buffer (via the
drivers write_room() call.)  Could you add a BUG() on this condition
and get some call traces please?

> 	I'll try to workaround that in IrCOMM.

I don't think that should be necessary.  The tty layer is obviously
doing something it shouldn't (trying to stuff characters into a buffer
of zero size) so it should get fixed.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [BUG/PATCH] : bug in tty_default_put_char()

[Jean Tourrilhes]

On Mon, Aug 26, 2002 at 09:22:23PM +0100, Russell King wrote:
> On Mon, Aug 26, 2002 at 01:17:32PM -0700, Jean Tourrilhes wrote:
> > 	It's possible that it's a bit more complex than that, but the
> > TTY stuff is a bit magic to me.
> 
> The patch appears to be in 2.5.31.
> 
> Ok, do you want me to cook up a patch to make ircomm bug() when put_char
> gets called when write_room would have returned 0 ?
> 
> Is it possible for the user who experienced the problem to try out such
> a patch?

	Hi,

	After sending you bogus path and bogus bug reports, I though
it would be time to send you a *real* patch.
	Patch below allow to set "uart=none", which is necessary for
FIR hardware. Patch for 2.5.31.

----------------------------------------------
diff -u -p linux/drivers/serial/core.r1.c linux/drivers/serial/core.c
--- linux/drivers/serial/core.r1.c      Wed Aug 28 11:54:20 2002
+++ linux/drivers/serial/core.c Wed Aug 28 11:58:21 2002
@@ -730,12 +730,17 @@ uart_set_info(struct uart_info *info, st
                 */
                if (port->type != PORT_UNKNOWN)
                        retval = port->ops->request_port(port);
+               else
+                       /* If we set the uart to unknown, we should *always*
+                        * succeed at the test below. - Jean II */
+                       retval = 0;
 
                /*
                 * If we fail to request resources for the
                 * new port, try to restore the old settings.
                 */
                if (retval && old_type != PORT_UNKNOWN) {
+                       DPRINTK("uart_set_info(%d): could not set new serial configuration, reverting to old one\n", MINOR(tty->device) - tty->driver.minor_start);
                        port->iobase = old_iobase;
                        port->type = old_type;
                        port->hub6 = old_hub6;
----------------------------------------------

	Regarding the initial bug I reported.
	The write is comming from the PPP line discipline. If PPP
can't transmit the data, it just drops it and assume higher layers
will retry. This is true for TCP, but not for "chat".
	I implemented the workaround, and the user report that it has
fixed his problem.
	I've also found a bunch of other bugs related to that in the
process, but I'll spare you the details.

	Have fun...

	Jean

Re: [BUG/PATCH] : bug in tty_default_put_char()

[Russell King]

On Wed, Aug 28, 2002 at 03:41:03PM -0700, Jean Tourrilhes wrote:
> 	Patch below allow to set "uart=none", which is necessary for
> FIR hardware. Patch for 2.5.31.

Ok, thanks.

> 	The write is comming from the PPP line discipline. If PPP
> can't transmit the data, it just drops it and assume higher layers
> will retry. This is true for TCP, but not for "chat".

Last time I read the pppd code, chat doesn't talk via the ppp code, but
via the serial device itself.  I'm still confused about this report,
since it could mean something is very wrong somewhere and not knowing
where is really bugging me.  I really don't like sleeping problems that
come back to bite later.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html