Issue with linux threaded level triggered interrupts

Issue with linux threaded level triggered interrupts

[Gunter Grau]

Hi,

during our development on imx6 with kernel 5.4 and Xenomai 3.1 we discovered complete system stalls when using phylib with phy irqs level triggerd. This irq is connected on the imx-gpio.

Investigation showed that it seems to be an issue in the ipipe irq end function for level interrupts. Please review the patch proposal.
I am not sure if just calling unmask_irq() is ok here since this functin also does some locking. The other way would be to just call additionally the irq_state_clr_masked(desc) in the level end function itself.

The patch applies on ipipe/master in ipipe-arm.
It looks like this is an issue in all ipipe patches, but is triggered
by the introduction of a check in unmask_irq().

Thanks in advance,
Gunter Grau




________________________________
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.

[PATCH] ipipe: Fix ipipe level irq end

[Gunter Grau]

The following commit in the vanilla kernel introduced
a check for the cached interrupt mask flag in mask_irq():

bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")

This means if the flag is not serviced correctly
the real bit in the hardware interrupt controller may not be
cleared or set.
The __ipipe_end_level_irq() function does not follow this rule.
It unmasks the bit in the hardware without setting the cached flags
accordingly. So after the first level interrupt is finished the
mask cache has a wrong state. If now the next interrupt fires,
the mask_irq() function will not really mask the interrupt in
the hardware which causes a interrupt storm after reenabeling
the hard irqs.
The fix now calls unmask_irq() which is handling the cached flags
correctly.

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 7c03e2931189..79a09b92a1b1 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -987,7 +987,7 @@ 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);
+       unmask_irq(desc);
 }

 void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
--
2.25.1


________________________________
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.

Re: [PATCH] ipipe: Fix ipipe level irq end

[Greg Gallagher]

On Thu, Feb 10, 2022 at 10:35 AM Gunter Grau via Xenomai <
xenomai@xenomai.org> wrote:

> The following commit in the vanilla kernel introduced
> a check for the cached interrupt mask flag in mask_irq():
>
> bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
>
> This means if the flag is not serviced correctly
> the real bit in the hardware interrupt controller may not be
> cleared or set.
> The __ipipe_end_level_irq() function does not follow this rule.
> It unmasks the bit in the hardware without setting the cached flags
> accordingly. So after the first level interrupt is finished the
> mask cache has a wrong state. If now the next interrupt fires,
> the mask_irq() function will not really mask the interrupt in
> the hardware which causes a interrupt storm after reenabeling
> the hard irqs.
> The fix now calls unmask_irq() which is handling the cached flags
> correctly.
>
> 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 7c03e2931189..79a09b92a1b1 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -987,7 +987,7 @@ 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);
> +       unmask_irq(desc);
>  }
>
>  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> --
> 2.25.1
>
>
> ________________________________
> 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.
>

I’ll add this to the arm/arm64 ipipe kernels this weekend.

Thanks

Greg

>
>

Re: Issue with linux threaded level triggered interrupts

[Greg Gallagher]

On Thu, Feb 10, 2022 at 10:34 AM Gunter Grau via Xenomai <
xenomai@xenomai.org> wrote:

> Hi,
>
> during our development on imx6 with kernel 5.4 and Xenomai 3.1 we
> discovered complete system stalls when using phylib with phy irqs level
> triggerd. This irq is connected on the imx-gpio.
>
> Investigation showed that it seems to be an issue in the ipipe irq end
> function for level interrupts. Please review the patch proposal.
> I am not sure if just calling unmask_irq() is ok here since this functin
> also does some locking. The other way would be to just call additionally
> the irq_state_clr_masked(desc) in the level end function itself.
>
> The patch applies on ipipe/master in ipipe-arm.
> It looks like this is an issue in all ipipe patches, but is triggered
> by the introduction of a check in unmask_irq().
>
> Thanks in advance,
> Gunter Grau
>
>
>
>
> ________________________________
> 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.
>

I’ll integrate and test this patch out over the weekend, I can’t say off
the top of my head what is the better solution. I’ll have feedback in a few
days.

Thanks for the patch :)

Greg

>
>

Re: [PATCH] ipipe: Fix ipipe level irq end

[Jan Kiszka]

On 10.02.22 16:33, Gunter Grau via Xenomai wrote:
> The following commit in the vanilla kernel introduced
> a check for the cached interrupt mask flag in mask_irq():
> 
> bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
> 
> This means if the flag is not serviced correctly
> the real bit in the hardware interrupt controller may not be
> cleared or set.
> The __ipipe_end_level_irq() function does not follow this rule.
> It unmasks the bit in the hardware without setting the cached flags
> accordingly. So after the first level interrupt is finished the
> mask cache has a wrong state. If now the next interrupt fires,
> the mask_irq() function will not really mask the interrupt in
> the hardware which causes a interrupt storm after reenabeling
> the hard irqs.
> The fix now calls unmask_irq() which is handling the cached flags
> correctly.
> 
> 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 7c03e2931189..79a09b92a1b1 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -987,7 +987,7 @@ 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);
> +       unmask_irq(desc);
>  }
> 
>  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> --
> 2.25.1
> 

