| Message ID | 20220608105057.2607812-1-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/irq-imx-irqsteer: Get/put PM runtime in ->irq_unmask()/irq_mask() | expand |
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 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 = {
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 = { > >
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. Regards, Lucas > [ 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 = { > > > > >
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? Thanks, M. From 1426cadd87717f1d876c7563f2a29b00283a847e Mon Sep 17 00:00:00 2001 From: Marc Zyngier <maz@kernel.org> Date: Wed, 8 Jun 2022 14:45:35 +0100 Subject: [PATCH] genirq: PM: Use runtime PM for chained interrupts When requesting an interrupt, we correctly call into the runtime PM framework to guarantee that the underlying interrupt controller is up and running. However, we fail to do so for chained interrupt controllers, as the mux interrupt is not requested along the same path. Augment __irq_do_set_handler() to call into the runtime PM code in this case, making sure the PM flow is the same for all interrupts. Reported-by: Lucas Stach <l.stach@pengutronix.de> Signed-off-by: Marc Zyngier <maz@kernel.org> Link: https://lore.kernel.org/r/26973cddee5f527ea17184c0f3fccb70bc8969a0.camel@pengutronix.de --- kernel/irq/chip.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index e6b8e564b37f..886789dcee43 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -1006,8 +1006,10 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle, if (desc->irq_data.chip != &no_irq_chip) mask_ack_irq(desc); irq_state_set_disabled(desc); - if (is_chained) + if (is_chained) { desc->action = NULL; + WARN_ON(irq_chip_pm_put(irq_desc_get_irq_data(desc))); + } desc->depth = 1; } desc->handle_irq = handle; @@ -1033,6 +1035,7 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle, irq_settings_set_norequest(desc); irq_settings_set_nothread(desc); desc->action = &chained_action; + WARN_ON(irq_chip_pm_get(irq_desc_get_irq_data(desc))); irq_activate_and_startup(desc, IRQ_RESEND); } }
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. Regards, Liu Ying > > Thanks, > > M. > > From 1426cadd87717f1d876c7563f2a29b00283a847e Mon Sep 17 00:00:00 > 2001 > From: Marc Zyngier <maz@kernel.org> > Date: Wed, 8 Jun 2022 14:45:35 +0100 > Subject: [PATCH] genirq: PM: Use runtime PM for chained interrupts > > When requesting an interrupt, we correctly call into the runtime > PM framework to guarantee that the underlying interrupt controller > is up and running. > > However, we fail to do so for chained interrupt controllers, as > the mux interrupt is not requested along the same path. > > Augment __irq_do_set_handler() to call into the runtime PM code > in this case, making sure the PM flow is the same for all interrupts. > > Reported-by: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Marc Zyngier <maz@kernel.org> > Link: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F26973cddee5f527ea17184c0f3fccb70bc8969a0.camel%40pengutronix.de&data=05%7C01%7Cvictor.liu%40nxp.com%7C2ce8f74775494461d51808da49566940%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637902932736234542%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jqe8nsRFWQpVjRl7C6Bgz%2BlwtkaMv%2BKq%2Fui5F1jwf4c%3D&reserved=0 > --- > kernel/irq/chip.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index e6b8e564b37f..886789dcee43 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -1006,8 +1006,10 @@ __irq_do_set_handler(struct irq_desc *desc, > irq_flow_handler_t handle, > if (desc->irq_data.chip != &no_irq_chip) > mask_ack_irq(desc); > irq_state_set_disabled(desc); > - if (is_chained) > + if (is_chained) { > desc->action = NULL; > + WARN_ON(irq_chip_pm_put(irq_desc_get_irq_data(d > esc))); > + } > desc->depth = 1; > } > desc->handle_irq = handle; > @@ -1033,6 +1035,7 @@ __irq_do_set_handler(struct irq_desc *desc, > irq_flow_handler_t handle, > irq_settings_set_norequest(desc); > irq_settings_set_nothread(desc); > desc->action = &chained_action; > + WARN_ON(irq_chip_pm_get(irq_desc_get_irq_data(desc))); > irq_activate_and_startup(desc, IRQ_RESEND); > } > } > -- > 2.34.1 > >
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.
On Thu, 2022-06-09 at 12:25 +0100, Marc Zyngier wrote: > 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? Yes, you can. Regards, Liu Ying
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 = {
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. 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(+)