On Wed, Mar 12, 2025 at 02:19:27PM +0100, Stephan Gerhold wrote: > When submitting the TLMM test driver, Bjorn reported that some of the test > cases are failing for GPIOs that not are backed by PDC (i.e. "non-wakeup" > GPIOs that are handled directly in pinctrl-msm). Basically, lingering > latched interrupt state is still being delivered at IRQ request time, e.g.: > > ok 1 tlmm_test_silent_rising > tlmm_test_silent_falling: ASSERTION FAILED at drivers/pinctrl/qcom/tlmm-test.c:178 > Expected atomic_read(&priv->intr_count) == 0, but > atomic_read(&priv->intr_count) == 1 (0x1) > not ok 2 tlmm_test_silent_falling > tlmm_test_silent_low: ASSERTION FAILED at drivers/pinctrl/qcom/tlmm-test.c:178 > Expected atomic_read(&priv->intr_count) == 0, but > atomic_read(&priv->intr_count) == 1 (0x1) > not ok 3 tlmm_test_silent_low > ok 4 tlmm_test_silent_high > > 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. > (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. > > 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. > > With this change, all 16 test cases are now passing for the non-wakeup > GPIOs in the TLMM. > > Cc: stable@xxxxxxxxxxxxxxx > Fixes: cf9d052aa600 ("pinctrl: qcom: Don't clear pending interrupts when enabling") > Reported-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxxxx> > Closes: https://lore.kernel.org/r/20250227-tlmm-test-v1-1-d18877b4a5db@xxxxxxxxxxxxxxxx/ > Signed-off-by: Stephan Gerhold <stephan.gerhold@xxxxxxxxxx> Tested-by: Bjorn Andersson <andersson@xxxxxxxxxx> Reviewed-by: Bjorn Andersson <andersson@xxxxxxxxxx> Regards, Bjorn > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 47daa47153c970190b0d469dc8d245b3cbeace5e..82f0cc43bbf4f4d24f078af2d0a515d3a03b961a 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -1045,8 +1045,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) > const struct msm_pingroup *g; > u32 intr_target_mask = GENMASK(2, 0); > unsigned long flags; > - bool was_enabled; > - u32 val; > + u32 val, oldval; > > if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) { > set_bit(d->hwirq, pctrl->dual_edge_irqs); > @@ -1108,8 +1107,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) > * internal circuitry of TLMM, toggling the RAW_STATUS > * could cause the INTR_STATUS to be set for EDGE interrupts. > */ > - val = msm_readl_intr_cfg(pctrl, g); > - was_enabled = val & BIT(g->intr_raw_status_bit); > + val = oldval = msm_readl_intr_cfg(pctrl, g); > val |= BIT(g->intr_raw_status_bit); > if (g->intr_detection_width == 2) { > val &= ~(3 << g->intr_detection_bit); > @@ -1162,9 +1160,11 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) > /* > * The first time we set RAW_STATUS_EN it could trigger an interrupt. > * Clear the interrupt. This is safe because we have > - * IRQCHIP_SET_TYPE_MASKED. > + * IRQCHIP_SET_TYPE_MASKED. When changing the interrupt type, we could > + * also still have a non-matching interrupt latched, so clear whenever > + * making changes to the interrupt configuration. > */ > - if (!was_enabled) > + if (val != oldval) > msm_ack_intr_status(pctrl, g); > > if (test_bit(d->hwirq, pctrl->dual_edge_irqs)) > > --- > base-commit: e058c5f49ceff38bf1579a679a5ca20842718579 > change-id: 20250311-pinctrl-msm-type-latch-6099aede7d92 > > Best regards, > -- > Stephan Gerhold <stephan.gerhold@xxxxxxxxxx> >