request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Ulf Samuelsson]

I am currently working on an I2C driver which requires an irq.

The driver was developed on a Beagleboard running linux-2.6.29-rc3
(Android 1.6) where everything works OK.
I then try the driver on a customer board, where they use 2.6.27.16
(also Android 1.6) on a Qualcomm Snapdragon.

In the "probe" for the driver, I request an irq.
If the code below is active, then the driver completes the "probe"
and can communicate with other I2C peripherals.
(This is verified by listening to the I2C bus communications)
After ~20 I2C messages to 3 different peripherals, the kernel freezes.
The customer board does not have a console port!,
so you can only read the bootlog if the boot succeeds.

If the code below is ifdef'ed away. the boot succeeds.
I have implemented a workaround, using sysfs, which allows me to request
the irq manually after the system has completed the boot.

If I do that, then the interrupts are accepted and the correct interrupt
routine is called.


#ifdef	MAXINT
if (mxt->irq) {
	error = request_irq(
			mxt->irq,
			mxt_irq_handler,
			IRQF_TRIGGER_FALLING,
			client->dev.driver->name,
			mxt
		);
	if (error < 0) {
		dev_err(&client->dev,
			"failed to allocate irq %d\n", mxt->irq);
		goto err_free_device;
	}
}
#endif

Some notes:
"request_irq" completes with (error >= 0) so the error handling
is not processed. This is verified by listening to the I2C bus.

Since we take the appropriate interrupt, "mxt->irq" must be correct.
Since we call the correct irq handler,"mxt_irq_handler" must be correct.
"mxt" is a pointer to the driver local data.
We use the same pointer when we request the irq in sysfs.
Not sure about the contents of "client->dev.driver->name".

If the interrupt is executing, then we would see some I2C communication
as a result, but we do not see this, before the kernel freezes.

The interrupt is (and should be) called on the falling edge of the
interrupt.

I am currently scratching my head, and need help with ideas...


Best Regards
Ulf Samuelsson

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Russell King - ARM Linux]

On Sat, Mar 27, 2010 at 12:16:58AM +0100, Ulf Samuelsson wrote:
> If the interrupt is executing, then we would see some I2C communication
> as a result, but we do not see this, before the kernel freezes.
> 
> The interrupt is (and should be) called on the falling edge of the
> interrupt.
> 
> I am currently scratching my head, and need help with ideas...

Do you always return IRQ_HANDLED from this handler, or do you return
IRQ_NONE if it does no work?

If you always return IRQ_HANDLED even if no work was done, it could be
that you're spinning on this interrupt, and because you're returning
IRQ_HANDLED, the core interrupt handling code thinks progress is being
made.

If you return IRQ_NONE, then the "bad IRQ" detection code will kick in
and disable the IRQ, which should result in some further progress.

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Ulf Samuelsson]

Russell King - ARM Linux skrev:
> On Sat, Mar 27, 2010 at 12:16:58AM +0100, Ulf Samuelsson wrote:
>> If the interrupt is executing, then we would see some I2C communication
>> as a result, but we do not see this, before the kernel freezes.
>>
>> The interrupt is (and should be) called on the falling edge of the
>> interrupt.
>>
>> I am currently scratching my head, and need help with ideas...
> 
> Do you always return IRQ_HANDLED from this handler, or do you return
> IRQ_NONE if it does no work?
> 
> If you always return IRQ_HANDLED even if no work was done, it could be
> that you're spinning on this interrupt, and because you're returning
> IRQ_HANDLED, the core interrupt handling code thinks progress is being
> made.
> 
> If you return IRQ_NONE, then the "bad IRQ" detection code will kick in
> and disable the IRQ, which should result in some further progress.
> 

Thanks for fast reply.

This is my interrupt routine, which always return IRQ_HANDLED.
sysfs shows that "mxt->invalid_irq_counter" is never incremented
even after I successfully enable the interrupt in sysfs.

mxt->dwork will always access the I2C bus but we dont see that.

static irqreturn_t mxt_irq_handler(int irq, void *_mxt)
{
	struct	mxt_data *mxt = _mxt;
	unsigned long	flags;
	mxt->irq_counter++;
	spin_lock_irqsave(&mxt->lock, flags);

	if (mxt_valid_interrupt()) {
		/* Macro, always returning 1  on these boards */
		cancel_delayed_work(&mxt->dwork);
		schedule_delayed_work(&mxt->dwork, 0);
		mxt->valid_irq_counter++;
	} else {
		mxt->invalid_irq_counter++;
	}
	spin_unlock_irqrestore(&mxt->lock, flags);

	return IRQ_HANDLED;
}


