[ARM][OMAP] TWL4030 IRQ

[ARM][OMAP] TWL4030 IRQ

[Shilimkar, Santosh]

(resending wrapped message)

Russell, 
> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Saturday, June 20, 2009 8:55 PM
> To: 'Tony Lindgren'; 'linux-omap@vger.kernel.org'
> Cc: 'Russell King - ARM Linux'
> Subject: [ARM][OMAP] TWL4030 IRQ
> 
> Just to brief you all, I came across a dead lock scenario on 
> the arm-smp kernel. After discussing with Russell on mailing 
> list, the actual bug seems be located in twl4030 IRQ implementation.
> 
> Initially I was suspecting the arm-generic area and hence 
> didn't include linux-omap list in the mail thread
> 
> For more information on this issue, please read this email thread.
> http://marc.info/?l=linux-arm-kernel&m=124550947525299&w=2
> 
> I will create a patch for "twl4030" and also would be 
> correcting upcoming "twl6030" accordingly.
> 
> If anyone on this mailing list has any other opinion, please 
> react to the email thread. 


After digging a bit, the proposed twl_irq change would need some major 
changes.

1. All the T2(TWL)drivers ( rtc, madc, bci, gpio, keypad, usb, mmc)needs
modification.Currently twl_irq fw implements kind of "twl4030_irq_chip" 
which allows all the T2 drivers to get virtual interrupt numbers and can
make use of standard "request_irq" linux api.
2.Above also helps to have useful entries 
like /proc/interrupts/rtc etc.
With proposed change this also won't be available.
3. Register/unregister irq handlers to T2 from all the drivers

On the reported lock-up issue, I tried alternative to call mask/unmask 
functions with disabling local cpu interrupts.
This worked for me but not sure whether it is entirely correct.

Something like this:
 static void handle_twl4030_pih(unsigned int irq, irq_desc_t *desc)
 {
+	unsigned long flags;
+
 	/* Acknowledge, clear *AND* mask the interrupt... */
+	local_irq_save(flags);
 	desc->chip->ack(irq);
+	local_irq_restore(flags);
 	complete(&irq_event);
 }

Do you think above approach is fine to get around the spin-lock lockup
issue ? 
 
Regards,
Santosh

Re: [ARM][OMAP] TWL4030 IRQ

[Russell King - ARM Linux]

On Wed, Jun 24, 2009 at 01:16:52PM +0530, Shilimkar, Santosh wrote:
> After digging a bit, the proposed twl_irq change would need some major 
> changes.
> 
> 1. All the T2(TWL)drivers ( rtc, madc, bci, gpio, keypad, usb, mmc)needs
> modification.Currently twl_irq fw implements kind of "twl4030_irq_chip" 
> which allows all the T2 drivers to get virtual interrupt numbers and can
> make use of standard "request_irq" linux api.

You can still use virtual interrupt numbers.  All you're changing is
_how_ you're handling the interrupts internally.

I'll see about creating a patch tomorrow.

Re: [ARM][OMAP] TWL4030 IRQ

[Russell King - ARM Linux]

On Wed, Jun 24, 2009 at 08:57:56AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 24, 2009 at 01:16:52PM +0530, Shilimkar, Santosh wrote:
> > After digging a bit, the proposed twl_irq change would need some major 
> > changes.
> > 
> > 1. All the T2(TWL)drivers ( rtc, madc, bci, gpio, keypad, usb, mmc)needs
> > modification.Currently twl_irq fw implements kind of "twl4030_irq_chip" 
> > which allows all the T2 drivers to get virtual interrupt numbers and can
> > make use of standard "request_irq" linux api.
> 
> You can still use virtual interrupt numbers.  All you're changing is
> _how_ you're handling the interrupts internally.
> 
> I'll see about creating a patch tomorrow.

And here it is - I've only build-tested it so far.

 drivers/mfd/twl4030-irq.c |   55 ++++++++++++++++++--------------------------
 1 files changed, 23 insertions(+), 32 deletions(-)

diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index bae61b2..4bb1ea7 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -180,15 +180,9 @@ static struct completion irq_event;
 static int twl4030_irq_thread(void *data)
 {
 	long irq = (long)data;
-	struct irq_desc *desc = irq_to_desc(irq);
 	static unsigned i2c_errors;
 	static const unsigned max_i2c_errors = 100;
 
-	if (!desc) {
-		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
-		return -EINVAL;
-	}
-
 	current->flags |= PF_NOFREEZE;
 
 	while (!kthread_should_stop()) {
@@ -240,38 +234,25 @@ static int twl4030_irq_thread(void *data)
 		}
 		local_irq_enable();
 
-		desc->chip->unmask(irq);
+		enable_irq(irq);
 	}
 
 	return 0;
 }
 
 /*
- * handle_twl4030_pih() is the desc->handle method for the twl4030 interrupt.
- * This is a chained interrupt, so there is no desc->action method for it.
+ * handle_twl4030_pih() is the handler for the main twl4030 interrupt.
  * Now we need to query the interrupt controller in the twl4030 to determine
  * which module is generating the interrupt request.  However, we can't do i2c
  * transactions in interrupt context, so we must defer that work to a kernel
  * thread.  All we do here is acknowledge and mask the interrupt and wakeup
  * the kernel thread.
  */
