public inbox for linuxppc-dev@ozlabs.org 
 help / color / mirror / Atom feed

From: Thomas Gleixner <tglx@linutronix•de>
To: Eliav Farber <farbere@amazon•com>,
	linux@armlinux•org.uk, catalin.marinas@arm•com, will@kernel•org,
	mpe@ellerman•id.au, npiggin@gmail•com,
	christophe.leroy@csgroup•eu, naveen@kernel•org,
	maddy@linux•ibm.com, paul.walmsley@sifive•com,
	palmer@dabbelt•com, aou@eecs•berkeley.edu,
	akpm@linux-foundation•org, bhe@redhat•com, farbere@amazon•com,
	hbathini@linux•ibm.com, adityag@linux•ibm.com,
	songshuaishuai@tinylab•org, takakura@valinux•co.jp,
	linux-arm-kernel@lists•infradead.org,
	linux-kernel@vger•kernel.org, linuxppc-dev@lists•ozlabs.org,
	linux-riscv@lists•infradead.org
Cc: jonnyc@amazon•com
Subject: Re: [PATCH v2] arm64: kexec: Check if IRQ is already masked before masking
Date: Thu, 28 Nov 2024 11:39:11 +0100	[thread overview]
Message-ID: <87o71zy75c.ffs@tglx> (raw)
In-Reply-To: <20241127152236.26122-1-farbere@amazon.com>

On Wed, Nov 27 2024 at 15:22, Eliav Farber wrote:
> diff --git a/arch/arm/kernel/machine_kexec.c b/arch/arm/kernel/machine_kexec.c
> index 80ceb5bd2680..54d0bd1bd449 100644
> --- a/arch/arm/kernel/machine_kexec.c
> +++ b/arch/arm/kernel/machine_kexec.c
> @@ -142,11 +142,8 @@ static void machine_kexec_mask_interrupts(void)
>  		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
>  			chip->irq_eoi(&desc->irq_data);
>  
> -		if (chip->irq_mask)
> -			chip->irq_mask(&desc->irq_data);
> -
> -		if (chip->irq_disable && !irqd_irq_disabled(&desc->irq_data))
> -			chip->irq_disable(&desc->irq_data);
> +		irq_set_status_flags(i, IRQ_DISABLE_UNLAZY);
> +		irq_disable(desc);

This is just wrong. If the interrupt was torn down, then its state is
deactivated and it was masked already. So the EOI handling and the
mask/disable dance are neither required nor make sense.

So this whole thing should be:

		chip = irq_desc_get_chip(desc);
-		if (!chip)
+		if (!chip || !irqd_is_started(&desc->irq_data))
                	continue;

But what's worse is that we have 4 almost identical variants of the same
code.

So instead of exposing core functionality and "fixing" up four variants,
can we please have a consolidated version of this function in the core
code:
                struct irq_chip *chip;
                int check_eoi = 1;

		chip = irq_desc_get_chip(desc);
		if (!chip || !irqd_is_started(&desc->irq_data))
                	continue;

                if (IS_ENABLED(CONFIG_.....)) {
                        /*
                         * Add a sensible comment which explains this.
                         */
                	check_eoi = irq_set_irqchip_state(....);
                }

		if (check_eoi && ....)
                	chip->irq_eoi(&desc->irq_data);

		irq_shutdown(desc);

No?

Thanks,

        tglx

next prev parent reply	other threads:[~2024-11-28 10:39 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-11-27 15:22 [PATCH v2] arm64: kexec: Check if IRQ is already masked before masking Eliav Farber
2024-11-28 10:39 ` Thomas Gleixner [this message]
2024-11-28 10:43 ` Thomas Gleixner
  -- strict thread matches above, loose matches on Subject: below --
2024-11-28 20:07 Farber, Eliav

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=87o71zy75c.ffs@tglx \
    --to=tglx@linutronix$(echo .)de \
    --cc=adityag@linux$(echo .)ibm.com \
    --cc=akpm@linux-foundation$(echo .)org \
    --cc=aou@eecs$(echo .)berkeley.edu \
    --cc=bhe@redhat$(echo .)com \
    --cc=catalin.marinas@arm$(echo .)com \
    --cc=christophe.leroy@csgroup$(echo .)eu \
    --cc=farbere@amazon$(echo .)com \
    --cc=hbathini@linux$(echo .)ibm.com \
    --cc=jonnyc@amazon$(echo .)com \
    --cc=linux-arm-kernel@lists$(echo .)infradead.org \
    --cc=linux-kernel@vger$(echo .)kernel.org \
    --cc=linux-riscv@lists$(echo .)infradead.org \
    --cc=linux@armlinux$(echo .)org.uk \
    --cc=linuxppc-dev@lists$(echo .)ozlabs.org \
    --cc=maddy@linux$(echo .)ibm.com \
    --cc=mpe@ellerman$(echo .)id.au \
    --cc=naveen@kernel$(echo .)org \
    --cc=npiggin@gmail$(echo .)com \
    --cc=palmer@dabbelt$(echo .)com \
    --cc=paul.walmsley@sifive$(echo .)com \
    --cc=songshuaishuai@tinylab$(echo .)org \
    --cc=takakura@valinux$(echo .)co.jp \
    --cc=will@kernel$(echo .)org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox