From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751007AbdE3XC5 (ORCPT ); Tue, 30 May 2017 19:02:57 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:56671 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750821AbdE3XC4 (ORCPT ); Tue, 30 May 2017 19:02:56 -0400 Date: Wed, 31 May 2017 01:02:51 +0200 (CEST) From: Thomas Gleixner To: "jeffy.chen" cc: Tomasz Figa , LKML , Brian Norris , Douglas Anderson , Marc Zyngier Subject: Re: [PATCH v3] genirq: Check irq disabled & masked states in irq_shutdown In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 29 May 2017, jeffy.chen wrote: > i think if we want to make all irq enable/disable balance, maybe we can: > > 1/ only call irq_enable/disable from enable/disable_irq(change other > irq_enable/disable to enable/disable_irq), so they would be protected by the > refcnt(deph) You cannot call enable/disable_irq() from places which call irq_enable/disable() due to locking reasons. __disable_irq()/__enable_irq() can/must be called with desc->lock held, but __enable_irq() does more than just calling irq_enable(). So no, it's not that simple. > 2/ disable lazy stuff in irq_shoutdown(we already did this in free_irq) No, irq_shutdown() is called from other places as well. > 3/ in irq_shutdown, set depth to 0 if it's not disabled and masked(for lazy stuff) > before calling disable_irq, Uurgh, no. That's a unholy hack. So what should be done to fix this is to make consequent use of the state bits. irq_disable() if (irqd_irq_disabled()) return; irqd_set_irq_disabled(); .... This should be done for both mask/unmask disable/enable. You get the idea. We probably want a third state bit for STARTED_UP and do the same dance in startup/shutdown as well. Which brings me to a different issue, which is outside the scope of your problem, but looking at the code made me find it. If a interrupt is marked IRQ_NOAUTOEN then request_irq() will not invoke irq_startup(). The interrupt is just completely set up, but stays disabled. It is enabled later via enable_irq(). That works so far with no complaints, but there is an interesting twist: In that NOAUTOEN case nothing ever calls irq_startup() on that irq, which means that in case the irq_chip has a irq_startup() callback nothing will invoke it and also irq_domain_activate_irq() will never be invoked on such an irq. Looks like all implementations which use IRQ_NOAUTOEN are not sensitive to that. It's been broken forever. Fixing this needs the extra state bit IRQD_ STARTED_UP as we cannot reuse the IRQD_ACTIVATED bit because some of the interrupts are actually activated before they are requested. Too tired to fix that now, but I'll have a look tomorrow. Once this is fixed, you can add the extra bits to prevent this disable/enable calls which cause the imbalance deeper down. Thanks, tglx