* [RFC PATCH] pinctrl: rockchip: add irq_shutdown
@ 2017-06-23 13:03 Jeffy Chen
2017-06-29 22:05 ` Brian Norris
0 siblings, 1 reply; 4+ messages in thread
From: Jeffy Chen @ 2017-06-23 13:03 UTC (permalink / raw)
To: linux-kernel
Cc: tfiga, briannorris, heiko, dianders, Jeffy Chen, linux-rockchip,
Linus Walleij, linux-gpio, linux-arm-kernel
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] [<ffffffc0007ac270>] clk_core_disable+0x28/0x194
[ 37.280395] [<ffffffc0007ac6a8>] clk_disable+0x34/0x48
[ 37.285544] [<ffffffc0004f3bf0>] rockchip_irq_disable+0x30/0x3c
[ 37.291472] [<ffffffc00027a7e0>] __irq_disable+0x40/0x64
[ 37.296791] [<ffffffc00027a86c>] irq_shutdown+0x68/0x8c
[ 37.302023] [<ffffffc0002777fc>] __free_irq+0x110/0x218
[ 37.307254] [<ffffffc0002779a8>] free_irq+0x54/0x64
[ 37.312138] [<ffffffc00027ba24>] devm_irq_release+0x24/0x30
Add an irq_shutdown callback, and do a sanity check for irq state to
prevent that.
Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [RFC PATCH] pinctrl: rockchip: add irq_shutdown
2017-06-23 13:03 [RFC PATCH] pinctrl: rockchip: add irq_shutdown Jeffy Chen
@ 2017-06-29 22:05 ` Brian Norris
2017-06-30 1:24 ` jeffy
0 siblings, 1 reply; 4+ messages in thread
From: Brian Norris @ 2017-06-29 22:05 UTC (permalink / raw)
To: Jeffy Chen
Cc: linux-kernel, tfiga, heiko, dianders, linux-rockchip,
Linus Walleij, linux-gpio, linux-arm-kernel
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] [<ffffffc0007ac270>] clk_core_disable+0x28/0x194
> [ 37.280395] [<ffffffc0007ac6a8>] clk_disable+0x34/0x48
> [ 37.285544] [<ffffffc0004f3bf0>] rockchip_irq_disable+0x30/0x3c
> [ 37.291472] [<ffffffc00027a7e0>] __irq_disable+0x40/0x64
> [ 37.296791] [<ffffffc00027a86c>] irq_shutdown+0x68/0x8c
> [ 37.302023] [<ffffffc0002777fc>] __free_irq+0x110/0x218
> [ 37.307254] [<ffffffc0002779a8>] free_irq+0x54/0x64
> [ 37.312138] [<ffffffc00027ba24>] 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
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 <jeffy.chen@rock-chips.com>
> ---
>
> 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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH] pinctrl: rockchip: add irq_shutdown
2017-06-29 22:05 ` Brian Norris
@ 2017-06-30 1:24 ` jeffy
2017-06-30 1:29 ` Brian Norris
0 siblings, 1 reply; 4+ messages in thread
From: jeffy @ 2017-06-30 1:24 UTC (permalink / raw)
To: Brian Norris
Cc: linux-kernel, tfiga, heiko, dianders, linux-rockchip,
Linus Walleij, linux-gpio, linux-arm-kernel
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] [<ffffffc0007ac270>] clk_core_disable+0x28/0x194
>> [ 37.280395] [<ffffffc0007ac6a8>] clk_disable+0x34/0x48
>> [ 37.285544] [<ffffffc0004f3bf0>] rockchip_irq_disable+0x30/0x3c
>> [ 37.291472] [<ffffffc00027a7e0>] __irq_disable+0x40/0x64
>> [ 37.296791] [<ffffffc00027a86c>] irq_shutdown+0x68/0x8c
>> [ 37.302023] [<ffffffc0002777fc>] __free_irq+0x110/0x218
>> [ 37.307254] [<ffffffc0002779a8>] free_irq+0x54/0x64
>> [ 37.312138] [<ffffffc00027ba24>] 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 <jeffy.chen@rock-chips.com>
>> ---
>>
>> 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
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH] pinctrl: rockchip: add irq_shutdown
2017-06-30 1:24 ` jeffy
@ 2017-06-30 1:29 ` Brian Norris
0 siblings, 0 replies; 4+ messages in thread
From: Brian Norris @ 2017-06-30 1:29 UTC (permalink / raw)
To: jeffy
Cc: linux-kernel, tfiga, heiko, dianders, linux-rockchip,
Linus Walleij, linux-gpio, linux-arm-kernel
On Fri, Jun 30, 2017 at 09:24:54AM +0800, Jeffy Chen wrote:
> 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] [<ffffffc0007ac270>] clk_core_disable+0x28/0x194
> >>[ 37.280395] [<ffffffc0007ac6a8>] clk_disable+0x34/0x48
> >>[ 37.285544] [<ffffffc0004f3bf0>] rockchip_irq_disable+0x30/0x3c
> >>[ 37.291472] [<ffffffc00027a7e0>] __irq_disable+0x40/0x64
> >>[ 37.296791] [<ffffffc00027a86c>] irq_shutdown+0x68/0x8c
> >>[ 37.302023] [<ffffffc0002777fc>] __free_irq+0x110/0x218
> >>[ 37.307254] [<ffffffc0002779a8>] free_irq+0x54/0x64
> >>[ 37.312138] [<ffffffc00027ba24>] 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.
Oh, I didn't notice Thomas applied those. I'll have to take a look.
Brian
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-30 1:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-23 13:03 [RFC PATCH] pinctrl: rockchip: add irq_shutdown Jeffy Chen
2017-06-29 22:05 ` Brian Norris
2017-06-30 1:24 ` jeffy
2017-06-30 1:29 ` Brian Norris
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).