* [PATCH 0/3] genirq: oneshot-safe threaded EOIs @ 2022-11-26 23:41 ` Samuel Holland 0 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2022-11-26 23:41 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel This is the optimization that I promised back in https://lore.kernel.org/lkml/20220701202440.59059-1-samuel@sholland.org/ and got back around to cleaning up. To quote that cover letter: "A further optimization is to take advantage of the fact that multiple IRQs can be claimed at once. This allows removing the mask operations for oneshot IRQs -- i.e. the combination of IRQCHIP_ONESHOT_SAFE and IRQCHIP_EOI_THREADED, which is not currently supported." Or in other words, we reuse the oneshot infrastructure for EOIs instead of masking. There a a few functions/variables that probably should be renamed now that I have muddied what "oneshot" means. I elected not to do that in this version to avoid distracting from the functional changes. Samuel Holland (3): genirq: Simplify cond_unmask_eoi_irq() genirq: Add support for oneshot-safe threaded EOIs irqchip/sifive-plic: Enable oneshot-safe threaded EOIs drivers/irqchip/irq-sifive-plic.c | 2 ++ kernel/irq/chip.c | 21 ++++++++++++--------- kernel/irq/manage.c | 15 ++++++++++----- 3 files changed, 24 insertions(+), 14 deletions(-) -- 2.37.4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 0/3] genirq: oneshot-safe threaded EOIs @ 2022-11-26 23:41 ` Samuel Holland 0 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2022-11-26 23:41 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel This is the optimization that I promised back in https://lore.kernel.org/lkml/20220701202440.59059-1-samuel@sholland.org/ and got back around to cleaning up. To quote that cover letter: "A further optimization is to take advantage of the fact that multiple IRQs can be claimed at once. This allows removing the mask operations for oneshot IRQs -- i.e. the combination of IRQCHIP_ONESHOT_SAFE and IRQCHIP_EOI_THREADED, which is not currently supported." Or in other words, we reuse the oneshot infrastructure for EOIs instead of masking. There a a few functions/variables that probably should be renamed now that I have muddied what "oneshot" means. I elected not to do that in this version to avoid distracting from the functional changes. Samuel Holland (3): genirq: Simplify cond_unmask_eoi_irq() genirq: Add support for oneshot-safe threaded EOIs irqchip/sifive-plic: Enable oneshot-safe threaded EOIs drivers/irqchip/irq-sifive-plic.c | 2 ++ kernel/irq/chip.c | 21 ++++++++++++--------- kernel/irq/manage.c | 15 ++++++++++----- 3 files changed, 24 insertions(+), 14 deletions(-) -- 2.37.4 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq() 2022-11-26 23:41 ` Samuel Holland @ 2022-11-26 23:41 ` Samuel Holland -1 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2022-11-26 23:41 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel This function calls .irq_eoi in three places, making the logic hard to follow. Rearrange the function so that .irq_eoi is called only once. The only time .irq_eoi is not called is when all three if checks fail, so return early in that case. threads_oneshot can only be nonzero if IRQS_ONESHOT is set, so the IRQS_ONESHOT check can be omitted there. The IRQS_ONESHOT condition from the first if statement must then be copied to the unmask_irq() condition. Furthermore, if IRQS_ONESHOT is set, mask_irq() must have been called in the parent function, so the irqd_irq_masked() check is redundant. Signed-off-by: Samuel Holland <samuel@sholland.org> --- kernel/irq/chip.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 8ac37e8e738a..672bad021a1f 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -658,10 +658,15 @@ EXPORT_SYMBOL_GPL(handle_level_irq); static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) { - if (!(desc->istate & IRQS_ONESHOT)) { - chip->irq_eoi(&desc->irq_data); + /* + * Do not send an EOI if the thread will do it later in + * unmask_threaded_irq(). + */ + if ((chip->flags & IRQCHIP_EOI_THREADED) && desc->threads_oneshot) return; - } + + chip->irq_eoi(&desc->irq_data); + /* * We need to unmask in the following cases: * - Oneshot irq which did not wake the thread (caused by a @@ -669,12 +674,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) * completely). */ if (!irqd_irq_disabled(&desc->irq_data) && - irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) { - chip->irq_eoi(&desc->irq_data); + (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot) unmask_irq(desc); - } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) { - chip->irq_eoi(&desc->irq_data); - } } /** -- 2.37.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq() @ 2022-11-26 23:41 ` Samuel Holland 0 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2022-11-26 23:41 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel This function calls .irq_eoi in three places, making the logic hard to follow. Rearrange the function so that .irq_eoi is called only once. The only time .irq_eoi is not called is when all three if checks fail, so return early in that case. threads_oneshot can only be nonzero if IRQS_ONESHOT is set, so the IRQS_ONESHOT check can be omitted there. The IRQS_ONESHOT condition from the first if statement must then be copied to the unmask_irq() condition. Furthermore, if IRQS_ONESHOT is set, mask_irq() must have been called in the parent function, so the irqd_irq_masked() check is redundant. Signed-off-by: Samuel Holland <samuel@sholland.org> --- kernel/irq/chip.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 8ac37e8e738a..672bad021a1f 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -658,10 +658,15 @@ EXPORT_SYMBOL_GPL(handle_level_irq); static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) { - if (!(desc->istate & IRQS_ONESHOT)) { - chip->irq_eoi(&desc->irq_data); + /* + * Do not send an EOI if the thread will do it later in + * unmask_threaded_irq(). + */ + if ((chip->flags & IRQCHIP_EOI_THREADED) && desc->threads_oneshot) return; - } + + chip->irq_eoi(&desc->irq_data); + /* * We need to unmask in the following cases: * - Oneshot irq which did not wake the thread (caused by a @@ -669,12 +674,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) * completely). */ if (!irqd_irq_disabled(&desc->irq_data) && - irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) { - chip->irq_eoi(&desc->irq_data); + (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot) unmask_irq(desc); - } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) { - chip->irq_eoi(&desc->irq_data); - } } /** -- 2.37.4 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq() 2022-11-26 23:41 ` Samuel Holland @ 2022-11-30 14:48 ` Thomas Gleixner -1 siblings, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2022-11-30 14:48 UTC (permalink / raw) To: Samuel Holland, Marc Zyngier, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel Samuel! On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote: > This function calls .irq_eoi in three places, making the logic hard to > follow. Rearrange the function so that .irq_eoi is called only once. > > The only time .irq_eoi is not called is when all three if checks fail, > so return early in that case. threads_oneshot can only be nonzero if > IRQS_ONESHOT is set, so the IRQS_ONESHOT check can be omitted there. > > The IRQS_ONESHOT condition from the first if statement must then be > copied to the unmask_irq() condition. > > Furthermore, if IRQS_ONESHOT is set, mask_irq() must have been called > in the parent function, so the irqd_irq_masked() check is redundant. Not really convinced that all this is functionaly equivalent. > static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) > { > - if (!(desc->istate & IRQS_ONESHOT)) { > - chip->irq_eoi(&desc->irq_data); > + /* > + * Do not send an EOI if the thread will do it later in > + * unmask_threaded_irq(). > + */ > + if ((chip->flags & IRQCHIP_EOI_THREADED) && desc->threads_oneshot) > return; > - } > + > + chip->irq_eoi(&desc->irq_data); This now issues EOI when the interrupt is in disabled state, which was not done before. That's probably a non-issue, but clearly a undocumented change. > + > /* > * We need to unmask in the following cases: > * - Oneshot irq which did not wake the thread (caused by a > @@ -669,12 +674,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) > * completely). > */ > if (!irqd_irq_disabled(&desc->irq_data) && > - irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) { > - chip->irq_eoi(&desc->irq_data); > + (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot) > unmask_irq(desc); This breaks the mask logic of handle_fasteoi_mask_irq() for an interrupt which does not have IRQS_ONESHOT set. So no, it's not the same and it even breaks stuff. Thanks, tglx ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq() @ 2022-11-30 14:48 ` Thomas Gleixner 0 siblings, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2022-11-30 14:48 UTC (permalink / raw) To: Samuel Holland, Marc Zyngier, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel Samuel! On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote: > This function calls .irq_eoi in three places, making the logic hard to > follow. Rearrange the function so that .irq_eoi is called only once. > > The only time .irq_eoi is not called is when all three if checks fail, > so return early in that case. threads_oneshot can only be nonzero if > IRQS_ONESHOT is set, so the IRQS_ONESHOT check can be omitted there. > > The IRQS_ONESHOT condition from the first if statement must then be > copied to the unmask_irq() condition. > > Furthermore, if IRQS_ONESHOT is set, mask_irq() must have been called > in the parent function, so the irqd_irq_masked() check is redundant. Not really convinced that all this is functionaly equivalent. > static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) > { > - if (!(desc->istate & IRQS_ONESHOT)) { > - chip->irq_eoi(&desc->irq_data); > + /* > + * Do not send an EOI if the thread will do it later in > + * unmask_threaded_irq(). > + */ > + if ((chip->flags & IRQCHIP_EOI_THREADED) && desc->threads_oneshot) > return; > - } > + > + chip->irq_eoi(&desc->irq_data); This now issues EOI when the interrupt is in disabled state, which was not done before. That's probably a non-issue, but clearly a undocumented change. > + > /* > * We need to unmask in the following cases: > * - Oneshot irq which did not wake the thread (caused by a > @@ -669,12 +674,8 @@ static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) > * completely). > */ > if (!irqd_irq_disabled(&desc->irq_data) && > - irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) { > - chip->irq_eoi(&desc->irq_data); > + (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot) > unmask_irq(desc); This breaks the mask logic of handle_fasteoi_mask_irq() for an interrupt which does not have IRQS_ONESHOT set. So no, it's not the same and it even breaks stuff. Thanks, tglx _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq() 2022-11-30 14:48 ` Thomas Gleixner @ 2022-11-30 15:29 ` Thomas Gleixner -1 siblings, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2022-11-30 15:29 UTC (permalink / raw) To: Samuel Holland, Marc Zyngier, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel On Wed, Nov 30 2022 at 15:48, Thomas Gleixner wrote: > On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote: >> + (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot) >> unmask_irq(desc); > > This breaks the mask logic of handle_fasteoi_mask_irq() for an interrupt > which does not have IRQS_ONESHOT set. Ignore this one. That handler is broken by design. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq() @ 2022-11-30 15:29 ` Thomas Gleixner 0 siblings, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2022-11-30 15:29 UTC (permalink / raw) To: Samuel Holland, Marc Zyngier, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel On Wed, Nov 30 2022 at 15:48, Thomas Gleixner wrote: > On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote: >> + (desc->istate & IRQS_ONESHOT) && !desc->threads_oneshot) >> unmask_irq(desc); > > This breaks the mask logic of handle_fasteoi_mask_irq() for an interrupt > which does not have IRQS_ONESHOT set. Ignore this one. That handler is broken by design. _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs 2022-11-26 23:41 ` Samuel Holland @ 2022-11-26 23:41 ` Samuel Holland -1 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2022-11-26 23:41 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel For irqchips such as the SiFive PLIC with a claim/EOI flow, each IRQ is implicitly masked during the claim operation and unmasked after the EOI. By delaying the EOI until after the thread runs, we can support threaded IRQs without any explicit mask/unmask operations. irqchips can declare this capability using the combination of flags IRQCHIP_ONESHOT_SAFE | IRQCHIP_EOI_THREADED. In this case, we still set IRQF_ONESHOT and thus action->thread_mask, so we know based on desc->threads_oneshot when to send the EOI. However, we do not set IRQS_ONESHOT, so we skip the actual mask/unmask operations. Signed-off-by: Samuel Holland <samuel@sholland.org> --- kernel/irq/chip.c | 4 +++- kernel/irq/manage.c | 15 ++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 672bad021a1f..7a4b3fa85da0 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -446,7 +446,9 @@ void unmask_threaded_irq(struct irq_desc *desc) if (chip->flags & IRQCHIP_EOI_THREADED) chip->irq_eoi(&desc->irq_data); - unmask_irq(desc); + if (!irqd_irq_disabled(&desc->irq_data) && + (desc->istate & IRQS_ONESHOT)) + unmask_irq(desc); } /* diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 40fe7806cc8c..b9edb66428cd 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1074,9 +1074,10 @@ static int irq_wait_for_interrupt(struct irqaction *action) static void irq_finalize_oneshot(struct irq_desc *desc, struct irqaction *action) { - if (!(desc->istate & IRQS_ONESHOT) || + if (!(action->flags & IRQF_ONESHOT) || action->handler == irq_forced_secondary_handler) return; + again: chip_bus_lock(desc); raw_spin_lock_irq(&desc->lock); @@ -1112,8 +1113,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc, desc->threads_oneshot &= ~action->thread_mask; - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && - irqd_irq_masked(&desc->irq_data)) + if (!desc->threads_oneshot) unmask_threaded_irq(desc); out_unlock: @@ -1565,8 +1565,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) * MSI based interrupts are per se one shot safe. Check the * chip flags, so we can avoid the unmask dance at the end of * the threaded handler for those. + * + * If IRQCHIP_EOI_THREADED is also set, we do an EOI dance + * instead of a mask/unmask dance. */ - if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE) + if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE && + !(desc->irq_data.chip->flags & IRQCHIP_EOI_THREADED)) new->flags &= ~IRQF_ONESHOT; /* @@ -1755,7 +1759,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (noirqdebug) irq_settings_set_no_debug(desc); - if (new->flags & IRQF_ONESHOT) + if (new->flags & IRQF_ONESHOT && + !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) desc->istate |= IRQS_ONESHOT; /* Exclude IRQ from balancing if requested */ -- 2.37.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs @ 2022-11-26 23:41 ` Samuel Holland 0 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2022-11-26 23:41 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel For irqchips such as the SiFive PLIC with a claim/EOI flow, each IRQ is implicitly masked during the claim operation and unmasked after the EOI. By delaying the EOI until after the thread runs, we can support threaded IRQs without any explicit mask/unmask operations. irqchips can declare this capability using the combination of flags IRQCHIP_ONESHOT_SAFE | IRQCHIP_EOI_THREADED. In this case, we still set IRQF_ONESHOT and thus action->thread_mask, so we know based on desc->threads_oneshot when to send the EOI. However, we do not set IRQS_ONESHOT, so we skip the actual mask/unmask operations. Signed-off-by: Samuel Holland <samuel@sholland.org> --- kernel/irq/chip.c | 4 +++- kernel/irq/manage.c | 15 ++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 672bad021a1f..7a4b3fa85da0 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -446,7 +446,9 @@ void unmask_threaded_irq(struct irq_desc *desc) if (chip->flags & IRQCHIP_EOI_THREADED) chip->irq_eoi(&desc->irq_data); - unmask_irq(desc); + if (!irqd_irq_disabled(&desc->irq_data) && + (desc->istate & IRQS_ONESHOT)) + unmask_irq(desc); } /* diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index 40fe7806cc8c..b9edb66428cd 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1074,9 +1074,10 @@ static int irq_wait_for_interrupt(struct irqaction *action) static void irq_finalize_oneshot(struct irq_desc *desc, struct irqaction *action) { - if (!(desc->istate & IRQS_ONESHOT) || + if (!(action->flags & IRQF_ONESHOT) || action->handler == irq_forced_secondary_handler) return; + again: chip_bus_lock(desc); raw_spin_lock_irq(&desc->lock); @@ -1112,8 +1113,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc, desc->threads_oneshot &= ~action->thread_mask; - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && - irqd_irq_masked(&desc->irq_data)) + if (!desc->threads_oneshot) unmask_threaded_irq(desc); out_unlock: @@ -1565,8 +1565,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) * MSI based interrupts are per se one shot safe. Check the * chip flags, so we can avoid the unmask dance at the end of * the threaded handler for those. + * + * If IRQCHIP_EOI_THREADED is also set, we do an EOI dance + * instead of a mask/unmask dance. */ - if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE) + if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE && + !(desc->irq_data.chip->flags & IRQCHIP_EOI_THREADED)) new->flags &= ~IRQF_ONESHOT; /* @@ -1755,7 +1759,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) if (noirqdebug) irq_settings_set_no_debug(desc); - if (new->flags & IRQF_ONESHOT) + if (new->flags & IRQF_ONESHOT && + !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) desc->istate |= IRQS_ONESHOT; /* Exclude IRQ from balancing if requested */ -- 2.37.4 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs 2022-11-26 23:41 ` Samuel Holland @ 2022-11-30 15:55 ` Thomas Gleixner -1 siblings, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2022-11-30 15:55 UTC (permalink / raw) To: Samuel Holland, Marc Zyngier, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel Samuel! On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote: > For irqchips such as the SiFive PLIC with a claim/EOI flow, each IRQ is > implicitly masked during the claim operation and unmasked after the EOI. > By delaying the EOI until after the thread runs, we can support threaded > IRQs without any explicit mask/unmask operations. > > irqchips can declare this capability using the combination of flags > IRQCHIP_ONESHOT_SAFE | IRQCHIP_EOI_THREADED. > > In this case, we still set IRQF_ONESHOT and thus action->thread_mask, so > we know based on desc->threads_oneshot when to send the EOI. However, we > do not set IRQS_ONESHOT, so we skip the actual mask/unmask operations. Lots of 'we' here. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 672bad021a1f..7a4b3fa85da0 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -446,7 +446,9 @@ void unmask_threaded_irq(struct irq_desc *desc) > if (chip->flags & IRQCHIP_EOI_THREADED) > chip->irq_eoi(&desc->irq_data); > > - unmask_irq(desc); > + if (!irqd_irq_disabled(&desc->irq_data) && > + (desc->istate & IRQS_ONESHOT)) > + unmask_irq(desc); > > /* > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 40fe7806cc8c..b9edb66428cd 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1074,9 +1074,10 @@ static int irq_wait_for_interrupt(struct irqaction *action) > static void irq_finalize_oneshot(struct irq_desc *desc, > struct irqaction *action) > { > - if (!(desc->istate & IRQS_ONESHOT) || > + if (!(action->flags & IRQF_ONESHOT) || > action->handler == irq_forced_secondary_handler) > return; > + > again: > chip_bus_lock(desc); > raw_spin_lock_irq(&desc->lock); > @@ -1112,8 +1113,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc, > > desc->threads_oneshot &= ~action->thread_mask; > > - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && Same undocumented change vs. disabled and EOI. It might not matter, but if something like this is changed, then it has to be done in a separate commit with a very good explanation why this is correct and not silently burried into a hodgepodge of other changes. > - irqd_irq_masked(&desc->irq_data)) > + if (!desc->threads_oneshot) > unmask_threaded_irq(desc); > > out_unlock: > @@ -1565,8 +1565,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > * MSI based interrupts are per se one shot safe. Check the > * chip flags, so we can avoid the unmask dance at the end of > * the threaded handler for those. > + * > + * If IRQCHIP_EOI_THREADED is also set, we do an EOI dance > + * instead of a mask/unmask dance. > */ > - if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE) > + if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE && > + !(desc->irq_data.chip->flags & IRQCHIP_EOI_THREADED)) This flag combination makes my head spin. > new->flags &= ~IRQF_ONESHOT; > > /* > @@ -1755,7 +1759,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > if (noirqdebug) > irq_settings_set_no_debug(desc); > > - if (new->flags & IRQF_ONESHOT) > + if (new->flags & IRQF_ONESHOT && > + !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) > desc->istate |= IRQS_ONESHOT; And I really detest the resulting action to desc inconsistency: > - if (!(desc->istate & IRQS_ONESHOT) || > + if (!(action->flags & IRQF_ONESHOT) || It's clever, but completely non-obvious. So let's take a step back. You want to defer the EOI to the point where the threaded handler finishes, but you dont want the mask/unmask mechanism which is attached to IRQS_ONESHOT. So instead of playing games with IRQS_ONESHOT and IRQF_ONESHOT and creating something which cannot even be remotely followed, why not having explicit and understandable flags for theses cases. Something like the incomplete, uncompiled below. You get the idea, right? Thanks, tglx --- --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -656,23 +656,27 @@ void handle_level_irq(struct irq_desc *d } EXPORT_SYMBOL_GPL(handle_level_irq); +static inline cond_mask_eoi_irq(struct irq_desc *desc) +{ + if (desc->istate & IRQS_UNMASK_EOI_THREAD) + mask_irq(desc); +} + static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) { - if (!(desc->istate & IRQS_ONESHOT)) { - chip->irq_eoi(&desc->irq_data); - return; - } - /* - * We need to unmask in the following cases: - * - Oneshot irq which did not wake the thread (caused by a - * spurious interrupt or a primary handler handling it - * completely). - */ - if (!irqd_irq_disabled(&desc->irq_data) && - irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) { - chip->irq_eoi(&desc->irq_data); - unmask_irq(desc); - } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) { + if (desc->istate & IRQS_UNMASK_EOI_THREAD) { + /* + * We need to EOI and unmask when the primary handler + * did not wake the thread due to a spurious interrupt or + * the primary handler handling the interrupt completely. + */ + if (!irqd_irq_disabled(&desc->irq_data) && + irqd_irq_masked(&desc->irq_data) && + !desc->threads_oneshot) { + chip->irq_eoi(&desc->irq_data); + unmask_irq(desc); + } + } else if (!(desc->istate & IRQS_EOI_THREAD) || !desc->threads_oneshot) { chip->irq_eoi(&desc->irq_data); } } @@ -708,8 +712,7 @@ void handle_fasteoi_irq(struct irq_desc } kstat_incr_irqs_this_cpu(desc); - if (desc->istate & IRQS_ONESHOT) - mask_irq(desc); + cond_mask_eoi_irq(desc); handle_irq_event(desc); @@ -1219,8 +1222,7 @@ void handle_fasteoi_ack_irq(struct irq_d } kstat_incr_irqs_this_cpu(desc); - if (desc->istate & IRQS_ONESHOT) - mask_irq(desc); + cond_mask_eoi_irq(desc); /* Start handling the irq */ desc->irq_data.chip->irq_ack(&desc->irq_data); @@ -1266,16 +1268,14 @@ void handle_fasteoi_mask_irq(struct irq_ */ if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) { desc->istate |= IRQS_PENDING; - mask_irq(desc); goto out; } kstat_incr_irqs_this_cpu(desc); - if (desc->istate & IRQS_ONESHOT) - mask_irq(desc); handle_irq_event(desc); + /* Needs more fixes ... */ cond_unmask_eoi_irq(desc, chip); raw_spin_unlock(&desc->lock); --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -53,6 +53,8 @@ enum { * IRQS_SUSPENDED - irq is suspended * IRQS_NMI - irq line is used to deliver NMIs * IRQS_SYSFS - descriptor has been added to sysfs + * IRQS_UNMASK_EOI_THREAD - EOI and unmask is deferred to the threaded handler + * IRQS_EOI_THREAD - EOI is deferred to the threaded handler */ enum { IRQS_AUTODETECT = 0x00000001, @@ -66,6 +68,8 @@ enum { IRQS_TIMINGS = 0x00001000, IRQS_NMI = 0x00002000, IRQS_SYSFS = 0x00004000, + IRQS_UNMASK_EOI_THREAD = 0x00008000, + IRQS_EOI_THREAD = 0x00010000, }; #include "debug.h" ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs @ 2022-11-30 15:55 ` Thomas Gleixner 0 siblings, 0 replies; 14+ messages in thread From: Thomas Gleixner @ 2022-11-30 15:55 UTC (permalink / raw) To: Samuel Holland, Marc Zyngier, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel Samuel! On Sat, Nov 26 2022 at 17:41, Samuel Holland wrote: > For irqchips such as the SiFive PLIC with a claim/EOI flow, each IRQ is > implicitly masked during the claim operation and unmasked after the EOI. > By delaying the EOI until after the thread runs, we can support threaded > IRQs without any explicit mask/unmask operations. > > irqchips can declare this capability using the combination of flags > IRQCHIP_ONESHOT_SAFE | IRQCHIP_EOI_THREADED. > > In this case, we still set IRQF_ONESHOT and thus action->thread_mask, so > we know based on desc->threads_oneshot when to send the EOI. However, we > do not set IRQS_ONESHOT, so we skip the actual mask/unmask operations. Lots of 'we' here. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 672bad021a1f..7a4b3fa85da0 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -446,7 +446,9 @@ void unmask_threaded_irq(struct irq_desc *desc) > if (chip->flags & IRQCHIP_EOI_THREADED) > chip->irq_eoi(&desc->irq_data); > > - unmask_irq(desc); > + if (!irqd_irq_disabled(&desc->irq_data) && > + (desc->istate & IRQS_ONESHOT)) > + unmask_irq(desc); > > /* > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 40fe7806cc8c..b9edb66428cd 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1074,9 +1074,10 @@ static int irq_wait_for_interrupt(struct irqaction *action) > static void irq_finalize_oneshot(struct irq_desc *desc, > struct irqaction *action) > { > - if (!(desc->istate & IRQS_ONESHOT) || > + if (!(action->flags & IRQF_ONESHOT) || > action->handler == irq_forced_secondary_handler) > return; > + > again: > chip_bus_lock(desc); > raw_spin_lock_irq(&desc->lock); > @@ -1112,8 +1113,7 @@ static void irq_finalize_oneshot(struct irq_desc *desc, > > desc->threads_oneshot &= ~action->thread_mask; > > - if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) && Same undocumented change vs. disabled and EOI. It might not matter, but if something like this is changed, then it has to be done in a separate commit with a very good explanation why this is correct and not silently burried into a hodgepodge of other changes. > - irqd_irq_masked(&desc->irq_data)) > + if (!desc->threads_oneshot) > unmask_threaded_irq(desc); > > out_unlock: > @@ -1565,8 +1565,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > * MSI based interrupts are per se one shot safe. Check the > * chip flags, so we can avoid the unmask dance at the end of > * the threaded handler for those. > + * > + * If IRQCHIP_EOI_THREADED is also set, we do an EOI dance > + * instead of a mask/unmask dance. > */ > - if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE) > + if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE && > + !(desc->irq_data.chip->flags & IRQCHIP_EOI_THREADED)) This flag combination makes my head spin. > new->flags &= ~IRQF_ONESHOT; > > /* > @@ -1755,7 +1759,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) > if (noirqdebug) > irq_settings_set_no_debug(desc); > > - if (new->flags & IRQF_ONESHOT) > + if (new->flags & IRQF_ONESHOT && > + !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) > desc->istate |= IRQS_ONESHOT; And I really detest the resulting action to desc inconsistency: > - if (!(desc->istate & IRQS_ONESHOT) || > + if (!(action->flags & IRQF_ONESHOT) || It's clever, but completely non-obvious. So let's take a step back. You want to defer the EOI to the point where the threaded handler finishes, but you dont want the mask/unmask mechanism which is attached to IRQS_ONESHOT. So instead of playing games with IRQS_ONESHOT and IRQF_ONESHOT and creating something which cannot even be remotely followed, why not having explicit and understandable flags for theses cases. Something like the incomplete, uncompiled below. You get the idea, right? Thanks, tglx --- --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -656,23 +656,27 @@ void handle_level_irq(struct irq_desc *d } EXPORT_SYMBOL_GPL(handle_level_irq); +static inline cond_mask_eoi_irq(struct irq_desc *desc) +{ + if (desc->istate & IRQS_UNMASK_EOI_THREAD) + mask_irq(desc); +} + static void cond_unmask_eoi_irq(struct irq_desc *desc, struct irq_chip *chip) { - if (!(desc->istate & IRQS_ONESHOT)) { - chip->irq_eoi(&desc->irq_data); - return; - } - /* - * We need to unmask in the following cases: - * - Oneshot irq which did not wake the thread (caused by a - * spurious interrupt or a primary handler handling it - * completely). - */ - if (!irqd_irq_disabled(&desc->irq_data) && - irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) { - chip->irq_eoi(&desc->irq_data); - unmask_irq(desc); - } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) { + if (desc->istate & IRQS_UNMASK_EOI_THREAD) { + /* + * We need to EOI and unmask when the primary handler + * did not wake the thread due to a spurious interrupt or + * the primary handler handling the interrupt completely. + */ + if (!irqd_irq_disabled(&desc->irq_data) && + irqd_irq_masked(&desc->irq_data) && + !desc->threads_oneshot) { + chip->irq_eoi(&desc->irq_data); + unmask_irq(desc); + } + } else if (!(desc->istate & IRQS_EOI_THREAD) || !desc->threads_oneshot) { chip->irq_eoi(&desc->irq_data); } } @@ -708,8 +712,7 @@ void handle_fasteoi_irq(struct irq_desc } kstat_incr_irqs_this_cpu(desc); - if (desc->istate & IRQS_ONESHOT) - mask_irq(desc); + cond_mask_eoi_irq(desc); handle_irq_event(desc); @@ -1219,8 +1222,7 @@ void handle_fasteoi_ack_irq(struct irq_d } kstat_incr_irqs_this_cpu(desc); - if (desc->istate & IRQS_ONESHOT) - mask_irq(desc); + cond_mask_eoi_irq(desc); /* Start handling the irq */ desc->irq_data.chip->irq_ack(&desc->irq_data); @@ -1266,16 +1268,14 @@ void handle_fasteoi_mask_irq(struct irq_ */ if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) { desc->istate |= IRQS_PENDING; - mask_irq(desc); goto out; } kstat_incr_irqs_this_cpu(desc); - if (desc->istate & IRQS_ONESHOT) - mask_irq(desc); handle_irq_event(desc); + /* Needs more fixes ... */ cond_unmask_eoi_irq(desc, chip); raw_spin_unlock(&desc->lock); --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -53,6 +53,8 @@ enum { * IRQS_SUSPENDED - irq is suspended * IRQS_NMI - irq line is used to deliver NMIs * IRQS_SYSFS - descriptor has been added to sysfs + * IRQS_UNMASK_EOI_THREAD - EOI and unmask is deferred to the threaded handler + * IRQS_EOI_THREAD - EOI is deferred to the threaded handler */ enum { IRQS_AUTODETECT = 0x00000001, @@ -66,6 +68,8 @@ enum { IRQS_TIMINGS = 0x00001000, IRQS_NMI = 0x00002000, IRQS_SYSFS = 0x00004000, + IRQS_UNMASK_EOI_THREAD = 0x00008000, + IRQS_EOI_THREAD = 0x00010000, }; #include "debug.h" _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/3] irqchip/sifive-plic: Enable oneshot-safe threaded EOIs 2022-11-26 23:41 ` Samuel Holland @ 2022-11-26 23:41 ` Samuel Holland -1 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2022-11-26 23:41 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel This defers the EOI until the IRQ thread has finished, avoiding the need to mask the IRQ while the thread is pending. Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/irqchip/irq-sifive-plic.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index ff47bd0dec45..d8fc3354b38c 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -203,6 +203,8 @@ static struct irq_chip plic_chip = { #endif .irq_set_type = plic_irq_set_type, .flags = IRQCHIP_SKIP_SET_WAKE | + IRQCHIP_ONESHOT_SAFE | + IRQCHIP_EOI_THREADED | IRQCHIP_AFFINITY_PRE_STARTUP, }; -- 2.37.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] irqchip/sifive-plic: Enable oneshot-safe threaded EOIs @ 2022-11-26 23:41 ` Samuel Holland 0 siblings, 0 replies; 14+ messages in thread From: Samuel Holland @ 2022-11-26 23:41 UTC (permalink / raw) To: Marc Zyngier, Thomas Gleixner, Palmer Dabbelt Cc: linux-riscv, Samuel Holland, Paul Walmsley, linux-kernel This defers the EOI until the IRQ thread has finished, avoiding the need to mask the IRQ while the thread is pending. Signed-off-by: Samuel Holland <samuel@sholland.org> --- drivers/irqchip/irq-sifive-plic.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index ff47bd0dec45..d8fc3354b38c 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -203,6 +203,8 @@ static struct irq_chip plic_chip = { #endif .irq_set_type = plic_irq_set_type, .flags = IRQCHIP_SKIP_SET_WAKE | + IRQCHIP_ONESHOT_SAFE | + IRQCHIP_EOI_THREADED | IRQCHIP_AFFINITY_PRE_STARTUP, }; -- 2.37.4 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-11-30 15:56 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-26 23:41 [PATCH 0/3] genirq: oneshot-safe threaded EOIs Samuel Holland 2022-11-26 23:41 ` Samuel Holland 2022-11-26 23:41 ` [PATCH 1/3] genirq: Simplify cond_unmask_eoi_irq() Samuel Holland 2022-11-26 23:41 ` Samuel Holland 2022-11-30 14:48 ` Thomas Gleixner 2022-11-30 14:48 ` Thomas Gleixner 2022-11-30 15:29 ` Thomas Gleixner 2022-11-30 15:29 ` Thomas Gleixner 2022-11-26 23:41 ` [PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs Samuel Holland 2022-11-26 23:41 ` Samuel Holland 2022-11-30 15:55 ` Thomas Gleixner 2022-11-30 15:55 ` Thomas Gleixner 2022-11-26 23:41 ` [PATCH 3/3] irqchip/sifive-plic: Enable " Samuel Holland 2022-11-26 23:41 ` Samuel Holland
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.