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

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

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