Received-SPF: pass (google.com: domain of linux-kernel+bounces-207381-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: takakura@valinux.co.jp
To: alex@ghiti.fr,
	songshuaishuai@tinylab.org,
	paul.walmsley@sifive.com
Cc: palmer@dabbelt.com,
	aou@eecs.berkeley.edu,
	guoren@kernel.org,
	xianting.tian@linux.alibaba.com,
	takahiro.akashi@linaro.org,
	takakura@valinux.co.jp,
	linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [-fixes] riscv: kexec: Avoid deadlock in kexec crash path
Date: Sun,  9 Jun 2024 23:18:02 +0900
Message-Id: <20240609141802.6976-1-takakura@valinux.co.jp>
In-Reply-To: <545a7f3e-436e-4fd7-a45f-4f800bd58b20@ghiti.fr>
References: <545a7f3e-436e-4fd7-a45f-4f800bd58b20@ghiti.fr>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

Hi Alex, Song,

On Fri, 24 May 2024, Alexandre Ghiti wrote:
>Hi Song, Ryo,
>
>On 06/05/2024 07:10, takakura@valinux.co.jp wrote:
>> Hi Song and Paul!
>>
>>>> To avoid the deadlock, this patch directly EOI the irq regardless of
>>>> the active status of irqchip.
>>> Taking a quick look at the other architectures, looks like no one else is
>>> doing this.  Is this addressing a RISC-V-only problem?
>>>
>>>> diff --git a/arch/riscv/kernel/machine_kexec.c b/arch/riscv/kernel/machine_kexec.c
>>>> index f6c7135b00d7..d7ddf4d2b243 100644
>>>> --- a/arch/riscv/kernel/machine_kexec.c
>>>> +++ b/arch/riscv/kernel/machine_kexec.c
>>>> @@ -149,20 +149,12 @@ static void machine_kexec_mask_interrupts(void)
>>>>   
>>>>   	for_each_irq_desc(i, desc) {
>>>>   		struct irq_chip *chip;
>>>> -		int ret;
>>>>   
>>>>   		chip = irq_desc_get_chip(desc);
>>>>   		if (!chip)
>>>>   			continue;
>>>>   
>>>> -		/*
>>>> -		 * First try to remove the active state. If this
>>>> -		 * fails, try to EOI the interrupt.
>>>> -		 */
>>>> -		ret = irq_set_irqchip_state(i, IRQCHIP_STATE_ACTIVE, false);
>>>> -
>>>> -		if (ret && irqd_irq_inprogress(&desc->irq_data) &&
>>>> -		    chip->irq_eoi)
>>>> +		if (chip->irq_eoi && irqd_irq_inprogress(&desc->irq_data))
>>>>   			chip->irq_eoi(&desc->irq_data);
>> I think this deadlock is relevant to riscv and arm64 as they both
>> acquire irqdesc spinlock by calling irq_set_irqchip_state() during their
>> machine_kexec_mask_interrupts().
>>
>> However, I think calling irq_set_irqchip_state() during
>> machine_kexec_mask_interrupts() is arm64 specific way of handling EOI
>> which is not necessary for riscv.
>> For arm64, its interrupt controller(gic) seems to have two ways of EOIing
>> an interrupt depending on the mode which gic is configured. One of them
>> treats EOI as two step procedure, priority drop and deactivation. I think
>> irq_set_irqchip_state() is there to handle the deactivation part of
>> the procedure.
>> For riscv, EOI only requires irq_eoi handler to complete EOI and I think
>> keeping irq_set_irqchip_state() will only leave this possible deadlock
>> without any use.
>> So I think it's best we simply remove irq_set_irqchip_state() as Song did.
>
>
>I think this ^ is relevant and should be added to the commit log. @Song 
>can you respin another version with the updated commit log? @Ryo can you 
>add your Reviewed-by when it's done?

Sure!

>This fix has been lagging behind for quite some time, it would be nice 
>to merge this in 6.10 and backport to stable.

Sincerely,

Ryo Takakura

>Thanks,
>
>Alex