OK, so this is 4.14+, i.e. possibly our 4.19 and 5.4 ipipe trees, and
that generically. Need to check why we didn't see this in our tests so
far...

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH] ipipe: Fix ipipe level irq end

[Jan Kiszka]

On 10.02.22 16:33, Gunter Grau via Xenomai wrote:
> The following commit in the vanilla kernel introduced
> a check for the cached interrupt mask flag in mask_irq():
> 
> bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
> 
> This means if the flag is not serviced correctly
> the real bit in the hardware interrupt controller may not be
> cleared or set.
> The __ipipe_end_level_irq() function does not follow this rule.
> It unmasks the bit in the hardware without setting the cached flags
> accordingly. So after the first level interrupt is finished the
> mask cache has a wrong state. If now the next interrupt fires,
> the mask_irq() function will not really mask the interrupt in
> the hardware which causes a interrupt storm after reenabeling
> the hard irqs.
> The fix now calls unmask_irq() which is handling the cached flags
> correctly.
> 
> 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 7c03e2931189..79a09b92a1b1 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -987,7 +987,7 @@ 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);
> +       unmask_irq(desc);

Finally looked at this: Issue confirmed, solution if fine, just not
optimal. I would also suggest to simply add irq_state_clr_masked().
unmask_irq is overkill because all the extra checks there are needless
in the end-irq case, and also hard_irq_disable is redundant.

Thanks,
Jan

>  }
> 
>  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> --
> 2.25.1
-- 
Siemens AG, Technology
Competence Center Embedded Linux

[PATCH] ipipe: Fix ipipe level irq end

[Gunter Grau]

The following commit in the vanilla kernel introduced
a check for the cached interrupt mask flag in mask_irq():

bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")

This means if the flag is not serviced correctly
the real bit in the hardware interrupt controller may not be
cleared or set.
The __ipipe_end_level_irq() function does not follow this rule.
It unmasks the bit in the hardware without setting the cached flags
accordingly. So after the first level interrupt is finished the
mask cache has a wrong state. If now the next interrupt fires,
the mask_irq() function will not really mask the interrupt in
the hardware which causes a interrupt storm after reenabeling
the hard irqs.
The fix now also updates the shadow flag correctly.
---
 kernel/irq/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 7c03e2931189..ff9a8b3f33db 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -988,6 +988,7 @@ 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);
 }

 void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
--
2.25.1


________________________________
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.

Re: [PATCH] ipipe: Fix ipipe level irq end

[Jan Kiszka]

On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
> The following commit in the vanilla kernel introduced
> a check for the cached interrupt mask flag in mask_irq():
> 
> bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
> 
> This means if the flag is not serviced correctly
> the real bit in the hardware interrupt controller may not be
> cleared or set.
> The __ipipe_end_level_irq() function does not follow this rule.
> It unmasks the bit in the hardware without setting the cached flags
> accordingly. So after the first level interrupt is finished the
> mask cache has a wrong state. If now the next interrupt fires,
> the mask_irq() function will not really mask the interrupt in
> the hardware which causes a interrupt storm after reenabeling
> the hard irqs.
> The fix now also updates the shadow flag correctly.
> ---
>  kernel/irq/chip.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 7c03e2931189..ff9a8b3f33db 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -988,6 +988,7 @@ 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);
>  }
> 
>  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> --
> 2.25.1
> 

Thanks, applied to ipipe-noarch. I had to do that manually as you email
client managed the patch (tabs->spaces, trailing spaces removed). Use
git-send-email or a client that avoids this mangling.

Greg, I will apply this to ipipe-x86 (4.19-cip and 5.4), could you do
the same for arm and arm64?

Thanks,
Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

RE: [PATCH] ipipe: Fix ipipe level irq end

[Grau, Gunter]

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com>
> Sent: Donnerstag, 17. Februar 2022 10:16
> To: Grau, Gunter <gunter.grau@philips.com>; xenomai@xenomai.org; Greg
> Gallagher <greg@embeddedgreg.com>
> Subject: Re: [PATCH] ipipe: Fix ipipe level irq end
>
> Caution: This e-mail originated from outside of Philips, be careful for
> phishing.
>
>
> On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
> > The following commit in the vanilla kernel introduced a check for the
> > cached interrupt mask flag in mask_irq():
> >
> > bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
> > calls")
> >
> > This means if the flag is not serviced correctly the real bit in the
> > hardware interrupt controller may not be cleared or set.
> > The __ipipe_end_level_irq() function does not follow this rule.
> > It unmasks the bit in the hardware without setting the cached flags
> > accordingly. So after the first level interrupt is finished the mask
> > cache has a wrong state. If now the next interrupt fires, the
> > mask_irq() function will not really mask the interrupt in the hardware
> > which causes a interrupt storm after reenabeling the hard irqs.
> > The fix now also updates the shadow flag correctly.
> > ---
> >  kernel/irq/chip.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index
> > 7c03e2931189..ff9a8b3f33db 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -988,6 +988,7 @@ 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);
> >  }
> >
> >  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> > --
> > 2.25.1
> >
>
> Thanks, applied to ipipe-noarch. I had to do that manually as you email client
> managed the patch (tabs->spaces, trailing spaces removed). Use git-send-
> email or a client that avoids this mangling.
>
> Greg, I will apply this to ipipe-x86 (4.19-cip and 5.4), could you do the same
> for arm and arm64?
>
> Thanks,
> Jan

