Hello.Indeed there is a recursive loop that I have witnessed whilst using SLIP over USB HID.
This patch removes the direct call to tty_wakeup in pty_write. I have
not noticed any drawbacks with this but I am not familiar with the pty
driver at all. I think what happens is a recursive loop,
write_wakeup->write->write_wakeup ...
The documentation for the tty interface forbids this direct call:I also saw that documentation and the code seems to be breaking that description.
(from Documentation/serial/tty.txt)
write_wakeup() - May be called at any point between open and close.
The TTY_DO_WRITE_WAKEUP flag indicates if a call
is needed but always races versus calls. Thus the
ldisc must be careful about setting order and to
handle unexpected calls. Must not sleep.
The driver is forbidden from calling this directly
from the ->write call from the ldisc as the ldisc
is permitted to call the driver write method from
this function. In such a situation defer it.
I agree that this looks to be a simple remedy to the issue. However, this code has existed in this state for many years so there is a risk that some applications rely on this "feature" in order to work. For example, SLIP uses this "feature" although I am not certain that the existing SLIP code would break if your patch were applied. The TTY wakeup event should take care of completing the transmission (in theory).
The direct call caused a reproducable kernel panic (see bottom of this
mail) for me with the following setup:
- using can-utils from git://gitorious.org/linux-can/can-utils.git
slcan_attach and cangen are used
- create a network link between two serial CAN interfaces with:
$ socat PTY,link=/tmp/slcan0,raw TCP4-LISTEN:50000 &
$ socat TCP4:localhost:50000 PTY,link=/tmp/slcan1,raw &
$ slcan_attach /tmp/slcan0
$ slcan_attach /tmp/slcan1
$ ip link set slcan0 up
$ ip link set slcan1 up
- produce a kernel panic by overloading the CAN interfaces:
$ cangen slcan0 -g0
Please keep me in CC. I am not subscribed to the list.
If I can provide any more information, I will be glad to do so.
This is the patch. It applies to the current linux master branch:
From 9f67139bebb938026406a66c1411e0b50628a238 Mon Sep 17 00:00:00 2001
From: Andre Naujoks <nautsch2@xxxxxxxxxxxxxx>
Date: Mon, 1 Jul 2013 15:45:13 +0200
Subject: [PATCH 1/2] remove direct call to tty_wakeup in pty_write.
Signed-off-by: Andre Naujoks <nautsch2@xxxxxxxxxxxxxx>
---
drivers/tty/pty.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index abfd990..5dcb782 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -127,7 +127,6 @@ static int pty_write(struct tty_struct *tty, const
unsigned char *buf, int c)
/* And shovel */
if (c) {
tty_flip_buffer_push(to->port);
- tty_wakeup(tty);
}
}
return c;