Re: [PATCH] irqchip/irq-imx-irqsteer: Get/put PM runtime in ->irq_unmask()/irq_mask()

[Liu Ying <victor.liu@nxp.com>]

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
[    1.207642]  dpu_core_probe+0x368/0xbd4
[    1.207653]  platform_probe+0x68/0xe0
[    1.207667]  really_probe.part.0+0x9c/0x28c
[    1.207678]  __driver_probe_device+0x98/0x144
[    1.207692]  driver_probe_device+0xac/0x140
[    1.207704]  __device_attach_driver+0xb4/0x120
[    1.207716]  bus_for_each_drv+0x78/0xd0
[    1.207727]  __device_attach+0xdc/0x184
[    1.207739]  device_initial_probe+0x14/0x20
[    1.207749]  bus_probe_device+0x9c/0xa4
[    1.207762]  deferred_probe_work_func+0x88/0xc0
[    1.207774]  process_one_work+0x1d0/0x320
[    1.207788]  worker_thread+0x2c8/0x444
[    1.207799]  kthread+0x10c/0x110
[    1.207812]  ret_from_fork+0x10/0x20
[    1.207829] Code: f94002a3 531e7662 aa0003e1 8b22c062 (b9400040)
[    1.207839] ---[ end trace 0000000000000000 ]---

DPU DT node references an imx-irqsteer DT node as the interrupt parent.
The DPU driver adds an irq domain by itself.

[1] 
https://patchwork.kernel.org/project/dri-devel/patch/20220407091156.1211923-6-victor.liu@nxp.com/

Regards,
Liu Ying

> already keeps the chip runtime resumed as soon as a IRQ is requested,
> so why would it be in runtime suspend at mask/unmask?
> 
> Regards,
> Lucas
> 
> > Fixes: 4730d2233311 ("irqchip/imx-irqsteer: Add runtime PM
> > support")
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Cc: NXP Linux Team <linux-imx@nxp.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> >  drivers/irqchip/irq-imx-irqsteer.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c
> > b/drivers/irqchip/irq-imx-irqsteer.c
> > index 96230a04ec23..a5eabe71e8ab 100644
> > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > @@ -45,11 +45,14 @@ static int imx_irqsteer_get_reg_index(struct
> > irqsteer_data *data,
> >  
> >  static void imx_irqsteer_irq_unmask(struct irq_data *d)
> >  {
> > +	struct device *dev = d->domain->dev;
> >  	struct irqsteer_data *data = d->chip_data;
> >  	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
> >  	unsigned long flags;
> >  	u32 val;
> >  
> > +	pm_runtime_get_sync(dev);
> > +
> >  	raw_spin_lock_irqsave(&data->lock, flags);
> >  	val = readl_relaxed(data->regs + CHANMASK(idx, data->reg_num));
> >  	val |= BIT(d->hwirq % 32);
> > @@ -59,6 +62,7 @@ static void imx_irqsteer_irq_unmask(struct
> > irq_data *d)
> >  
> >  static void imx_irqsteer_irq_mask(struct irq_data *d)
> >  {
> > +	struct device *dev = d->domain->dev;
> >  	struct irqsteer_data *data = d->chip_data;
> >  	int idx = imx_irqsteer_get_reg_index(data, d->hwirq);
> >  	unsigned long flags;
> > @@ -69,6 +73,8 @@ static void imx_irqsteer_irq_mask(struct irq_data
> > *d)
> >  	val &= ~BIT(d->hwirq % 32);
> >  	writel_relaxed(val, data->regs + CHANMASK(idx, data->reg_num));
> >  	raw_spin_unlock_irqrestore(&data->lock, flags);
> > +
> > +	pm_runtime_put(dev);
> >  }
> >  
> >  static const struct irq_chip imx_irqsteer_irq_chip = {
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel