| Message ID | 1413985427-20918-2-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ezequiel, On 22/10/2014 15:43, Ezequiel Garcia wrote: > The map, mask and unmask is unnecessarily complicated, with a different > implementation for shared and per CPU interrupts. The current code does > the following: > > At probe time, all interrupts are disabled and masked on all CPUs. > > Shared interrupts: > > * When the interrupt is mapped(), it gets disabled and unmasked on the > calling CPU. > > * When the interrupt is unmasked(), masked(), it gets enabled and > disabled. > > Per CPU interrupts: > > * When the interrupt is mapped, it gets masked on the calling CPU and > enabled. > > * When the interrupt is unmasked(), masked(), it gets unmasked and masked, > on the calling CPU. > > This commit simplifies this code, with a much simpler implementation, common > to shared and per CPU interrupts. > > * When the interrupt is mapped, it's enabled. > > * When the interrupt is unmasked(), masked(), it gets unmasked and masked, > on the calling CPU. By doing this you change the behavior of the irqchip. Before this patch, masking a shared interrupt was masking it on all the CPUs in the same time whereas with this change it will mask the interrupt only on the calling CPU. It worth checking it is the expected behavior. Moreover I wonder how it is supposed to work with the irq affinity. Let say that the irq was enabled on the CPU0, then there is irq_unmask call on CPU1, then the irq would be enabled on both CPUs. It will modify the irq affinity and moreover it will also lead to an invalidate state with the MPIC because we can't manage an interrupt on more than one CPU. From the point of the view of the armada_370_xp_irq driver there are potential bug, but maybe the use case I described can't happen. Could you check it? Thanks, Gregory > > Tested on a Armada XP SoC with SMP and UP configurations, with chained and > regular interrupts. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > drivers/irqchip/irq-armada-370-xp.c | 27 ++++----------------------- > 1 file changed, 4 insertions(+), 23 deletions(-) > > diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c > index 3e238cd..af4e307 100644 > --- a/drivers/irqchip/irq-armada-370-xp.c > +++ b/drivers/irqchip/irq-armada-370-xp.c > @@ -73,33 +73,18 @@ static DEFINE_MUTEX(msi_used_lock); > static phys_addr_t msi_doorbell_addr; > #endif > > -/* > - * In SMP mode: > - * For shared global interrupts, mask/unmask global enable bit > - * For CPU interrupts, mask/unmask the calling CPU's bit > - */ > static void armada_370_xp_irq_mask(struct irq_data *d) > { > irq_hw_number_t hwirq = irqd_to_hwirq(d); > > - if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) > - writel(hwirq, main_int_base + > - ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS); > - else > - writel(hwirq, per_cpu_int_base + > - ARMADA_370_XP_INT_SET_MASK_OFFS); > + writel(hwirq, per_cpu_int_base + ARMADA_370_XP_INT_SET_MASK_OFFS); > } > > static void armada_370_xp_irq_unmask(struct irq_data *d) > { > irq_hw_number_t hwirq = irqd_to_hwirq(d); > > - if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) > - writel(hwirq, main_int_base + > - ARMADA_370_XP_INT_SET_ENABLE_OFFS); > - else > - writel(hwirq, per_cpu_int_base + > - ARMADA_370_XP_INT_CLEAR_MASK_OFFS); > + writel(hwirq, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS); > } > > #ifdef CONFIG_PCI_MSI > @@ -282,12 +267,8 @@ static struct irq_chip armada_370_xp_irq_chip = { > static int armada_370_xp_mpic_irq_map(struct irq_domain *h, > unsigned int virq, irq_hw_number_t hw) > { > - armada_370_xp_irq_mask(irq_get_irq_data(virq)); > - if (hw != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) > - writel(hw, per_cpu_int_base + > - ARMADA_370_XP_INT_CLEAR_MASK_OFFS); > - else > - writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS); > + writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS); > + > irq_set_status_flags(virq, IRQ_LEVEL); > > if (hw == ARMADA_370_XP_TIMER0_PER_CPU_IRQ) { >
Hi Gregory, On 10/31/2014 01:36 PM, Gregory CLEMENT wrote: > On 22/10/2014 15:43, Ezequiel Garcia wrote: >> The map, mask and unmask is unnecessarily complicated, with a different >> implementation for shared and per CPU interrupts. The current code does >> the following: >> >> At probe time, all interrupts are disabled and masked on all CPUs. >> >> Shared interrupts: >> >> * When the interrupt is mapped(), it gets disabled and unmasked on the >> calling CPU. >> >> * When the interrupt is unmasked(), masked(), it gets enabled and >> disabled. >> >> Per CPU interrupts: >> >> * When the interrupt is mapped, it gets masked on the calling CPU and >> enabled. >> >> * When the interrupt is unmasked(), masked(), it gets unmasked and masked, >> on the calling CPU. >> >> This commit simplifies this code, with a much simpler implementation, common >> to shared and per CPU interrupts. >> >> * When the interrupt is mapped, it's enabled. >> >> * When the interrupt is unmasked(), masked(), it gets unmasked and masked, >> on the calling CPU. > > By doing this you change the behavior of the irqchip. Before this patch, masking > a shared interrupt was masking it on all the CPUs in the same time whereas with this > change it will mask the interrupt only on the calling CPU. It worth checking it is > the expected behavior. > The kernel will always mask/unmask on the appropriate CPU, so I don't see any issues with this. > Moreover I wonder how it is supposed to work with the irq affinity. Let say that the > irq was enabled on the CPU0, If you mean enabled as in irq_enable(irq_desc), then let's keep in mind that currently that calls .irq_unmask. > then there is irq_unmask call on CPU1, then the irq would be enabled on both CPUs. > It will modify the irq affinity and moreover it will also lead > to an invalidate state with the MPIC because we can't manage an interrupt on more than > one CPU. > > From the point of the view of the armada_370_xp_irq driver there are potential bug, but > maybe the use case I described can't happen. Could you check it? > In this irqchip driver, shared interrupts are handled via handle_level_irq(), which masks() and unmasks() the interrupt. In other words, it's always done in the same CPU. I guess this is the most frequent mask/unmask operation. On the other side, if a driver calls enable_irq() on one CPU and then enable_irq() on the other CPU, that's exactly what will happen. Although the smp_affinity setting will prevent an interrupt from being dispatched to more than one CPU. I really don't see an issue with this patch, but I think it needs as much discussion and as much testing as possible. This piece of code is rather critical, and is working just fine, so I'd like to have an excellent reason before changing any of it.
Hi Ezequiel, On 04/11/2014 16:11, Ezequiel Garcia wrote: > Hi Gregory, > > On 10/31/2014 01:36 PM, Gregory CLEMENT wrote: >> On 22/10/2014 15:43, Ezequiel Garcia wrote: >>> The map, mask and unmask is unnecessarily complicated, with a different >>> implementation for shared and per CPU interrupts. The current code does >>> the following: >>> >>> At probe time, all interrupts are disabled and masked on all CPUs. >>> >>> Shared interrupts: >>> >>> * When the interrupt is mapped(), it gets disabled and unmasked on the >>> calling CPU. >>> >>> * When the interrupt is unmasked(), masked(), it gets enabled and >>> disabled. >>> >>> Per CPU interrupts: >>> >>> * When the interrupt is mapped, it gets masked on the calling CPU and >>> enabled. >>> >>> * When the interrupt is unmasked(), masked(), it gets unmasked and masked, >>> on the calling CPU. >>> >>> This commit simplifies this code, with a much simpler implementation, common >>> to shared and per CPU interrupts. >>> >>> * When the interrupt is mapped, it's enabled. >>> >>> * When the interrupt is unmasked(), masked(), it gets unmasked and masked, >>> on the calling CPU. >> >> By doing this you change the behavior of the irqchip. Before this patch, masking >> a shared interrupt was masking it on all the CPUs in the same time whereas with this >> change it will mask the interrupt only on the calling CPU. It worth checking it is >> the expected behavior. >> > > The kernel will always mask/unmask on the appropriate CPU, so I don't see any > issues with this. > >> Moreover I wonder how it is supposed to work with the irq affinity. Let say that the >> irq was enabled on the CPU0, > > If you mean enabled as in irq_enable(irq_desc), then let's keep in mind that > currently that calls .irq_unmask. > >> then there is irq_unmask call on CPU1, then the irq would be enabled on both CPUs. >> It will modify the irq affinity and moreover it will also lead >> to an invalidate state with the MPIC because we can't manage an interrupt on more than >> one CPU. >> >> From the point of the view of the armada_370_xp_irq driver there are potential bug, but >> maybe the use case I described can't happen. Could you check it? >> > > In this irqchip driver, shared interrupts are handled via handle_level_irq(), > which masks() and unmasks() the interrupt. In other words, it's always done > in the same CPU. I guess this is the most frequent mask/unmask operation. > > On the other side, if a driver calls enable_irq() on one CPU and then > enable_irq() on the other CPU, that's exactly what will happen. Although the > smp_affinity setting will prevent an interrupt from being dispatched to more > than one CPU. > > I really don't see an issue with this patch, but I think it needs as much > discussion and as much testing as possible. This piece of code is rather > critical, and is working just fine, so I'd like to have an excellent > reason before changing any of it. We already talked about it on IRC but I write it agin o let the other people know our status on this subject. If a driver call irq_enable() then this functions will call irq_enable() and as we didn't implement a .enbale() operation, it will call our only unmask() function. So if the IRQ was unmasked on a CPU and a driver call an irq_enable() from an other CPU then we will end up with the IRQ enabled on 2 different CPUs. It is a problem for 2 reasons: - our hardware don't handle a IRQ enable on more than one CPU - it will modify the affinity at the hardware level because a new CPU will be able to receive an irq whereas we setup the affinity on only one CPU. By only using the mask and unmask registers we will need to handle the affinity by software instead of using the hardware. I agree that the current code is not trivial, but adding more comment instead of removing this capability should be better. Thanks, Gregory
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c index 3e238cd..af4e307 100644 --- a/drivers/irqchip/irq-armada-370-xp.c +++ b/drivers/irqchip/irq-armada-370-xp.c @@ -73,33 +73,18 @@ static DEFINE_MUTEX(msi_used_lock); static phys_addr_t msi_doorbell_addr; #endif -/* - * In SMP mode: - * For shared global interrupts, mask/unmask global enable bit - * For CPU interrupts, mask/unmask the calling CPU's bit - */ static void armada_370_xp_irq_mask(struct irq_data *d) { irq_hw_number_t hwirq = irqd_to_hwirq(d); - if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) - writel(hwirq, main_int_base + - ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS); - else - writel(hwirq, per_cpu_int_base + - ARMADA_370_XP_INT_SET_MASK_OFFS); + writel(hwirq, per_cpu_int_base + ARMADA_370_XP_INT_SET_MASK_OFFS); } static void armada_370_xp_irq_unmask(struct irq_data *d) { irq_hw_number_t hwirq = irqd_to_hwirq(d); - if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) - writel(hwirq, main_int_base + - ARMADA_370_XP_INT_SET_ENABLE_OFFS); - else - writel(hwirq, per_cpu_int_base + - ARMADA_370_XP_INT_CLEAR_MASK_OFFS); + writel(hwirq, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS); } #ifdef CONFIG_PCI_MSI @@ -282,12 +267,8 @@ static struct irq_chip armada_370_xp_irq_chip = { static int armada_370_xp_mpic_irq_map(struct irq_domain *h, unsigned int virq, irq_hw_number_t hw) { - armada_370_xp_irq_mask(irq_get_irq_data(virq)); - if (hw != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) - writel(hw, per_cpu_int_base + - ARMADA_370_XP_INT_CLEAR_MASK_OFFS); - else - writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS); + writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS); + irq_set_status_flags(virq, IRQ_LEVEL); if (hw == ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
The map, mask and unmask is unnecessarily complicated, with a different implementation for shared and per CPU interrupts. The current code does the following: At probe time, all interrupts are disabled and masked on all CPUs. Shared interrupts: * When the interrupt is mapped(), it gets disabled and unmasked on the calling CPU. * When the interrupt is unmasked(), masked(), it gets enabled and disabled. Per CPU interrupts: * When the interrupt is mapped, it gets masked on the calling CPU and enabled. * When the interrupt is unmasked(), masked(), it gets unmasked and masked, on the calling CPU. This commit simplifies this code, with a much simpler implementation, common to shared and per CPU interrupts. * When the interrupt is mapped, it's enabled. * When the interrupt is unmasked(), masked(), it gets unmasked and masked, on the calling CPU. Tested on a Armada XP SoC with SMP and UP configurations, with chained and regular interrupts. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- drivers/irqchip/irq-armada-370-xp.c | 27 ++++----------------------- 1 file changed, 4 insertions(+), 23 deletions(-)