How to handle a level-triggered interrupt that is slow to de-assert itself

How to handle a level-triggered interrupt that is slow to de-assert itself

[Jamie McClymont]

Hi all,

Background context:

I'm continuing my efforts to reverse-engineer and write a driver for the Goodix GXFP5187 fingerprint sensor in my Huawei Matebook X Pro (the host is an Intel i5-8250U).

The device is connected via SPI plus a GPIO Interrupt pin, defined like so in the ACPI tables:

    GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
        "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,) { 0x0000 }

This line is held down by the device when it has a message for the host, and stays held down until the host finishes reading the message out over SPI.

I'm handling this with a devm_request_threaded_irq-type handler, where the irq part is just "return IRQ_WAKE_THREAD", and the threaded part does all the work. My understanding is that this is a reasonable approach since I don't have tight latency requirements (and the sleeping spi functions are convenient, plus I don't want to introduce any unnecessary jitter to the system) -- please correct me if I shouldn't actually be using a threaded handler here.

---

Here's my problem:

the IRQ line actually stays held down for roughly 180us after I've finished reading out the message over SPI. That means that as soon as the handler finishes, a new one starts, and it reads out corrupted data, since the sensor doesn't have anything to say.

This is okay in theory -- the corrupted message header can be detected by its checksum, and disregarded. However, this leads to a race condition where the chip can decide it DOES have something to say to the host, WHILE the host is reading out the corrupted header. At that point, the two sides de-sync in their ideas of what needs to be read, and everything stops working.

So, I'd like some way to pause interrupt handling for 200us+, and only re-run the handler if the line is still held down after that time.

My first approach was to add a sleep (usleep_range) at the end of the threaded handler, right before returning IRQ_HANDLED. However, it appears that after the sleep finishes, the IRQ is triggered one more time -- presumably it has been set as pending before/during the sleep?

My new workaround is to save a ktime_get_ns timestamp at the end of the handler, and check it against the current ktime at the start, returning early if not enough time has yet elapsed. This is unsatisfactory, as it is effectively a 180us busy-wait, and gets in the way of whatever the core could better be doing (presumably idling and saving power :).

Is it possible to return to the first approach, but prevent that one spurious interrupt from firing after the handler ends?

Thanks
-- Jamie McClymont

Re: How to handle a level-triggered interrupt that is slow to de-assert itself

[Andy Shevchenko]

On Mon, Nov 9, 2020 at 2:57 PM Jamie McClymont <jamie@kwiius.com> wrote:

Looking into the problem I think the better people to answer are ones
from the input subsystem (or closer), so I have added a few to the Cc
list.

> Background context:
>
> I'm continuing my efforts to reverse-engineer and write a driver for the Goodix GXFP5187 fingerprint sensor in my Huawei Matebook X Pro (the host is an Intel i5-8250U).
>
> The device is connected via SPI plus a GPIO Interrupt pin, defined like so in the ACPI tables:
>
>     GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>         "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,) { 0x0000 }
>
> This line is held down by the device when it has a message for the host, and stays held down until the host finishes reading the message out over SPI.
>
> I'm handling this with a devm_request_threaded_irq-type handler, where the irq part is just "return IRQ_WAKE_THREAD", and the threaded part does all the work. My understanding is that this is a reasonable approach since I don't have tight latency requirements (and the sleeping spi functions are convenient, plus I don't want to introduce any unnecessary jitter to the system) -- please correct me if I shouldn't actually be using a threaded handler here.
>
> ---
>
> Here's my problem:
>
> the IRQ line actually stays held down for roughly 180us after I've finished reading out the message over SPI. That means that as soon as the handler finishes, a new one starts, and it reads out corrupted data, since the sensor doesn't have anything to say.
>
> This is okay in theory -- the corrupted message header can be detected by its checksum, and disregarded. However, this leads to a race condition where the chip can decide it DOES have something to say to the host, WHILE the host is reading out the corrupted header. At that point, the two sides de-sync in their ideas of what needs to be read, and everything stops working.
>
> So, I'd like some way to pause interrupt handling for 200us+, and only re-run the handler if the line is still held down after that time.
>
> My first approach was to add a sleep (usleep_range) at the end of the threaded handler, right before returning IRQ_HANDLED. However, it appears that after the sleep finishes, the IRQ is triggered one more time -- presumably it has been set as pending before/during the sleep?
>
> My new workaround is to save a ktime_get_ns timestamp at the end of the handler, and check it against the current ktime at the start, returning early if not enough time has yet elapsed. This is unsatisfactory, as it is effectively a 180us busy-wait, and gets in the way of whatever the core could better be doing (presumably idling and saving power :).
>
> Is it possible to return to the first approach, but prevent that one spurious interrupt from firing after the handler ends?
>
> Thanks
> -- Jamie McClymont



-- 
With Best Regards,
Andy Shevchenko

Re: How to handle a level-triggered interrupt that is slow to de-assert itself

[Felipe Balbi]

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


Hi,

Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> On Mon, Nov 9, 2020 at 2:57 PM Jamie McClymont <jamie@kwiius.com> wrote:
>
> Looking into the problem I think the better people to answer are ones
> from the input subsystem (or closer), so I have added a few to the Cc
> list.
>
>> Background context:
>>
>> I'm continuing my efforts to reverse-engineer and write a driver for
>> the Goodix GXFP5187 fingerprint sensor in my Huawei Matebook X Pro
>> (the host is an Intel i5-8250U).
>>
>> The device is connected via SPI plus a GPIO Interrupt pin, defined
>> like so in the ACPI tables:
>>
>>     GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>>         "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,) { 0x0000 }
>>
>> This line is held down by the device when it has a message for the
>> host, and stays held down until the host finishes reading the message
>> out over SPI.
>>
>> I'm handling this with a devm_request_threaded_irq-type handler,
>> where the irq part is just "return IRQ_WAKE_THREAD", and the threaded

I think you should pass NULL as the top half and make sure you have
IRQF_ONESHOT flag while requesting the interrupt. This way, the line
will be disabled by IRQ subsystem for the duration of the bottom half.

>> part does all the work. My understanding is that this is a reasonable
>> approach since I don't have tight latency requirements (and the
>> sleeping spi functions are convenient, plus I don't want to introduce
>> any unnecessary jitter to the system) -- please correct me if I
>> shouldn't actually be using a threaded handler here.
>>
>> ---
>>
>> Here's my problem:
>>
>> the IRQ line actually stays held down for roughly 180us after I've
>> finished reading out the message over SPI. That means that as soon as
>> the handler finishes, a new one starts, and it reads out corrupted
>> data, since the sensor doesn't have anything to say.
>>
>> This is okay in theory -- the corrupted message header can be
>> detected by its checksum, and disregarded. However, this leads to a
>> race condition where the chip can decide it DOES have something to
>> say to the host, WHILE the host is reading out the corrupted
>> header. At that point, the two sides de-sync in their ideas of what
>> needs to be read, and everything stops working.
>>
>> So, I'd like some way to pause interrupt handling for 200us+, and
>> only re-run the handler if the line is still held down after that
>> time.

usleep_range(180, 200) before exitting the handler? You're in the bottom
half anyway.

>> My first approach was to add a sleep (usleep_range) at the end of the
>> threaded handler, right before returning IRQ_HANDLED. However, it
>> appears that after the sleep finishes, the IRQ is triggered one more
>> time -- presumably it has been set as pending before/during the
>> sleep?
>>
>> My new workaround is to save a ktime_get_ns timestamp at the end of
>> the handler, and check it against the current ktime at the start,
>> returning early if not enough time has yet elapsed. This is
>> unsatisfactory, as it is effectively a 180us busy-wait, and gets in
>> the way of whatever the core could better be doing (presumably idling
>> and saving power :).
>>
>> Is it possible to return to the first approach, but prevent that one
>> spurious interrupt from firing after the handler ends?

IRQF_ONESHOT would probably help with this part, I guess. Could you give
it a shot?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: How to handle a level-triggered interrupt that is slow to de-assert itself

[Hans de Goede]

Hi,

On 11/9/20 3:30 PM, Andy Shevchenko wrote:
> On Mon, Nov 9, 2020 at 2:57 PM Jamie McClymont <jamie@kwiius.com> wrote:
> 
> Looking into the problem I think the better people to answer are ones
> from the input subsystem (or closer), so I have added a few to the Cc
> list.
> 
>> Background context:
>>
>> I'm continuing my efforts to reverse-engineer and write a driver for the Goodix GXFP5187 fingerprint sensor in my Huawei Matebook X Pro (the host is an Intel i5-8250U).
>>
>> The device is connected via SPI plus a GPIO Interrupt pin, defined like so in the ACPI tables:
>>
>>     GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>>         "\\_SB.PCI0.GPI0", 0x00, ResourceConsumer, ,) { 0x0000 }
>>
>> This line is held down by the device when it has a message for the host, and stays held down until the host finishes reading the message out over SPI.
>>
>> I'm handling this with a devm_request_threaded_irq-type handler, where the irq part is just "return IRQ_WAKE_THREAD", and the threaded part does all the work. My understanding is that this is a reasonable approach since I don't have tight latency requirements (and the sleeping spi functions are convenient, plus I don't want to introduce any unnecessary jitter to the system) -- please correct me if I shouldn't actually be using a threaded handler here.
>>
>> ---
>>
>> Here's my problem:
>>
>> the IRQ line actually stays held down for roughly 180us after I've finished reading out the message over SPI. That means that as soon as the handler finishes, a new one starts, and it reads out corrupted data, since the sensor doesn't have anything to say.
>>
>> This is okay in theory -- the corrupted message header can be detected by its checksum, and disregarded. However, this leads to a race condition where the chip can decide it DOES have something to say to the host, WHILE the host is reading out the corrupted header. At that point, the two sides de-sync in their ideas of what needs to be read, and everything stops working.
>>
>> So, I'd like some way to pause interrupt handling for 200us+, and only re-run the handler if the line is still held down after that time.
>>
>> My first approach was to add a sleep (usleep_range) at the end of the threaded handler, right before returning IRQ_HANDLED. However, it appears that after the sleep finishes, the IRQ is triggered one more time -- presumably it has been set as pending before/during the sleep?

That should not happen, as long as the threaded handler clears the interrupt source before it returns,
then the IRQ should not be triggered a second time.

The IRQ will be masked as soon as it registers the first time and then stay masked until
the threaded handler is done, note this behavior requires setting the IRQF_ONESHOT flag.
Which AFAIK is mandatory for threaded handlers anyways unless you are also providing a
non-threaded handler ?

I'm not sure what the conventions are if you supply both and not set IRQF_ONESHOT.

But it sounds to me like you should only provide a threaded-handler and pass
IRQF_ONESHOT when requesting the IRQ.

As long as you do the following:

1) Clear the reason why the device is asserting its IRQ in the threaded handler
2) Wait at minimum any IRQ clearing latency before exiting the threaded handler

Then the IRQ should not fire a second time after 2.

Probably something which you have already tried, but have you tried using a slightly
longer sleep ?

To me using usleep_range at the end of the threaded handler seems like it is
exactly what you should do; and it should work.

Regards,

Hans