[v4.1.10-rt10][PATCH 0/2] PCI: dra7xx/dwc: fix "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu"

[v4.1.10-rt10][PATCH 0/2] PCI: dra7xx/dwc: fix "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu"

[Grygorii Strashko]

Now in kernel below code pattern is used by many drivers:
static irqreturn_t driver_xx_hw_irq_handler(int irq, void *arg)
{
	<read IRQ status register>
  	<perform HW specific operations>

	for (<each set bit in IRQ status register>) {
		<get Linux IRQ number>
		generic_handle_irq(<Linux IRQ number>);
		|- handle_simple_irq()
		|-or- handle_level_irq()
		|-or- handle_edge_irq()
		   |-handle_irq_event()
		     |-handle_irq_event_percpu()
===
"WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
 irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts"
===
	}
}

On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
will be forced threaded (by default) and, as result, generic_handle_irq()
will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).

Instead of spreading this W/A directly in many drivers this series
introduces -RT specific version of generic_handle_irq() API -
generic_handle_irq_rt_wa(). This new generic_handle_irq_rt_wa() just calls
generic_handle_irq() surrounded by raw_spin_lock_irqsave/irqrestore().
If -RT is disabled It will fallback to generic_handle_irq().

And generic_handle_irq_rt_wa() is used then to W/A similar issues
in pcie-designware.c and pcie-dra7xx.c.

Grygorii Strashko (2):
  genirq: introduce new generic_handle_irq_rt_wa() api
  PCI: dra7xx: fix "WARNING: CPU: 1 PID: 82 at  kernel/irq/handle.c:150
    handle_irq_event_percpu"

 drivers/pci/host/pci-dra7xx.c      |  3 ++-
 drivers/pci/host/pcie-designware.c |  2 +-
 include/linux/irqdesc.h            |  5 +++++
 kernel/irq/irqdesc.c               | 19 +++++++++++++++++++
 4 files changed, 27 insertions(+), 2 deletions(-)

-- 
2.6.2

[v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

[Grygorii Strashko]

Now in kernel below code pattern is used by many drivers:
static irqreturn_t driver_xx_hw_irq_handler(int irq, void *arg)
{
	<read IRQ status register>
  	<perform HW specific operations>

	for (<each set bit in IRQ status register>) {
		<get Linux IRQ number>
		generic_handle_irq(<Linux IRQ number>);
		|- handle_simple_irq()
		|-or- handle_level_irq()
		|-or- handle_edge_irq()
		   |-handle_irq_event()
		     |-handle_irq_event_percpu()
===
"WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
 irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts"
===
	}
}

On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
will be forced threaded (by default) and, as result, generic_handle_irq()
will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).

Instead of spreading this W/A directly in many drivers this patch
introduces -RT specific version of generic_handle_irq() API -
generic_handle_irq_rt_wa(). This new generic_handle_irq_rt_wa() just calls
generic_handle_irq() surrounded by raw_spin_lock_irqsave/irqrestore().
If -RT is disabled It will fallback to generic_handle_irq().

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 include/linux/irqdesc.h |  5 +++++
 kernel/irq/irqdesc.c    | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index 9d97cd5..beea9a5 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -131,6 +131,11 @@ static inline void generic_handle_irq_desc(unsigned int irq, struct irq_desc *de
 }
 
 int generic_handle_irq(unsigned int irq);