Hi,

That’s weird, because I used git send-email for this. Maybe something in the mail server configuration.
Same patch for ipipe-arm64 will follow, but I don't know how to make it better than using git send-email ☹.

Thanks for applying.
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.

Re: [PATCH] ipipe: Fix ipipe level irq end

[Jan Kiszka]

On 17.02.22 13:16, Grau, Gunter wrote:
> 
> 
>> -----Original Message-----
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> Sent: Donnerstag, 17. Februar 2022 10:16
>> To: Grau, Gunter <gunter.grau@philips.com>; xenomai@xenomai.org; Greg
>> Gallagher <greg@embeddedgreg.com>
>> Subject: Re: [PATCH] ipipe: Fix ipipe level irq end
>>
>> Caution: This e-mail originated from outside of Philips, be careful for
>> phishing.
>>
>>
>> On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
>>> The following commit in the vanilla kernel introduced a check for the
>>> cached interrupt mask flag in mask_irq():
>>>
>>> bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
>>> calls")
>>>
>>> This means if the flag is not serviced correctly the real bit in the
>>> hardware interrupt controller may not be cleared or set.
>>> The __ipipe_end_level_irq() function does not follow this rule.
>>> It unmasks the bit in the hardware without setting the cached flags
>>> accordingly. So after the first level interrupt is finished the mask
>>> cache has a wrong state. If now the next interrupt fires, the
>>> mask_irq() function will not really mask the interrupt in the hardware
>>> which causes a interrupt storm after reenabeling the hard irqs.
>>> The fix now also updates the shadow flag correctly.
>>> ---
>>>  kernel/irq/chip.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index
>>> 7c03e2931189..ff9a8b3f33db 100644
>>> --- a/kernel/irq/chip.c
>>> +++ b/kernel/irq/chip.c
>>> @@ -988,6 +988,7 @@ 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);
>>>  }
>>>
>>>  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
>>> --
>>> 2.25.1
>>>
>>
>> Thanks, applied to ipipe-noarch. I had to do that manually as you email client
>> managed the patch (tabs->spaces, trailing spaces removed). Use git-send-
>> email or a client that avoids this mangling.
>>
>> Greg, I will apply this to ipipe-x86 (4.19-cip and 5.4), could you do the same
>> for arm and arm64?
>>
>> Thanks,
>> Jan
> 
> Hi,
> 
> That’s weird, because I used git send-email for this. Maybe something in the mail server configuration.
> Same patch for ipipe-arm64 will follow, but I don't know how to make it better than using git send-email ☹.
> 

Strange indeed. You can see what reached the list here:

https://xenomai.org/pipermail/xenomai/2022-February/047217.html

Maybe worth to take to your IT with this. I know from own painful
experiences that professional mail servers trying to modify email, eg.
to filter out information that should not leave an org, can decide to
re-encode the content, and that rather unprofessionally.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH] ipipe: Fix ipipe level irq end

[Jan Kiszka]

On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
> The following commit in the vanilla kernel introduced
> a check for the cached interrupt mask flag in mask_irq():
> 
> bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
> 
> This means if the flag is not serviced correctly
> the real bit in the hardware interrupt controller may not be
> cleared or set.
> The __ipipe_end_level_irq() function does not follow this rule.
> It unmasks the bit in the hardware without setting the cached flags
> accordingly. So after the first level interrupt is finished the
> mask cache has a wrong state. If now the next interrupt fires,
> the mask_irq() function will not really mask the interrupt in
> the hardware which causes a interrupt storm after reenabeling
> the hard irqs.
> The fix now also updates the shadow flag correctly.

Missed that your "signed-of-by" was not here. Mind to follow up to confirm

https://developercertificate.org/

? A reply with that line would suffice (need to fix up the patch anyway,
wrong send address).

Thanks,
Jan

> ---
>  kernel/irq/chip.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index 7c03e2931189..ff9a8b3f33db 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -988,6 +988,7 @@ 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);
>  }
> 
>  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> --
> 2.25.1
-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH] ipipe: Fix ipipe level irq end

[Jan Kiszka]

On 17.02.22 15:05, Jan Kiszka via Xenomai wrote:
> On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
>> The following commit in the vanilla kernel introduced
>> a check for the cached interrupt mask flag in mask_irq():
>>
>> bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
>>
>> This means if the flag is not serviced correctly
>> the real bit in the hardware interrupt controller may not be
>> cleared or set.
>> The __ipipe_end_level_irq() function does not follow this rule.
>> It unmasks the bit in the hardware without setting the cached flags
>> accordingly. So after the first level interrupt is finished the
>> mask cache has a wrong state. If now the next interrupt fires,
>> the mask_irq() function will not really mask the interrupt in
>> the hardware which causes a interrupt storm after reenabeling
>> the hard irqs.
>> The fix now also updates the shadow flag correctly.
> 
> Missed that your "signed-of-by" was not here. Mind to follow up to confirm
> 

