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

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Technology
Competence Center Embedded Linux