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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 11/9/20 3:30 PM, Andy Shevchenko wrote:
> On Mon, Nov 9, 2020 at 2:57 PM Jamie McClymont <jamie@xxxxxxxxxx> 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