-static void handle_twl4030_pih(unsigned int irq, struct irq_desc *desc)
-{
-	/* Acknowledge, clear *AND* mask the interrupt... */
-	desc->chip->ack(irq);
-	complete(&irq_event);
-}
-
-static struct task_struct *start_twl4030_irq_thread(long irq)
+static irqreturn_t handle_twl4030_pih(int irq, void *devid)
 {
-	struct task_struct *thread;
-
-	init_completion(&irq_event);
-	thread = kthread_run(twl4030_irq_thread, (void *)irq, "twl4030-irq");
-	if (!thread)
-		pr_err("twl4030: could not create irq %ld thread!\n", irq);
-
-	return thread;
+	disable_irq_nosync(irq);
+	complete(devid);
+	return IRQ_HANDLED;
 }
 
 /*----------------------------------------------------------------------*/
@@ -734,18 +715,28 @@ int twl_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
 	}
 
 	/* install an irq handler to demultiplex the TWL4030 interrupt */
-	task = start_twl4030_irq_thread(irq_num);
-	if (!task) {
-		pr_err("twl4030: irq thread FAIL\n");
-		status = -ESRCH;
-		goto fail;
+	init_completion(&irq_event);
+
+	status = request_irq(irq_num, handle_twl4030_pih, IRQF_DISABLED,
+				"TWL4030-PIH", &irq_event);
+	if (status < 0) {
+		pr_err("twl4030: could not claim irq%d: %d\n", irq_num, status);
+		goto fail_rqirq;
 	}
 
-	set_irq_data(irq_num, task);
-	set_irq_chained_handler(irq_num, handle_twl4030_pih);
+	task = kthread_run(twl4030_irq_thread, (void *)irq_num, "twl4030-irq");
+	if (IS_ERR(task)) {
+		pr_err("twl4030: could not create irq %d thread!\n", irq_num);
+		status = PTR_ERR(task);
+		goto fail_kthread;
+	}
 
 	return status;
 
+fail_kthread:
+	free_irq(irq_num, &irq_event);
+fail_rqirq:
+	/* clean up twl4030_sih_setup */
 fail:
 	for (i = irq_base; i < irq_end; i++)
 		set_irq_chip_and_handler(i, NULL, NULL);

RE: [ARM][OMAP] TWL4030 IRQ

[Shilimkar, Santosh]

> And here it is - I've only build-tested it so far.
> 
>  drivers/mfd/twl4030-irq.c |   55 
> ++++++++++++++++++--------------------------
>  1 files changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> index bae61b2..4bb1ea7 100644
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -180,15 +180,9 @@ static struct completion irq_event;
>  static int twl4030_irq_thread(void *data)
>  {
>  	long irq = (long)data;
> -	struct irq_desc *desc = irq_to_desc(irq);
>  	static unsigned i2c_errors;
>  	static const unsigned max_i2c_errors = 100;
>  
> -	if (!desc) {
> -		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
> -		return -EINVAL;
> -	}
> -
>  	current->flags |= PF_NOFREEZE;
>  
>  	while (!kthread_should_stop()) {
> @@ -240,38 +234,25 @@ static int twl4030_irq_thread(void *data)
>  		}
>  		local_irq_enable();
>  
> -		desc->chip->unmask(irq);
> +		enable_irq(irq);
>  	}
>  
>  	return 0;
>  }
Russell,
Just a question here.

In the enable_irq(irq) and disable_irq(irq) call tree, internally there are calls to 
the interrupt controller chip.

In disable_irq() path:
	desc->chip->disable(irq);
And in emable_irq() path:
	desc->chip->enable(irq);

But the in gic gic_chip, enable/disable fn are not populated.

static struct irq_chip gic_chip = {
	.name		= "GIC",
	.ack		= gic_ack_irq,
	.mask		= gic_mask_irq,
	.unmask		= gic_unmask_irq,
#ifdef CONFIG_SMP
	.set_affinity	= gic_set_cpu,
#endif
}
Do we need these(disable/enable) hooks in gic_chip as well ?

Regards,
Santosh
 

  

Re: [ARM][OMAP] TWL4030 IRQ

[Russell King - ARM Linux]

On Mon, Jun 29, 2009 at 09:04:53PM +0530, Shilimkar, Santosh wrote:
> Russell,
> Just a question here.
> 
> In the enable_irq(irq) and disable_irq(irq) call tree, internally there are calls to 
> the interrupt controller chip.
> 
> In disable_irq() path:
> 	desc->chip->disable(irq);
> And in emable_irq() path:
> 	desc->chip->enable(irq);
> 
> But the in gic gic_chip, enable/disable fn are not populated.

There are defaults for these.  The enable method will call the unmask
method.  Disabling of ARM IRQs has always been lazy, and this is no
different here.

> Do we need these(disable/enable) hooks in gic_chip as well ?

No.

Re: [ARM][OMAP] TWL4030 IRQ

[Kevin Hilman]

"Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:

>> And here it is - I've only build-tested it so far.
>> 
>>  drivers/mfd/twl4030-irq.c |   55 
>> ++++++++++++++++++--------------------------
>>  1 files changed, 23 insertions(+), 32 deletions(-)
>> 
>> diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
>> index bae61b2..4bb1ea7 100644
>> --- a/drivers/mfd/twl4030-irq.c
>> +++ b/drivers/mfd/twl4030-irq.c
>> @@ -180,15 +180,9 @@ static struct completion irq_event;
>>  static int twl4030_irq_thread(void *data)
>>  {
>>  	long irq = (long)data;
>> -	struct irq_desc *desc = irq_to_desc(irq);
>>  	static unsigned i2c_errors;
>>  	static const unsigned max_i2c_errors = 100;
>>  
>> -	if (!desc) {
>> -		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
>> -		return -EINVAL;
>> -	}
>> -
>>  	current->flags |= PF_NOFREEZE;
>>  
>>  	while (!kthread_should_stop()) {
>> @@ -240,38 +234,25 @@ static int twl4030_irq_thread(void *data)
>>  		}
>>  		local_irq_enable();
>>  
>> -		desc->chip->unmask(irq);
>> +		enable_irq(irq);
>>  	}
>>  
>>  	return 0;
>>  }
> Russell,
> Just a question here.
>
> In the enable_irq(irq) and disable_irq(irq) call tree, internally there are calls to 
> the interrupt controller chip.
>
> In disable_irq() path:
> 	desc->chip->disable(irq);
> And in emable_irq() path:
> 	desc->chip->enable(irq);

> But the in gic gic_chip, enable/disable fn are not populated.

And if you look at the OMAP's MPU irq_chip implementation, these are 
not populated either.  We rely on the default lazy enable via unmask
and the lazy disable.

Kevin

Re: [ARM][OMAP] TWL4030 IRQ

[Russell King - ARM Linux]

On Mon, Jun 29, 2009 at 10:51:09AM -0700, Kevin Hilman wrote:
> And if you look at the OMAP's MPU irq_chip implementation, these are 
> not populated either.  We rely on the default lazy enable via unmask
> and the lazy disable.

There's no lazy enable - it's immediate.  There's only lazy disable.

Re: [ARM][OMAP] TWL4030 IRQ

[Kevin Hilman]

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> On Mon, Jun 29, 2009 at 10:51:09AM -0700, Kevin Hilman wrote:
>> And if you look at the OMAP's MPU irq_chip implementation, these are 
>> not populated either.  We rely on the default lazy enable via unmask
>> and the lazy disable.
>
> There's no lazy enable - it's immediate.  There's only lazy disable.

Correct, that was a mis-edit.  It should've read: "We rely on the
default enable via unmask and the lazy disable."

Kevin

RE: [ARM][OMAP] TWL4030 IRQ

[Shilimkar, Santosh]

[-- Attachment #1: Type: text/plain, Size: 3788 bytes --]

Russell,
>  drivers/mfd/twl4030-irq.c |   55 
> ++++++++++++++++++--------------------------
>  1 files changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> index bae61b2..4bb1ea7 100644
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -180,15 +180,9 @@ static struct completion irq_event;
>  static int twl4030_irq_thread(void *data)
>  {
>  	long irq = (long)data;
> -	struct irq_desc *desc = irq_to_desc(irq);
>  	static unsigned i2c_errors;
>  	static const unsigned max_i2c_errors = 100;
>  
> -	if (!desc) {
> -		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
> -		return -EINVAL;
> -	}
> -
>  	current->flags |= PF_NOFREEZE;
>  
>  	while (!kthread_should_stop()) {
> @@ -240,38 +234,25 @@ static int twl4030_irq_thread(void *data)
>  		}
>  		local_irq_enable();
>  
> -		desc->chip->unmask(irq);
> +		enable_irq(irq);
>  	}
>  
>  	return 0;
>  }
>  
>  /*
> - * handle_twl4030_pih() is the desc->handle method for the 
> twl4030 interrupt.
> - * This is a chained interrupt, so there is no desc->action 
> method for it.
> + * handle_twl4030_pih() is the handler for the main twl4030 
> interrupt.
>   * Now we need to query the interrupt controller in the 
> twl4030 to determine
>   * which module is generating the interrupt request.  
> However, we can't do i2c
>   * transactions in interrupt context, so we must defer that 
> work to a kernel
>   * thread.  All we do here is acknowledge and mask the 
> interrupt and wakeup
>   * the kernel thread.
>   */
> -static void handle_twl4030_pih(unsigned int irq, struct 
> irq_desc *desc)
> -{
> -	/* Acknowledge, clear *AND* mask the interrupt... */
> -	desc->chip->ack(irq);
> -	complete(&irq_event);
> -}
> -
> -static struct task_struct *start_twl4030_irq_thread(long irq)
> +static irqreturn_t handle_twl4030_pih(int irq, void *devid)
>  {
> -	struct task_struct *thread;
> -
> -	init_completion(&irq_event);
> -	thread = kthread_run(twl4030_irq_thread, (void *)irq, 
> "twl4030-irq");
> -	if (!thread)
> -		pr_err("twl4030: could not create irq %ld 
> thread!\n", irq);
> -
> -	return thread;
> +	disable_irq_nosync(irq);
> +	complete(devid);
> +	return IRQ_HANDLED;
>  }
>  
>  
> /*------------------------------------------------------------
> ----------*/
> @@ -734,18 +715,28 @@ int twl_init_irq(int irq_num, unsigned 
> irq_base, unsigned irq_end)
>  	}
>  
>  	/* install an irq handler to demultiplex the TWL4030 
> interrupt */
> -	task = start_twl4030_irq_thread(irq_num);
> -	if (!task) {
> -		pr_err("twl4030: irq thread FAIL\n");
> -		status = -ESRCH;
> -		goto fail;
> +	init_completion(&irq_event);
> +
> +	status = request_irq(irq_num, handle_twl4030_pih, IRQF_DISABLED,
> +				"TWL4030-PIH", &irq_event);
> +	if (status < 0) {
> +		pr_err("twl4030: could not claim irq%d: %d\n", 
> irq_num, status);
> +		goto fail_rqirq;
>  	}
>  
> -	set_irq_data(irq_num, task);
> -	set_irq_chained_handler(irq_num, handle_twl4030_pih);
> +	task = kthread_run(twl4030_irq_thread, (void *)irq_num, 
> "twl4030-irq");
> +	if (IS_ERR(task)) {
> +		pr_err("twl4030: could not create irq %d 
> thread!\n", irq_num);
> +		status = PTR_ERR(task);
> +		goto fail_kthread;
> +	}
>  
>  	return status;
>  
> +fail_kthread:
> +	free_irq(irq_num, &irq_event);
> +fail_rqirq:
> +	/* clean up twl4030_sih_setup */
>  fail:
>  	for (i = irq_base; i < irq_end; i++)
>  		set_irq_chip_and_handler(i, NULL, NULL);

I have tested this patch on OMAP3430SDP with Triton RTC module and works well. 
Thanks!! 

Kevin/Vikram,
Can this patch be included on omap_pm branch to check for any regression?

Regards,
Santosh
  
 

[-- Attachment #2: twl_mainline_irq.patch --]
[-- Type: application/octet-stream, Size: 2758 bytes --]

---
 drivers/mfd/twl4030-irq.c |   54 ++++++++++++++++++----------------------------
 1 files changed, 22 insertions(+), 32 deletions(-)

Index: linux-2.6/drivers/mfd/twl4030-irq.c
===================================================================
--- linux-2.6.orig/drivers/mfd/twl4030-irq.c
+++ linux-2.6/drivers/mfd/twl4030-irq.c
@@ -180,14 +180,9 @@ static struct completion irq_event;
 static int twl4030_irq_thread(void *data)
 {
 	long irq = (long)data;
-	struct irq_desc *desc = irq_to_desc(irq);
 	static unsigned i2c_errors;
 	static const unsigned max_i2c_errors = 100;
 
-	if (!desc) {
-		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
-		return -EINVAL;
-	}
 
 	current->flags |= PF_NOFREEZE;
 
@@ -240,7 +235,7 @@ static int twl4030_irq_thread(void *data
 		}
 		local_irq_enable();
 
-		desc->chip->unmask(irq);
+		enable_irq(irq);
 	}
 
 	return 0;
@@ -255,25 +250,12 @@ static int twl4030_irq_thread(void *data
  * thread.  All we do here is acknowledge and mask the interrupt and wakeup
  * the kernel thread.
  */
-static void handle_twl4030_pih(unsigned int irq, struct irq_desc *desc)
+static irqreturn_t handle_twl4030_pih(int irq, void *devid)
 {
-	/* Acknowledge, clear *AND* mask the interrupt... */
-	desc->chip->ack(irq);
-	complete(&irq_event);
-}
-
-static struct task_struct *start_twl4030_irq_thread(long irq)
-{
-	struct task_struct *thread;
-
-	init_completion(&irq_event);
-	thread = kthread_run(twl4030_irq_thread, (void *)irq, "twl4030-irq");
-	if (!thread)
-		pr_err("twl4030: could not create irq %ld thread!\n", irq);
-
-	return thread;
+	disable_irq_nosync(irq);
+	complete(devid);
+	return IRQ_HANDLED;
 }
-
 /*----------------------------------------------------------------------*/
 
 /*
@@ -734,18 +716,26 @@ int twl_init_irq(int irq_num, unsigned i
 	}
 
 	/* install an irq handler to demultiplex the TWL4030 interrupt */
-	task = start_twl4030_irq_thread(irq_num);
-	if (!task) {
-		pr_err("twl4030: irq thread FAIL\n");
-		status = -ESRCH;
-		goto fail;
-	}
+	init_completion(&irq_event);
 
-	set_irq_data(irq_num, task);
-	set_irq_chained_handler(irq_num, handle_twl4030_pih);
+	status = request_irq(irq_num, handle_twl4030_pih, IRQF_DISABLED,
+				"TWL4030-PIH", &irq_event);
+	if (status < 0) {
+		pr_err("twl4030: could not claim irq%d: %d\n", irq_num, status);
+		goto fail_rqirq;
+	}
 
+	task = kthread_run(twl4030_irq_thread, (void *)irq_num, "twl4030-irq");
+	if (IS_ERR(task)) {
+		pr_err("twl4030: could not create irq %d thread!\n", irq_num);
+		status = PTR_ERR(task);
+		goto fail_kthread;
+	}
 	return status;
-
+fail_kthread:
+	free_irq(irq_num, &irq_event);
+fail_rqirq:
+	/* clean up twl4030_sih_setup */
 fail:
 	for (i = irq_base; i < irq_end; i++)
 		set_irq_chip_and_handler(i, NULL, NULL);

Re: [ARM][OMAP] TWL4030 IRQ

[Kevin Hilman]

"Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:

> Kevin/Vikram,
> Can this patch be included on omap_pm branch to check for any regression?

Sure, I have it applied to the PM branch locally, but before I push,
can you (or Russell) send me a descriptive changelog for this patch.

Thanks,

Kevin

RE: [ARM][OMAP] TWL4030 IRQ

[Shilimkar, Santosh]

[-- Attachment #1: Type: text/plain, Size: 4381 bytes --]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
> Sent: Wednesday, July 01, 2009 4:36 AM
> To: Shilimkar, Santosh
> Cc: Russell King - ARM Linux; 
> linux-arm-kernel@lists.arm.linux.org.uk; linux-omap@vger.kernel.org
> Subject: Re: [ARM][OMAP] TWL4030 IRQ
> 
> "Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:
> 
> > Kevin/Vikram,
> > Can this patch be included on omap_pm branch to check for 
> any regression?
> 
> Sure, I have it applied to the PM branch locally, but before I push,
> can you (or Russell) send me a descriptive changelog for this patch.

(Here is the patch with some description.)


>From 67d399fd88629f37b8debea1aa51bf20ff8957f6 Mon Sep 17 00:00:00 2001
From: Russell King <rmk+kernel@arm.linux.org.uk>
Date: Wed, 1 Jul 2009 10:31:17 +0530
Subject: [PATCH] ARM: OMAP: TWL4030 IRQ

The TWL4030 IRQ handler has a bug which leads to spinlock lock-up. It is
calling the 'unmask' function in a process context. The mask/unmask/ack
functions are only designed to be called from the IRQ handler code,
or the proper API interfaces found in linux/interrupt.h.

Also there is no need to have IRQ chaining mechanism. The right way to
handle this is to claim the parent interrupt as a standard interrupt
and arrange for handle_twl4030_pih to take care of the rest of the devices.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/mfd/twl4030-irq.c |   54 ++++++++++++++++++++-------------------------
 1 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index aca2670..e30e7bc 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -180,14 +180,9 @@ static struct completion irq_event;
 static int twl4030_irq_thread(void *data)
 {
 	long irq = (long)data;
-	struct irq_desc *desc = irq_to_desc(irq);
 	static unsigned i2c_errors;
 	static const unsigned max_i2c_errors = 100;
 
-	if (!desc) {
-		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
-		return -EINVAL;
-	}
 
 	current->flags |= PF_NOFREEZE;
 
@@ -240,7 +235,7 @@ static int twl4030_irq_thread(void *data)
 		}
 		local_irq_enable();
 
-		desc->chip->unmask(irq);
+		enable_irq(irq);
 	}
 
 	return 0;
@@ -255,23 +250,12 @@ static int twl4030_irq_thread(void *data)
  * thread.  All we do here is acknowledge and mask the interrupt and wakeup
  * the kernel thread.
  */
-static void handle_twl4030_pih(unsigned int irq, irq_desc_t *desc)
+static irqreturn_t handle_twl4030_pih(int irq, void *devid)
 {
 	/* Acknowledge, clear *AND* mask the interrupt... */
-	desc->chip->ack(irq);
-	complete(&irq_event);
-}
-
-static struct task_struct *start_twl4030_irq_thread(long irq)
-{
-	struct task_struct *thread;
-
-	init_completion(&irq_event);
-	thread = kthread_run(twl4030_irq_thread, (void *)irq, "twl4030-irq");
-	if (!thread)
-		pr_err("twl4030: could not create irq %ld thread!\n", irq);
-
-	return thread;
+	disable_irq_nosync(irq);
+	complete(devid);
+	return IRQ_HANDLED;
 }
 
 /*----------------------------------------------------------------------*/
@@ -734,18 +718,28 @@ int twl_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
 	}
 
 	/* install an irq handler to demultiplex the TWL4030 interrupt */
-	task = start_twl4030_irq_thread(irq_num);
-	if (!task) {
-		pr_err("twl4030: irq thread FAIL\n");
-		status = -ESRCH;
-		goto fail;
-	}
 
-	set_irq_data(irq_num, task);
-	set_irq_chained_handler(irq_num, handle_twl4030_pih);
 
-	return status;
+	init_completion(&irq_event);
 
+	status = request_irq(irq_num, handle_twl4030_pih, IRQF_DISABLED,
+				"TWL4030-PIH", &irq_event);
+	if (status < 0) {
+		pr_err("twl4030: could not claim irq%d: %d\n", irq_num, status);
+		goto fail_rqirq;
+	}
+
+	task = kthread_run(twl4030_irq_thread, (void *)irq_num, "twl4030-irq");
+	if (IS_ERR(task)) {
+		pr_err("twl4030: could not create irq %d thread!\n", irq_num);
+		status = PTR_ERR(task);
+		goto fail_kthread;
+	}
+	return status;
+fail_kthread:
+	free_irq(irq_num, &irq_event);
+fail_rqirq:
+	/* clean up twl4030_sih_setup */
 fail:
 	for (i = irq_base; i < irq_end; i++)
 		set_irq_chip_and_handler(i, NULL, NULL);
-- 
1.5.4.7


Regards,
Santosh
 

[-- Attachment #2: 0001-ARM-OMAP-TWL4030-IRQ.patch --]
[-- Type: application/octet-stream, Size: 3572 bytes --]

From 67d399fd88629f37b8debea1aa51bf20ff8957f6 Mon Sep 17 00:00:00 2001
From: Russell King <rmk+kernel@arm.linux.org.uk>
Date: Wed, 1 Jul 2009 10:31:17 +0530
Subject: [PATCH] ARM: OMAP: TWL4030 IRQ

The TWL4030 IRQ handler has a bug which leads to spinlock lock-up. It is
calling the 'unmask' function in a process context. :The mask/unmask/ack
functions are only designed to be called from the IRQ handler code,
or the proper API interfaces found in linux/interrupt.h.

Also there is no need to have IRQ chaining mechanism. The right way to
handle this is to claim the parent interrupt as a standard interrupt
and arrange for handle_twl4030_pih to take care of the rest of the devices.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/mfd/twl4030-irq.c |   54 ++++++++++++++++++++-------------------------
 1 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
index aca2670..e30e7bc 100644
--- a/drivers/mfd/twl4030-irq.c
+++ b/drivers/mfd/twl4030-irq.c
@@ -180,14 +180,9 @@ static struct completion irq_event;
 static int twl4030_irq_thread(void *data)
 {
 	long irq = (long)data;
-	struct irq_desc *desc = irq_to_desc(irq);
 	static unsigned i2c_errors;
 	static const unsigned max_i2c_errors = 100;
 
-	if (!desc) {
-		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
-		return -EINVAL;
-	}
 
 	current->flags |= PF_NOFREEZE;
 
@@ -240,7 +235,7 @@ static int twl4030_irq_thread(void *data)
 		}
 		local_irq_enable();
 
-		desc->chip->unmask(irq);
+		enable_irq(irq);
 	}
 
 	return 0;
@@ -255,23 +250,12 @@ static int twl4030_irq_thread(void *data)
  * thread.  All we do here is acknowledge and mask the interrupt and wakeup
  * the kernel thread.
  */
-static void handle_twl4030_pih(unsigned int irq, irq_desc_t *desc)
+static irqreturn_t handle_twl4030_pih(int irq, void *devid)
 {
 	/* Acknowledge, clear *AND* mask the interrupt... */
-	desc->chip->ack(irq);
-	complete(&irq_event);
-}
-
-static struct task_struct *start_twl4030_irq_thread(long irq)
-{
-	struct task_struct *thread;
-
-	init_completion(&irq_event);
-	thread = kthread_run(twl4030_irq_thread, (void *)irq, "twl4030-irq");
-	if (!thread)
-		pr_err("twl4030: could not create irq %ld thread!\n", irq);
-
-	return thread;
+	disable_irq_nosync(irq);
+	complete(devid);
+	return IRQ_HANDLED;
 }
 
 /*----------------------------------------------------------------------*/
@@ -734,18 +718,28 @@ int twl_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
 	}
 
 	/* install an irq handler to demultiplex the TWL4030 interrupt */
-	task = start_twl4030_irq_thread(irq_num);
-	if (!task) {
-		pr_err("twl4030: irq thread FAIL\n");
-		status = -ESRCH;
-		goto fail;
-	}
 
-	set_irq_data(irq_num, task);
-	set_irq_chained_handler(irq_num, handle_twl4030_pih);
 
-	return status;
+	init_completion(&irq_event);
 
+	status = request_irq(irq_num, handle_twl4030_pih, IRQF_DISABLED,
+				"TWL4030-PIH", &irq_event);
+	if (status < 0) {
+		pr_err("twl4030: could not claim irq%d: %d\n", irq_num, status);
+		goto fail_rqirq;
+	}
+
+	task = kthread_run(twl4030_irq_thread, (void *)irq_num, "twl4030-irq");
+	if (IS_ERR(task)) {
+		pr_err("twl4030: could not create irq %d thread!\n", irq_num);
+		status = PTR_ERR(task);
+		goto fail_kthread;
+	}
+	return status;
+fail_kthread:
+	free_irq(irq_num, &irq_event);
+fail_rqirq:
+	/* clean up twl4030_sih_setup */
 fail:
 	for (i = irq_base; i < irq_end; i++)
 		set_irq_chip_and_handler(i, NULL, NULL);
-- 
1.5.4.7

Re: [ARM][OMAP] TWL4030 IRQ

[Kevin Hilman]

"Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:

> From: "Shilimkar, Santosh" <santosh.shilimkar@ti.com>
> Subject: RE: [ARM][OMAP] TWL4030 IRQ
> To: Kevin Hilman <khilman@deeprootsystems.com>
> CC: Russell King - ARM Linux <linux@arm.linux.org.uk>,
>         "linux-arm-kernel@lists.arm.linux.org.uk"
> 	<linux-arm-kernel@lists.arm.linux.org.uk>,
>         "linux-omap@vger.kernel.org"
> 	<linux-omap@vger.kernel.org>
> Date: Wed, 1 Jul 2009 10:33:13 +0530
>
>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> Sent: Wednesday, July 01, 2009 4:36 AM
>> To: Shilimkar, Santosh
>> Cc: Russell King - ARM Linux; 
>> linux-arm-kernel@lists.arm.linux.org.uk; linux-omap@vger.kernel.org
>> Subject: Re: [ARM][OMAP] TWL4030 IRQ
>> 
>> "Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes:
>> 
>> > Kevin/Vikram,
>> > Can this patch be included on omap_pm branch to check for 
>> any regression?
>> 
>> Sure, I have it applied to the PM branch locally, but before I push,
>> can you (or Russell) send me a descriptive changelog for this patch.
>
> (Here is the patch with some description.)

Thanks, will merge into next PM branch.

Kevin

>
> From 67d399fd88629f37b8debea1aa51bf20ff8957f6 Mon Sep 17 00:00:00 2001
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> Date: Wed, 1 Jul 2009 10:31:17 +0530
> Subject: [PATCH] ARM: OMAP: TWL4030 IRQ
>
> The TWL4030 IRQ handler has a bug which leads to spinlock lock-up. It is
> calling the 'unmask' function in a process context. The mask/unmask/ack
> functions are only designed to be called from the IRQ handler code,
> or the proper API interfaces found in linux/interrupt.h.
>
> Also there is no need to have IRQ chaining mechanism. The right way to
> handle this is to claim the parent interrupt as a standard interrupt
> and arrange for handle_twl4030_pih to take care of the rest of the devices.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  drivers/mfd/twl4030-irq.c |   54 ++++++++++++++++++++-------------------------
>  1 files changed, 24 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> index aca2670..e30e7bc 100644
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -180,14 +180,9 @@ static struct completion irq_event;
>  static int twl4030_irq_thread(void *data)
>  {
>  	long irq = (long)data;
> -	struct irq_desc *desc = irq_to_desc(irq);
>  	static unsigned i2c_errors;
>  	static const unsigned max_i2c_errors = 100;
>  
> -	if (!desc) {
> -		pr_err("twl4030: Invalid IRQ: %ld\n", irq);
> -		return -EINVAL;
> -	}
>  
>  	current->flags |= PF_NOFREEZE;
>  
> @@ -240,7 +235,7 @@ static int twl4030_irq_thread(void *data)
>  		}
>  		local_irq_enable();
>  
> -		desc->chip->unmask(irq);
> +		enable_irq(irq);
>  	}
>  
>  	return 0;
> @@ -255,23 +250,12 @@ static int twl4030_irq_thread(void *data)
>   * thread.  All we do here is acknowledge and mask the interrupt and wakeup
>   * the kernel thread.
>   */
> -static void handle_twl4030_pih(unsigned int irq, irq_desc_t *desc)
> +static irqreturn_t handle_twl4030_pih(int irq, void *devid)
>  {
>  	/* Acknowledge, clear *AND* mask the interrupt... */
> -	desc->chip->ack(irq);
> -	complete(&irq_event);
> -}
> -
> -static struct task_struct *start_twl4030_irq_thread(long irq)
> -{
> -	struct task_struct *thread;
> -
> -	init_completion(&irq_event);
> -	thread = kthread_run(twl4030_irq_thread, (void *)irq, "twl4030-irq");
> -	if (!thread)
> -		pr_err("twl4030: could not create irq %ld thread!\n", irq);
> -
> -	return thread;
> +	disable_irq_nosync(irq);
> +	complete(devid);
> +	return IRQ_HANDLED;
>  }
>  
>  /*----------------------------------------------------------------------*/
> @@ -734,18 +718,28 @@ int twl_init_irq(int irq_num, unsigned irq_base, unsigned irq_end)
>  	}
>  
>  	/* install an irq handler to demultiplex the TWL4030 interrupt */
> -	task = start_twl4030_irq_thread(irq_num);
> -	if (!task) {
> -		pr_err("twl4030: irq thread FAIL\n");
> -		status = -ESRCH;
> -		goto fail;
> -	}
>  
> -	set_irq_data(irq_num, task);
> -	set_irq_chained_handler(irq_num, handle_twl4030_pih);
>  
> -	return status;
> +	init_completion(&irq_event);
>  
> +	status = request_irq(irq_num, handle_twl4030_pih, IRQF_DISABLED,
> +				"TWL4030-PIH", &irq_event);
> +	if (status < 0) {
> +		pr_err("twl4030: could not claim irq%d: %d\n", irq_num, status);
> +		goto fail_rqirq;
> +	}
> +
> +	task = kthread_run(twl4030_irq_thread, (void *)irq_num, "twl4030-irq");
> +	if (IS_ERR(task)) {
> +		pr_err("twl4030: could not create irq %d thread!\n", irq_num);
> +		status = PTR_ERR(task);
> +		goto fail_kthread;
> +	}
> +	return status;
> +fail_kthread:
> +	free_irq(irq_num, &irq_event);
> +fail_rqirq:
> +	/* clean up twl4030_sih_setup */
>  fail:
>  	for (i = irq_base; i < irq_end; i++)
>  		set_irq_chip_and_handler(i, NULL, NULL);
> -- 
> 1.5.4.7
>
>
> Regards,

RE: [ARM][OMAP] TWL4030 IRQ

[Shilimkar, Santosh]

Russell, 

After digging a bit, the proposed twl_irq change would need some major changes.

1. All the T2(TWL)drivers ( rtc, madc, bci, gpio, keypad, usb, mmc)needs modification. Currently twl_irq fw implements kind of "twl4030_irq_chip" which allows all the T2 drivers to get virtual interrupt numbers and can make use of standard "request_irq" linux api.
2.Above also helps to have useful entries like /proc/interrupts/rtc etc. With proposed change this also won't be available.
3. Register/unregister irq handlers to T2 from all the drivers


On the reported lock-up issue, I tried alternative to call mask/unmask functions with disabling local cpu interrupts. This worked for me but not sure whether it is entirely correct.

Something like this:
 static void handle_twl4030_pih(unsigned int irq, irq_desc_t *desc)
 {
+	unsigned long flags;
+
 	/* Acknowledge, clear *AND* mask the interrupt... */
+	local_irq_save(flags);
 	desc->chip->ack(irq);
+	local_irq_restore(flags);
 	complete(&irq_event);
 }

Do you think above approach is fine to get around the spin-lock lockup issue ? 
 
> -----Original Message-----
> From: Shilimkar, Santosh 
> Sent: Saturday, June 20, 2009 8:55 PM
> To: 'Tony Lindgren'; 'linux-omap@vger.kernel.org'
> Cc: 'Russell King - ARM Linux'
> Subject: [ARM][OMAP] TWL4030 IRQ
> 
> Just to brief you all, I came across a dead lock scenario on 
> the arm-smp kernel. After discussing with Russell on mailing 
> list, the actual bug seems be located in twl4030 IRQ implementation.
> 
> Initially I was suspecting the arm-generic area and hence 
> didn't include linux-omap list in the mail thread
> 
> For more information on this issue, please read this email thread.
> http://marc.info/?l=linux-arm-kernel&m=124550947525299&w=2
> 
> I will create a patch for "twl4030" and also would be 
> correcting upcoming "twl6030" accordingly.
> 
> If anyone on this mailing list has any other opinion, please 
> react to the email thread. 