I do
	INIT_DELAYED_WORK(&mxt->dwork, mxt_worker);
	spin_lock_init(&mxt->lock);

before I request the irq


BR
Ulf Samuelsson.


> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Ulf Samuelsson]

Ulf Samuelsson skrev:
> Russell King - ARM Linux skrev:
>> On Sat, Mar 27, 2010 at 12:16:58AM +0100, Ulf Samuelsson wrote:
>>> If the interrupt is executing, then we would see some I2C communication
>>> as a result, but we do not see this, before the kernel freezes.
>>>
>>> The interrupt is (and should be) called on the falling edge of the
>>> interrupt.
>>>
>>> I am currently scratching my head, and need help with ideas...
>> Do you always return IRQ_HANDLED from this handler, or do you return
>> IRQ_NONE if it does no work?
>>
>> If you always return IRQ_HANDLED even if no work was done, it could be
>> that you're spinning on this interrupt, and because you're returning
>> IRQ_HANDLED, the core interrupt handling code thinks progress is being
>> made.
>>
>> If you return IRQ_NONE, then the "bad IRQ" detection code will kick in
>> and disable the IRQ, which should result in some further progress.
>>
> 
> Thanks for fast reply.
> 
> This is my interrupt routine, which always return IRQ_HANDLED.
> sysfs shows that "mxt->invalid_irq_counter" is never incremented
> even after I successfully enable the interrupt in sysfs.
> 
> mxt->dwork will always access the I2C bus but we dont see that.
> 
> static irqreturn_t mxt_irq_handler(int irq, void *_mxt)
> {
> 	struct	mxt_data *mxt = _mxt;
> 	unsigned long	flags;
> 	mxt->irq_counter++;
> 	spin_lock_irqsave(&mxt->lock, flags);
> 
> 	if (mxt_valid_interrupt()) {
> 		/* Macro, always returning 1  on these boards */
> 		cancel_delayed_work(&mxt->dwork);
> 		schedule_delayed_work(&mxt->dwork, 0);
> 		mxt->valid_irq_counter++;
> 	} else {
> 		mxt->invalid_irq_counter++;
> 	}
> 	spin_unlock_irqrestore(&mxt->lock, flags);
> 
> 	return IRQ_HANDLED;
> }
> 

I tried doing an I2C transfer inside the interrupt routine
(this is my only way of logging)
and then the I2C probe routine locked up already when
doing the request_irq.


I do not know a lot about the internals of  "schedule_delayed_work"
at the moment - Have to start digging I guess.
I have assumed so far, that if this is called,then mxt->dwork will be
executed.

Can it be so, that nothing happens and the situation you describe
above occur?

The interrupt line will, after asserted low, remain low
until a status register is read over the I2C bus,
and the interrupt is triggered by falling edge
so I assume that the interrupt should only be called once.

If the interrupt is enabled by the register_irq, and it does
occur during the probe, and not afterwards,
can this affect the mxt_worker routine?

The probe does a significant amount of i2c communication
after the register_irq, and if the interrupt has been asserted
already, the interrupt will be deasserted before the probe exits.

Somehow I have a feeling that the freeze occurs because
the kernel wants to process the delayed work, sometimes
after the probe exits.

Maybe the i2c communication should occur before the register irq?

BR
Ulf Samuelsson


> 
> I do
> 	INIT_DELAYED_WORK(&mxt->dwork, mxt_worker);
> 	spin_lock_init(&mxt->lock);
> 
> before I request the irq
> 
> 
> BR
> Ulf Samuelsson.
> 
> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Ben Dooks]

