Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Wed, 07 Jun 2023 14:13:02 +0100
From:   Marc Zyngier <maz@kernel.org>
To:     "Gowans, James" <jgowans@amazon.com>
Cc:     tglx@linutronix.de, "Raslan, KarimAllah" <karahmed@amazon.com>,
        liaochang1@huawei.com, linux-kernel@vger.kernel.org,
        zouyipeng@huawei.com, chris.zjh@huawei.com
Subject: Re: [PATCH v3 2/2] genirq: fasteoi supports resend on concurrent
 invoke
In-Reply-To: <9c831b0b0b0f067f48841903b5173e3c97f84292.camel@amazon.com>
References: <20230605155723.2628097-1-jgowans@amazon.com>
 <20230605155723.2628097-2-jgowans@amazon.com> <87jzwgo715.wl-maz@kernel.org>
 <9c831b0b0b0f067f48841903b5173e3c97f84292.camel@amazon.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <2fc0a230d79414ab2c2027ec02a022e3@kernel.org>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

I'd prefer that. It is in general better to decouple driver stuff from
core code.

>> 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...)

Eventually, someone will bite the bullet and use BIT() everywhere.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...