Regards,
Santosh 

Re: [ARM][OMAP] TWL4030 IRQ

[Russell King - ARM Linux]

On Wed, Jun 24, 2009 at 10:37:45AM +0530, Shilimkar, Santosh wrote:
> Russell, 
> 
> After digging a bit, the proposed twl_irq change would need some major
> 
> 1. All the T2(TWL)drivers ( rtc, madc, bci, gpio, keypad, usb, mmc)needs
> 2.Above also helps to have useful entries like /proc/interrupts/rtc etc.
> 3. Register/unregister irq handlers to T2 from all the drivers

Please resend, wrapping your message sensibly.  Then I'll reply.  Thanks.

[ARM][OMAP] TWL4030 IRQ

[Shilimkar, Santosh]

Just to brief you all, I came across a dead lock scenario on the arm-smp kernel. After discussing with Russell on mailing list, the actual bug seems be located in twl4030 IRQ implementation.

Initially I was suspecting the arm-generic area and hence didn't include linux-omap list in the mail thread

For more information on this issue, please read this email thread.
http://marc.info/?l=linux-arm-kernel&m=124550947525299&w=2

I will create a patch for "twl4030" and also would be correcting upcoming "twl6030" accordingly.

If anyone on this mailing list has any other opinion, please react to the email thread. 

Thanks !!

Regards,
Santosh