From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751392AbdFZGXe (ORCPT ); Mon, 26 Jun 2017 02:23:34 -0400 Received: from regular1.263xmail.com ([211.150.99.131]:35052 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751092AbdFZGXb (ORCPT ); Mon, 26 Jun 2017 02:23:31 -0400 X-263anti-spam: BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-ADDR-CHECKED4: 1 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ANTISPAM-LEVEL: 2 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: tglx@linutronix.de X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <5950A83B.8010303@rock-chips.com> Date: Mon, 26 Jun 2017 14:22:51 +0800 From: jeffy User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20130126 Thunderbird/19.0 MIME-Version: 1.0 To: Thomas Gleixner CC: Tomasz Figa , LKML , Brian Norris , Douglas Anderson , Marc Zyngier Subject: Re: [PATCH v3] genirq: Check irq disabled & masked states in irq_shutdown References: In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Thomas Gleixner, On 05/31/2017 07:02 AM, Thomas Gleixner wrote: > 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. ok > > 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. i saw your patches landed, so i sent a patch for enable/disable/unmask/mask_irq, please help to review :) > > Thanks, > > tglx > > > > > > > > > > > >