Signed-off-by: ...

> https://developercertificate.org/
> 
> ? A reply with that line would suffice (need to fix up the patch anyway,
> wrong send address).
> 
> Thanks,
> Jan
> 
>> ---
>>  kernel/irq/chip.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> index 7c03e2931189..ff9a8b3f33db 100644
>> --- a/kernel/irq/chip.c
>> +++ b/kernel/irq/chip.c
>> @@ -988,6 +988,7 @@ 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);
>>  }
>>
>>  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
>> --
>> 2.25.1


-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH] ipipe: Fix ipipe level irq end

[Greg Gallagher]

On Thu, Feb 17, 2022 at 4:15 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
> > The following commit in the vanilla kernel introduced
> > a check for the cached interrupt mask flag in mask_irq():
> >
> > bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")
> >
> > This means if the flag is not serviced correctly
> > the real bit in the hardware interrupt controller may not be
> > cleared or set.
> > The __ipipe_end_level_irq() function does not follow this rule.
> > It unmasks the bit in the hardware without setting the cached flags
> > accordingly. So after the first level interrupt is finished the
> > mask cache has a wrong state. If now the next interrupt fires,
> > the mask_irq() function will not really mask the interrupt in
> > the hardware which causes a interrupt storm after reenabeling
> > the hard irqs.
> > The fix now also updates the shadow flag correctly.
> > ---
> >  kernel/irq/chip.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index 7c03e2931189..ff9a8b3f33db 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -988,6 +988,7 @@ 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);
> >  }
> >
> >  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> > --
> > 2.25.1
> >
>
> Thanks, applied to ipipe-noarch. I had to do that manually as you email
> client managed the patch (tabs->spaces, trailing spaces removed). Use
> git-send-email or a client that avoids this mangling.
>
> Greg, I will apply this to ipipe-x86 (4.19-cip and 5.4), could you do
> the same for arm and arm64?
>
> Thanks,
> Jan
>
> --
> Siemens AG, Technology


> Competence Center Embedded Linux


Yep, I’ve applied the patch locally, I’ll update arm and arm64 shortly.

-Greg

>
>

[PATCH] ipipe: Fix ipipe level irq end

[Gunter Grau]

The following commit in the vanilla kernel introduced
a check for the cached interrupt mask flag in mask_irq():

bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function calls")

This means if the flag is not serviced correctly
the real bit in the hardware interrupt controller may not be
cleared or set.
The __ipipe_end_level_irq() function does not follow this rule.
It unmasks the bit in the hardware without setting the cached flags
accordingly. So after the first level interrupt is finished the
mask cache has a wrong state. If now the next interrupt fires,
the mask_irq() function will not really mask the interrupt in
the hardware which causes a interrupt storm after reenabeling
the hard irqs.
The fix now also updates the shadow flag correctly.

Signed-off-by: Gunter Grau <gunter.grau@philips.com>
---
 kernel/irq/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 7c03e2931189..ff9a8b3f33db 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -988,6 +988,7 @@ 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);
 }

 void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
--
2.25.1


________________________________
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.

Re: [PATCH] ipipe: Fix ipipe level irq end

[Jan Kiszka]

On 17.02.22 15:11, Greg Gallagher wrote:
> 
> 
> On Thu, Feb 17, 2022 at 4:15 AM Jan Kiszka <jan.kiszka@siemens.com
> <mailto:jan.kiszka@siemens.com>> wrote:
> 
>     On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
>     > The following commit in the vanilla kernel introduced
>     > a check for the cached interrupt mask flag in mask_irq():
>     >
>     > bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
>     calls")
>     >
>     > This means if the flag is not serviced correctly
>     > the real bit in the hardware interrupt controller may not be
>     > cleared or set.
>     > The __ipipe_end_level_irq() function does not follow this rule.
>     > It unmasks the bit in the hardware without setting the cached flags
>     > accordingly. So after the first level interrupt is finished the
>     > mask cache has a wrong state. If now the next interrupt fires,
>     > the mask_irq() function will not really mask the interrupt in
>     > the hardware which causes a interrupt storm after reenabeling
>     > the hard irqs.
>     > The fix now also updates the shadow flag correctly.
>     > ---
>     >  kernel/irq/chip.c | 1 +
>     >  1 file changed, 1 insertion(+)
>     >
>     > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>     > index 7c03e2931189..ff9a8b3f33db 100644
>     > --- a/kernel/irq/chip.c
>     > +++ b/kernel/irq/chip.c
>     > @@ -988,6 +988,7 @@ 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);
>     >  }
>     >
>     >  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
>     > --
>     > 2.25.1
>     >
> 
>     Thanks, applied to ipipe-noarch. I had to do that manually as you email
>     client managed the patch (tabs->spaces, trailing spaces removed). Use
>     git-send-email or a client that avoids this mangling.
> 
>     Greg, I will apply this to ipipe-x86 (4.19-cip and 5.4), could you do
>     the same for arm and arm64?
> 
>     Thanks,
>     Jan
> 
>     -- 
>     Siemens AG, Technology
> 
> 
>     Competence Center Embedded Linux
> 
> 
> Yep, I’ve applied the patch locally, I’ll update arm and arm64 shortly.
> 

