| Message ID | 20200113101251.37471-1-jiaxun.yang@flygoat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Burton |
| Headers | show |
| Series | irqchip: mips-cpu: Remove eoi operation | expand |
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@flygoat.com> > --- > 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..? Thanks, Paul
于 2020年1月15日 GMT+08:00 上午7:30:25, Paul Burton <paulburton@kernel.org> 写到: >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@flygoat.com> >> --- >> 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..? Hi Paul, Sorry I didn't discover that by myself as all of my drivers are using chained handler. So how should we deal with the chained case? Probably we need a check in percpu IRQ handler to determine whether it's or not level type? > >Thanks, > Paul
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@flygoat.com> >> --- >> 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 just adds to my feeling... M.
于 2020年1月15日 GMT+08:00 下午9:40:31, Marc Zyngier <maz@kernel.org> 写到: >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@flygoat.com> >>> --- >>> 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.
On 2020-01-15 14:23, Jiaxun Yang wrote: > 于 2020年1月15日 GMT+08:00 下午9:40:31, Marc Zyngier <maz@kernel.org> 写到: >> 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@flygoat.com> >>>> --- >>>> 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? Post the patch, and we'll discuss it. Thanks, M.
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, };
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@flygoat.com> --- drivers/irqchip/irq-mips-cpu.c | 1 - 1 file changed, 1 deletion(-)