From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756689Ab3BEUWb (ORCPT ); Tue, 5 Feb 2013 15:22:31 -0500 Received: from mailout39.mail01.mtsvc.net ([216.70.64.83]:54265 "EHLO n12.mail01.mtsvc.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755681Ab3BEUW0 (ORCPT ); Tue, 5 Feb 2013 15:22:26 -0500 From: Peter Hurley To: Greg Kroah-Hartman , Alan Cox , Jiri Slaby , Sasha Levin , Sebastian Andrzej Siewior Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, Ilya Zykov , Dave Jones , Peter Hurley Subject: [PATCH v3 00/23] ldisc fixes Date: Tue, 5 Feb 2013 15:20:15 -0500 Message-Id: <1360095638-6624-1-git-send-email-peter@hurleysoftware.com> X-Mailer: git-send-email 1.8.1.2 In-Reply-To: <1355509370-5883-1-git-send-email-peter@hurleysoftware.com> References: <1355509370-5883-1-git-send-email-peter@hurleysoftware.com> X-Authenticated-User: 125194 peter@hurleysoftware.com X-MT-ID: 8fa290c2a27252aacf65dbc4a42f3ce3735fb2a4 X-MT-INTERNAL-ID: 8fa290c2a27252aacf65dbc4a42f3ce3735fb2a4 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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