Quoting Bjorn Andersson (2025-03-16 19:49:01) > On Sat, Mar 15, 2025 at 12:07:14AM -0700, Stephen Boyd wrote: > > Quoting Stephan Gerhold (2025-03-12 06:19:27) > > > > > > Whether to report interrupts that came in while the IRQ was unclaimed > > > doesn't seem to be well-defined in the Linux IRQ API. However, looking > > > closer at these specific cases, we're actually reporting events that do not > > > match the interrupt type requested by the driver: > > > > > > 1. After "ok 1 tlmm_test_silent_rising", the GPIO is in low state and > > > configured for IRQF_TRIGGER_RISING. > > > > > > 2. (a) In preparation for "tlmm_test_silent_falling", the GPIO is switched > > > to high state. The rising interrupt gets latched. > > > > Is the interrupt unmasked here while the test is driving the GPIO line > > high and the interrupt trigger is IRQF_TRIGGER_RISING? If so, this is > > correct behavior. > > > > Why wouldn't the trigger be set to IRQF_TRIGGER_FALLING, then the GPIO > > driven high, and then the GPIO driven low for the test to confirm > > falling edges work? > > > > So you're saying that the interrupt consumer needs to take into > consideration any previous interrupt handler being setup for this GPIO? No. > > Test #1 request the interrupt as rising then releases the interrupt, > then before initiating test #2 the GPIO line is driven high, the > interrupt is requested FALLING and the test is that we don't get any > interrupts. Ok. In this case maybe we should mask the irq in struct irq_chip::irq_shutdown() and clear out any interrupt that comes in because we touched the TLMM hardware. I don't see why we want to continue to monitor the GPIO when the interrupt handler is removed. > > > Have you seen the big comment in msm_gpio_irq_mask() and how it says we > > want to latch edge interrupts even when the interrupt is masked? > > > > So if the bootloader (or hardware default?) configures an interrupt for > e.g. RISING, and sometime during boot there's a rising edge, then a > client driver should expect to get a spurious interrupt? No? If there isn't an interrupt handler registered we shouldn't be latching the interrupt. > > > > (b) The GPIO is re-configured for IRQF_TRIGGER_FALLING, but the latched > > > interrupt isn't cleared. > > > (c) The IRQ handler is called for the latched interrupt, but there > > > wasn't any falling edge. > > > > > > 3. (a) For "tlmm_test_silent_low", the GPIO remains in high state. > > > (b) The GPIO is re-configured for IRQF_TRIGGER_LOW. This seems to > > > result in a phantom interrupt that gets latched. > > > (c) The IRQ handler is called for the latched interrupt, but the GPIO > > > isn't in low state. > > > > Is the test causing phantom behavior by writing to the interrupt > > hardware? > > > > > > > > 4. (a) For "tlmm_test_silent_high", the GPIO is switched to low state. > > > (b) This doesn't result in a latched interrupt, because RAW_STATUS_EN > > > was cleared when masking the level-triggered interrupt. > > > > > > Fix this by clearing the interrupt state whenever making any changes to the > > > interrupt configuration. This includes previously disabled interrupts, but > > > also any changes to interrupt polarity or detection type. > > > > How do we avoid the case where an interrupt happens to come in while the > > polarity is being changed? Won't we ignore such an interrupt now? If > > these are edge interrupts that's quite bad because we may never see the > > interrupt again. > > > > Are you referring to the "both edge"-detection dance we're doing in the > driver? No I'm thinking of gpio-keys driver where it changes the irq type during suspend to get the wakeup event. I worry that the gpio-keys driver is going to miss the edge now that there's a possibility the interrupt is going to be ignored and we'll never wakeup. But maybe that isn't a problem because PDC behavior works per your tests? > > > I think we erred on the side of caution here and let extra edge > > interrupts through because a rising or falling edge usually means the > > interrupt handler just wants to run when there's some event and it will > > do the work to find out if it was spurious or not. It's been years > > though so I may have forgotten how this hardware works. It just makes me > > very nervous that we're going to miss edges now that we always clear the > > interrupt. > > I'm not saying that you're wrong, or that your concerns are unwarranted. > I wrote some simple tests to validate the behavior I expected to see. I > didn't expect to see different results between wakeup (PDC) GPIOs and > non-wakeup GPIOs, and now both passes the test case as written. > I suspect PDC based GPIOs are working better because PDC sits in between the GIC and the TLMM hardware and we actually mask the interrupts in PDC properly instead of letting the summary line toggle all the time. Thanks for writing hardware tests in KUnit, it's nice.