Re: [PATCH] irqchip: mips-cpu: Remove eoi operation

[Jiaxun Yang]

ä 2020å1æ15æ GMT+08:00 äå9:40:31, Marc Zyngier <maz@xxxxxxxxxx> åå:
>On 2020-01-14 23:30, Paul Burton wrote:
>> Hi Jiaxun,
>> 
>> On Mon, Jan 13, 2020 at 06:12:51PM +0800, Jiaxun Yang wrote:
>>> The eoi opreation in mips_cpu_irq_controller caused
>chained_irq_enter
>>> falsely consider CPU IP interrupt as a FastEOI type IRQ. So the 
>>> interrupt
>>> won't be masked during in handler. Which might lead to spurious 
>>> interrupt.
>>> 
>>> Thus we simply remove eoi operation for mips_cpu_irq_controller,
>>> 
>>> Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
>>> ---
>>>  drivers/irqchip/irq-mips-cpu.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>> 
>>> diff --git a/drivers/irqchip/irq-mips-cpu.c 
>>> b/drivers/irqchip/irq-mips-cpu.c
>>> index 95d4fd8f7a96..0ad7f1f9a58b 100644
>>> --- a/drivers/irqchip/irq-mips-cpu.c
>>> +++ b/drivers/irqchip/irq-mips-cpu.c
>>> @@ -55,7 +55,6 @@ static struct irq_chip mips_cpu_irq_controller = {
>>>  	.irq_mask	= mask_mips_irq,
>>>  	.irq_mask_ack	= mask_mips_irq,
>>>  	.irq_unmask	= unmask_mips_irq,
>>> -	.irq_eoi	= unmask_mips_irq,
>>>  	.irq_disable	= mask_mips_irq,
>>>  	.irq_enable	= unmask_mips_irq,
>>>  };
>> 
>> This one scares me; something doesn't seem right. The irq_eoi (nÃe
>eoi)
>> callback was first added way back in commit 1417836e81c0 ("[MIPS] use
>> generic_handle_irq, handle_level_irq, handle_percpu_irq"). The commit
>> message there states that the motivation was to allow use of
>> handle_percpu_irq(), and indeed handle_percpu_irq() does:
>> 
>>     irq_ack() (ie. mask)
>>     invoke the handler(s)
>>     irq_eoi() (ie. unmask)
>> 
>> By removing the irq_eoi callback I don't see how we'd ever unmask the
>> interrupt again..?
>
>To be completely blunt, the fact that unmask and eoi are implemented
>the
>same way is a clear sign that this is a bit broken.
>
>irq_eoi is used if the irqchip tracks the IRQ life-cycle in HW, and
>it's
>not obvious that this is the case. The fact that ack is also mapped to 
>mask

It's just a kind of hack to workaround the fact that our current percpu irq handler assumed
all percpu irqs are edge triggered or fasteoi type.

However MIPS processor implemented it in level triggered way.

My solution would be add a check. If neither ack nor eoi exist for the chip,
than we assume it's level triggered and process precpu irq in mask/unmask way.

Could it be a possible option?

Thanks.

>just adds to my feeling...
>
>         M.

-- 
Jiaxun Yang