Re: [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32

Re: [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32

[Ben Nizette]

On Tue, 2009-04-14 at 14:59 +1000, Ben Nizette wrote:

> Note it's that very last line which, when triggered, causes the hang.
> As you can see, it's the only one connected through the gpio interrupt
> chip.

Okhayy..  This is probably a red herring.  I just connected a button to
a gpio irq on that board and it sat there happily interrupting all night
long.

If anyone's reading with an ADS7846 on some other arch can they try to
reproduce?

If no one beats me to it i'll try and scatter printks and narrow down
exactly *where* it all explodes tomorrow.

Thx,
	--Ben.

Re: [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32

[Ben Nizette]

On Tue, 2009-04-14 at 18:37 +1000, Ben Nizette wrote:

> If no one beats me to it i'll try and scatter printks and narrow down
> exactly *where* it all explodes tomorrow.

static irqreturn_t ads7846_irq(int irq, void *handle)
{
	struct ads7846 *ts = handle;
	unsigned long flags;

	spin_lock_irqsave(&ts->lock, flags);
	if (likely(get_pendown_state(ts))) {
		if (!ts->irq_disabled) {
			/* The ARM do_simple_IRQ() dispatcher doesn't act
			 * like the other dispatchers:  it will report IRQs
			 * even after they've been disabled.  We work around
			 * that here.  (The "generic irq" framework may help...)
			 */
			ts->irq_disabled = 1;
			disable_irq(ts->spi->irq);
			ts->pending = 1;
			hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
					HRTIMER_MODE_REL);
		}
	}
	spin_unlock_irqrestore(&ts->lock, flags);

	return IRQ_HANDLED;
}

Right, a scattering of printks in there shows execution gets at least as
far as inside the inner-most code block.  A printk directly before the
disable_irq() doesn't make it to my console so I'm guessing that the
hang is in there somewhere.

Hope this helps,
	--Ben.

Re: [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32

[Haavard Skinnemoen]

Ben Nizette wrote:
> static irqreturn_t ads7846_irq(int irq, void *handle)
> {
> 	struct ads7846 *ts = handle;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&ts->lock, flags);
> 	if (likely(get_pendown_state(ts))) {
> 		if (!ts->irq_disabled) {
> 			/* The ARM do_simple_IRQ() dispatcher doesn't act
> 			 * like the other dispatchers:  it will report IRQs
> 			 * even after they've been disabled.  We work around
> 			 * that here.  (The "generic irq" framework may help...)
> 			 */
> 			ts->irq_disabled = 1;
> 			disable_irq(ts->spi->irq);

Shouldn't that be disable_irq_nosync()?

Haavard

Re: [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32

[Haavard Skinnemoen]

Haavard Skinnemoen wrote:
> Ben Nizette wrote:
> > static irqreturn_t ads7846_irq(int irq, void *handle)
> > {
> > 	struct ads7846 *ts = handle;
> > 	unsigned long flags;
> > 
> > 	spin_lock_irqsave(&ts->lock, flags);
> > 	if (likely(get_pendown_state(ts))) {
> > 		if (!ts->irq_disabled) {
> > 			/* The ARM do_simple_IRQ() dispatcher doesn't act
> > 			 * like the other dispatchers:  it will report IRQs
> > 			 * even after they've been disabled.  We work around
> > 			 * that here.  (The "generic irq" framework may help...)
> > 			 */
> > 			ts->irq_disabled = 1;
> > 			disable_irq(ts->spi->irq);  
> 
> Shouldn't that be disable_irq_nosync()?

Ok, simply stating that without providing an explanation was probably
not so helpful...

I think the problem is that disable_irq() calls synchronize_irq(),
which waits until the corresponding interrupt handler is no longer
running on any CPU. And since we're calling it from the interrupt
handler, we'll have a deadlock because the interrupt handler won't
return until synchronize_irq() returns, and that won't happen until the
interrupt handler returns and so on.

So I think changing disable_irq() into disable_irq_nosync() (which
doesn't call synchronize_irq()) will fix it, though I'm not sure what
difference the threaded interrupt handling code makes -- AFAICT, this
code has always been dangerous.

Haavard

Re: [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32

[Bill Gatliff]

Haavard Skinnemoen wrote:
> AFAICT, this code has always been dangerous.
>   

Can you propose a better way to handle the problem?  It's a case that 
crops up with a lot of touch screen controllers.


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

Re: [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32

[Haavard Skinnemoen]

Bill Gatliff wrote:
> Haavard Skinnemoen wrote:
> > AFAICT, this code has always been dangerous.
> >     
> 
> Can you propose a better way to handle the problem?  It's a case that 
> crops up with a lot of touch screen controllers.

I thought I already did: Use disable_irq_nosync() instead.

Haavard

Re: [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32

[Bill Gatliff]

Haavard Skinnemoen wrote:
> Bill Gatliff wrote:
>   
>> Haavard Skinnemoen wrote:
>>     
>>> AFAICT, this code has always been dangerous.
>>>     
>>>       
>> Can you propose a better way to handle the problem?  It's a case that 
>> crops up with a lot of touch screen controllers.
>>     
>
> I thought I already did: Use disable_irq_nosync() instead.
>   

Aah.  I misinterpreted what you wrote.  Thanks!


b.g.

-- 
Bill Gatliff
bgat@billgatliff.com

[PATCH] ads7846: fix unsafe disable_irq (was [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32)

[Ben Nizette]

On Wed, 2009-04-15 at 09:57 +0200, Haavard Skinnemoen wrote:
> Shouldn't that be disable_irq_nosync()?

Indeed, good catch.  That fixes it.

--------------8<--------------------

The use of disable_irq inside the handler for the interrupt being
disabled has always been dangerous.  disable_irq should wait for that
handler to complete before returning -> deadlock.

For some reason this wasn't actually the case until 3aa551c9b was merged
but since this time, the ads7846 driver has deadlocked the system on
first interrupt.

Convert the driver to use the handler-safe _nosync variant.

Signed-off-by: Ben Nizette <bn@niasdigital.com>

---

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 61b4a02..f2513e6 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -746,7 +746,7 @@ static irqreturn_t ads7846_irq(int irq, void *handle)
 			 * that here.  (The "generic irq" framework may help...)
 			 */
 			ts->irq_disabled = 1;
-			disable_irq(ts->spi->irq);
+			disable_irq_nosync(ts->spi->irq);
 			ts->pending = 1;
 			hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
 					HRTIMER_MODE_REL);

Re: [PATCH] ads7846: fix unsafe disable_irq (was [REGRESSION] threaded interrupt handler support breaks (some) irq handling on AVR32)

[Dmitry Torokhov]

On Thu, Apr 16, 2009 at 10:41:57AM +1000, Ben Nizette wrote:
> On Wed, 2009-04-15 at 09:57 +0200, Haavard Skinnemoen wrote:
> > Shouldn't that be disable_irq_nosync()?
> 
> Indeed, good catch.  That fixes it.
> 
> --------------8<--------------------
> 
> The use of disable_irq inside the handler for the interrupt being
> disabled has always been dangerous.  disable_irq should wait for that
> handler to complete before returning -> deadlock.
> 
> For some reason this wasn't actually the case until 3aa551c9b was merged
> but since this time, the ads7846 driver has deadlocked the system on
> first interrupt.
> 
> Convert the driver to use the handler-safe _nosync variant.
> 

Applied, thank you very much Ben.

-- 
Dmitry