Sorry for the long delay. Yearly gnome desktop upgrade with lots of new bugs, holidays, etc. Alan, I included you in this series because you know more than most about how the ldisc layer works. I apologize if you find this unwelcome. Greg, I'd like special maintainer dispension for using printk(KERN_DEBUG...) in tty: Bracket ldisc release with TTY_DEBUG_HANGUP messages tty: Add ldisc hangup debug messages I think this usage is in keeping with existing form (in any event, one of messages is simply being relocated). Changes in v3: - Because of new changes to the way drivers interact with tty flip buffers, this series no longer purports to prevent the flush_to_ldisc() warning. That said, this series does fix the warnings generated by trinity and the various test jigs run without complaint. Ilya's latest stress_test_tty.c runs to completion (with the read and write threads enabled) with no WARNs (or BUGs with the series "tty: Fix parallel master and slave pty opens" -- which just got pushed to -tty-next) - Since the lock correction initially reported on by Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> here https://lkml.org/lkml/2012/11/21/267 was fixed by Sebastian in v3 of "tty: don't deadlock while flushing workqueue" and was pushed to -next, the patches "tty: Don't reenable already enable ldisc" "tty: Don't restart ldisc on hangup if racing ldisc kill" "tty: Make core responsible for synchronizing its work" fix the other races which may occur when async hangup races with the tty final close. Please re-test with your dummy_hcd/g_nokia testcase, although I'm not convinced that usb gadget is using tty_hangup() appropriately. tty drivers use this for async carrier loss coming from an IRQ which will be disabled if the tty has been shutdown. Does gserial prevent async hangup to a dead tty in a similar fashion? - Jiri's debug patch was removed to avoid grumblings about using IS_ERR_OR_NULL() -- see http://lkml.indiana.edu/hypermail/linux/kernel/1301.1/00913.html - "tty: WARN if buffer work racing with tty free" was removed because this happens all the time now that drivers can push i/o to a dead tty. - The false-positive diagnostic reported by Sasha Levin here http://lkml.indiana.edu/hypermail/linux/kernel/1212.2/01017.html was fixed in: "n_tty: Fully initialize ldisc before restarting buffer work" - Fixed various comments which were either wrong or hadn't followed the code they belonged to (but only after endless hours of auditing various ldisc code paths) - Added more debug log messages for use with debugging tty hangup Changes in v2: - Please review "tty: Don't flush buffer when closing ldisc". This patch replaces the earlier "tty: Don't reschedule buffer work while closing". The text of this commit details why not calling n_tty_flush_buffer() is the correct thing to do, so I won't repeat it here. - The test jig has been included in the commit message for "tty: Don't flush buffer when closing ldisc" as Alan requested. - Ilya Zykov was added as the Signed-off-by: for the test jig in that same commit message. - Sasha Levin was added as the Reported-by: in that same patch. This patch series addresses various causes of flip buffer work being scheduled for a dead ldisc & tty. The most common cause stems from the n_tty_close() path flushing (which schedules buffer work), when the ldisc has already been halted (meaning userspace has been locked out of further i/o and existing users have finished). This is fixed in 'tty: Don't flush buffer when closing ldisc' The other causes have a central theme: incorrect order-of-operations when halting a line discipline. In general, to prevent buffer work from being scheduled requires: 1. Disallowing further ldisc references 2. Waiting for all existing ldisc references to be released 3. Cancelling existing buffer work If the wait takes place _after_ cancellation, then new work can be scheduled by existing holder(s) of ldisc reference(s). That's bad. Halting the line discipline is performed when, - hanging up the tty (tty_ldisc_hangup()) - TIOCSETD ioctl (tty_set_ldisc()) - finally closing the tty (pair) (tty_ldisc_release()) Concurrent halts are governed by the following requirements: 1. tty_ldisc_release is not concurrent with the other two and so does not need lock or state protection during the ldiscs halt. 2. Accesses through tty->ldisc must be protected by the ldisc_mutex. The wait operation checks the user count (ldisc references) in tty->ldisc->users. 3. tty_set_ldisc() reschedules buffer work that was pending when the ldiscs were halted. That must be an atomic operation in conjunction with re-enabling the ldisc -- which necessitates locking concurrent halts (tty_ldisc_release is exempt here) 4. The legacy mutex cannot be held while waiting for ldisc reference(s) release -or- for cancelling buffer work. 5. Because of #4, the legacy mutex must be dropped prior to or during the halt. Which means reacquiring after the halt. But to preserve lock order the ldisc_mutex must be dropped and reacquired after reacquiring the legacy mutex. 6. Because of #5, the ldisc state may have changed while the ldisc mutex was dropped. Peter Hurley (23): tty: Add diagnostic for halted line discipline n_tty: Factor packet mode status change for reuse n_tty: Don't flush buffer when closing ldisc tty: Refactor wait for ldisc refs out of tty_ldisc_hangup() tty: Remove unnecessary re-test of ldisc ref count tty: Fix ldisc halt sequence on hangup tty: Strengthen no-subsequent-use guarantee of tty_ldisc_halt() tty: Halt both ldiscs concurrently tty: Remove unnecessary buffer work flush tty: Wait for SAK work before waiting for hangup work n_tty: Correct unthrottle-with-buffer-flush comments tty: Kick waiters _after_ the ldisc is locked n_tty: Fully initialize ldisc before restarting buffer work tty: Don't reenable already enabled ldisc tty: Don't restart ldisc on hangup if racing ldisc kill tty: Make core responsible for synchronizing its work tty: Document lock requirements to halt ldisc tty: Remove stale comment re: tty_ldisc_flush_works() tty: Fix 'deferred reopen' ldisc comment tty: Remove stale comment re: locking in tty_ldisc_release() tty: Re-parent orphaned tty_set_ldisc() comments tty: Bracket ldisc release with TTY_DEBUG_HANGUP messages tty: Add ldisc hangup debug messages drivers/tty/n_tty.c | 62 +++++++----- drivers/tty/tty_io.c | 23 ++++- drivers/tty/tty_ldisc.c | 248 ++++++++++++++++++++++++++++-------------------- include/linux/tty.h | 1 + 4 files changed, 205 insertions(+), 129 deletions(-) -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
