Hi Greg and David, I'm for not responding to this sooner, but I've been travelling for the last week and have had very limited mail access. I would like to ask you Greg not to apply this patch yet as I have some doubts regarding it's correctness. On Mon, Aug 02, 2010 at 10:46:00AM -0700, David VomLehn wrote: > Fix USB console hang due to non-atomic URB allocation > > This is intended to fix a problem seen with the Amtel sam-ba tool by > Alexander Stein (alexander.stein@xxxxxxxxxxxxxxxxxxxxx). It appears > when using an A91 controller. He bisected it to commit: > > 8e8dce065088833fc418bfa5fbf035cb0726c04c: USB: use kfifo to buffer usb-generic serial writes Firstly, the description is somewhat inaccurate (as David points out below) as the bisected commit is completely unrelated to the locking issue David intends to solve (in fact, write handling has been completely rewritten in 2.6.35, after the commit in question). The reason for Sam-ba not working anymore is quite likely to the fact that the above mentioned commit introduces merges of write requests (the fifo). If the Sam-ba firmware is not happy with that, it should be fixed or a specific driver which does not use a fifo needs to be written. I have access to such an Atmel board and could have a look at it soon. > The current patch fixes a problem when using a USB serial device as the > kernel console and as /dev/console. The URB allocation is not atomic and > an URB can be doubly allocated, leading to a continual rejection of URB > submissions. The fix puts all pieces of the URB allocation in the same > spinlock-protected section of code. > > Having said that... > > This fix was developed because, after Alexander's email, I took a look > at my USB console and found that it, too, was experiencing a hang. I > assumed that this hang was the same as the one Alexander is seeing and > developed this fix. It's a good fix for *a* hang, but after thinking > about this a bit, I'm not sure this is a fix for *Alexander's* hang. > > Signed-off-by: <dvomlehn@xxxxxxxxx> > --- > drivers/usb/serial/generic.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c > index a817ced..7085809 100644 > --- a/drivers/usb/serial/generic.c > +++ b/drivers/usb/serial/generic.c > @@ -199,6 +199,9 @@ retry: > } > i = (int)find_first_bit(&port->write_urbs_free, > ARRAY_SIZE(port->write_urbs)); > + if (i == ARRAY_SIZE(port->write_urbs)) > + return 0; > + clear_bit(i, &port->write_urbs_free); Secondly, I doubt that this is a real fix for the problem David is having. AFAIK, the locking is indeed correct as the whole write code is protected by the USB_SERIAL_WRITE_BUSY flag. Thus, after checking that write_urbs_free is indeed non-zero, no bit can be cleared before write returns. This means that find_first_bit will never return ARRAY_SIZE(port->write_urbs). The write-busy flag also guarantees that there cannot be any double urb allocation. Thirdly, this very piece of code also introduces a locking imbalance as the port lock is not unlocked before returning. > spin_unlock_irqrestore(&port->lock, flags); > > urb = port->write_urbs[i]; > @@ -213,9 +216,9 @@ retry: > dev_err(&port->dev, "%s - error submitting urb: %d\n", > __func__, result); > clear_bit_unlock(USB_SERIAL_WRITE_BUSY, &port->flags); > + set_bit(i, &port->write_urbs_free); > return result; > } > - clear_bit(i, &port->write_urbs_free); > > spin_lock_irqsave(&port->lock, flags); > port->tx_bytes += count; David, could you let me know in which set-up you're experiencing the lock-up so I can try to reproduce it here? Do you have any logs from running with debugging enabled on 2.6.35? Thanks, Johan -- 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
