"USB: use kfifo to buffer usb-generic serial writes" causes gobi_loader to hang
in [Kernel]
|
Prev: [REGRESSION] "USB: use kfifo to buffer usb-generic serial writes" causes gobi_loader to hang
Next: "USB: use kfifo to buffer usb-generic serial writes" causes gobi_loader to hang
From: Johan Hovold on 19 Jan 2010 11:00 > > > The log shows no call to usb_serial_generic_write_room() > > > Do you consider this a bug in the tty layer? > > > > Actually this all makes sense because of where it was hanging. A reply of > > 0 to the tty->ops->write will cause it to either return (O_NONBLOCK) or > > sleep in the n_tty write code waiting for a write_wait wakeup > > (tty_wakeup(tty)) > > > > So the fix does indeed look correct. > > Is it really a fix? If the fifo is already full the write urb should be > in use and Oliver's patch would amount to only a minor optimisation as > usb_serial_generic_write_start would return 0 anyway. So the question seems to be: why doesn't the on-going transfer finish (so that tty_wakeup is called)? > drivers/usb/serial/usb-serial.c: usb_serial_port_work - port 0 > drivers/usb/serial/usb-serial.c: serial_write - port 0, 29 byte(s) > drivers/usb/serial/generic.c: usb_serial_generic_write - port 0, 29 > bytes > drivers/usb/serial/generic.c: usb_serial_generic_write - put 0 bytes > into fifo Writer got woken but fifo is still full so write returns 0 and writer goes back to sleep. > drivers/usb/serial/generic.c: usb_serial_generic_write_bulk_callback - > port 0 > drivers/usb/serial/generic.c: usb_serial_generic_write_start - starting > IO > qcserial ttyUSB0: usb_serial_generic_write_start - length = 512, data = > f0 f8 0a 01 f0 00 0b 01 f0 0c 10 01 f0 38 10 01 f0 0f 22 a0 e3 04 40 92 > e5 1c 30 94 e5 a3 32 a0 e1 7f 3f c3 e3 ff 33 c3 e3 0f 35 c3 e3 0e 12 83 > e2 08 00 91 e5 00 00 50 e3 0a 00 00 0a 0a 20 d0 e5 02 00 52 e3 0d 00 00 > 0a 60 31 91 e5 0a 30 c0 e5 64 21 91 e5 0b 20 c0 e5 38 10 81 e2 44 30 91 > e5 c0 30 c3 e3 44 30 81 e5 04 00 a0 e1 96 fc ff eb 50 31 94 e5 fe df 8d > e3 03 f0 a0 e1 fe ff ff ea 08 30 94 e5 0a 20 c3 e5 38 20 84 e2 44 30 92 > e5 80 30 83 e3 44 30 82 e5 ea ff ff ea 02 60 a0 e1 01 50 a0 e1 04 d0 4d > e2 00 70 a0 e1 03 80 a0 e1 4d ff ff eb a5 2f a0 e1 86 30 82 e1 00 00 a0 > e3 2f 20 e0 e3 00 00 8d e5 b2 20 cd e1 00 10 9d e5 03 19 81 e3 00 10 8d > e5 a6 4f a0 e1 a3 20 a0 e1 84 3f 82 e1 08 20 97 e5 48 30 82 e5 0f 32 a0 > e3 40 10 82 e5 0c 00 82 e5 44 50 82 e5 04 10 93 e5 2c 30 91 e5 e4 00 97 > e5 20 20 9f e5 01 30 83 e3 2c 30 81 e5 80 20 87 e5 50 81 87 e5 10 30 9f > e5 00 10 a0 e1 0c e0 9f e5 03 f0 a0 e1 fe ff ff ea 20 81 00 f0 8c 89 00 > f0 10 b6 00 f0 0f 32 a0 e3 04 40 93 e5 08 20 94 e5 40 30 92 e5 04 d0 4d > e2 00 30 8d e5 04 00 a0 e1 5b fc ff eb d1 30 dd e1 00 00 53 e3 03 00 00 > ba 50 31 94 e5 fe df 8d e3 03 f0 a0 e1 fe ff ff ea 04 00 9f e5 73 05 00 > eb f8 ff ff ea 4c 10 01 f0 0f c2 a0 e3 18 40 dc e5 18 d0 4d e2 00 00 54 > e3 0e 60 a0 e1 14 00 8d e5 01 a0 a0 e1 10 20 8d e5 03 90 a0 e1 20 b0 9d > e5 51 01 00 1a 0f 32 a0 e3 04 80 93 e5 50 26 9f e5 0c 30 98 e5 00 10 92 > e5 01 00 53 e1 00 00 a0 13 01 00 a0 03 00 00 50 e3 41 01 00 0a 14 50 9d > e5 01 20 75 e2 00 20 a0 33 00 00 52 e3 0d 00 00 1a b4 31 dd e1 3f 00 13 > e3 02 30 a0 e1 03 00 00 New transfer started from completion handler. > drivers/usb/serial/usb-serial.c: usb_serial_port_work - port 0 Writer woken up from completion handler after having started the next transfer. > drivers/usb/serial/usb-serial.c: serial_write - port 0, 29 byte(s) > drivers/usb/serial/generic.c: usb_serial_generic_write - port 0, 29 > bytes > drivers/usb/serial/generic.c: usb_serial_generic_write - put 29 bytes > into fifo > drivers/usb/serial/usb-serial.c: serial_write - port 0, 2048 byte(s) > drivers/usb/serial/generic.c: usb_serial_generic_write - port 0, 2048 > bytes > drivers/usb/serial/generic.c: usb_serial_generic_write - put 483 bytes > into fifo > drivers/usb/serial/usb-serial.c: serial_write - port 0, 1565 byte(s) > drivers/usb/serial/generic.c: usb_serial_generic_write - port 0, 1565 > bytes > drivers/usb/serial/generic.c: usb_serial_generic_write - put 0 bytes > into fifo Now there was some room (29 + 483 = 512 bytes bytes just transfered). > drivers/usb/serial/generic.c: usb_serial_generic_read_bulk_callback - > port 0 Waiting forever for on-going transfer to finish... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Cox on 19 Jan 2010 11:30 On Tue, 19 Jan 2010 16:25:36 +0100 Johan Hovold <jhovold(a)gmail.com> wrote: > > > The log shows no call to usb_serial_generic_write_room() > > > Do you consider this a bug in the tty layer? > > > > Actually this all makes sense because of where it was hanging. A reply of > > 0 to the tty->ops->write will cause it to either return (O_NONBLOCK) or > > sleep in the n_tty write code waiting for a write_wait wakeup > > (tty_wakeup(tty)) > > > > So the fix does indeed look correct. > > Is it really a fix? If the fifo is already full the write urb should be > in use and Oliver's patch would amount to only a minor optimisation as > usb_serial_generic_write_start would return 0 anyway. IF the write returns a zero then it will sleep in n_tty waiting for a wakeup when the FIFO level drops sufficiently. If that isn't working check that all cases where data is cleared from the FIFO called tty_wakeup and do so *after* the FIFO has been partly emptied and the locking has ensured the space is visible to the write side. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Johan Hovold on 19 Jan 2010 14:50 On Tue, Jan 19, 2010 at 07:44:54PM +0100, Oliver Neukum wrote: > Am Dienstag, 19. Januar 2010 17:27:46 schrieb Alan Cox: > > On Tue, 19 Jan 2010 16:25:36 +0100 > > Johan Hovold <jhovold(a)gmail.com> wrote: > > > > Is it really a fix? If the fifo is already full the write urb should be > > > in use and Oliver's patch would amount to only a minor optimisation as > > > usb_serial_generic_write_start would return 0 anyway. > > > > IF the write returns a zero then it will sleep in n_tty waiting for a > > wakeup when the FIFO level drops sufficiently. If that isn't working > > check that all cases where data is cleared from the FIFO called > > tty_wakeup and do so *after* the FIFO has been partly emptied and the > > locking has ensured the space is visible to the write side. > > > usb_serial_generic_write_bulk_callback() always calls the softint > in the single URB case. Therefore the test with the patch I sent. > It is unlikely to be chance that the hang happens just as the FIFO > is full. But that is the correct behaviour: for every kfifo_get, exactly one urb is submitted and on completion softint (tty_wake) is called. I guess we could call softint from usb_serial_generic_write_start (after the kfifo_get is made) but we would still have a 1-1 mapping. In the log posted earlier everything looks fine with respect to fifo put/get. What is apparent though is that the last write urb submitted never completes (and consequently, the writing process is never woken). And by the way, as the application in question is doing megabyte writes at a time, isn't the fifo quite likely to be full most of the time? /Johan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on 28 Jan 2010 13:10 On Tue, Jan 19, 2010 at 02:20:29PM +0100, Oliver Neukum wrote: > Am Montag, 18. Januar 2010 21:14:16 schrieb Matthew Garrett: > > > drivers/usb/serial/generic.c: usb_serial_generic_write - port 0, 1565 > > bytes > > drivers/usb/serial/generic.c: usb_serial_generic_write - put 0 bytes > > into fifo > > drivers/usb/serial/generic.c: usb_serial_generic_read_bulk_callback - > > port 0 > > If the FIFO is full we can do nothing. Please try the attached patch. Did we ever determine if the patch below is needed or not? thanks, greg k-h > From d7317bae0772b794a1cc9b832bc3d3e1b3642a13 Mon Sep 17 00:00:00 2001 > From: Oliver Neukum <oliver(a)neukum.org> > Date: Tue, 19 Jan 2010 14:16:41 +0100 > Subject: [PATCH] usb:serial:Deal with filled FIFO > > Bail out if the FIFO is filled > > Signed-off-by: Oliver Neukum <oliver(a)neukum.org> > --- > drivers/usb/serial/generic.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c > index 76e5514..053f7f1 100644 > --- a/drivers/usb/serial/generic.c > +++ b/drivers/usb/serial/generic.c > @@ -349,6 +349,10 @@ int usb_serial_generic_write(struct tty_struct *tty, > > count = kfifo_in_locked(&port->write_fifo, buf, count, &port->lock); > dbg("%s - put %d bytes into fifo", __func__, count); > + if (!count) { > + dbg("%s - FIFO is full", __func__); > + return 0; > + } > result = usb_serial_generic_write_start(port); > > if (result >= 0) > -- > 1.6.4.2 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on 28 Jan 2010 17:50
On Thu, Jan 28, 2010 at 07:40:12PM +0100, Oliver Neukum wrote: > Am Donnerstag, 28. Januar 2010 18:45:46 schrieb Greg KH: > > On Tue, Jan 19, 2010 at 02:20:29PM +0100, Oliver Neukum wrote: > > > Am Montag, 18. Januar 2010 21:14:16 schrieb Matthew Garrett: > > > > > > > drivers/usb/serial/generic.c: usb_serial_generic_write - port 0, 1565 > > > > bytes > > > > drivers/usb/serial/generic.c: usb_serial_generic_write - put 0 bytes > > > > into fifo > > > > drivers/usb/serial/generic.c: usb_serial_generic_read_bulk_callback - > > > > port 0 > > > > > > If the FIFO is full we can do nothing. Please try the attached patch. > > > > Did we ever determine if the patch below is needed or not? > > No. Further discussion cast serious doubt on it, but we have no > experimental evidence I am aware of. Ok, thanks for letting me know. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |