From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752020AbdF3BZH (ORCPT ); Thu, 29 Jun 2017 21:25:07 -0400 Received: from lucky1.263xmail.com ([211.157.147.131]:55835 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751784AbdF3BZF (ORCPT ); Thu, 29 Jun 2017 21:25:05 -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: 0 X-SKE-CHECKED: 0 X-ANTISPAM-LEVEL: 2 X-RL-SENDER: jeffy.chen@rock-chips.com X-FST-TO: briannorris@chromium.org X-SENDER-IP: 103.29.142.67 X-LOGIN-NAME: jeffy.chen@rock-chips.com X-UNIQUE-TAG: <02713a9053f787c663e5bb75eef21156> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <5955A866.20308@rock-chips.com> Date: Fri, 30 Jun 2017 09:24:54 +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: Brian Norris CC: linux-kernel@vger.kernel.org, tfiga@chromium.org, heiko@sntech.de, dianders@chromium.org, linux-rockchip@lists.infradead.org, Linus Walleij , linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH] pinctrl: rockchip: add irq_shutdown References: <1498223019-9123-1-git-send-email-jeffy.chen@rock-chips.com> <20170629220532.GA54678@google.com> In-Reply-To: <20170629220532.GA54678@google.com> 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 brian, On 06/30/2017 06:05 AM, Brian Norris wrote: > On Fri, Jun 23, 2017 at 09:03:39PM +0800, Jeffy Chen wrote: >> Currently the rockchip pinctrl driver would try to enable/disable the >> gpio bank clk when enable/disable an irq. >> >> So when the irq core trying to shutdown an already disabled irq, it >> would result in unbalanced clk disable request: >> [ 35.911955] WARNING: at drivers/clk/clk.c:680 >> ... >> [ 37.272271] Call trace: >> [ 37.274729] [] clk_core_disable+0x28/0x194 >> [ 37.280395] [] clk_disable+0x34/0x48 >> [ 37.285544] [] rockchip_irq_disable+0x30/0x3c >> [ 37.291472] [] __irq_disable+0x40/0x64 >> [ 37.296791] [] irq_shutdown+0x68/0x8c >> [ 37.302023] [] __free_irq+0x110/0x218 >> [ 37.307254] [] free_irq+0x54/0x64 >> [ 37.312138] [] devm_irq_release+0x24/0x30 >> >> Add an irq_shutdown callback, and do a sanity check for irq state to >> prevent that. > > IMO, this patch is completely the wrong approach. Either we: > > (a) follow the current semantics of the irqchip core handling (which > tglx described more or less as "no refcounting; no guarantee that > enable/disable are balanced") or > > (b) fix the irqchip core to provide the above guarantee right, since we have: bf22ff45 genirq: Avoid unnecessary low level irq function calls d829b8f genirq: Set irq masked state when initializing irq_desc this issue should be fixed now. > > To do (a) properly would be rather trivial too; just keep an extra > bitmask in the bank struct to track the "enabled" state of each > interrupt. If the mask is non-zero, enable the clock; if zero, disable. > (Incidentally, this is pretty similar logic to what you ended up with on > your SPI runtime PM / set_cs() patches recently.) > > This patch does neither (a) nor (b), and so I'd tend to reject it. > > Brian > >> Signed-off-by: Jeffy Chen >> --- >> >> drivers/pinctrl/pinctrl-rockchip.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c >> index 607f52c..b0e3130 100644 >> --- a/drivers/pinctrl/pinctrl-rockchip.c >> +++ b/drivers/pinctrl/pinctrl-rockchip.c >> @@ -2551,6 +2551,12 @@ static void rockchip_irq_disable(struct irq_data *d) >> clk_disable(bank->clk); >> } >> >> +static void rockchip_irq_shutdown(struct irq_data *d) >> +{ >> + if (!irqd_irq_disabled(d)) >> + rockchip_irq_disable(d); >> +} >> + >> static void rockchip_irq_bus_lock(struct irq_data *d) >> { >> struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); >> @@ -2641,6 +2647,7 @@ static int rockchip_interrupts_register(struct platform_device *pdev, >> gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit; >> gc->chip_types[0].chip.irq_enable = rockchip_irq_enable; >> gc->chip_types[0].chip.irq_disable = rockchip_irq_disable; >> + gc->chip_types[0].chip.irq_shutdown = rockchip_irq_shutdown; >> gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake; >> gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; >> gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; >> -- >> 2.1.4 >> >> > > >