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

[Gowans, James]

On Tue, 2023-06-06 at 18:05 +0100, Marc Zyngier wrote:
> 
> On Mon, 05 Jun 2023 16:57:23 +0100,
> James Gowans <jgowans@xxxxxxxxxx> 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