Re: TTY loosing data with u_serial gadget

[Felipe Balbi]

On Thu, Mar 24, 2011 at 12:51:45PM +0000, Toby Gray wrote:
> On 24/03/2011 12:37, Felipe Balbi wrote:
> >Hi,
> >
> >On Thu, Mar 24, 2011 at 12:07:56PM +0000, Toby Gray wrote:
> >>Is this patch missing the changes to tty_buffer.c that were in
> >>http://permalink.gmane.org/gmane.linux.usb.general/28976
> >>
> >>Without the changes to tty_buffer.c to use the value returned from
> >>the receive_buf call then doesn't this patch not work correctly?
> >>
> <snip>
> >you are right, thanks for noticing that. Attached is an updated patch.
> >I left removal of receive_room out of this patch to prevent too invasive
> >change, that can be done on a separate patch.
> 
> I've just tried removing receive_room myself and noticed that it is
> still used in flush_to_ldisc to decide if it needs to schedule work
> to be done later:
> 
>    if (!tty->receive_room || seen_tail) {
>       schedule_work(&tty->buf.work);
>       break;
>    }
> 
> If receive_room is no longer being updated then isn't this the wrong
> thing to do? Shouldn't it check if some data was copied after calling
> receive_buf, and if there wasn't any then it should schedule the work
> and break?
> 
> The other query I've got is about the return value from receive_buf.
> I noticed that you've modified some drivers (such as
> bluetooth/hci_ldisc.c) so that they can return error values, such as
> -ENODEV. Won't this cause things to go wrong when flush_to_ldisc and
> paste_selection use the return value without checking for it being
> negative?
> 
> Thank you for your quick reply to my first query, it's appreciated.

Can you try this:

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 2f119e2..f0b9fb6 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -443,17 +443,19 @@ static void flush_to_ldisc(struct work_struct *work)
                           line discipline as we want to empty the queue */
                        if (test_bit(TTY_FLUSHPENDING, &tty->flags))
                                break;
-                       if (!tty->receive_room || seen_tail) {
-                               schedule_delayed_work(&tty->buf.work, 1);
-                               break;
-                       }
                        char_buf = head->char_buf_ptr + head->read;
                        flag_buf = head->flag_buf_ptr + head->read;
                        spin_unlock_irqrestore(&tty->buf.lock, flags);
                        copied = disc->ops->receive_buf(tty, char_buf,
                                                        flag_buf, count);
                        spin_lock_irqsave(&tty->buf.lock, flags);
+
                        head->read += copied;
+
+                       if (copied == 0 || seen_tail) {
+                               schedule_delayed_work(&tty->buf.work, 1);
+                               break;
+                       }
                }
                clear_bit(TTY_FLUSHING, &tty->flags);
        }


if I read the code correctly, when we have no space to receive bytes,
then we schedule work, that should be the same.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/