Messages in this thread | | | Date | Thu, 13 Aug 2020 11:08:54 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH 1/1] arm64: kexec: no need to do irq_chip->irq_mask if it already masked |
| |
On 2020-08-13 07:03, Jason Liu wrote: >> -----Original Message----- >> From: Marc Zyngier <maz@kernel.org> >> Sent: Thursday, August 6, 2020 8:26 PM >> To: Jason Liu <jason.hui.liu@nxp.com> >> Cc: Sudeep Holla <sudeep.holla@arm.com>; catalin.marinas@arm.com; >> will@kernel.org; sashal@kernel.org; linux-kernel@vger.kernel.org; >> linux-arm-kernel@lists.infradead.org >> Subject: Re: [PATCH 1/1] arm64: kexec: no need to do >> irq_chip->irq_mask if it >> already masked >> >> On 2020-08-06 11:05, Jason Liu wrote: >> >> -----Original Message----- >> >> From: Marc Zyngier <maz@kernel.org> >> >> [...] >> >> >> > No, this patch is not papering over a much deeper issue in the driver. >> >> > This is just to make things better for the ARM64 kexec. >> >> >> >> Yes, I'm sure it is... However: >> >> >> >> request_irq() >> >> <goes into suspend, panic somewhere after having turned the irqchip >> >> clock off> if (chip->irq_mask && !irqd_irq_masked(&desc->irq_data)) >> >> <explodes, as the interrupt isn't masked> >> >> >> >> This is because the PM in the irqsteer driver is completely busted: >> >> request_irq() should get a reference on the driver to prevent it from >> >> being suspended. Since you don't implement it correctly, this doesn't >> >> happen and your "improvement" doesn't help at all. >> > >> > The request_irq will get a reference to prevent the irqchip from being >> > suspended due to it call irq_chip_pm_get(). I am pretty sure we have >> > implemented correctly and that is also the common Linux code. >> >> Then it seems you cannot read your own driver. At no point do you set >> the >> parent_device that would give you a fighting chance to get the device >> clocked >> and powered on. How does it work? Magic? >> >> > In order to save power and let the irqchip enter into runtime SUSPEND >> > mode, the driver will call free_irq() When it was not used(idle). Then >> > free_irq() will decrease the reference and let the irqchip enter >> > suspend state. >> >> The reference count on *what*? There is nothing to take a reference >> on. So yes, >> you understand how the core kernel works. But you don't seem to notice >> that >> there is no link between the irq and the device that implements the >> controller. > > See the code, it will call irq_chip_pm_put(&desc->irq_data) > > /* > * Internal function to unregister an irqaction - used to free > * regular and special interrupts that are part of the architecture. > */ > static struct irqaction *__free_irq(struct irq_desc *desc, void > *dev_id) > { > .. > irq_chip_pm_put(&desc->irq_data); > module_put(desc->owner); > kfree(action->secondary); > return action; > }
This is getting tiresome. You want to play the code-quote game?
int irq_chip_pm_put(struct irq_data *data) { int retval = 0;
if (IS_ENABLED(CONFIG_PM) && data->chip->parent_device) retval = pm_runtime_put(data->chip->parent_device);
return (retval < 0) ? retval : 0; }
What is parent_device set to in your driver? Oh wait... Nothing. So what does the code you quoted do? Not much.
>> > So, when the irqchip entered suspend, which means there is no user for >> > the irqchip and all the irqs were DISABLED + MASKED. >> > Due to the runtimePM support for the irqchip, when kexec runs, it will >> > sometimes meet the situation that the irqchip is suspend due to no >> > users for it. So from either the performance(time cost) or coding >> > logic, the machine_kexec_mask_interrupts() should not do double mask >> > for the irqs which already masked. >> >> I strongly suggest you start by fixing the damn driver first. > > Our driver does not have issue at all. What to fix?
[I've censored myself here...]
> >> >> In the meantime, NAK to this patch. > > Anyway, it seems don't really understand this issue and you just > simply reject one reasonable fix. Sounds not good at all.
I reject it because your approach is flawed, and that you are papering over a glaring bug in your driver that you are refusing to fix.
Maybe the right thing to do is to remove this driver from the code base altogether. I will prepare a patch to that effect.
M. -- Jazz is not dead. It just smells funny...
| |