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 <[email protected]>
---
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);
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:
--
2.42.1
On Mon, Dec 4, 2023 at 9:35 PM Ben Wolsieffer
<[email protected]> 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 <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
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 <[email protected]> wrote:
> On Mon, Dec 4, 2023 at 9:35 PM Ben Wolsieffer
> <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Linus Walleij <[email protected]>
>
> 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 <[email protected]>
> ---
> 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: