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 1/3] genirq: Simplify cond_unmask_eoi_irq() Date: Wed, 30 Nov 2022 15:48:18 +0100 [thread overview] Message-ID: <87edtkts5p.ffs@tglx> (raw) In-Reply-To: <20221126234134.32660-2-samuel@sholland.org> 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
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 1/3] genirq: Simplify cond_unmask_eoi_irq() Date: Wed, 30 Nov 2022 15:48:18 +0100 [thread overview] Message-ID: <87edtkts5p.ffs@tglx> (raw) In-Reply-To: <20221126234134.32660-2-samuel@sholland.org> 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
next prev parent reply other threads:[~2022-11-30 14:48 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 [this message] 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
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=87edtkts5p.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.