+#ifdef CONFIG_PREEMPT_RT_FULL
+int generic_handle_irq_rt_wa(unsigned int irq);
+#else
+#define generic_handle_irq_rt_wa generic_handle_irq
+#endif
 
 #ifdef CONFIG_HANDLE_DOMAIN_IRQ
 /*
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 99793b9..b77e01b 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -353,6 +353,25 @@ int generic_handle_irq(unsigned int irq)
 }
 EXPORT_SYMBOL_GPL(generic_handle_irq);
 
+#ifdef CONFIG_PREEMPT_RT_FULL
+int generic_handle_irq_rt_wa(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	DEFINE_RAW_SPINLOCK(wa_lock);
+	unsigned long wa_lock_flags;
+
+	if (!desc)
+		return -EINVAL;
+
+	raw_spin_lock_irqsave(&wa_lock, wa_lock_flags);
+	generic_handle_irq_desc(irq, desc);
+	raw_spin_unlock_irqrestore(&wa_lock, wa_lock_flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(generic_handle_irq_rt_wa);
+#endif
+
 #ifdef CONFIG_HANDLE_DOMAIN_IRQ
 /**
  * __handle_domain_irq - Invoke the handler for a HW irq belonging to a domain
-- 
2.6.2

Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

[Thomas Gleixner]

On Mon, 2 Nov 2015, Grygorii Strashko wrote:
> Now in kernel below code pattern is used by many drivers:
> static irqreturn_t driver_xx_hw_irq_handler(int irq, void *arg)
> {
> 	<read IRQ status register>
>   	<perform HW specific operations>
> 
> 	for (<each set bit in IRQ status register>) {
> 		<get Linux IRQ number>
> 		generic_handle_irq(<Linux IRQ number>);
> 		|- handle_simple_irq()
> 		|-or- handle_level_irq()
> 		|-or- handle_edge_irq()
> 		   |-handle_irq_event()
> 		     |-handle_irq_event_percpu()
> ===
> "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
>  irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts"
> ===
> 	}
> }
> 
> On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
> will be forced threaded (by default) and, as result, generic_handle_irq()
> will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
> be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).
> 
> Instead of spreading this W/A directly in many drivers this patch
> introduces -RT specific version of generic_handle_irq() API -
> generic_handle_irq_rt_wa(). This new generic_handle_irq_rt_wa() just calls
> generic_handle_irq() surrounded by raw_spin_lock_irqsave/irqrestore().
> If -RT is disabled It will fallback to generic_handle_irq().

Why aren't you simply marking these demultiplex handlers with IRQ_NO_THREAD?

Thanks,

	tglx

Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

[Grygorii Strashko]

On 11/02/2015 09:38 PM, Thomas Gleixner wrote:
> On Mon, 2 Nov 2015, Grygorii Strashko wrote:
>> Now in kernel below code pattern is used by many drivers:
>> static irqreturn_t driver_xx_hw_irq_handler(int irq, void *arg)
>> {
>> 	<read IRQ status register>
>>    	<perform HW specific operations>
>>
>> 	for (<each set bit in IRQ status register>) {
>> 		<get Linux IRQ number>
>> 		generic_handle_irq(<Linux IRQ number>);
>> 		|- handle_simple_irq()
>> 		|-or- handle_level_irq()
>> 		|-or- handle_edge_irq()
>> 		   |-handle_irq_event()
>> 		     |-handle_irq_event_percpu()
>> ===
>> "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
>>   irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts"
>> ===
>> 	}
>> }
>>
>> On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
>> will be forced threaded (by default) and, as result, generic_handle_irq()
>> will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
>> be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).
>>
>> Instead of spreading this W/A directly in many drivers this patch
>> introduces -RT specific version of generic_handle_irq() API -
>> generic_handle_irq_rt_wa(). This new generic_handle_irq_rt_wa() just calls
>> generic_handle_irq() surrounded by raw_spin_lock_irqsave/irqrestore().
>> If -RT is disabled It will fallback to generic_handle_irq().
> 
> Why aren't you simply marking these demultiplex handlers with IRQ_NO_THREAD?
> 

In general, it's possible. But, in this case, worst scenario will look like:
dra7xx_pcie_msi_irq_handler()
-> dw_handle_msi_irq()
   [code simplified]
   -> for (i = 0; i < MAX_MSI_IRQS; i++) {
	...
	generic_handle_irq(Y(i));
	...
   }
where MAX_MSI_IRQS = 32 now, but potentially can be increased up to 256.

Thanks.
-- 
regards,
-grygorii

Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

[Thomas Gleixner]

On Tue, 3 Nov 2015, Grygorii Strashko wrote:
> On 11/02/2015 09:38 PM, Thomas Gleixner wrote:
> > 
> > Why aren't you simply marking these demultiplex handlers with IRQ_NO_THREAD?
> > 
> In general, it's possible. But, in this case, worst scenario will look like:

> dra7xx_pcie_msi_irq_handler()
> -> dw_handle_msi_irq()
>    [code simplified]
>    -> for (i = 0; i < MAX_MSI_IRQS; i++) {
> 	...
> 	generic_handle_irq(Y(i));
> 	...
>    }
> where MAX_MSI_IRQS = 32 now, but potentially can be increased up to 256.

And you really oversimplified the code above. The reality is:

    for (i = 0; i < MAX_MSI_CTRLS: i++) {
    	u32 status = read_msi_ctrl(i);

	for_each_bit(status)
		handle_irq();
    }

So sure, the worst case here is MAX_MSI_CTRLS * 32, but if all
possible 256 MSI interrupts are pending at the same time, you have
other problems than that.

In the current configuration (32 interrupts), which cannot change
because it's hardwired in silicon, this is a single status read and
assuming that only a few (most of the time it will be exactly ONE) of
those interrupts are pending at the same time is pretty much a sane
assumption. If it wouldn't be then all users of chained interrupt
handlers which usually demultiplex 32 interrupts would suffer from
that problem already.

Aside of that, you would prevent that any of these PCIe interrupts can
be utilized as a "fast" non threaded interrupt on RT. And that I would
consider a real bad limitation for no value. 

MSI has been invented to overcome the issues of wired interrupts
(demultiplexing and sharing), so I don't know why the involved
hardware designers came to the conclusion that demultiplexing MSI
interrupts in software is a sane approach. But then I really gave up
trying to understand hardware designers long ago.

The only sane way to deal with that is to actually mark those handlers
NOTRHEAD and document the limitations of your hardware, so your
customers won't trip over it. If they insist on having 32 MSI
producers on that PCIe bus and make them fire all at the same time,
then you still can provide them your "solution".

Just face it, it's a bad hardware design decision and adding a half
baken hackery which actually hurts sane use cases is not making it any
better.

Thanks,

	tglx

Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

[Sebastian Andrzej Siewior]

On 11/03/2015 08:51 PM, Thomas Gleixner wrote:
>> where MAX_MSI_IRQS = 32 now, but potentially can be increased up to 256.
> 
> And you really oversimplified the code above. The reality is:
> 
>     for (i = 0; i < MAX_MSI_CTRLS: i++) {
>     	u32 status = read_msi_ctrl(i);
> 
> 	for_each_bit(status)
> 		handle_irq();
>     }
> 
> So sure, the worst case here is MAX_MSI_CTRLS * 32, but if all
> possible 256 MSI interrupts are pending at the same time, you have
> other problems than that.

With threaded interrupts we would have 256 invocations of
wake_up_process() so nothing should take ages.

> Thanks,
> 
> 	tglx

Sebastian

Re: [v4.1.10-rt10][PATCH 1/2] genirq: introduce new generic_handle_irq_rt_wa() api

[Grygorii Strashko]

Hi Thomas, Sebastian,

On 11/03/2015 10:18 PM, Sebastian Andrzej Siewior wrote:
> On 11/03/2015 08:51 PM, Thomas Gleixner wrote:
>>> where MAX_MSI_IRQS = 32 now, but potentially can be increased up to 256.
>>
>> And you really oversimplified the code above. The reality is:
>>
>>      for (i = 0; i < MAX_MSI_CTRLS: i++) {
>>      	u32 status = read_msi_ctrl(i);
>>
>> 	for_each_bit(status)
>> 		handle_irq();
>>      }
>>
>> So sure, the worst case here is MAX_MSI_CTRLS * 32, but if all
>> possible 256 MSI interrupts are pending at the same time, you have
>> other problems than that.
>
> With threaded interrupts we would have 256 invocations of
> wake_up_process() so nothing should take ages.
>

Thanks a lot for your time and comments - I'll follow your
recommendations and use IRQF_NO_THREAD.

-- 
regards,
-grygorii

[v4.1.10-rt10] [PATCH 2/2] PCI: dra7xx: fix "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu"

[Grygorii Strashko]

Use generic_handle_irq_rt_wa() to W/A below backtrace, which is
triggered because dra7xx_pcie_msi_irq_handler() forced to be
threaded on -RT:

------------[ cut here ]------------
WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu+0x14c/0x174()
irq 460 handler irq_default_primary_handler+0x0/0x14 enabled interrupts
Modules linked in: ahci_platform(+) ti_vip(+) libahci_platform xhci_pci(+) c_can_platform(+)
libahci xhci_hcd ti_vpe c_can libata v4l2_mem2mem can_dev gpio_keys leds_gpio
snd_soc_simple_card usbcore spi_ti_qspi videobuf2_core extcon_usb_gpio omap_wdt
ti_vpdma videobuf2_dma_contig ti_soc_thermal videobuf2_memops dwc3_omap extcon
rtc_omap ov1063x v4l2_common videodev media snd_soc_tlv320aic3x omap_rng rng_core omap_remoteproc
CPU: 1 PID: 82 Comm: irq/26-dra7-pci Not tainted 4.1.10-rt10-02638-g6486d7e-dirty #1
Hardware name: Generic DRA74X (Flattened Device Tree)
Backtrace:
[<c0013060>] (dump_backtrace) from [<c0013280>] (show_stack+0x18/0x1c)
 r7:c07acca0 r6:00000000 r5:c09034e4 r4:00000000
[<c0013268>] (show_stack) from [<c061ef14>] (dump_stack+0x90/0xac)
[<c061ee84>] (dump_stack) from [<c00436e4>] (warn_slowpath_common+0x7c/0xb8)
 r7:c07acca0 r6:00000096 r5:00000009 r4:ee4d3e38
[<c0043668>] (warn_slowpath_common) from [<c0043758>] (warn_slowpath_fmt+0x38/0x40)
 r8:ee3fcc00 r7:000001cc r6:00000000 r5:00000002 r4:c07acc78
[<c0043724>] (warn_slowpath_fmt) from [<c0085fd0>] (handle_irq_event_percpu+0x14c/0x174)
 r3:000001cc r2:c07acc78
 r4:ecfcdd80
[<c0085e84>] (handle_irq_event_percpu) from [<c008607c>] (handle_irq_event+0x84/0xb8)
 r10:00000001 r9:ee4b59c0 r8:ee1d0900 r7:00000001 r6:00000000 r5:ecfcdd80
 r4:ee3fcc00
[<c0085ff8>] (handle_irq_event) from [<c0089240>] (handle_simple_irq+0x90/0x118)
 r5:ee4a9020 r4:ee3fcc00
[<c00891b0>] (handle_simple_irq) from [<c0085514>] (generic_handle_irq+0x30/0x44)
 r5:ee4a9020 r4:000001cc
[<c00854e4>] (generic_handle_irq) from [<c034a7d4>] (dra7xx_pcie_msi_irq_handler+0x7c/0x8c)
 r5:ee4a9020 r4:00000001
[<c034a758>] (dra7xx_pcie_msi_irq_handler) from [<c0086f08>] (irq_forced_thread_fn+0x28/0x5c)
 r5:ee1d0900 r4:ee4b59c0
[<c0086ee0>] (irq_forced_thread_fn) from [<c00871dc>] (irq_thread+0x128/0x204)
 r7:00000001 r6:00000000 r5:ee4d2000 r4:ee4b59e4
[<c00870b4>] (irq_thread) from [<c005e2fc>] (kthread+0xd4/0xec)
 r10:00000000 r9:00000000 r8:00000000 r7:c00870b4 r6:ee4b59c0 r5:ee4b5a00
 r4:00000000
[<c005e228>] (kthread) from [<c000faa8>] (ret_from_fork+0x14/0x2c)
 r7:00000000 r6:00000000 r5:c005e228 r4:ee4b5a00
---[ end trace 0000000000000002 ]---

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/pci/host/pci-dra7xx.c      | 3 ++-
 drivers/pci/host/pcie-designware.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 6589e7e..8c241cb 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -220,7 +220,8 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg)
 	case INTB:
 	case INTC:
 	case INTD:
-		generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg)));
+		generic_handle_irq_rt_wa(irq_find_mapping(pp->irq_domain,
+					 ffs(reg)));
 		break;
 	}
 
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 2e9f84f..6ba0ee3 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -177,7 +177,7 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
 				dw_pcie_wr_own_conf(pp,
 						PCIE_MSI_INTR0_STATUS + i * 12,
 						4, 1 << pos);
-				generic_handle_irq(irq);
+				generic_handle_irq_rt_wa(irq);
 				pos++;
 			}
 		}
-- 
2.6.2

Re: [v4.1.10-rt10][PATCH 0/2] PCI: dra7xx/dwc: fix "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu"

[Sebastian Andrzej Siewior]

On 11/02/2015 08:30 PM, Grygorii Strashko wrote:
> On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
> will be forced threaded (by default) and, as result, generic_handle_irq()
> will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
> be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).

and what happens without -RT if you boot threadedirqs?

Sebastian

Re: [v4.1.10-rt10][PATCH 0/2] PCI: dra7xx/dwc: fix "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu"

[Grygorii Strashko]

On 11/03/2015 03:46 PM, Sebastian Andrzej Siewior wrote:
> On 11/02/2015 08:30 PM, Grygorii Strashko wrote:
>> On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
>> will be forced threaded (by default) and, as result, generic_handle_irq()
>> will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
>> be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).
> 
> and what happens without -RT if you boot threadedirqs?
> 

Yep. It will still warn :( Can be reworked to cover this case also, i think
(if overall concept will be accepted) 


Thanks.
-- 
regards,
-grygorii

Re: [v4.1.10-rt10][PATCH 0/2] PCI: dra7xx/dwc: fix "WARNING: CPU: 1 PID: 82 at kernel/irq/handle.c:150 handle_irq_event_percpu"

[Thomas Gleixner]

On Tue, 3 Nov 2015, Grygorii Strashko wrote:
> On 11/03/2015 03:46 PM, Sebastian Andrzej Siewior wrote:
> > On 11/02/2015 08:30 PM, Grygorii Strashko wrote:
> >> On -RT above code will generate warnings, because driver_xx_hw_irq_handler()
> >> will be forced threaded (by default) and, as result, generic_handle_irq()
> >> will be called with IRQs enabled. To W/A this issue generic_handle_irq() can
> >> be surrounded by raw_spin_lock_irqsave/irqrestore(wa_lock).
> > 
> > and what happens without -RT if you boot threadedirqs?
> > 
> 
> Yep. It will still warn :( Can be reworked to cover this case also, i think
> (if overall concept will be accepted) 

wa_lock - I read that "Warning Annihilation Lock" - does not really
qualify as a concept.

Thanks,

	tglx