Just force-updated ipipe/noarch to have Gunter's address and signed-off
fixed up. Please use that version.

Thanks,
Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH] ipipe: Fix ipipe level irq end

[Greg Gallagher]

On Thu, Feb 17, 2022 at 10:12 AM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 17.02.22 15:11, Greg Gallagher wrote:
> >
> >
> > On Thu, Feb 17, 2022 at 4:15 AM Jan Kiszka <jan.kiszka@siemens.com
> > <mailto:jan.kiszka@siemens.com>> wrote:
> >
> >     On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
> >     > The following commit in the vanilla kernel introduced
> >     > a check for the cached interrupt mask flag in mask_irq():
> >     >
> >     > bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
> >     calls")
> >     >
> >     > This means if the flag is not serviced correctly
> >     > the real bit in the hardware interrupt controller may not be
> >     > cleared or set.
> >     > The __ipipe_end_level_irq() function does not follow this rule.
> >     > It unmasks the bit in the hardware without setting the cached flags
> >     > accordingly. So after the first level interrupt is finished the
> >     > mask cache has a wrong state. If now the next interrupt fires,
> >     > the mask_irq() function will not really mask the interrupt in
> >     > the hardware which causes a interrupt storm after reenabeling
> >     > the hard irqs.
> >     > The fix now also updates the shadow flag correctly.
> >     > ---
> >     >  kernel/irq/chip.c | 1 +
> >     >  1 file changed, 1 insertion(+)
> >     >
> >     > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> >     > index 7c03e2931189..ff9a8b3f33db 100644
> >     > --- a/kernel/irq/chip.c
> >     > +++ b/kernel/irq/chip.c
> >     > @@ -988,6 +988,7 @@ 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);
> >     >  }
> >     >
> >     >  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> >     > --
> >     > 2.25.1
> >     >
> >
> >     Thanks, applied to ipipe-noarch. I had to do that manually as you
> email
> >     client managed the patch (tabs->spaces, trailing spaces removed). Use
> >     git-send-email or a client that avoids this mangling.
> >
> >     Greg, I will apply this to ipipe-x86 (4.19-cip and 5.4), could you do
> >     the same for arm and arm64?
> >
> >     Thanks,
> >     Jan
> >
> >     --
> >     Siemens AG, Technology
> >
> >
> >     Competence Center Embedded Linux
> >
> >
> > Yep, I’ve applied the patch locally, I’ll update arm and arm64 shortly.
> >
>
> Just force-updated ipipe/noarch to have Gunter's address and signed-off
> fixed up. Please use that version.
>
> Thanks,
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux


Ack

>
>

RE: [PATCH] ipipe: Fix ipipe level irq end

[Grau, Gunter]

Hi,

Unfortunately I have to come back to this.

On Thu, Feb 17, 2022 at 10:12 AM Jan Kiszka <mailto:jan.kiszka@siemens.com> wrote:
On 17.02.22 15:11, Greg Gallagher wrote:
>
>
> On Thu, Feb 17, 2022 at 4:15 AM Jan Kiszka <mailto:jan.kiszka@siemens.com
> <mailto:mailto:jan.kiszka@siemens.com>> wrote:
>
>     On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
>     > The following commit in the vanilla kernel introduced
>     > a check for the cached interrupt mask flag in mask_irq():
>     >
>     > bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
>     calls")
>     >
>     > This means if the flag is not serviced correctly
>     > the real bit in the hardware interrupt controller may not be
>     > cleared or set.
>     > The __ipipe_end_level_irq() function does not follow this rule.
>     > It unmasks the bit in the hardware without setting the cached flags
>     > accordingly. So after the first level interrupt is finished the
>     > mask cache has a wrong state. If now the next interrupt fires,
>     > the mask_irq() function will not really mask the interrupt in
>     > the hardware which causes a interrupt storm after reenabeling
>     > the hard irqs.
>     > The fix now also updates the shadow flag correctly.
>     > ---
>     >  kernel/irq/chip.c | 1 +
>     >  1 file changed, 1 insertion(+)
>     >
>     > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>     > index 7c03e2931189..ff9a8b3f33db 100644
>     > --- a/kernel/irq/chip.c
>     > +++ b/kernel/irq/chip.c
>     > @@ -988,6 +988,7 @@ 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);
>     >  }
>     >
>     >  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
>     > --
>     > 2.25.1
>     >

We discovered some situations (with iio drivers) where this fix seem not to work.
Obviously in the moment where the irq is unmasked, the new interrupt seems to fire again and the irq_ack() code of the new interrupt runs before the irq_stats_clr_masked() is called.
When I wrote the patch I thought the irqs are disabled at this moment. Looks like this is not the case. At least when I change the order to:

