From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B85CFC433E1 for ; Thu, 13 Aug 2020 10:10:21 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 856F420781 for ; Thu, 13 Aug 2020 10:10:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="dmIoLWwS"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="OJ+zI1X3" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 856F420781 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:References:In-Reply-To:Subject:To:From: Date:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=btk/U5hff33PTRrVi9SPYx3aUMrPri77c6Y63QiR1Mo=; b=dmIoLWwS4ggziBUXCrj9MLird /O9z90CWWCL2q271tQhRCWHipzExlgBCF9tOveIf8ADND1k6T2bH807Kgkuleum4A9mJmYbIC04CY 6H0ELDOFOjjDlVJgzHbcOjDY3opjoE+nAMTY9xIMz66WnHbIo052xofCgNx2jLKcj2tDC2uZap9Ep n9O87E0NHuhkkpRYhfVrgLrXFy0FTtAkeZE/8DMaG0I5phjTsnvfbP+b6uJXZEUyRTXjh0jH3QlEu +MALwEXR/hGM/eBjw1sxPtrBkNiPLZvyH/Ne3Faycc3TcK98Az8Jmr0EejAU01Pbl01aEr5lC7MxO 5sugeBEJQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k6AAO-0001xQ-22; Thu, 13 Aug 2020 10:09:00 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k6AAL-0001wm-FR for linux-arm-kernel@lists.infradead.org; Thu, 13 Aug 2020 10:08:58 +0000 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id EE2CC20781; Thu, 13 Aug 2020 10:08:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1597313336; bh=nsoK2lVbOKDQ4GSO14CBSj3EPe+//8yAv/WHujwP4Jk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=OJ+zI1X3X0dfUgs29Jm/QGJTVyzT4CufV1NIRx15ih+0h3wzlVKKRQf9ZZBZcaRH0 UI9UPmVyj2SbGvZUX+Yw9S5rGvROaBqeuzGlKyuVIN6H3781Us0hGUE5sEb4RuchfK lFWI2RgPgBwVzeLKfEnppRfA34/emwBv7Xts62Js= Received: from disco-boy.misterjones.org ([51.254.78.96] helo=www.loen.fr) by disco-boy.misterjones.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1k6AAI-001ko4-CX; Thu, 13 Aug 2020 11:08:54 +0100 MIME-Version: 1.0 Date: Thu, 13 Aug 2020 11:08:54 +0100 From: Marc Zyngier To: Jason Liu Subject: Re: [PATCH 1/1] arm64: kexec: no need to do irq_chip->irq_mask if it already masked In-Reply-To: References: <20200804085657.10776-1-jason.hui.liu@nxp.com> <20200804113850.GB15199@bogus> <3c63ae0cc3a7b5bad5d4638a9870340e@kernel.org> <1e4496c263e486be3438f2797630164a@kernel.org> User-Agent: Roundcube Webmail/1.4.5 Message-ID: <01b7091e69d2b4e51f280b05f6218a65@kernel.org> X-Sender: maz@kernel.org X-SA-Exim-Connect-IP: 51.254.78.96 X-SA-Exim-Rcpt-To: jason.hui.liu@nxp.com, 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 X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200813_060857_640107_E896B29F X-CRM114-Status: GOOD ( 36.51 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: sashal@kernel.org, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, Sudeep Holla , will@kernel.org, linux-arm-kernel@lists.infradead.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2020-08-13 07:03, Jason Liu wrote: >> -----Original Message----- >> From: Marc Zyngier >> Sent: Thursday, August 6, 2020 8:26 PM >> To: Jason Liu >> Cc: Sudeep Holla ; 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 >> >> [...] >> >> >> > 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() >> >> > >> clock off> if (chip->irq_mask && !irqd_irq_masked(&desc->irq_data)) >> >> >> >> >> >> 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... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel