Messages in this thread | | | Subject | Re: 2.4.27 Potential race in n_tty.c:write_chan() | From | Ryan Reading <> | Date | Sun, 05 Dec 2004 23:42:22 -0500 |
| |
On Sun, 2004-12-05 at 17:40, Paul Fulghum wrote: > On Sun, 2004-12-05 at 15:54 -0500, Ryan Reading wrote: > > So when write_chan() calls usb_driver::write(), typically the driver > > calls usb_submit_urb(). The write() call then returns immediately > > indicating that all of the data has been written (assuming it is less > > than the USB packets size). The driver however is still waiting for an > > interrupt to complete the write and wakeup the current kernel path. If > > write_chan() is called again and the interrupt is received within the > > window I outlined above, the current_state will be reset to TASK_RUNNING > > before the next usb_driver::write() is ever called! If this happens, it > > seems that we would lose synchronisity and potentially lock the kernel > > path. > > The line discipline write routine is serialized > on a per tty basis in do_tty_write() of tty_io.c > using tty->atomic_write semaphore, so you will not > reenter write_chan() for a particular tty instance.
Per tty instance this is true, but this is not the case for multiple userland::write() calls.
> Even if this were not the case, if the task state > changes to TASK_RUNNING inside the window > you describe, the only thing that happens is the loop > executes again. The driver must decide if it can accept > more data or not and return the appropriate value. > > There is no potential for deadlock.
Yes this does seem to be true. However, it can throw a driver into a wierd state if this happens. Ideally the driver should be able to recover. I'm not sure if we can guarantee that user_land protocol could recover though, especially if write()s aren't guaranteed to complete successfully.
> > It is also my understanding that the usb interrupt is generated from the > > ACK/NAK of the original usb_submit_urb(). If the driver is returning > > immediately without waiting on the interrupt and schedule() is never > > being called, there is no guarantee that the write() happened > > successfully (although we return that it has). It seems if a driver > > wanted to guarantee this, it would have to artificially wait of the > > interrupt before returning. > > True, but this is a matter of layering. > > The line discipline knows nothing about the driver's concept > of write completion apart from the driver's write method > return value. If it is critical for the write not to complete > until the URB is sent, it is up to the driver to block > and return the appropriate return value.
I guess my biggest concern here is the definition of the tty_driver::write() interface. Should a driver be able to rely on the schedule() call to complete its write? It seems that since the driver is responsible for waking up the tty->write_wait, it should be able to rely on the schedule() call. However in this implementation it cannot which I'm sure is for performance reasons. Because of this, I don't think the USB layer should be interacting with the tty layer in this fashion.
So for this implementation of write_chan(), the tty_driver::write() interface should note that the driver needs only to wakeup the tty->write_wait iff it does not write all of the requested data in this call. Is this a fair assessment and can this be depended on for future implementations?
Anyway, part of this discussion needs to happen on the USB mailing list since the usb-skeleton driver outlines this method of interacting with the tty layer. Given the current implementation I don't think the usb-skeleton is correct.
Thanks for your help.
-- Ryan
> -- > Paul Fulghum > paulkf@microgate.com >
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |