Threaded irq handler question

Threaded irq handler question

[Will Newton]

Hi all,

I have a threaded irq handler attached to a level-triggered gpio
interrupt line. The check handler checks the status of the gpio line
and disables the irq line amd returns WAKE_THREAD in that case:

static irqreturn_t isr_check(int irq, void *p)
{
        if (gpio_status()) {
                disable_irq_nosync(irq);
                return IRQ_WAKE_THREAD;
        }

        return IRQ_NONE;
}

The thread then does some i2c transactions that will eventually
deassert the gpio line:

static irqreturn_t isr(int irq, void *p)
{
        /* do some i2c transfers */
        enable_irq(irq);
        return IRQ_HANDLED;
}

My problem is that this structure does not work, because once I call
disable_irq_nosync() on the irq in the check handler the thread will
no longer run because the irq is disabled. However if I don't call
disable_irq_nosync() I will get endless irqs because the line is
level-triggered and will not be deasserted until the thread has run.

Could someone tell me what I'm doing wrong here?

Thanks,

Re: Threaded irq handler question

[Jonathan Corbet]

On Wed, 21 Apr 2010 17:35:32 +0100
Will Newton <will.newton@gmail.com> wrote:

> My problem is that this structure does not work, because once I call
> disable_irq_nosync() on the irq in the check handler the thread will
> no longer run because the irq is disabled. However if I don't call
> disable_irq_nosync() I will get endless irqs because the line is
> level-triggered and will not be deasserted until the thread has run.

Trying to disable IRQs at this level is the wrong approach.  You need to
do enough in the primary interrupt handler to cause the hardware to
stop interrupting in the first place; usually that's just a matter of
some sort of acknowledgment.  Then the threaded handler can move data
around in peace.

jon

Re: Threaded irq handler question

[Will Newton]

On Wed, Apr 21, 2010 at 6:28 PM, Jonathan Corbet <corbet@lwn.net> wrote:
> On Wed, 21 Apr 2010 17:35:32 +0100
> Will Newton <will.newton@gmail.com> wrote:
>
>> My problem is that this structure does not work, because once I call
>> disable_irq_nosync() on the irq in the check handler the thread will
>> no longer run because the irq is disabled. However if I don't call
>> disable_irq_nosync() I will get endless irqs because the line is
>> level-triggered and will not be deasserted until the thread has run.
>
> Trying to disable IRQs at this level is the wrong approach.  You need to
> do enough in the primary interrupt handler to cause the hardware to
> stop interrupting in the first place; usually that's just a matter of
> some sort of acknowledgment.  Then the threaded handler can move data
> around in peace.

Unfortunately this device has no way of doing that - deasserting the
interrupt line involves doing i2c transactions which will likely sleep
so cannot be done in the check handler. This is quite a common problem
for i2c connected devices.

Re: Threaded irq handler question

[Yong Zhang]

On Wed, Apr 21, 2010 at 05:35:32PM +0100, Will Newton wrote:
> Hi all,
> My problem is that this structure does not work, because once I call
> disable_irq_nosync() on the irq in the check handler the thread will
> no longer run because the irq is disabled. However if I don't call
> disable_irq_nosync() I will get endless irqs because the line is
> level-triggered and will not be deasserted until the thread has run.
> 
> Could someone tell me what I'm doing wrong here?

Does IRQF_ONESHOT meet your need?

Thanks,
Yong

Re: Threaded irq handler question

[Will Newton]

On Thu, Apr 22, 2010 at 2:37 AM, Yong Zhang <yong.zhang@windriver.com> wrote:
> On Wed, Apr 21, 2010 at 05:35:32PM +0100, Will Newton wrote:
>> Hi all,
>> My problem is that this structure does not work, because once I call
>> disable_irq_nosync() on the irq in the check handler the thread will
>> no longer run because the irq is disabled. However if I don't call
>> disable_irq_nosync() I will get endless irqs because the line is
>> level-triggered and will not be deasserted until the thread has run.
>>
>> Could someone tell me what I'm doing wrong here?
>
> Does IRQF_ONESHOT meet your need?

Almost I think, but I believe if my check handler does not wake the
thread then I don't get another interrupt ever. I can fix this by
making my check handler always return WAKE_THREAD, which is slightly
sub-optimal, but not a big problem.

Thank you for your help!

Re: Threaded irq handler question

[Mark Brown]

On Thu, Apr 22, 2010 at 11:51:06AM +0100, Will Newton wrote:
> On Thu, Apr 22, 2010 at 2:37 AM, Yong Zhang <yong.zhang@windriver.com> wrote:

> > Does IRQF_ONESHOT meet your need?

> Almost I think, but I believe if my check handler does not wake the
> thread then I don't get another interrupt ever. I can fix this by
> making my check handler always return WAKE_THREAD, which is slightly
> sub-optimal, but not a big problem.

You shouldn't have a primary IRQ handler at all for I2C type devices
where there's no ability to interact with the chip.

[Sorry, hadn't noticed this subthread.]

Re: Threaded irq handler question

[Mark Brown]

On Wed, Apr 21, 2010 at 05:35:32PM +0100, Will Newton wrote:

> I have a threaded irq handler attached to a level-triggered gpio
> interrupt line. The check handler checks the status of the gpio line
> and disables the irq line amd returns WAKE_THREAD in that case:

...

> My problem is that this structure does not work, because once I call
> disable_irq_nosync() on the irq in the check handler the thread will
> no longer run because the irq is disabled. However if I don't call
> disable_irq_nosync() I will get endless irqs because the line is
> level-triggered and will not be deasserted until the thread has run.

> Could someone tell me what I'm doing wrong here?

The genirq framework has native support for doing this using a oneshot
IRQ handler - if you request the IRQ with IRQF_ONESHOT and provide only 
a threaded IRQ handler then genirq will disable the interrupt when the
primary IRQ fires and schedule the threaded handler.

See wm831x or wm8350 for an example.