From: Thomas Gleixner <tglx@linutronix.de> To: Samuel Holland <samuel@sholland.org>, Marc Zyngier <maz@kernel.org>, Palmer Dabbelt <palmer@dabbelt.com> Cc: linux-riscv@lists.infradead.org, Samuel Holland <samuel@sholland.org>, Paul Walmsley <paul.walmsley@sifive.com>, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs Date: Wed, 30 Nov 2022 16:55:47 +0100 [thread overview] Message-ID: <877czctp18.ffs@tglx> (raw) In-Reply-To: <20221126234134.32660-3-samuel@sholland.org> 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"
WARNING: multiple messages have this Message-ID (diff)
From: Thomas Gleixner <tglx@linutronix.de> To: Samuel Holland <samuel@sholland.org>, Marc Zyngier <maz@kernel.org>, Palmer Dabbelt <palmer@dabbelt.com> Cc: linux-riscv@lists.infradead.org, Samuel Holland <samuel@sholland.org>, Paul Walmsley <paul.walmsley@sifive.com>, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3] genirq: Add support for oneshot-safe threaded EOIs Date: Wed, 30 Nov 2022 16:55:47 +0100 [thread overview] Message-ID: <877czctp18.ffs@tglx> (raw) In-Reply-To: <20221126234134.32660-3-samuel@sholland.org> 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
next prev parent reply other threads:[~2022-11-30 15:56 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 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
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=877czctp18.ffs@tglx \ --to=tglx@linutronix.de \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=maz@kernel.org \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=samuel@sholland.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.