| Message ID | 20231204203357.2897008-3-ben.wolsieffer@hefring.com |
|---|---|
| State | New |
| Headers | show |
| Series | stm32: fix GPIO level interrupts | expand |
On Mon, Dec 4, 2023 at 9:35 PM Ben Wolsieffer <ben.wolsieffer@hefring.com> wrote: > The STM32 doesn't support GPIO level interrupts in hardware, so the > driver tries to emulate them using edge interrupts, by retriggering the > interrupt if necessary based on the pin state after the handler > finishes. > > Currently, this functionality does not work because the irqchip uses > handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask() > callbacks after handling the interrupt. This patch fixes this by using > handle_level_irq() for level interrupts, which causes irq_unmask() to be > called to retrigger the interrupt. > > Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Marc Z can apply all the patches to the irq tree once he's happy with the solution. Yours, Linus Walleij
On Thu, Dec 7, 2023 at 10:56 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Dec 4, 2023 at 9:35 PM Ben Wolsieffer > <ben.wolsieffer@hefring.com> wrote: > > > The STM32 doesn't support GPIO level interrupts in hardware, so the > > driver tries to emulate them using edge interrupts, by retriggering the > > interrupt if necessary based on the pin state after the handler > > finishes. > > > > Currently, this functionality does not work because the irqchip uses > > handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask() > > callbacks after handling the interrupt. This patch fixes this by using > > handle_level_irq() for level interrupts, which causes irq_unmask() to be > > called to retrigger the interrupt. > > > > Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Marc Z can apply all the patches to the irq tree once he's happy > with the solution. I see Marc stopped doing irq_chips, so tglx then! Yours, Linus Walleij
Ben! On Mon, Dec 04 2023 at 15:33, Ben Wolsieffer wrote: > The STM32 doesn't support GPIO level interrupts in hardware, so the > driver tries to emulate them using edge interrupts, by retriggering the > interrupt if necessary based on the pin state after the handler > finishes. > > Currently, this functionality does not work because the irqchip uses > handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask() > callbacks after handling the interrupt. This patch fixes this by using > handle_level_irq() for level interrupts, which causes irq_unmask() to be > called to retrigger the interrupt. This does not make any sense at all. irq_unmask() does not retrigger anything. It sets the corresponding bit in the mask register, not more not less. Switching to handle_level_irq() makes the following difference vs. handle_edge_irq() when an interrupt is handled (ignoring the inner loop): + irq_mask(); irq_ack(); .... handle(); .... + irq_unmask(); So in both cases irq_ack() clears the interrupt in the Pending register, right? Now comes the interesting difference. When the interrupt is raised again after irq_ack() while the handler is running, i.e. a full toggle from active to inactive and back to active where the back to active transition causes the edge detector to trigger, then: 1) in case of handle_edge_irq() this should immediately set it in the pending register again and raise another CPU interrupt, which should be handled once the interrupt service routine returned. 2) in case of handle_level_irq() this does not set it in the pending register because it's masked. The unmask will set the pending register bit _if_ and only _if_ the edge detector has latched the detection. No idea whether that's the case. The manual is exceptionally blury about this. So in theory both #1 and #2 should work. But the explanation in the changelog is fairy tale material. As I couldn't figure out why #1 would not work, I looked at the driver in more detail and also at the STM32 manual. That tells me that the irqchip driver is at least suboptimal. Why? The EXTI controller is just an intermediate between the peripheral (including GPIO pins) and the NVIC: |--------------| | Edge config | |-----------------| Source -| |---| Int. Mask logic |---> Dedicated NVIC interrupt | Edge detect | | |-----------------| |--------------| | | |-----------------| |-| Evt. Mask logic |---> CPU event input | |-----------------| | | |-----------------| |-| Wakeup logic |--->.... |-----------------| So there are two classes of sources conntect to EXTI: 1) Direct events - Have a fixed edge - Can be masked for Interrupt and Event generation - No software trigger - Not tracked in the Pending register - Can evtl. wakeup the CPUs or from D3 2) Configurable events - Have a configurable edge - Can be masked for Interrupt and Event generation - Software trigger - Tracked in the Pending register - Can evtl. wakeup the CPUs or from D3 The CPU event is a single input to the CPU which can be triggered by any source which has the Event mask enabled. For both classes there are sources which have no connection to the NVIC, they can only be used to generate CPU events or trigger the wakeup logic. For direct events there is a category where the peripherial interrupt is routed to both the EXTI and the NVIC directly. The EXTI does not provide a connection to the NVIC and the event cannot be masked in EXTI to prevent CPU interrupts. Only the CPU event masking works. GPIO pins are configurable events which are connected to the NVIC via the EXTI. But the EXTI driver implements a chained interrupt handler which listens on a ton of NVIC interrupts. I.e. for the STM32H7 on: <1>, <2>, <3>, <6>, <7>, <8>, <9>, <10>, <23>, <40>, <41>, <62>, <76> NVIC 1: PVD_PVM EXTI-SRC 16 NVIC 2: RTC_TAMP_STAMP EXTI-SRC 18 NVIC 3: RTC_WAKEUP EXTI-SRC 19 NVIC 6: EXTI0 EXTI-SRC 0 NVIC 7: EXTI1 EXTI-SRC 1 NVIC 8: EXTI2 EXTI-SRC 2 NVIC 9: EXTI3 EXTI-SRC 3 NVIC 10: EXTI4 EXTI-SRC 4 NVIC 23: EXTI5-9 EXTI-SRC 5-9 NVIC 40: EXTI10-15 EXTI-SRC 10-15 NVIC 41: RTC_ALARM EXTI-SRC 17 NVIC 62: ETH_WKUP EXTI-SRC 86 NVIC 76: OTG_HS_WKUP EXTI-SRC 43 Each of these chained interrupts handles the full EXTI interrupt domain with all three banks. This does not make any sense at all especially not on a SMP machine. Though it _should_ work, but it might cause interrupts handlers to be invoked when nothing is pending when the edge handler is active. Which in turn can confuse the underlying device driver depending on the quality... CPU0 CPU1 NVIC int X NVIC int Y // read_pending() is serialized by a lock, but both read the same state pend = read_pending() pend = read_pending() for_each_bit(bit, pend) for_each_bit(bit, pend) handle_irq(domain, base + bit) handle_irq(domain, base + bit) lock(desc); lock(desc); ack(); do { clear(PENDING); set(IN_PROGRESS); unlock(desc); handle(); if (IN_PROGRESS) { lock(desc); ack(); set(PENDING); unlock(desc); clear(IN_PROGRESS); return; } } while (PENDING); <- Will loop See? In fact the only NVIC interrupts which actually need demultiplexing are NVIC #23 and NVIC #40 and those should only care about the EXTI interrupts which are actually multiplexed on them. This let's randomly run whatever is pending on any demux handler is far from correct. All others are direct NVIC interrupts which just have the extra EXTI interrupt masking, event masking and the wakeup magic. The indirection via the chained handler is just pointless overhead and not necessarily correct. The exti_h variant of that driver does the right thing and installs a hierarchical interrupt domain which acts as man in the middle between the source and the NVIC. Though granted they don't have the odd problem of multiplexing several GPIO interrupts to a single NVIC interrupt. But in fact the regular exti driver could do the same and just handle the two NVIC interrupts which need demultiplexing separately and let everything else go through the hierarchy without bells and whistles. Thanks, tglx
Thank you for taking the time to look through the STM32 manual and review the driver. Unfortunately, I don't have enough experience with the Linux IRQ subsystem to give you a correspondingly in depth response, but I will attempt to address the parts that relate to the bug in question. On Fri, Dec 08, 2023 at 09:43:21PM +0100, Thomas Gleixner wrote: > Ben! > > On Mon, Dec 04 2023 at 15:33, Ben Wolsieffer wrote: > > The STM32 doesn't support GPIO level interrupts in hardware, so the > > driver tries to emulate them using edge interrupts, by retriggering the > > interrupt if necessary based on the pin state after the handler > > finishes. > > > > Currently, this functionality does not work because the irqchip uses > > handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask() > > callbacks after handling the interrupt. This patch fixes this by using > > handle_level_irq() for level interrupts, which causes irq_unmask() to be > > called to retrigger the interrupt. > > This does not make any sense at all. irq_unmask() does not retrigger > anything. It sets the corresponding bit in the mask register, not more > not less. I don't think this is correct. I was referring to stm32_gpio_irq_unmask(), which calls stm32_gpio_irq_trigger(), which in turn (for level interrupts) checks the GPIO pin state and retriggers the interrupt if necessary. > > Switching to handle_level_irq() makes the following difference > vs. handle_edge_irq() when an interrupt is handled (ignoring the inner > loop): > > + irq_mask(); > irq_ack(); > .... > handle(); > .... > + irq_unmask(); Yes, the additional call to irq_unmask() is the key difference here, as that callback performs the retriggering for level interrupts. > > So in both cases irq_ack() clears the interrupt in the Pending register, > right? > > Now comes the interesting difference. > > When the interrupt is raised again after irq_ack() while the handler is > running, i.e. a full toggle from active to inactive and back to active > where the back to active transition causes the edge detector to trigger, > then: I don't see how this is relevant. The bug occurs with level interrupts in the case where there are no new transitions while the handler is running. For example, with a high level interrupt, if the pin is still high after the handler finishes, the interrupt should be immediately triggered again. > > 1) in case of handle_edge_irq() this should immediately set it in the > pending register again and raise another CPU interrupt, which > should be handled once the interrupt service routine returned. > > 2) in case of handle_level_irq() this does not set it in the pending > register because it's masked. The unmask will set the pending > register bit _if_ and only _if_ the edge detector has latched the > detection. No idea whether that's the case. The manual is > exceptionally blury about this. > > So in theory both #1 and #2 should work. But the explanation in the > changelog is fairy tale material. > > As I couldn't figure out why #1 would not work, I looked at the driver > in more detail and also at the STM32 manual. That tells me that the > irqchip driver is at least suboptimal. Why? > > The EXTI controller is just an intermediate between the peripheral > (including GPIO pins) and the NVIC: > > |--------------| > | Edge config | |-----------------| > Source -| |---| Int. Mask logic |---> Dedicated NVIC interrupt > | Edge detect | | |-----------------| > |--------------| | > | |-----------------| > |-| Evt. Mask logic |---> CPU event input > | |-----------------| > | > | |-----------------| > |-| Wakeup logic |--->.... > |-----------------| > > So there are two classes of sources conntect to EXTI: > > 1) Direct events > > - Have a fixed edge > - Can be masked for Interrupt and Event generation > - No software trigger > - Not tracked in the Pending register > - Can evtl. wakeup the CPUs or from D3 > > 2) Configurable events > > - Have a configurable edge > - Can be masked for Interrupt and Event generation > - Software trigger > - Tracked in the Pending register > - Can evtl. wakeup the CPUs or from D3 > > The CPU event is a single input to the CPU which can be triggered by any > source which has the Event mask enabled. > > For both classes there are sources which have no connection to the NVIC, > they can only be used to generate CPU events or trigger the wakeup > logic. > > For direct events there is a category where the peripherial interrupt is > routed to both the EXTI and the NVIC directly. The EXTI does not provide > a connection to the NVIC and the event cannot be masked in EXTI to > prevent CPU interrupts. Only the CPU event masking works. > > GPIO pins are configurable events which are connected to the NVIC via > the EXTI. > > But the EXTI driver implements a chained interrupt handler which listens > on a ton of NVIC interrupts. I.e. for the STM32H7 on: > > <1>, <2>, <3>, <6>, <7>, <8>, <9>, <10>, <23>, <40>, <41>, <62>, <76> > > NVIC 1: PVD_PVM EXTI-SRC 16 > NVIC 2: RTC_TAMP_STAMP EXTI-SRC 18 > NVIC 3: RTC_WAKEUP EXTI-SRC 19 > NVIC 6: EXTI0 EXTI-SRC 0 > NVIC 7: EXTI1 EXTI-SRC 1 > NVIC 8: EXTI2 EXTI-SRC 2 > NVIC 9: EXTI3 EXTI-SRC 3 > NVIC 10: EXTI4 EXTI-SRC 4 > NVIC 23: EXTI5-9 EXTI-SRC 5-9 > NVIC 40: EXTI10-15 EXTI-SRC 10-15 > NVIC 41: RTC_ALARM EXTI-SRC 17 > NVIC 62: ETH_WKUP EXTI-SRC 86 > NVIC 76: OTG_HS_WKUP EXTI-SRC 43 > > Each of these chained interrupts handles the full EXTI interrupt domain > with all three banks. This does not make any sense at all especially not > on a SMP machine. > > Though it _should_ work, but it might cause interrupts handlers to be > invoked when nothing is pending when the edge handler is active. Which > in turn can confuse the underlying device driver depending on the > quality... > > CPU0 CPU1 > > NVIC int X NVIC int Y > > // read_pending() is serialized by a lock, but both read the same state > pend = read_pending() pend = read_pending() > > for_each_bit(bit, pend) for_each_bit(bit, pend) > handle_irq(domain, base + bit) handle_irq(domain, base + bit) > lock(desc); lock(desc); > ack(); > do { > clear(PENDING); > set(IN_PROGRESS); > unlock(desc); > handle(); if (IN_PROGRESS) { > lock(desc); ack(); > set(PENDING); > unlock(desc); > clear(IN_PROGRESS); return; > } > } while (PENDING); <- Will loop > > See? Currently, this driver is only used on single core machines, and the more complex devices use exti_h, so this shouldn't be a problem in practice. > > In fact the only NVIC interrupts which actually need demultiplexing are > NVIC #23 and NVIC #40 and those should only care about the EXTI > interrupts which are actually multiplexed on them. This let's randomly > run whatever is pending on any demux handler is far from correct. > > All others are direct NVIC interrupts which just have the extra EXTI > interrupt masking, event masking and the wakeup magic. The indirection > via the chained handler is just pointless overhead and not necessarily > correct. > > The exti_h variant of that driver does the right thing and installs a > hierarchical interrupt domain which acts as man in the middle between > the source and the NVIC. Though granted they don't have the odd problem > of multiplexing several GPIO interrupts to a single NVIC interrupt. > > But in fact the regular exti driver could do the same and just handle > the two NVIC interrupts which need demultiplexing separately and let > everything else go through the hierarchy without bells and whistles. This sounds reasonable to me. It did seem strange to me that the exti and exti_h drivers used such different approaches, although I wasn't aware of the reasons behind them. I think this refactoring is out of scope of this bug fix though. > > Thanks, > > tglx Thanks, Ben
Ben! On Tue, Dec 12 2023 at 10:35, Ben Wolsieffer wrote: > On Fri, Dec 08, 2023 at 09:43:21PM +0100, Thomas Gleixner wrote: >> On Mon, Dec 04 2023 at 15:33, Ben Wolsieffer wrote: >> > The STM32 doesn't support GPIO level interrupts in hardware, so the >> > driver tries to emulate them using edge interrupts, by retriggering the >> > interrupt if necessary based on the pin state after the handler >> > finishes. >> > >> > Currently, this functionality does not work because the irqchip uses >> > handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask() >> > callbacks after handling the interrupt. This patch fixes this by using >> > handle_level_irq() for level interrupts, which causes irq_unmask() to be >> > called to retrigger the interrupt. >> >> This does not make any sense at all. irq_unmask() does not retrigger >> anything. It sets the corresponding bit in the mask register, not more >> not less. > > I don't think this is correct. I was referring to > stm32_gpio_irq_unmask(), which calls stm32_gpio_irq_trigger(), which in > turn (for level interrupts) checks the GPIO pin state and retriggers the > interrupt if necessary. Ah. That makes a lot more sense. Sorry that I missed that driver detail. The changelog could mention explicitely where the retrigger comes from. >> Switching to handle_level_irq() makes the following difference >> vs. handle_edge_irq() when an interrupt is handled (ignoring the inner >> loop): >> >> + irq_mask(); >> irq_ack(); >> .... >> handle(); >> .... >> + irq_unmask(); > > Yes, the additional call to irq_unmask() is the key difference here, as > that callback performs the retriggering for level interrupts. Sorry to be pedantic here. irq_unmask() is a function in the interrupt core code which eventually ends up invoking the irqchip::irq_unmask(). >> When the interrupt is raised again after irq_ack() while the handler is >> running, i.e. a full toggle from active to inactive and back to active >> where the back to active transition causes the edge detector to trigger, >> then: > > I don't see how this is relevant. The bug occurs with level interrupts > in the case where there are no new transitions while the handler is > running. For example, with a high level interrupt, if the pin is still > high after the handler finishes, the interrupt should be immediately > triggered again. Ah. That's the problem you are trying to solve. Now we are getting closer to a proper description :) >> But in fact the regular exti driver could do the same and just handle >> the two NVIC interrupts which need demultiplexing separately and let >> everything else go through the hierarchy without bells and whistles. > > This sounds reasonable to me. It did seem strange to me that the exti > and exti_h drivers used such different approaches, although I wasn't > aware of the reasons behind them. I think this refactoring is out of > scope of this bug fix though. Sure. It just occured to me while looking at this stuff.. Care to resend with a proper explanation in the changelog? Thanks, tglx
On Mon, 2023-12-04 at 15:33 -0500, Ben Wolsieffer wrote: > The STM32 doesn't support GPIO level interrupts in hardware, so the > driver tries to emulate them using edge interrupts, by retriggering the > interrupt if necessary based on the pin state after the handler > finishes. > > Currently, this functionality does not work because the irqchip uses > handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask() > callbacks after handling the interrupt. This patch fixes this by using > handle_level_irq() for level interrupts, which causes irq_unmask() to be > called to retrigger the interrupt. > > Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> > --- > drivers/pinctrl/stm32/pinctrl-stm32.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c > index 603f900e88c1..fb9532601cbb 100644 > --- a/drivers/pinctrl/stm32/pinctrl-stm32.c > +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c > @@ -348,12 +348,15 @@ static int stm32_gpio_set_type(struct irq_data *d, unsigned int type) > case IRQ_TYPE_EDGE_RISING: > case IRQ_TYPE_EDGE_FALLING: > case IRQ_TYPE_EDGE_BOTH: > + irq_set_handler_locked(d, handle_edge_irq); Hi, this patch causes a NULL pointer dereference and crashes the kernel boot on STM32 MPU's, either STM32MP13x, STM32MP15x and the new STM32MP25x. Please do not merge it as is. This pinctrl-stm32 driver is shared between STM32 MCUs and MPUs. In both cases the EXTI is the parent interrupt controller of this pinctrl, but despite the fact that there is a single file irq-stm32-exti.c, it contains two independent drivers, one for MCUs and the other for MPUs. Swapping in this function the irq_desc::handle_irq between handle_edge_irq() and handle_level_irq() is probably fine for MCU (I have not tested it). But on MPUs the default handler is handle_fasteoi_irq(); should not be changed here. Checking quickly ... this function calls irq_chip_set_type_parent() at the very end. It will in turn call EXTI's irq_set_type(), which has different implementations for MCU and MPU. By moving this handler swap in the MCU specific stm32_irq_set_type() it will not impact MPUs. Best Regards, Antonio > parent_type = type; > break; > case IRQ_TYPE_LEVEL_HIGH: > + irq_set_handler_locked(d, handle_level_irq); > parent_type = IRQ_TYPE_EDGE_RISING; > break; > case IRQ_TYPE_LEVEL_LOW: > + irq_set_handler_locked(d, handle_level_irq); > parent_type = IRQ_TYPE_EDGE_FALLING; > break; > default:
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c index 603f900e88c1..fb9532601cbb 100644 --- a/drivers/pinctrl/stm32/pinctrl-stm32.c +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c @@ -348,12 +348,15 @@ static int stm32_gpio_set_type(struct irq_data *d, unsigned int type) case IRQ_TYPE_EDGE_RISING: case IRQ_TYPE_EDGE_FALLING: case IRQ_TYPE_EDGE_BOTH: + irq_set_handler_locked(d, handle_edge_irq); parent_type = type; break; case IRQ_TYPE_LEVEL_HIGH: + irq_set_handler_locked(d, handle_level_irq); parent_type = IRQ_TYPE_EDGE_RISING; break; case IRQ_TYPE_LEVEL_LOW: + irq_set_handler_locked(d, handle_level_irq); parent_type = IRQ_TYPE_EDGE_FALLING; break; default:
The STM32 doesn't support GPIO level interrupts in hardware, so the driver tries to emulate them using edge interrupts, by retriggering the interrupt if necessary based on the pin state after the handler finishes. Currently, this functionality does not work because the irqchip uses handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask() callbacks after handling the interrupt. This patch fixes this by using handle_level_irq() for level interrupts, which causes irq_unmask() to be called to retrigger the interrupt. Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> --- drivers/pinctrl/stm32/pinctrl-stm32.c | 3 +++ 1 file changed, 3 insertions(+)