Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1651756 > unrolled thread

[PATCH v2] genirq: Check irq disabled & masked states in irq_shutdown

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] genirq: Check irq disabled & masked states in irq_shutdown Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-05-27 07:00 +0200
    Re: [PATCH v2] genirq: Check irq disabled & masked states in  irq_shutdown Thomas Gleixner <tglx@linutronix.de> - 2017-05-27 10:40 +0200
      Re: [PATCH v2] genirq: Check irq disabled & masked states in irq_shutdown jeffy <jeffy.chen@rock-chips.com> - 2017-05-27 12:10 +0200
        Re: [PATCH v2] genirq: Check irq disabled & masked states in  irq_shutdown Thomas Gleixner <tglx@linutronix.de> - 2017-05-27 13:00 +0200

#1651756 — [PATCH v2] genirq: Check irq disabled & masked states in irq_shutdown

If a irq is already disabled & masked, free_irq may cause a unbalanced
irq shutdown/disable/mask, for example:

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.

v2: Rewrite commit message.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v2:
Rewrite commit message.

 kernel/irq/chip.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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;
+
+	irq_state_set_disabled(desc);
 	if (desc->irq_data.chip->irq_shutdown)
 		desc->irq_data.chip->irq_shutdown(&desc->irq_data);
 	else if (desc->irq_data.chip->irq_disable)
 		desc->irq_data.chip->irq_disable(&desc->irq_data);
 	else
 		desc->irq_data.chip->irq_mask(&desc->irq_data);
+out:
 	irq_domain_deactivate_irq(&desc->irq_data);
 	irq_state_set_masked(desc);
 }
-- 
2.1.4

[toc] | [next] | [standalone]

#1651806 — Re: [PATCH v2] genirq: Check irq disabled & masked states in irq_shutdown

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().

> 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".

> 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.

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

[toc] | [prev] | [next] | [standalone]

#1651835

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
>
>
>

[toc] | [prev] | [next] | [standalone]

#1651851 — Re: [PATCH v2] genirq: Check irq disabled & masked states in irq_shutdown

On Sat, 27 May 2017, jeffy wrote:
> 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 :)

If we make them balanced then we do it proper and not just for the single
issue you are facing. And we do not try. We analyze it proper and fix it.

It's not rocket science, but yes it takes a bit longer than cobbling
something together which works just for you.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web