From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755446AbdE0KJj (ORCPT ); Sat, 27 May 2017 06:09:39 -0400 Received: from regular1.263xmail.com ([211.150.99.133]:58149 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750844AbdE0KJi (ORCPT ); Sat, 27 May 2017 06:09:38 -0400 X-263anti-spam: KSV:0;BIG:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 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: <706082171b119cb607b2c3c90c2d7c33> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <59295059.7070509@rock-chips.com> Date: Sat, 27 May 2017 18:09:29 +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: linux-kernel@vger.kernel.org, briannorris@chromium.org, dianders@chromium.org, tfiga@chromium.org Subject: Re: [PATCH v2] genirq: Check irq disabled & masked states in irq_shutdown References: <1495860765-32399-1-git-send-email-jeffy.chen@rock-chips.com> 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, On 05/27/2017 04:30 PM, Thomas Gleixner wrote: > On Sat, 27 May 2017, Jeffy Chen wrote: > >> If a irq is already disabled & masked, free_irq may cause a unbalanced >> irq shutdown/disable/mask, for example: > > No, it's not. irq_shutdown/disable/mask are low level access functions > which can be invoked at any given time. > > The only interface which has refcounting is disable/enable_irq(). but i think it still be good trying to make them balance, at least for irq enable/disable :) > >> devm_request_irq->irq_startup->irq_enable >> disable_irq <-- disabled and masked >> devm_free_irq->irq_shutdown <-- try to disable it again >> >> This would confuse some pinctrl drivers which would control clk in >> irq_enable/irq_disable, for example pinctrl-rockchip/pinctrl-nomadik. >> >> This patch add a state check in irq_shutdown to prevent that. > > Please read Documentation/process/SubmittingPatches and search for "this > patch". oops > >> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >> index 686be4b..816da03 100644 >> --- a/kernel/irq/chip.c >> +++ b/kernel/irq/chip.c >> @@ -206,14 +206,20 @@ int irq_startup(struct irq_desc *desc, bool resend) >> >> void irq_shutdown(struct irq_desc *desc) >> { >> - irq_state_set_disabled(desc); >> desc->depth = 1; >> + >> + if (unlikely(irqd_irq_disabled(&desc->irq_data) && >> + irqd_irq_masked(&desc->irq_data))) >> + goto out; > > This is just wrong. > > It's perfectly legit to call disable_irq() and then free_irq(). Still > free_irq() has to be able to invoke chip->irq_shutdown(). Preventing that > will leave some interrupt chips in a half initialized state. right, irq_shutdown is not just to disable irq, may also do some cleanups. will upload new patch. > > The irq core does not guarntee that the unmask/mask enable/disable > startup/shutdown callbacks are perfectly balanced. irq_shutdown() is only > one place where this can happen. This needs more thought than this 'works > for me' hackery. > > Thanks, > > tglx > > >