Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Jason Liu <jason.hui.liu@nxp.com>
To:     Marc Zyngier <maz@kernel.org>
CC:     Sudeep Holla <sudeep.holla@arm.com>,
        "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "will@kernel.org" <will@kernel.org>,
        "sashal@kernel.org" <sashal@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.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
Thread-Topic: [PATCH 1/1] arm64: kexec: no need to do irq_chip->irq_mask if it
 already masked
Thread-Index: AQHWaj3ZMAA/XWH8SkmG3qCWNnZEoaknx/iAgAALMQCAAQxYoIAATcYAgAGqTLCAAC1PgIAKk02g
Date:   Thu, 13 Aug 2020 06:03:13 +0000
Message-ID: <VI1PR0402MB3824B4D3BF37335FE48A12F1AE430@VI1PR0402MB3824.eurprd04.prod.outlook.com>
References: <20200804085657.10776-1-jason.hui.liu@nxp.com>
 <b4444c737f9d6fd8fd99dbb809d35a0f@kernel.org> <20200804113850.GB15199@bogus>
 <VI1PR0402MB38243230C51FDB1C3F490853AE4B0@VI1PR0402MB3824.eurprd04.prod.outlook.com>
 <3c63ae0cc3a7b5bad5d4638a9870340e@kernel.org>
 <VI1PR0402MB38249815EA37338953E8AAC1AE480@VI1PR0402MB3824.eurprd04.prod.outlook.com>
 <1e4496c263e486be3438f2797630164a@kernel.org>
In-Reply-To: <1e4496c263e486be3438f2797630164a@kernel.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f928ba6e-84af-46b4-f232-08d83f4e9097
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Aug 2020 06:03:13.7671
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: QBwELqBt7Oarl9XgtALNqrQ46Iyc2Q4rELL2QGEGUJwow35Dd/L07Ne2K3SPIkoMz48WUvYv87maPSjRmTB9fQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB3598
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----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 i=
f it
> already masked
>=20
> On 2020-08-06 11:05, Jason Liu wrote:
> >> -----Original Message-----
> >> From: Marc Zyngier <maz@kernel.org>
>=20
> [...]
>=20
> >> > No, this patch is not papering over a much deeper issue in the drive=
r.
> >> > 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.
>=20
> 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 clo=
cked
> and powered on. How does it work? Magic?
>=20
> > 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.
>=20
> The reference count on *what*? There is nothing to take a reference on. S=
o yes,
> you understand how the core kernel works. But you don't seem to notice th=
at
> there is no link between the irq and the device that implements the contr=
oller.

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;
}

>=20
> > 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.
>=20
> I strongly suggest you start by fixing the damn driver first.

Our driver does not have issue at all. What to fix?

>=20
> In the meantime, NAK to this patch.

Anyway, it seems don't really understand this issue and you just simply rej=
ect one reasonable fix. Sounds not good at all.

>=20
>          M.
> --
> Jazz is not dead. It just smells funny...