irq_state_clr_masked(desc);
desc->irq_data.chip->irq_unmask(&desc->irq_data);

My use case works again.
Now I am unsure if the above reordering is the correct solution or maybe just calling unmask_irq(); which was the first proposal is better?
We dropped unmask_irq() proposal because it does additionally some locking, but maybe this is needed. I am unsure.

Do you have some advice?

Thanks in advance,
Gunter

>
>     Thanks, applied to ipipe-noarch. I had to do that manually as you email
>     client managed the patch (tabs->spaces, trailing spaces removed). Use
>     git-send-email or a client that avoids this mangling.
>
>     Greg, I will apply this to ipipe-x86 (4.19-cip and 5.4), could you do
>     the same for arm and arm64?
>
>     Thanks,
>     Jan
>
>     --
>     Siemens AG, Technology
>
>
>     Competence Center Embedded Linux
>
>
> Yep, I’ve applied the patch locally, I’ll update arm and arm64 shortly.
>



________________________________
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.

Re: [PATCH] ipipe: Fix ipipe level irq end

[Jan Kiszka]

On 11.08.22 16:05, Grau, Gunter wrote:
> 
> Hi,
> 
> Unfortunately I have to come back to this.
> 
> On Thu, Feb 17, 2022 at 10:12 AM Jan Kiszka <mailto:jan.kiszka@siemens.com> wrote:
> On 17.02.22 15:11, Greg Gallagher wrote:
>>
>>
>> On Thu, Feb 17, 2022 at 4:15 AM Jan Kiszka <mailto:jan.kiszka@siemens.com
>> <mailto:mailto:jan.kiszka@siemens.com>> wrote:
>>
>>     On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
>>     > The following commit in the vanilla kernel introduced
>>     > a check for the cached interrupt mask flag in mask_irq():
>>     >
>>     > bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
>>     calls")
>>     >
>>     > This means if the flag is not serviced correctly
>>     > the real bit in the hardware interrupt controller may not be
>>     > cleared or set.
>>     > The __ipipe_end_level_irq() function does not follow this rule.
>>     > It unmasks the bit in the hardware without setting the cached flags
>>     > accordingly. So after the first level interrupt is finished the
>>     > mask cache has a wrong state. If now the next interrupt fires,
>>     > the mask_irq() function will not really mask the interrupt in
>>     > the hardware which causes a interrupt storm after reenabeling
>>     > the hard irqs.
>>     > The fix now also updates the shadow flag correctly.
>>     > ---
>>     >  kernel/irq/chip.c | 1 +
>>     >  1 file changed, 1 insertion(+)
>>     >
>>     > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>     > index 7c03e2931189..ff9a8b3f33db 100644
>>     > --- a/kernel/irq/chip.c
>>     > +++ b/kernel/irq/chip.c
>>     > @@ -988,6 +988,7 @@ 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);
>>     >  }
>>     >
>>     >  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
>>     > --
>>     > 2.25.1
>>     >
> 
> We discovered some situations (with iio drivers) where this fix seem not to work.
> Obviously in the moment where the irq is unmasked, the new interrupt seems to fire again and the irq_ack() code of the new interrupt runs before the irq_stats_clr_masked() is called.
> When I wrote the patch I thought the irqs are disabled at this moment. Looks like this is not the case. At least when I change the order to:
> 
> irq_state_clr_masked(desc);
> desc->irq_data.chip->irq_unmask(&desc->irq_data);
> 
> My use case works again.
> Now I am unsure if the above reordering is the correct solution or maybe just calling unmask_irq(); which was the first proposal is better?
> We dropped unmask_irq() proposal because it does additionally some locking, but maybe this is needed. I am unsure.
> 
> Do you have some advice?

I didn't wrap my head fully around this again yet, but if you can't
ensure a correct, race-free behavior by ordering, you need locking.
Maybe have a look again how dovetail works in this context as it should
have logically the same thing to do.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

RE: [PATCH] ipipe: Fix ipipe level irq end

[Grau, Gunter]

Hi,

