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

[Felipe Balbi <balbi@kernel.org>]

[-- 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 --]