[PATCHv2 0/2] arm-cc*: fix PMU interrupt flags

As discovered during review of the X-Gene SoC PMU [1], the arm-cc{i,n} drivers
don't ensure that IRQ balancers don't migrate interrupts.

This is problematic for the perf core code, which requires mutual exclusion of
certain operations (e.g. event rotation, cross-calls, and irq handling) to be
provided by disabling IRQs, which only works if all operations occur on the
same CPU. This is also required for safe (lockless) manipulation of some data
structures.

To avoid this problem, we must request interrupts with IRQF_NOBALANCING, as is
already the case for CPU PMU drivers which make use of interrupts.

To ensure synchronisation between IRQ handlers and other manipulation of said
data structures or HW state, we must also ensure that the interrupt handlers
are not threaded, by requesting them with IRQF_NO_THREAD, as is already the
case for CPU PMU drivers.

Since v1 [2]:
* Reword commit messages
* Drop IRQF_SHARED from arm-cci

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/439589.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/439739.html

Mark Rutland (2):
  arm-ccn: fix PMU interrupt flags
  arm-cci: fix PMU interrupt flags

 drivers/bus/arm-cci.c | 3 ++-
 drivers/bus/arm-ccn.c | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

-- 
1.9.1

Currently the IRQ core is permitted to make the CCN PMU IRQ handler
threaded, and will allow userspace to change the CPU affinity of the
interrupt behind our back. Both of these could violate our
synchronisation requirements with the core perf code, which relies upon
strict CPU affinity and disabling of interrupts to guarantee mutual
exclusion in some cases.

As with the CPU PMU drivers, we should request the interrupt with
IRQF_NOBALANCING and IRQF_NO_THREAD, to avoid these issues.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <redacted>
Cc: Pawel Moll <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Will Deacon <redacted>
Cc: Olof Johansson <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: <arm@kernel.org>
---
 drivers/bus/arm-ccn.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c
index acc3eb5..2c99ab2 100644
--- a/drivers/bus/arm-ccn.c
+++ b/drivers/bus/arm-ccn.c

@@ -1479,8 +1479,9 @@ static int arm_ccn_probe(struct platform_device *pdev)
 		/* Can set 'disable' bits, so can acknowledge interrupts */
 		writel(CCN_MN_ERRINT_STATUS__PMU_EVENTS__ENABLE,
 				ccn->base + CCN_MN_ERRINT_STATUS);
-		err = devm_request_irq(ccn->dev, irq, arm_ccn_irq_handler, 0,
-				dev_name(ccn->dev), ccn);
+		err = devm_request_irq(ccn->dev, irq, arm_ccn_irq_handler,
+				       IRQF_NOBALANCING | IRQF_NO_THREAD,
+				       dev_name(ccn->dev), ccn);
 		if (err)
 			return err;
 

-- 
1.9.1

Currently the IRQ core is permitted to make the CCI PMU IRQ handler
threaded, and will allow userspace to change the CPU affinity of the
interrupt behind our back. Both of these could violate our
synchronisation requirements with the core perf code, which relies upon
strict CPU affinity and disabling of interrupts to guarantee mutual
exclusion in some cases.

As with the CPU PMU drivers, we should request the interrupt with
IRQF_NOBALANCING and IRQF_NO_THREAD, to avoid these issues. At the same
time, we drop IRQF_SHARED. The interrupt is not shared in practice, and
the driver detects and skips duplicate IRQs at probe time, so it is not
necessary.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Marc Zyngier <redacted>
Cc: Punit Agrawal <redacted>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Will Deacon <redacted>
Cc: Olof Johansson <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: <arm@kernel.org>
---
 drivers/bus/arm-cci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index a49b283..ffce9e3 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c

@@ -881,7 +881,8 @@ static int pmu_request_irq(struct cci_pmu *cci_pmu, irq_handler_t handler)
 	 * This should allow handling of non-unique interrupt for the counters.
 	 */
 	for (i = 0; i < cci_pmu->nr_irqs; i++) {
-		int err = request_irq(cci_pmu->irqs[i], handler, IRQF_SHARED,
+		int err = request_irq(cci_pmu->irqs[i], handler,
+				IRQF_NOBALANCING | IRQF_NO_THREAD,
 				"arm-cci-pmu", cci_pmu);
 		if (err) {
 			dev_err(&pmu_device->dev, "unable to request IRQ%d for ARM CCI PMU counters\n",

-- 
1.9.1

On Mon, Jul 04, 2016 at 12:25:30PM +0100, Mark Rutland wrote:

As discovered during review of the X-Gene SoC PMU [1], the arm-cc{i,n} drivers
don't ensure that IRQ balancers don't migrate interrupts.

This is problematic for the perf core code, which requires mutual exclusion of
certain operations (e.g. event rotation, cross-calls, and irq handling) to be
provided by disabling IRQs, which only works if all operations occur on the
same CPU. This is also required for safe (lockless) manipulation of some data
structures.

To avoid this problem, we must request interrupts with IRQF_NOBALANCING, as is
already the case for CPU PMU drivers which make use of interrupts.

To ensure synchronisation between IRQ handlers and other manipulation of said
data structures or HW state, we must also ensure that the interrupt handlers
are not threaded, by requesting them with IRQF_NO_THREAD, as is already the
case for CPU PMU drivers.

Thanks, looks good now:

Reviewed-by: Will Deacon <redacted>

Will

On Mon, Jul 04, 2016 at 01:57:40PM +0100, Will Deacon wrote:

On Mon, Jul 04, 2016 at 12:25:30PM +0100, Mark Rutland wrote:

quoted

As discovered during review of the X-Gene SoC PMU [1], the arm-cc{i,n} drivers
don't ensure that IRQ balancers don't migrate interrupts.

This is problematic for the perf core code, which requires mutual exclusion of
certain operations (e.g. event rotation, cross-calls, and irq handling) to be
provided by disabling IRQs, which only works if all operations occur on the
same CPU. This is also required for safe (lockless) manipulation of some data
structures.

To avoid this problem, we must request interrupts with IRQF_NOBALANCING, as is
already the case for CPU PMU drivers which make use of interrupts.

To ensure synchronisation between IRQ handlers and other manipulation of said
data structures or HW state, we must also ensure that the interrupt handlers
are not threaded, by requesting them with IRQF_NO_THREAD, as is already the
case for CPU PMU drivers.

Thanks, looks good now:

Reviewed-by: Will Deacon <redacted>

Cheers!

I take it that as with the other recent CCI patch [1] you expect the
patches to go via arm-soc.

Arnd, Olof, are you happy to pick these patches?

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/440916.html