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-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
