From patchwork Tue Aug 21 22:34:32 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 1358231 Return-Path: X-Original-To: patchwork-linux-fbdev@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 30776DFB34 for ; Tue, 21 Aug 2012 22:35:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751834Ab2HUWfI (ORCPT ); Tue, 21 Aug 2012 18:35:08 -0400 Received: from mail-wg0-f44.google.com ([74.125.82.44]:42982 "EHLO mail-wg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754694Ab2HUWeu (ORCPT ); Tue, 21 Aug 2012 18:34:50 -0400 Received: by wgbdr13 with SMTP id dr13so266872wgb.1 for ; Tue, 21 Aug 2012 15:34:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references; bh=qXkDaHDCIWOLIF6K5iDzBxNSeHfcSXQlPHy9nOkO2IQ=; b=Q4quhnrAl9SHDV5LYs70wDQUbLRwbKLM6rB78D9XTQwpJ0bv+YLCZZrfrF0qv5lz+q dRuqGsh3pZWWBJhYUlWVFQdXrabz6IWr1gPxWnhKhTjrRRe7tMzMaUvtNXBqefRPtlEE h5jt9c/YSuOxI5OZXZNZqby70eIWHBKnNxiAQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:date:message-id:x-mailer:in-reply-to:references :x-gm-message-state; bh=qXkDaHDCIWOLIF6K5iDzBxNSeHfcSXQlPHy9nOkO2IQ=; b=HSuIdxgpIUOzu0i6NmjYxB7bLylnzgiAJHHy/mAca8WRXfusyG5uzcmkOfJOm/6iqD ub19UaR+HJbw7aU/gNXQ/WpJ0uKb4lqNa4GdHoSmIs3e+/yXiddRgmyqow48nUe6HIdb cfjkIdGHCzgz1OIfMojZOyBgJFZF3X3lnwKjLFzBJ44HnMymavH4EZxyCyr8Cvpt/rkP KIx8a8Rjoit7ozZxhXyZjyEEcatPVMpbvxcXe0XWfPooq3xH8N2WU7krk3oOQJG+ocgp 7mo5785lVXcThRHLPgxMAe40FlNIm4U1u3smcRJYtDl7i+dhqdu8tNVROY5x71FHh1Ks vRQg== Received: by 10.180.93.8 with SMTP id cq8mr399042wib.16.1345588489138; Tue, 21 Aug 2012 15:34:49 -0700 (PDT) Received: from aaron.ffwll.local (178-83-130-250.dynamic.hispeed.ch. [178.83.130.250]) by mx.google.com with ESMTPS id k20sm36045620wiv.11.2012.08.21.15.34.47 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 21 Aug 2012 15:34:48 -0700 (PDT) From: Daniel Vetter To: LKML Cc: DRI Development , linux-fbdev@vger.kernel.org, Alan Cox , Daniel Vetter , Dave Airlie , Thomas Gleixner Subject: [PATCH 2/2] console: implement lockdep support for console_lock Date: Wed, 22 Aug 2012 00:34:32 +0200 Message-Id: <1345588472-4055-3-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1345588472-4055-1-git-send-email-daniel.vetter@ffwll.ch> References: <1345588472-4055-1-git-send-email-daniel.vetter@ffwll.ch> X-Gm-Message-State: ALoCoQmaFv1HTqd2ILItwP62ixDE02CPgk9VnPb5icutCA/c29Ke/zbunIOUWT3nkS6sT/6zAhS9 Sender: linux-fbdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fbdev@vger.kernel.org Dave Airlie recently discovered a locking bug in the fbcon layer, where a timer_del_sync (for the blinking cursor) deadlocks with the timer itself, since both (want to) hold the console_lock: https://lkml.org/lkml/2012/8/21/36 Unfortunately the console_lock isn't a plain mutex and hence has no lockdep support. Which resulted in a few days wasted of tracking down this bug (complicated by the fact that printk doesn't show anything when the console is locked) instead of noticing the bug much earlier with the lockdep splat. Hence I've figured I need to fix that for the next deadlock involving console_lock - and with kms/drm growing ever more complex locking that'll eventually happen. Now the console_lock has rather funky semantics, so after a quick irc discussion with Thomas Gleixner and Dave Airlie I've quickly ditched the original idead of switching to a real mutex (since it won't work) and instead opted to annotate the console_lock with lockdep information manually. There are a few special cases: - The console_lock state is protected by the console_sem, and usually grabbed/dropped at _lock/_unlock time. But the suspend/resume code drops the semaphore without dropping the console_lock (see suspend_console/resume_console). But since the same thread that did the suspend will do the resume, we don't need to fix up anything. - In the printk code there's a special trylock, only used to kick off the logbuffer printk'ing in console_unlock. But all that happens while lockdep is disable (since printk does a few other evil tricks). So no issue there, either. - The console_lock can also be acquired form irq context (but only with a trylock). lockdep already handles that. This all leaves us with annotating the normal console_lock, _unlock and _trylock functions. And yes, it works - simply unloading a drm kms driver resulted in lockdep complaining about the deadlock in fbcon_deinit: ====================================================== [ INFO: possible circular locking dependency detected ] 3.6.0-rc2+ #552 Not tainted ------------------------------------------------------- kms-reload/3577 is trying to acquire lock: ((&info->queue)){+.+...}, at: [] wait_on_work+0x0/0xa7 but task is already holding lock: (console_lock){+.+.+.}, at: [] bind_con_driver+0x38/0x263 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (console_lock){+.+.+.}: [] lock_acquire+0x95/0x105 [] console_lock+0x59/0x5b [] fb_flashcursor+0x2e/0x12c [] process_one_work+0x1d9/0x3b4 [] worker_thread+0x1a7/0x24b [] kthread+0x7f/0x87 [] kernel_thread_helper+0x4/0x10 -> #0 ((&info->queue)){+.+...}: [] __lock_acquire+0x999/0xcf6 [] lock_acquire+0x95/0x105 [] wait_on_work+0x3b/0xa7 [] __cancel_work_timer+0xbf/0x102 [] cancel_work_sync+0xb/0xd [] fbcon_deinit+0x11c/0x1dc [] bind_con_driver+0x145/0x263 [] unbind_con_driver+0x14f/0x195 [] store_bind+0x1ad/0x1c1 [] dev_attr_store+0x13/0x1f [] sysfs_write_file+0xe9/0x121 [] vfs_write+0x9b/0xfd [] sys_write+0x3e/0x6b [] system_call_fastpath+0x16/0x1b other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(console_lock); lock((&info->queue)); lock(console_lock); lock((&info->queue)); *** DEADLOCK *** Cc: Dave Airlie Cc: Thomas Gleixner Signed-off-by: Daniel Vetter --- kernel/printk.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/kernel/printk.c b/kernel/printk.c index ed9af6a..ab2ab24 100644 --- a/kernel/printk.c +++ b/kernel/printk.c @@ -87,6 +87,12 @@ static DEFINE_SEMAPHORE(console_sem); struct console *console_drivers; EXPORT_SYMBOL_GPL(console_drivers); +#ifdef CONFIG_LOCKDEP +struct lockdep_map console_lock_dep_map = { + .name = "console_lock" +}; +#endif + /* * This is used for debugging the mess that is the VT code by * keeping track if we have the console semaphore held. It's @@ -1916,6 +1922,7 @@ void console_lock(void) return; console_locked = 1; console_may_schedule = 1; + mutex_acquire(&console_lock_dep_map, 0, 0, _RET_IP_); } EXPORT_SYMBOL(console_lock); @@ -1937,6 +1944,7 @@ int console_trylock(void) } console_locked = 1; console_may_schedule = 0; + mutex_acquire(&console_lock_dep_map, 0, 1, _RET_IP_); return 1; } EXPORT_SYMBOL(console_trylock); @@ -2097,6 +2105,7 @@ skip: local_irq_restore(flags); } console_locked = 0; + mutex_release(&console_lock_dep_map, 1, _RET_IP_); /* Release the exclusive_console once it is used */ if (unlikely(exclusive_console))