> -----Original Message-----
> From: Jan Kiszka <jan.kiszka@siemens.com>
> Sent: Montag, 15. August 2022 08:19
> To: Grau, Gunter <gunter.grau@philips.com>; xenomai@lists.linux.dev
> Subject: Re: [PATCH] ipipe: Fix ipipe level irq end
>
> Caution: This e-mail originated from outside of Philips, be careful for
> phishing.
>
>
> On 11.08.22 16:05, Grau, Gunter wrote:
> >
> > Hi,
> >
> > Unfortunately I have to come back to this.
> >
> > On Thu, Feb 17, 2022 at 10:12 AM Jan Kiszka
> <mailto:jan.kiszka@siemens.com> wrote:
> > On 17.02.22 15:11, Greg Gallagher wrote:
> >>
> >>
> >> On Thu, Feb 17, 2022 at 4:15 AM Jan Kiszka
> >> <mailto:jan.kiszka@siemens.com
> <mailto:mailto:jan.kiszka@siemens.com>> wrote:
> >>
> >>     On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
> >>     > The following commit in the vanilla kernel introduced
> >>     > a check for the cached interrupt mask flag in mask_irq():
> >>     >
> >>     > bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
> >>     calls")
> >>     >
> >>     > This means if the flag is not serviced correctly
> >>     > the real bit in the hardware interrupt controller may not be
> >>     > cleared or set.
> >>     > The __ipipe_end_level_irq() function does not follow this rule.
> >>     > It unmasks the bit in the hardware without setting the cached flags
> >>     > accordingly. So after the first level interrupt is finished the
> >>     > mask cache has a wrong state. If now the next interrupt fires,
> >>     > the mask_irq() function will not really mask the interrupt in
> >>     > the hardware which causes a interrupt storm after reenabeling
> >>     > the hard irqs.
> >>     > The fix now also updates the shadow flag correctly.
> >>     > ---
> >>     >  kernel/irq/chip.c | 1 +
> >>     >  1 file changed, 1 insertion(+)
> >>     >
> >>     > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> >>     > index 7c03e2931189..ff9a8b3f33db 100644
> >>     > --- a/kernel/irq/chip.c
> >>     > +++ b/kernel/irq/chip.c
> >>     > @@ -988,6 +988,7 @@ 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);
> >>     >  }
> >>     >
> >>     >  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
> >>     > --
> >>     > 2.25.1
> >>     >
> >
> > We discovered some situations (with iio drivers) where this fix seem not to
> work.
> > Obviously in the moment where the irq is unmasked, the new interrupt
> seems to fire again and the irq_ack() code of the new interrupt runs before
> the irq_stats_clr_masked() is called.
> > When I wrote the patch I thought the irqs are disabled at this moment.
> Looks like this is not the case. At least when I change the order to:
> >
> > irq_state_clr_masked(desc);
> > desc->irq_data.chip->irq_unmask(&desc->irq_data);
> >
> > My use case works again.
> > Now I am unsure if the above reordering is the correct solution or maybe
> just calling unmask_irq(); which was the first proposal is better?
> > We dropped unmask_irq() proposal because it does additionally some
> locking, but maybe this is needed. I am unsure.
> >
> > Do you have some advice?
>
> I didn't wrap my head fully around this again yet, but if you can't ensure a
> correct, race-free behavior by ordering, you need locking.
> Maybe have a look again how dovetail works in this context as it should have
> logically the same thing to do.
>

It looks like in dovetail the masked bit in the description is also not cleared (missing irq_state_clr_masked(desc)).
This might work because the corresponding mask function here seems also sourced from the dovetail patch and is not checking the bit before masking the irq.
No guaranties here, I am not firm with dovetail and that’s what it looks like when checking handle_percpu_irq().

Since your last comment about the first fix pointed out that unmask_irq() is way too much in this context I will send a patch with the commands reordered and no extra locking. That should work.

Thanks,
Gunter


> Jan
>
> --
> 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.

Re: [PATCH] ipipe: Fix ipipe level irq end

[Philippe Gerum]

"Grau, Gunter" <gunter.grau@philips.com> writes:

