RE: [[I-PIPE PATCH]] ipipe: noarch: Fix ipipe level end interrupt

["Grau, Gunter" <gunter.grau@philips.com>]

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com>
> Subject: Re: [[I-PIPE PATCH]] ipipe: noarch: Fix ipipe level end interrupt
>
> On 15.08.22 18:24, Grau, Gunter wrote:
> >
> >
> >> -----Original Message-----
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >> Subject: Re: [[I-PIPE PATCH]] ipipe: noarch: Fix ipipe level end
> >> interrupt On 15.08.22 15:31, Grau, Gunter wrote:
> >>> Hi,
> >>>
> >>>> -----Original Message-----
> >>>> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>>> Sent: Montag, 15. August 2022 13:56
> >>>> To: Grau, Gunter <gunter.grau@philips.com>; xenomai@lists.linux.dev
> >>>> Subject: Re: [[I-PIPE PATCH]] ipipe: noarch: Fix ipipe level end
> >>>> interrupt
> >>>>
> >>>> Caution: This e-mail originated from outside of Philips, be careful
> >>>> for phishing.
> >>>>
> >>>>
> >>>> On 15.08.22 13:27, Gunter Grau wrote:
> >>>>> From: Gunter Grau <gunter.grau@philips.com>
> >>>>>
> >>>>> There is the possibility that a new level triggered interrupt is
> >>>>> already waiting when __ipipe_end_level_irq() is called.
> >>>>> In this case at the moment when the irq is unmasked it may already
> >>>>> fire and call __ipipe_ack_level_irq() before the masked state is
> >>>>> cleared in the irq description. If this happens mask_irq() called
> >>>>> by
> >>>>> __ipipe_ack_level_irq() will not mask the interrupt and the system
> >>>>> my stall.
> >>>>> To prevent this, the masked state will now be cleared before
> >>>>> unmasking the interrupt. A following direct interrupt handler call
> >>>>> will then correctly mask the hardware interrupt as needed.
> >>>>>
> >>>>> Signed-off-by: Gunter Grau <gunter.grau@philips.com>
> >>>>> ---
> >>>>>  kernel/irq/chip.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index
> >>>>> 6a883a4cee87..25d0942b592a 100644
> >>>>> --- a/kernel/irq/chip.c
> >>>>> +++ b/kernel/irq/chip.c
> >>>>> @@ -1048,8 +1048,8 @@ void __ipipe_ack_level_irq(struct irq_desc
> >>>>> *desc)
> >>>>>
> >>>>>  void __ipipe_end_level_irq(struct irq_desc *desc)  {
> >>>>> -     desc->irq_data.chip->irq_unmask(&desc->irq_data);
> >>>>>       irq_state_clr_masked(desc);
> >>>>> +     desc->irq_data.chip->irq_unmask(&desc->irq_data);
> >>>>>  }
> >>>>>
> >>>>>  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> >>>>
> >>>> OK, but if we need this ordering here, why didn't we need it before
> >>>> for unmask_irq()? There, it was sufficient so far to disable local
> >>>> irqs around unmask + clr.
> >>>>
> >>>> Jan
> >>>
> >>> Interesting, I thought the disabling of the local irqs is exact the
> >>> reason why
> >> it works in contrast to __ipipe_end_level_irq() without disabling.
> >>> So did I miss something? Should I add a local disable here, too?
> >>
> >> That's not yet what I suggest. I just first of all try to understand
> >> the boundary conditions for the issue here and if those may apply
> elsewhere.
> >>
> >> If I look at the caller of ipipe_end_irq in Xenomai, none should have
> >> hard irqs on when invoking this. And that means, when local irq
> >> disabling is not sufficient, unmasq_irq may have the same issue you
> >> are trying to fix here by reordering.
> >>
> >> Jan
> >>
> >
> > I tried to follow my issue by dumping the stacktrace in
> __ipipe_end_level_irq(). Kernel is still 5.4:
> >
> > [   34.590806] [<8010f314>] (unwind_backtrace) from [<8010b824>]
> (show_stack+0x10/0x14)
> > [   34.598601] [<8010b824>] (show_stack) from [<8084724c>]
> (dump_stack+0xd8/0xf4)
> > [   34.605877] [<8084724c>] (dump_stack) from [<8017c160>]
> (__ipipe_end_level_irq+0x20/0x30)
> > [   34.618312] [<8017c160>] (__ipipe_end_level_irq) from [<80177f90>]
> (irq_finalize_oneshot.part.1+0x78/0xf8)
> > [   34.628031] [<80177f90>] (irq_finalize_oneshot.part.1) from [<80178080>]
> (irq_thread_fn+0x70/0x78)
> > [   34.637044] [<80178080>] (irq_thread_fn) from [<80177e44>]
> (irq_thread+0x170/0x244)
> > [   34.646751] [<80177e44>] (irq_thread) from [<80148fa8>]
> (kthread+0x14c/0x150)
> > [   34.654879] [<80148fa8>] (kthread) from [<80101118>]
> (ret_from_fork+0x18/0x24)
> >
> > I also thought the irqs are always off here. But I cannot explain the current
> issue otherwise.
>
> In that path, hard-IRQs are NOT off in fact. I only checked the Xenomai code
> paths but forgot about the kernel ones.
>
> But even if we disable hard-IRQs here as well, that would only protect us if
> that physical interrupt can only be raised on the local CPU. I don't think we
> can blindly rely on such an assumption (except for percpu irqs).
>
> Jan
>
[Grau, Gunter]
Hi,
So there is an explanation why the reordering is working. Using unmask_irq() would not be enough. At least on arm32 the interrupts are usually routed to cpu0 if you don't change the affinity settings and mostly also the irq thread keeps that cpu. So that would explain why unmask_irq() also worked for me. So is the suggested patch acceptable or do we need a solution where the two operations are really atomic?
Is there an issue hidden in unmask_irq() where ipipe is using hard_cond_local_irq_save()?

Regards,
Gunter

> --
> Siemens AG, Technology
> Competence Center Embedded Linux

________________________________
The information contained in this message may be confidential and legally protected under applicable law. The message is intended solely for the addressee(s). If you are not the intended recipient, you are hereby notified that any use, forwarding, dissemination, or reproduction of this message is strictly prohibited and may be unlawful. If you are not the intended recipient, please contact the sender by return e-mail and destroy all copies of the original message.