On Sat, Mar 27, 2010 at 01:41:52AM +0100, Ulf Samuelsson wrote:
> Ulf Samuelsson skrev:
> > Russell King - ARM Linux skrev:
> >> On Sat, Mar 27, 2010 at 12:16:58AM +0100, Ulf Samuelsson wrote:
> >>> If the interrupt is executing, then we would see some I2C communication
> >>> as a result, but we do not see this, before the kernel freezes.
> >>>
> >>> The interrupt is (and should be) called on the falling edge of the
> >>> interrupt.
> >>>
> >>> I am currently scratching my head, and need help with ideas...
> >> Do you always return IRQ_HANDLED from this handler, or do you return
> >> IRQ_NONE if it does no work?
> >>
> >> If you always return IRQ_HANDLED even if no work was done, it could be
> >> that you're spinning on this interrupt, and because you're returning
> >> IRQ_HANDLED, the core interrupt handling code thinks progress is being
> >> made.
> >>
> >> If you return IRQ_NONE, then the "bad IRQ" detection code will kick in
> >> and disable the IRQ, which should result in some further progress.
> >>
> > 
> > Thanks for fast reply.
> > 
> > This is my interrupt routine, which always return IRQ_HANDLED.
> > sysfs shows that "mxt->invalid_irq_counter" is never incremented
> > even after I successfully enable the interrupt in sysfs.
> > 
> > mxt->dwork will always access the I2C bus but we dont see that.
> > 
> > static irqreturn_t mxt_irq_handler(int irq, void *_mxt)
> > {
> > 	struct	mxt_data *mxt = _mxt;
> > 	unsigned long	flags;
> > 	mxt->irq_counter++;
> > 	spin_lock_irqsave(&mxt->lock, flags);
> > 
> > 	if (mxt_valid_interrupt()) {
> > 		/* Macro, always returning 1  on these boards */
> > 		cancel_delayed_work(&mxt->dwork);
> > 		schedule_delayed_work(&mxt->dwork, 0);
> > 		mxt->valid_irq_counter++;
> > 	} else {
> > 		mxt->invalid_irq_counter++;
> > 	}
> > 	spin_unlock_irqrestore(&mxt->lock, flags);
> > 
> > 	return IRQ_HANDLED;
> > }
> > 
> 
> I tried doing an I2C transfer inside the interrupt routine
> (this is my only way of logging)
> and then the I2C probe routine locked up already when
> doing the request_irq.

i2c transfers can sleep, thus you need somet thread or work-queue to
process them. 

> The interrupt line will, after asserted low, remain low
> until a status register is read over the I2C bus,
> and the interrupt is triggered by falling edge
> so I assume that the interrupt should only be called once.

are you sure the controller is setting the proper irq mode? have
you tried adding a disable_irq_nosync() to the handler?

> If the interrupt is enabled by the register_irq, and it does
> occur during the probe, and not afterwards,
> can this affect the mxt_worker routine?
> 
> The probe does a significant amount of i2c communication
> after the register_irq, and if the interrupt has been asserted
> already, the interrupt will be deasserted before the probe exits.
> 
> Somehow I have a feeling that the freeze occurs because
> the kernel wants to process the delayed work, sometimes
> after the probe exits.
> 
> Maybe the i2c communication should occur before the register irq?
> 
> BR
> Ulf Samuelsson
> 
> 
> > 
> > I do
> > 	INIT_DELAYED_WORK(&mxt->dwork, mxt_worker);
> > 	spin_lock_init(&mxt->lock);
> > 
> > before I request the irq
> > 
> > 
> > BR
> > Ulf Samuelsson.
> > 
> > 
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel at lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Ulf Samuelsson]

Ben Dooks skrev:
> On Sat, Mar 27, 2010 at 01:41:52AM +0100, Ulf Samuelsson wrote:
>> Ulf Samuelsson skrev:
>>> Russell King - ARM Linux skrev:
>>>> On Sat, Mar 27, 2010 at 12:16:58AM +0100, Ulf Samuelsson wrote:
>>>>> If the interrupt is executing, then we would see some I2C communication
>>>>> as a result, but we do not see this, before the kernel freezes.
>>>>>
>>>>> The interrupt is (and should be) called on the falling edge of the
>>>>> interrupt.
>>>>>
>>>>> I am currently scratching my head, and need help with ideas...
>>>> Do you always return IRQ_HANDLED from this handler, or do you return
>>>> IRQ_NONE if it does no work?
>>>>
>>>> If you always return IRQ_HANDLED even if no work was done, it could be
>>>> that you're spinning on this interrupt, and because you're returning
>>>> IRQ_HANDLED, the core interrupt handling code thinks progress is being
>>>> made.
>>>>
>>>> If you return IRQ_NONE, then the "bad IRQ" detection code will kick in
>>>> and disable the IRQ, which should result in some further progress.
>>>>
>>> Thanks for fast reply.
>>>
>>> This is my interrupt routine, which always return IRQ_HANDLED.
>>> sysfs shows that "mxt->invalid_irq_counter" is never incremented
>>> even after I successfully enable the interrupt in sysfs.
>>>
>>> mxt->dwork will always access the I2C bus but we dont see that.
>>>
>>> static irqreturn_t mxt_irq_handler(int irq, void *_mxt)
>>> {
>>> 	struct	mxt_data *mxt = _mxt;
>>> 	unsigned long	flags;
>>> 	mxt->irq_counter++;
>>> 	spin_lock_irqsave(&mxt->lock, flags);
>>>
>>> 	if (mxt_valid_interrupt()) {
>>> 		/* Macro, always returning 1  on these boards */
>>> 		cancel_delayed_work(&mxt->dwork);
>>> 		schedule_delayed_work(&mxt->dwork, 0);
>>> 		mxt->valid_irq_counter++;
>>> 	} else {
>>> 		mxt->invalid_irq_counter++;
>>> 	}
>>> 	spin_unlock_irqrestore(&mxt->lock, flags);
>>>
>>> 	return IRQ_HANDLED;
>>> }
>>>
>> I tried doing an I2C transfer inside the interrupt routine
>> (this is my only way of logging)
>> and then the I2C probe routine locked up already when
>> doing the request_irq.
> 
> i2c transfers can sleep, thus you need somet thread or work-queue to
> process them. 

OK, We quickly came to the conlusion that this was a bad idea...

> 
>> The interrupt line will, after asserted low, remain low
>> until a status register is read over the I2C bus,
>> and the interrupt is triggered by falling edge
>> so I assume that the interrupt should only be called once.
> 
> are you sure the controller is setting the proper irq mode? 

The "request_irq" requests IRQF_TRIGGER_FALLING.
Is this what you mean?

> have
> you tried adding a disable_irq_nosync() to the handler?
> 

No,
I assume that, since I request interrupt on falling edge
the interrupt will not be activated until the target
provides a new falling edge on the interrupt line.

Sine you need to access the chip over I2C for the chip
to do so, and we do not see such packets on the sniffer,
the interrupt should not be reactivated.

Even if I return IRQ_HANDLED, should the low interrupt
cause new interrupts to happen?

One difference between the Beagleboard implementation
which works, and the Snapdragon which does not work,
is that there are multiple targets on the bus of the latter.

Unfortunately, I have no access to the Snapdragon hardware until
after Easter holidays but I will try some ideas from you guys ASAP.
Thanks.

BR
Ulf

>> If the interrupt is enabled by the register_irq, and it does
>> occur during the probe, and not afterwards,
>> can this affect the mxt_worker routine?
>>
>> The probe does a significant amount of i2c communication
>> after the register_irq, and if the interrupt has been asserted
>> already, the interrupt will be deasserted before the probe exits.
>>
>> Somehow I have a feeling that the freeze occurs because
>> the kernel wants to process the delayed work, sometimes
>> after the probe exits.
>>
>> Maybe the i2c communication should occur before the register irq?
>>
>> BR
>> Ulf Samuelsson
>>
>>
>>> I do
>>> 	INIT_DELAYED_WORK(&mxt->dwork, mxt_worker);
>>> 	spin_lock_init(&mxt->lock);
>>>
>>> before I request the irq
>>>
>>>
>>> BR
>>> Ulf Samuelsson.
>>>
>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Russell King - ARM Linux]

On Sat, Mar 27, 2010 at 08:01:41AM +0100, Ulf Samuelsson wrote:
> I assume that, since I request interrupt on falling edge
> the interrupt will not be activated until the target
> provides a new falling edge on the interrupt line.

Does your interrupt controller support falling edge IRQs on the I2C
controller?  Most SoCs provide you with only one fixed triggering
method for on-board devices, which is normally only level sensitive.

request_irq in I2C driver causes kernel to freeze duringprobe, but if done later - no problem!

[Ulf Samuelsson]

> On Sat, Mar 27, 2010 at 08:01:41AM +0100, Ulf Samuelsson wrote:
>> I assume that, since I request interrupt on falling edge
>> the interrupt will not be activated until the target
>> provides a new falling edge on the interrupt line.
>
> Does your interrupt controller support falling edge IRQs on the I2C
> controller?  Most SoCs provide you with only one fixed triggering
> method for on-board devices, which is normally only level sensitive.
>


It is not the I2C interrupt, it is a GPIO interrupt used by the external 
device
indicating it needs service

The problem only occurs when the request_irq is executed in the probe.
I have written a sysfs routine which will do the request_irq, later when I 
get a shell.
With this, rather cumbesome, method, everything seems to work OK.

Best Regards
Ulf Samuelsson

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Mark Brown]

On Sat, Mar 27, 2010 at 01:12:23AM +0000, Ben Dooks wrote:
> On Sat, Mar 27, 2010 at 01:41:52AM +0100, Ulf Samuelsson wrote:

> > I tried doing an I2C transfer inside the interrupt routine
> > (this is my only way of logging)
> > and then the I2C probe routine locked up already when
> > doing the request_irq.

> i2c transfers can sleep, thus you need somet thread or work-queue to
> process them. 

Or with current (post 2.6.29, IIRC 2.6.33 or later) kernels use a
threaded IRQ handler for the I2C interactions.

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Russell King - ARM Linux]

On Sat, Mar 27, 2010 at 01:22:54AM +0100, Ulf Samuelsson wrote:
> Russell King - ARM Linux skrev:
> > On Sat, Mar 27, 2010 at 12:16:58AM +0100, Ulf Samuelsson wrote:
> >> If the interrupt is executing, then we would see some I2C communication
> >> as a result, but we do not see this, before the kernel freezes.
> >>
> >> The interrupt is (and should be) called on the falling edge of the
> >> interrupt.
> >>
> >> I am currently scratching my head, and need help with ideas...
> > 
> > Do you always return IRQ_HANDLED from this handler, or do you return
> > IRQ_NONE if it does no work?
> > 
> > If you always return IRQ_HANDLED even if no work was done, it could be
> > that you're spinning on this interrupt, and because you're returning
> > IRQ_HANDLED, the core interrupt handling code thinks progress is being
> > made.
> > 
> > If you return IRQ_NONE, then the "bad IRQ" detection code will kick in
> > and disable the IRQ, which should result in some further progress.
> > 
> 
> Thanks for fast reply.
> 
> This is my interrupt routine, which always return IRQ_HANDLED.
> sysfs shows that "mxt->invalid_irq_counter" is never incremented
> even after I successfully enable the interrupt in sysfs.
> 
> mxt->dwork will always access the I2C bus but we dont see that.
> 
> static irqreturn_t mxt_irq_handler(int irq, void *_mxt)
> {
> 	struct	mxt_data *mxt = _mxt;
> 	unsigned long	flags;
> 	mxt->irq_counter++;
> 	spin_lock_irqsave(&mxt->lock, flags);
> 
> 	if (mxt_valid_interrupt()) {
> 		/* Macro, always returning 1  on these boards */
> 		cancel_delayed_work(&mxt->dwork);
> 		schedule_delayed_work(&mxt->dwork, 0);
> 		mxt->valid_irq_counter++;
> 	} else {
> 		mxt->invalid_irq_counter++;
> 	}
> 	spin_unlock_irqrestore(&mxt->lock, flags);
> 
> 	return IRQ_HANDLED;

So yes, you're going to spin indefinitely when you receive the first valid
interrupt.

1. Your device raises an interrupt.
2. The CPU gets interrupted.
3. The kernel reads the IRQ number and calls your handler.
4. The handler schedules the delayed work and returns.
5. The kernel checks for any further pending interrupt, and finds the same
   interrupt is still raised, and calls your handler.
6. The handler schedules the delayed work and returns.
7. The kernel checks for any further pending interrupt, and finds the same
   interrupt is still raised, and calls your handler.
8. The handler schedules the delayed work and returns.
9. The kernel checks for any further pending interrupt, and finds the same
   interrupt is still raised, and calls your handler.
10. The handler schedules the delayed work and returns.
11. The kernel checks for any further pending interrupt, and finds the same
   interrupt is still raised, and calls your handler.
...

You must clear the interrupt, or disable the interrupt source.  Or make
use of the kernel's threaded IRQs which will deal with the interrupt
masking and running your handler in thread context for you.

request_irq in I2C driver causes kernel to freeze during probe, but if done later - no problem!

[Russell King - ARM Linux]

On Sat, Mar 27, 2010 at 01:22:54AM +0100, Ulf Samuelsson wrote:
> This is my interrupt routine, which always return IRQ_HANDLED.
> sysfs shows that "mxt->invalid_irq_counter" is never incremented
> even after I successfully enable the interrupt in sysfs.
> 
> mxt->dwork will always access the I2C bus but we dont see that.
> 
> static irqreturn_t mxt_irq_handler(int irq, void *_mxt)
> {
> 	struct	mxt_data *mxt = _mxt;
> 	unsigned long	flags;
> 	mxt->irq_counter++;
> 	spin_lock_irqsave(&mxt->lock, flags);
> 
> 	if (mxt_valid_interrupt()) {
> 		/* Macro, always returning 1  on these boards */
> 		cancel_delayed_work(&mxt->dwork);
> 		schedule_delayed_work(&mxt->dwork, 0);
> 		mxt->valid_irq_counter++;
> 	} else {
> 		mxt->invalid_irq_counter++;

The other thing is that in this case you should ultimately return
IRQ_NONE from this handler.