linux-kernel - Re: [PATCH v3 2/2] genirq: fasteoi supports resend on concurrent invoke

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9c831b0b0b0f067f48841903b5173e3c97f84292.camel@amazon.com>
Date:   Wed, 7 Jun 2023 12:21:10 +0000
From:   "Gowans, James" <jgowans@...zon.com>
To:     "maz@...nel.org" <maz@...nel.org>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "Raslan, KarimAllah" <karahmed@...zon.com>,
        "liaochang1@...wei.com" <liaochang1@...wei.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "zouyipeng@...wei.com" <zouyipeng@...wei.com>,
        "chris.zjh@...wei.com" <chris.zjh@...wei.com>
Subject: Re: [PATCH v3 2/2] genirq: fasteoi supports resend on concurrent
 invoke

On Tue, 2023-06-06 at 18:05 +0100, Marc Zyngier wrote:
> 
> On Mon, 05 Jun 2023 16:57:23 +0100,
> James Gowans <jgowans@...zon.com> wrote:
> > ... and enable that functionality for GIC-v3 only.
> 
> nit: drop the multi-line subject.

Would you prefer two commits - one to introduce the functionality and one
to enable it for GIC-v3?

> diff --git a/include/linux/irq.h b/include/linux/irq.h
> > index b1b28affb32a..b76cc90faebd 100644
> > --- a/include/linux/irq.h
> > +++ b/include/linux/irq.h
> > @@ -223,6 +223,8 @@ struct irq_data {
> >   *                             irq_chip::irq_set_affinity() when deactivated.
> >   * IRQD_IRQ_ENABLED_ON_SUSPEND       - Interrupt is enabled on suspend by irq pm if
> >   *                             irqchip have flag IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND set.
> > + * RQD_RESEND_WHEN_IN_PROGRESS       - Interrupt may fire when already in progress in which
> > + *                             case it must be resent at the next available opportunity.
> >   */
> >  enum {
> >       IRQD_TRIGGER_MASK               = 0xf,
> > @@ -249,6 +251,7 @@ enum {
> >       IRQD_HANDLE_ENFORCE_IRQCTX      = (1 << 28),
> >       IRQD_AFFINITY_ON_ACTIVATE       = (1 << 29),
> >       IRQD_IRQ_ENABLED_ON_SUSPEND     = (1 << 30),
> > +     IRQD_RESEND_WHEN_IN_PROGRESS    = (1 << 31),
> 
> Make this unsigned, as it otherwise has the potential to sign-extend
> and lead to "fun to debug" issues.

Ack, doing this change:

@@ -251,7 +251,7 @@ enum {
        IRQD_HANDLE_ENFORCE_IRQCTX      = (1 << 28),
        IRQD_AFFINITY_ON_ACTIVATE       = (1 << 29),
        IRQD_IRQ_ENABLED_ON_SUSPEND     = (1 << 30),
-       IRQD_RESEND_WHEN_IN_PROGRESS    = (1 << 31),
+       IRQD_RESEND_WHEN_IN_PROGRESS    = (1U << 31),
 };
 
(looks a bit odd having only this one unsigned though...)

Also fixing above typo which Randy pointed out.

> +++ b/kernel/irq/chip.c
> > @@ -692,8 +692,16 @@ void handle_fasteoi_irq(struct irq_desc *desc)
> > 
> >       raw_spin_lock(&desc->lock);
> > 
> > -     if (!irq_may_run(desc))
> > +     /*
> > +      * When an affinity change races with IRQ delivery, the next interrupt
> 
> The race is with the IRQ *handling*.

Reworded.

> 
> If you respin this series shortly (*with* a cover letter, please),
> I'll queue it so that it can simmer in -next for a bit.

Will do - just keen to see if you prefer the commit to be split.

JG