Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 09 Jun 2022 12:25:16 +0100
Message-ID: <87czfi3wpv.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Liu Ying <victor.liu@nxp.com>
Cc:     Lucas Stach <l.stach@pengutronix.de>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>
Subject: Re: [PATCH] irqchip/irq-imx-irqsteer: Get/put PM runtime in ->irq_unmask()/irq_mask()
In-Reply-To: <07e01555c4722daed2dc13871c42ef7764a157ae.camel@nxp.com>
References: <20220608105057.2607812-1-victor.liu@nxp.com>
        <2d79719b8670a3693b210af5ab45716dba23999a.camel@pengutronix.de>
        <17d3adc7d7d329cab65b54ce71db05bc070872d1.camel@nxp.com>
        <26973cddee5f527ea17184c0f3fccb70bc8969a0.camel@pengutronix.de>
        <87k09r45ww.wl-maz@kernel.org>
        <07e01555c4722daed2dc13871c42ef7764a157ae.camel@nxp.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 09 Jun 2022 02:41:55 +0100,
Liu Ying <victor.liu@nxp.com> wrote:
> 
> On Wed, 2022-06-08 at 14:54 +0100, Marc Zyngier wrote:
> > On Wed, 08 Jun 2022 13:02:46 +0100,
> > Lucas Stach <l.stach@pengutronix.de> wrote:
> > > 
> > > Am Mittwoch, dem 08.06.2022 um 19:29 +0800 schrieb Liu Ying:
> > > > On Wed, 2022-06-08 at 12:56 +0200, Lucas Stach wrote:
> > > > > Am Mittwoch, dem 08.06.2022 um 18:50 +0800 schrieb Liu Ying:
> > > > > > Now that runtime PM support was added in this driver, we have
> > > > > > to enable power before accessing irqchip registers.  And,
> > > > > > after
> > > > > > the access is done, we should disable power.  This patch
> > > > > > calls
> > > > > > pm_runtime_get_sync() in ->irq_unmask() and pm_runtime_put()
> > > > > > in
> > > > > > ->irq_mask() to make sure power is managed for the register
> > > > > > access.
> > > > > > 
> > > > > 
> > > > > Can you tell me in which case this is necessary? IIRC the IRQ
> > > > > core
> > > > 
> > > > With the i.MX8qxp DPU driver[1], I see below synchronous external
> > > > abort:
> > > > 
> > > > [    1.207270] Internal error: synchronous external abort:
> > > > 96000210
> > > > [#1] PREEMPT SMP
> > > > [    1.207287] Modules linked in:
> > > > [    1.207299] CPU: 1 PID: 64 Comm: kworker/u8:2 Not tainted
> > > > 5.18.0-
> > > > rc6-next-20220509-00053-gf01f74ee1c18 #272
> > > > [    1.207311] Hardware name: Freescale i.MX8QXP MEK (DT)
> > > > [    1.207319] Workqueue: events_unbound deferred_probe_work_func
> > > > [    1.207339] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT
> > > > -SSBS
> > > > BTYPE=--)
> > > > [    1.207349] pc : imx_irqsteer_irq_unmask+0x48/0x80
> > > > [    1.207360] lr : imx_irqsteer_irq_unmask+0x38/0x80
> > > > [    1.207368] sp : ffff80000a88b900
> > > > [    1.207372] x29: ffff80000a88b900 x28: ffff8000080fed90 x27:
> > > > ffff8000080fefe0
> > > > [    1.207388] x26: ffff8000080fef40 x25: ffff0008012538d4 x24:
> > > > ffff8000092fe388
> > > > [    1.207407] x23: 0000000000000001 x22: ffff0008013295b4 x21:
> > > > ffff000801329580
> > > > [    1.207425] x20: ffff0008003faa60 x19: 000000000000000e x18:
> > > > 0000000000000000
> > > > [    1.207443] x17: 0000000000000003 x16: 0000000000000162 x15:
> > > > 0000000000000001
> > > > [    1.207459] x14: 0000000000000002 x13: 0000000000000018 x12:
> > > > 0000000000000040
> > > > [    1.207477] x11: ffff000800682480 x10: ffff000800682482 x9 :
> > > > ffff80000a072678
> > > > [    1.207495] x8 : ffff0008006a64a8 x7 : 0000000000000000 x6 :
> > > > ffff0008006a6608
> > > > [    1.207513] x5 : ffff800009070a18 x4 : 0000000000000000 x3 :
> > > > ffff80000b240000
> > > > [    1.207529] x2 : ffff80000b240038 x1 : 00000000000000c0 x0 :
> > > > 00000000000000c0
> > > > [    1.207549] Call trace:
> > > > [    1.207553]  imx_irqsteer_irq_unmask+0x48/0x80
> > > > [    1.207562]  irq_enable+0x40/0x8c
> > > > [    1.207575]  __irq_startup+0x78/0xa4
> > > > [    1.207588]  irq_startup+0x78/0x16c
> > > > [    1.207601]  irq_activate_and_startup+0x38/0x70
> > > > [    1.207612]  __irq_do_set_handler+0xcc/0x1e0
> > > > [    1.207626]  irq_set_chained_handler_and_data+0x58/0xa0
> > > 
> > > Ooh, I think this is the problem. The IRQ is not requested in the
> > > usual
> > > way when a chained handler is added, so this might bypass the
> > > runtime
> > > PM handling normally done in the IRQ core. In that case this is a
> > > core
> > > issue and should not be worked around in the driver, but the core
> > > should take the RPM reference for the chained handler, just like it
> > > does for normal IRQs.
> > 
> > Well spotted. Could you please give the hack below (compile-tested
> > only) a go?
> 
> I don't see the splat after your patch is applied.

Can I take this as a formal "Tested-by:" tag?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.