On Wed, Mar 10, 2010 at 11:21:23PM +0100, ext Alan Cox wrote:
> we experienced several problems with tty layer. The biggest of them is
> a race between flush_to_ldisc() and ->receiv_buf() which makes the line
> discipline (I guess only n_tty) loose bytes in some circumstances.
That sounds like a bug we need to fix then, right?
It sounds like a bug that someone needs to provide a test case and
explanation for. The underlying buffering logic appears solid up to about
40MB/sec (beyond that the existing buffer settings and round trip time
for flow control would need addressing and possibly some data handling).
I remember talking to you about that Alan. I could reproduce easily the
problem with f_obex.c function driver (with g_serial use_acm=0
usb_obex=1) plus the opensource obexd. It was really difficult to
actually figure out why obexd was getting stuck on poll().
Then we came to the conclusion that flush_to_ldisc() doesn't play that
nicely when ldisc has an almost full buffer (btw, I was using n_tty
ldisc). You see, the only check we have for how many bytes ldisc can
actually receive is through tty->receive_room, which for some reason (we
couldn't figure that out) isn't always up-to-date.
flush_to_ldisc() believes it can always flush everything to ldisc based
on ->receive_room only, and that's where we were loosing a few bytes and
obexd getting stuck on poll() forever waiting for the lost bytes.
What we did (as a hack, but I want to finish the patch and send as RFC)
is make ->receive_buf() return the amount of bytes received. Then we can
get rid of ->receive_room.
Here's the hacked up version of the patch for reference, apply on
today's Mainline:
diff --git a/drivers/char/n_tty.c b/drivers/char/n_tty.c
index 2e50f4d..06f7d78 100644
--- a/drivers/char/n_tty.c
+++ b/drivers/char/n_tty.c
@@ -1348,17 +1348,18 @@ static void n_tty_write_wakeup(struct tty_struct *tty)
* calls one at a time and in order (or using flush_to_ldisc)
*/
-static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
- char *fp, int count)
+static unsigned int n_tty_receive_buf(struct tty_struct *tty,
+ const unsigned char *cp, char *fp, int count)
{
const unsigned char *p;
char *f, flags = TTY_NORMAL;
int i;
char buf[64];
unsigned long cpuflags;
+ int ret = 0;
if (!tty->read_buf)
- return;
+ return 0;
if (tty->real_raw) {
spin_lock_irqsave(&tty->read_lock, cpuflags);
@@ -1370,6 +1371,7 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
tty->read_cnt += i;
cp += i;
count -= i;
+ ret += i;
i = min(N_TTY_BUF_SIZE - tty->read_cnt,
N_TTY_BUF_SIZE - tty->read_head);
@@ -1377,8 +1379,10 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
memcpy(tty->read_buf + tty->read_head, cp, i);
tty->read_head = (tty->read_head + i) & (N_TTY_BUF_SIZE-1);
tty->read_cnt += i;
+ ret += i;
spin_unlock_irqrestore(&tty->read_lock, cpuflags);
} else {
+ ret = count;
for (i = count, p = cp, f = fp; i; i--, p++) {
if (f)
flags = *f++;
@@ -1421,6 +1425,8 @@ static void n_tty_receive_buf(struct tty_struct *tty, const unsigned char *cp,
*/
if (tty->receive_room < TTY_THRESHOLD_THROTTLE)
tty_throttle(tty);
+
+ return ret;
}
int is_ignored(int sig)
diff --git a/drivers/char/tty_buffer.c b/drivers/char/tty_buffer.c
index af8d977..eae2033 100644
--- a/drivers/char/tty_buffer.c
+++ b/drivers/char/tty_buffer.c
@@ -58,7 +58,7 @@ static struct tty_buffer *tty_buffer_alloc(struct tty_struct *tty, size_t size)
{
struct tty_buffer *p;
- if (tty->buf.memory_used + size > 65536)
+ if (tty->buf.memory_used + size > 64 * 1024)
return NULL;
p = kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
if (p == NULL)
@@ -415,6 +415,7 @@ static void flush_to_ldisc(struct work_struct *work)
if (!test_and_set_bit(TTY_FLUSHING, &tty->flags)) {
struct tty_buffer *head;
while ((head = tty->buf.head) != NULL) {
+ int copied;
int count;
char *char_buf;
unsigned char *flag_buf;
@@ -440,11 +441,11 @@ static void flush_to_ldisc(struct work_struct *work)
count = tty->receive_room;
char_buf = head->char_buf_ptr + head->read;
flag_buf = head->flag_buf_ptr + head->read;
- head->read += count;
spin_unlock_irqrestore(&tty->buf.lock, flags);
- disc->ops->receive_buf(tty, char_buf,
+ copied = disc->ops->receive_buf(tty, char_buf,
flag_buf, count);
spin_lock_irqsave(&tty->buf.lock, flags);
+ head->read += copied;
}
clear_bit(TTY_FLUSHING, &tty->flags);
}
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 0c4ee9b..7bad16e 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -76,7 +76,7 @@
* tty device. It is solely the responsibility of the line
* discipline to handle poll requests.
*
- * void (*receive_buf)(struct tty_struct *, const unsigned char *cp,
+ * unsigned int (*receive_buf)(struct tty_struct *, const unsigned char *cp,
* char *fp, int count);
*
* This function is called by the low-level tty driver to send
@@ -133,8 +133,8 @@ struct tty_ldisc_ops {
/*
* The following routines are called from below.
*/
- void (*receive_buf)(struct tty_struct *, const unsigned char *cp,
- char *fp, int count);
+ unsigned int (*receive_buf)(struct tty_struct *,
+ const unsigned char *cp, char *fp, int count);
void (*write_wakeup)(struct tty_struct *);
struct module *owner;
It's not perfect, but it help us a lot. I still don't know why the
receive_room approach was taken if it would be a lot simpler (at
least IMO)to let receive_buf() return the amount of received bytes.
--
balbi
--
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
