From patchwork Tue Sep 29 09:27:32 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jiri Kosina X-Patchwork-Id: 7284441 X-Patchwork-Delegate: jikos@jikos.cz Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id E0E129F32B for ; Tue, 29 Sep 2015 09:27:42 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0BDB3206CC for ; Tue, 29 Sep 2015 09:27:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0CBD7206C5 for ; Tue, 29 Sep 2015 09:27:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933838AbbI2J1g (ORCPT ); Tue, 29 Sep 2015 05:27:36 -0400 Received: from mx2.suse.de ([195.135.220.15]:36811 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933572AbbI2J1c (ORCPT ); Tue, 29 Sep 2015 05:27:32 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id D28A3AAC8; Tue, 29 Sep 2015 09:27:29 +0000 (UTC) Date: Tue, 29 Sep 2015 11:27:32 +0200 (CEST) From: Jiri Kosina X-X-Sender: jkosina@pobox.suse.cz To: Sedat Dilek cc: linux-input@vger.kernel.org, Tejun Heo , Lai Jiangshan , Steven Rostedt , Paul McKenney Subject: Re: [PATCH] usbhid: Fix lockdep unannotated irqs-off warning In-Reply-To: Message-ID: References: <1443427804-2957-1-git-send-email-sedat.dilek@gmail.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, 29 Sep 2015, Sedat Dilek wrote: > Did you look at the step-by-step moving of trace_hardirqs_off() and > the corresponding dmesg-logs? > What helps is a trace_hardirqs_off() before spin_unlock_irq() in the > if-statement. > So, yes "IRQs are enabled" but tracing does not like it. I so far fail to make any sense out of this behavior. > > Therefore ... > > > > [ ... snip ... ] > >> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c > >> index 36712e9f56c2..188f59348ec5 100644 > >> --- a/drivers/hid/usbhid/hid-core.c > >> +++ b/drivers/hid/usbhid/hid-core.c > >> @@ -729,16 +729,16 @@ void usbhid_close(struct hid_device *hid) > >> * data acquistion due to a resumption we no longer > >> * care about > >> */ > >> - spin_lock_irq(&usbhid->lock); > >> + spin_lock_bh(&usbhid->lock); > >> if (!--hid->open) { > >> - spin_unlock_irq(&usbhid->lock); > >> + spin_unlock_bh(&usbhid->lock); > >> hid_cancel_delayed_stuff(usbhid); > > > > I still don't understand how this should be improving anything. I believe > > spin_unlock_irq() should just re-enable interrupts, because we've been > > called with them enabled as well. > > Is spin_lock_bh() not an appropriate replacement? No, it's not. Plus, before we gain understanding why exactly is the warning being issued, I am not making any random changes to the code. > > Now if you are able to see how usbhid_close() can be called with IRQs > > off, that would be a completely different story. But if that's not the > > case, the warning is bogus, and gcc-compiled kernels are right about not > > issuing it. > > Again, I am new to tracing. > Steven encouraged me to look at the lockdep hints in dmesg - not ftrace [1]. > > "Actually, if you are looking for where interrupts were disabled last > before triggering the "sleeping function called from invalid context", > lockdep, not ftrace, would be your better bet. > > Enable lockdep with CONFIG_PROVE_LOCKING. It will give you better > information about where the last irq was disabled." > > Here, I have CONFIG_PROVE_LOCKING=y. > > I am doing a new kernel-build with the might_sleep() on top of > hid_cancel_delayed_stuff() which showed some lockdep/irqsoff hints in > dmesg-log. Yes. That dmesg doesn't still make any sense. It tells us that IRQs got enabled during spin_unlock_irq() (I believe it's the very one in usbhid_close(), but we'll see), and then it bugs because it thinks they are enabled. Actually, could you please run with the attached patch (you need lockdep enabled for it to build) and send me your dmesg? Ideally both from gcc-compiled kernel and llvm-compiled kernel as well. diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index 36712e9..aaae42e 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -38,6 +38,8 @@ #include #include "usbhid.h" +#include + /* * Version Information */ @@ -725,6 +727,9 @@ void usbhid_close(struct hid_device *hid) mutex_lock(&hid_open_mut); + if(WARN_ON(irqs_disabled())) + print_irqtrace_events(current); + /* protecting hid->open to make sure we don't restart * data acquistion due to a resumption we no longer * care about