> Hi,
>
>> -----Original Message-----
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>> Sent: Montag, 15. August 2022 08:19
>> To: Grau, Gunter <gunter.grau@philips.com>; xenomai@lists.linux.dev
>> Subject: Re: [PATCH] ipipe: Fix ipipe level irq end
>>
>> Caution: This e-mail originated from outside of Philips, be careful for
>> phishing.
>>
>>
>> On 11.08.22 16:05, Grau, Gunter wrote:
>> >
>> > Hi,
>> >
>> > Unfortunately I have to come back to this.
>> >
>> > On Thu, Feb 17, 2022 at 10:12 AM Jan Kiszka
>> <mailto:jan.kiszka@siemens.com> wrote:
>> > On 17.02.22 15:11, Greg Gallagher wrote:
>> >>
>> >>
>> >> On Thu, Feb 17, 2022 at 4:15 AM Jan Kiszka
>> >> <mailto:jan.kiszka@siemens.com
>> <mailto:mailto:jan.kiszka@siemens.com>> wrote:
>> >>
>> >>     On 17.02.22 09:48, Gunter Grau via Xenomai wrote:
>> >>     > The following commit in the vanilla kernel introduced
>> >>     > a check for the cached interrupt mask flag in mask_irq():
>> >>     >
>> >>     > bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
>> >>     calls")
>> >>     >
>> >>     > This means if the flag is not serviced correctly
>> >>     > the real bit in the hardware interrupt controller may not be
>> >>     > cleared or set.
>> >>     > The __ipipe_end_level_irq() function does not follow this rule.
>> >>     > It unmasks the bit in the hardware without setting the cached flags
>> >>     > accordingly. So after the first level interrupt is finished the
>> >>     > mask cache has a wrong state. If now the next interrupt fires,
>> >>     > the mask_irq() function will not really mask the interrupt in
>> >>     > the hardware which causes a interrupt storm after reenabeling
>> >>     > the hard irqs.
>> >>     > The fix now also updates the shadow flag correctly.
>> >>     > ---
>> >>     >  kernel/irq/chip.c | 1 +
>> >>     >  1 file changed, 1 insertion(+)
>> >>     >
>> >>     > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>> >>     > index 7c03e2931189..ff9a8b3f33db 100644
>> >>     > --- a/kernel/irq/chip.c
>> >>     > +++ b/kernel/irq/chip.c
>> >>     > @@ -988,6 +988,7 @@ 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);
>> >>     >  }
>> >>     >
>> >>     >  void __ipipe_ack_fasteoi_irq(struct irq_desc *desc)
>> >>     > --
>> >>     > 2.25.1
>> >>     >
>> >
>> > We discovered some situations (with iio drivers) where this fix seem not to
>> work.
>> > Obviously in the moment where the irq is unmasked, the new interrupt
>> seems to fire again and the irq_ack() code of the new interrupt runs before
>> the irq_stats_clr_masked() is called.
>> > When I wrote the patch I thought the irqs are disabled at this moment.
>> Looks like this is not the case. At least when I change the order to:
>> >
>> > irq_state_clr_masked(desc);
>> > desc->irq_data.chip->irq_unmask(&desc->irq_data);
>> >
>> > My use case works again.
>> > Now I am unsure if the above reordering is the correct solution or maybe
>> just calling unmask_irq(); which was the first proposal is better?
>> > We dropped unmask_irq() proposal because it does additionally some
>> locking, but maybe this is needed. I am unsure.
>> >
>> > Do you have some advice?
>>
>> I didn't wrap my head fully around this again yet, but if you can't ensure a
>> correct, race-free behavior by ordering, you need locking.
>> Maybe have a look again how dovetail works in this context as it should have
>> logically the same thing to do.
>>
>
> It looks like in dovetail the masked bit in the description is also not cleared (missing irq_state_clr_masked(desc)).
> This might work because the corresponding mask function here seems also sourced from the dovetail patch and is not checking the bit before masking the irq.
> No guaranties here, I am not firm with dovetail and that’s what it
> looks like when checking handle_percpu_irq().

Dovetail does not work the I-pipe did, __ipipe_end_level_irq() is now
handled internally. The only way to re-enable interrupts in this path
would be to reschedule a real-time task, which cannot happen until
irq_exit_pipeline() is called when unwinding the interrupt frame, at
which point the companion core may reschedule, never before. So unless
the core enables back hard IRQs in the handler, which would be 100%
broken behavior, there is no issue.

IOW, both the interrupt flow and event handlers run with (hard) IRQs
masked in the CPU all the way, preventing any interrupt storm, whether
the guard bit is set prior to or after unmasking. Also, the vanilla
[un]mask_irq() logic is kept, testing and setting irqd_irq_masked() as
appropriate, i.e. when using the state cache is applicable.

handle_percpu_irq() is a special case, the vanilla code does not have to
use a state cache because you do not mask per-CPU irqs, you might
disable them entirely if not handled.

This is tricky stuff, so a bug can never be excluded, but I don't think
the I-pipe scenario applies to Dovetail.

-- 
Philippe.

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

[Gunter Grau]

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)
-- 
2.25.1

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

[Jan Kiszka]

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

-- 
Siemens AG, Technology
Competence Center Embedded Linux

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

[Grau, Gunter]

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?

Thanks,
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.

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

[Jan Kiszka]

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

-- 
Siemens AG, Technology
Competence Center Embedded Linux

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

[Grau, Gunter]

> -----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.

Thanks,
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.

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

[Jan Kiszka]

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

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

[Grau, Gunter]

> -----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.

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

[Bezdeka, Florian]

Hi all,

On Tue, 2022-08-16 at 09:23 +0300, Jan Kiszka wrote:
> 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).

While trying to update our CI images to latest Debian testing/bookworm
and with that updating from gcc 10 to gcc 12 I ran into a similar
problem.

The problem seems to be limited to the arm64 ipipe-4.19-cip for now,
but that might be due to limited testing / small race windows.

With ipipe 4.19-cip "as is" I run into a kernel stack overflow. Logs
can be found at [1] for example. Searching for
"handle_arch_irq_pipelined" brings up the interesting stuff.

I tried:

  - Applied the proposal from Gunter, moving the 
    irq_state_clr_masked(desc) call in __ipipe_end_level_irq()
    in front of the desc->irq_data.chip->irq_unmask(&desc->irq_data)
    call.
    No success. No stack overflow anymore, but system hang.

  - Removed the recently added irq_state_clr_masked(desc) call in 
    __ipipe_end_level_irq(). No success. No stack overflow anymore,
    but system hang.

  - Moved to unmask_irq(). I ended up in the kernel stack overflow
    again [2].

I can't reproduce with qemu and I don't have a board at hand. The only
thing I can do is triggering CI jobs which have long round trip times,
so debugging isn't really possible. If you have ideas, let me know. I
will do my best to help.

[1] https://lava.xenomai.org/scheduler/job/5069
[2] https://lava.xenomai.org/scheduler/job/5199

Florian

> 
> Jan
>