[PATCH] perf/arm-cmn: Add shutdown routine

[PATCH] perf/arm-cmn: Add shutdown routine

[Geoff Blake]

The CMN driver does not gracefully handle all
restart cases, such as kexec.  On a kexec if the
arm-cmn driver is in use it can be left in a state
with still active  events that can cause spurious and/or
unhandled interrupts that appear as non-fatal kernel errors
like below, that can be confusing and misleading:

[    3.895093] irq 28: nobody cared (try booting with the "irqpoll" option)
[    3.895170] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.19.0-1011-aws #12
[    3.895172] Hardware name: Amazon EC2 c6g.metal/Not Specified, BIOS 1.0 10/16/2017
[    3.895174] Call trace:
[    3.895175]  dump_backtrace+0xe8/0x150
[    3.895181]  show_stack+0x28/0x70
[    3.895183]  dump_stack_lvl+0x68/0x9c
[    3.895188]  dump_stack+0x1c/0x48
[    3.895190]  __report_bad_irq+0x58/0x138
[    3.895193]  note_interrupt+0x23c/0x360
[    3.895196]  handle_irq_event+0x108/0x1a0
[    3.895198]  handle_fasteoi_irq+0xd0/0x24c
[    3.895201]  generic_handle_domain_irq+0x3c/0x70
[    3.895203]  __gic_handle_irq_from_irqson.isra.0+0xcc/0x2c0
[    3.895207]  gic_handle_irq+0x34/0xb0
[    3.895209]  call_on_irq_stack+0x40/0x50
[    3.895211]  do_interrupt_handler+0xb0/0xb4
[    3.895214]  el1_interrupt+0x4c/0xe0
[    3.895217]  el1h_64_irq_handler+0x1c/0x40
[    3.895220]  el1h_64_irq+0x78/0x7c
[    3.895222]  __do_softirq+0xd0/0x450
[    3.895223]  __irq_exit_rcu+0xcc/0x120
[    3.895227]  irq_exit_rcu+0x20/0x40
[    3.895229]  el1_interrupt+0x50/0xe0
[    3.895231]  el1h_64_irq_handler+0x1c/0x40
[    3.895233]  el1h_64_irq+0x78/0x7c
[    3.895235]  arch_cpu_idle+0x1c/0x6c
[    3.895238]  default_idle_call+0x4c/0x19c
[    3.895240]  cpuidle_idle_call+0x18c/0x1f0
[    3.895243]  do_idle+0xb0/0x11c
[    3.895245]  cpu_startup_entry+0x34/0x40
[    3.895248]  rest_init+0xec/0x104
[    3.895250]  arch_post_acpi_subsys_init+0x0/0x30
[    3.895254]  start_kernel+0x4d0/0x534
[    3.895256]  __primary_switched+0xc4/0xcc
[    3.895259] handlers:
[    3.895292] [<000000008f5364c7>] arm_cmn_handle_irq [arm_cmn]
[    3.895369] Disabling IRQ #28

This type of kernel error can be reproduced by running perf with
an arm_cmn event active and then forcing a kexec.  On return from
the kexec, this message can appear semi-regularly.

This patch adds a shutdown routine that gets called by the
reboot/kexec path to put the PMU nodes back into a clean state
to avoid leaked events and the above unhandled interrupts.
Additionally modify arm_cmn_discover() loop to initialize all PMU
capable nodes to have no active events as done for XP watchpoints
in arm_cmn_init_dtm().  Testing 100's of kexecs with this patch
has shown this error message goes away.

Signed-off-by: Geoff Blake <blakgeof@amazon.com>
---
 drivers/perf/arm-cmn.c | 58 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index b80a9b74662b..032e0c87e8b6 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -112,6 +112,7 @@
 #define CMN_DTM_UNIT_INFO		0x0910
 
 #define CMN_DTM_NUM_COUNTERS		4
+#define CMN_DTM_NUM_WPS			4
 /* Want more local counters? Why not replicate the whole DTM! Ugh... */
 #define CMN_DTM_OFFSET(n)		((n) * 0x200)
 
@@ -1865,7 +1866,7 @@ static void arm_cmn_init_dtm(struct arm_cmn_dtm *dtm, struct arm_cmn_node *xp, i
 
 	dtm->base = xp->pmu_base + CMN_DTM_OFFSET(idx);
 	dtm->pmu_config_low = CMN_DTM_PMU_CONFIG_PMU_EN;
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < CMN_DTM_NUM_WPS; i++) {
 		dtm->wp_event[i] = -1;
 		writeq_relaxed(0, dtm->base + CMN_DTM_WPn_MASK(i));
 		writeq_relaxed(~0ULL, dtm->base + CMN_DTM_WPn_VAL(i));
@@ -2137,6 +2138,7 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 			case CMN_TYPE_CCRA:
 			case CMN_TYPE_CCHA:
 			case CMN_TYPE_CCLA:
+				writeq_relaxed(0, dn->pmu_base + CMN_PMU_EVENT_SEL);
 				dn++;
 				break;
 			/* Nothing to see here */
@@ -2156,6 +2158,8 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)
 				dn[1] = dn[0];
 				dn[0].pmu_base += CMN_HNP_PMU_EVENT_SEL;
 				dn[1].type = arm_cmn_subtype(dn->type);
+				writeq_relaxed(0, dn[0].pmu_base + CMN_PMU_EVENT_SEL);
+				writeq_relaxed(0, dn[1].pmu_base + CMN_PMU_EVENT_SEL);
 				dn += 2;
 				break;
 			/* Something has gone horribly wrong */
@@ -2312,15 +2316,64 @@ static int arm_cmn_probe(struct platform_device *pdev)
 	return err;
 }
 
-static int arm_cmn_remove(struct platform_device *pdev)
+static void arm_cmn_shutdown(struct platform_device *pdev)
 {
 	struct arm_cmn *cmn = platform_get_drvdata(pdev);
+	struct arm_cmn_dtm *dtm = cmn->dtms;
+	struct arm_cmn_node *dn = cmn->dns;
+	int i, j = 0;
 
+	writel_relaxed(0, cmn->dtc[0].base + CMN_DT_PMCR);
 	writel_relaxed(0, cmn->dtc[0].base + CMN_DT_DTC_CTL);
 
+	/* Go through all DTMs and disable PMU events and WPs */
+	for (i = 0, dtm = cmn->dtms; i < cmn->num_xps; i++, dtm++) {
+		writeq_relaxed(0, dtm->base + CMN_DTM_PMU_CONFIG);
+		for (j = 0; j < CMN_DTM_NUM_WPS; j++) {
+			writeq_relaxed(0, dtm->base + CMN_DTM_WPn_CONFIG(j));
+			writeq_relaxed(0, dtm->base + CMN_DTM_WPn_VAL(j));
+			writeq_relaxed(0, dtm->base + CMN_DTM_WPn_MASK(j));
+		}
+	}
+
+	/* Go through all relevant DN types and disable any leaked PMU events */
+	for (i = 0, dn = cmn->dns; i < cmn->num_dns; i++, dn++) {
+		switch (dn->type) {
+		case CMN_TYPE_DVM:
+		case CMN_TYPE_HNI:
+		case CMN_TYPE_HNF:
+		case CMN_TYPE_HNP:
+		case CMN_TYPE_SBSX:
+		case CMN_TYPE_RNI:
+		case CMN_TYPE_RND:
+		case CMN_TYPE_MTSX:
+		case CMN_TYPE_CXRA:
+		case CMN_TYPE_CXHA:
+		case CMN_TYPE_CCRA:
+		case CMN_TYPE_CCHA:
+		case CMN_TYPE_CCLA:
+		case CMN_TYPE_CCLA_RNI:
+		case CMN_TYPE_XP:
+			writeq_relaxed(0, dn->pmu_base + CMN_PMU_EVENT_SEL);
+			break;
+		case CMN_TYPE_DTC:
+		case CMN_TYPE_MPAM_S:
+		case CMN_TYPE_MPAM_NS:
+		case CMN_TYPE_RNSAM:
+		case CMN_TYPE_CXLA:
+		default:
+			break;
+		}
+	}
+
 	perf_pmu_unregister(&cmn->pmu);
 	cpuhp_state_remove_instance_nocalls(arm_cmn_hp_state, &cmn->cpuhp_node);
 	debugfs_remove(cmn->debug);
+}
+
+static int arm_cmn_remove(struct platform_device *pdev)
+{
+	arm_cmn_shutdown(pdev);
 	return 0;
 }
 
@@ -2353,6 +2406,7 @@ static struct platform_driver arm_cmn_driver = {
 	},
 	.probe = arm_cmn_probe,
 	.remove = arm_cmn_remove,
+	.shutdown = arm_cmn_shutdown,
 };
 
 static int __init arm_cmn_init(void)
-- 
2.24.3 (Apple Git-128)

Re: [PATCH] perf/arm-cmn: Add shutdown routine

[Will Deacon]

On Fri, 25 Nov 2022 17:01:53 -0600, Geoff Blake wrote:
> The CMN driver does not gracefully handle all
> restart cases, such as kexec.  On a kexec if the
> arm-cmn driver is in use it can be left in a state
> with still active  events that can cause spurious and/or
> unhandled interrupts that appear as non-fatal kernel errors
> like below, that can be confusing and misleading:
> 
> [...]

Applied to will (for-next/perf), thanks!

[1/1] perf/arm-cmn: Add shutdown routine
      https://git.kernel.org/will/c/316f862a787c

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH] perf/arm-cmn: Add shutdown routine

[Robin Murphy]

On 2022-11-29 19:52, Will Deacon wrote:
> On Fri, 25 Nov 2022 17:01:53 -0600, Geoff Blake wrote:
>> The CMN driver does not gracefully handle all
>> restart cases, such as kexec.  On a kexec if the
>> arm-cmn driver is in use it can be left in a state
>> with still active  events that can cause spurious and/or
>> unhandled interrupts that appear as non-fatal kernel errors
>> like below, that can be confusing and misleading:
>>
>> [...]
> 
> Applied to will (for-next/perf), thanks!
> 
> [1/1] perf/arm-cmn: Add shutdown routine
>        https://git.kernel.org/will/c/316f862a787c

Oh, if I'd seen this I'd have said the same thing as when asked about it 
off-list, that it's needlessly overcomplicated and doesn't really solve 
the problem anyway. If there's a need to be robust against spurious 
interrupts then that needs to be done in the interrupt handler.

Even if we do think it's worth stopping the PMU on shutdown, as we do on 
remove, that still only needs a single register write (per the current 
remove implementation).

Thanks,
Robin.

Re: [PATCH] perf/arm-cmn: Add shutdown routine

[Will Deacon]

On Wed, Nov 30, 2022 at 12:28:17PM +0000, Robin Murphy wrote:
> On 2022-11-29 19:52, Will Deacon wrote:
> > On Fri, 25 Nov 2022 17:01:53 -0600, Geoff Blake wrote:
> > > The CMN driver does not gracefully handle all
> > > restart cases, such as kexec.  On a kexec if the
> > > arm-cmn driver is in use it can be left in a state
> > > with still active  events that can cause spurious and/or
> > > unhandled interrupts that appear as non-fatal kernel errors
> > > like below, that can be confusing and misleading:
> > > 
> > > [...]
> > 
> > Applied to will (for-next/perf), thanks!
> > 
> > [1/1] perf/arm-cmn: Add shutdown routine
> >        https://git.kernel.org/will/c/316f862a787c
> 
> Oh, if I'd seen this I'd have said the same thing as when asked about it
> off-list, that it's needlessly overcomplicated and doesn't really solve the
> problem anyway. If there's a need to be robust against spurious interrupts
> then that needs to be done in the interrupt handler.
> 
> Even if we do think it's worth stopping the PMU on shutdown, as we do on
> remove, that still only needs a single register write (per the current
> remove implementation).

Sorry Robin, I didn't spot that you weren't on cc for this. I've dropped the
patch locally and I'll update the branches later on today.

Please can you help Geoff come up with something better?

Will

Re: [PATCH] perf/arm-cmn: Add shutdown routine

[Will Deacon]

On Thu, Jan 19, 2023 at 11:10:29AM -0600, Geoff Blake wrote:
> On Thu, 19 Jan 2023, Robin Murphy wrote:
>  
> > If you have a convincing argument that returning IRQ_NONE for unexpected
> > spurious interrupts is a real and important concern, then please propose
> > a general solution, because if it matters for arm-cmn then it matters
> > for hundreds of other drivers too, by rough estimate:
> > 
> > $ git grep -l IRQ_NONE '*.c' | xargs git grep -L IRQF_SHARED | wc -l
> > 834
> 
> The general solution for IRQ_NONE exists in the layer above the 
> driver, it complains with a visible warning that something might be wrong 
> and then moves on. Nothing more is needed.
> 
> Your shutdown routine that flips DT_EN in CMN_DT_DTC_CTL is sufficient, as 
> after some testing it solves the problem with left over IRQs for my kexec 
> use case.  

Please can you two let me know when you've settled on a fix for this? I'm
not going to queue something that you don't both agree on, although it seems
like we're quibbling over details at this point so maybe let's get
_something_ in and then work to improve it later?

Cheers,

Will

RE: [PATCH] perf/arm-cmn: Add shutdown routine

[Geoff Blake]

On Thu, 19 Jan 2023, Robin Murphy wrote:
 
> If you have a convincing argument that returning IRQ_NONE for unexpected
> spurious interrupts is a real and important concern, then please propose
> a general solution, because if it matters for arm-cmn then it matters
> for hundreds of other drivers too, by rough estimate:
> 
> $ git grep -l IRQ_NONE '*.c' | xargs git grep -L IRQF_SHARED | wc -l
> 834

The general solution for IRQ_NONE exists in the layer above the 
driver, it complains with a visible warning that something might be wrong 
and then moves on. Nothing more is needed.

Your shutdown routine that flips DT_EN in CMN_DT_DTC_CTL is sufficient, as 
after some testing it solves the problem with left over IRQs for my kexec 
use case.  

-Geoff

Re: [PATCH] perf/arm-cmn: Add shutdown routine

[Robin Murphy]

On 04/01/2023 3:55 pm, Geoff Blake wrote:
> Robin, Will,
> 
> Happy new year!  Hope I can get some attention back on this patch.
> Only difference from Robin's is it will do limited logging if spurious
> interrupts still happen to occur on current or future CMN implementations.

In all honesty I'm not sure what you want me to say... now you've 
written the same patch that I already sent, but still with an incorrect 
commit message, and with some unrelated changes that aren't mentioned 
and have nothing to do with shutdown anyway. Please see:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes

If you have a convincing argument that returning IRQ_NONE for unexpected 
spurious interrupts is a real and important concern, then please propose 
a general solution, because if it matters for arm-cmn then it matters 
for hundreds of other drivers too, by rough estimate:

$ git grep -l IRQ_NONE '*.c' | xargs git grep -L IRQF_SHARED | wc -l
834

Thanks,
Robin.

Re: [PATCH] perf/arm-cmn: Add shutdown routine

[Geoff Blake]

Robin, Will,

Still looking for a follow-up on this patch, would appreciate another 
review cycle. 

-Geoff

Re: [PATCH] perf/arm-cmn: Add shutdown routine

[Geoff Blake]

Robin, Will,

Happy new year!  Hope I can get some attention back on this patch.  
Only difference from Robin's is it will do limited logging if spurious 
interrupts still happen to occur on current or future CMN implementations.

Thanks,
Geoff

[PATCH] perf/arm-cmn: Add shutdown routine

[Geoff Blake]

Attempt #2 with all the feedback from Robin to do the minimal amount of
shutdown and handle spurious IRQs within the CMN driver but still do
limited logging in the event a spurious IRQ still occurs in the future.
Tested over 100's of kexec's and have no reproduced the spurious IRQs.

The CMN driver does not gracefully handle all
restart cases, such as kexec.  On a kexec if the
arm-cmn driver is in use it can be left in a state
with still active  events that can cause spurious and/or
unhandled interrupts that appear as non-fatal kernel errors
like below, that can be confusing and misleading:

[    3.895093] irq 28: nobody cared (try booting with the "irqpoll" option)
[    3.895170] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.19.0-1011-aws #12
[    3.895172] Hardware name: Amazon EC2 c6g.metal/Not Specified, BIOS 1.0 10/16/2017
[    3.895174] Call trace:
[    3.895175]  dump_backtrace+0xe8/0x150
[    3.895181]  show_stack+0x28/0x70
[    3.895183]  dump_stack_lvl+0x68/0x9c
[    3.895188]  dump_stack+0x1c/0x48
[    3.895190]  __report_bad_irq+0x58/0x138
[    3.895193]  note_interrupt+0x23c/0x360
[    3.895196]  handle_irq_event+0x108/0x1a0
[    3.895198]  handle_fasteoi_irq+0xd0/0x24c
[    3.895201]  generic_handle_domain_irq+0x3c/0x70
[    3.895203]  __gic_handle_irq_from_irqson.isra.0+0xcc/0x2c0
[    3.895207]  gic_handle_irq+0x34/0xb0
[    3.895209]  call_on_irq_stack+0x40/0x50
[    3.895211]  do_interrupt_handler+0xb0/0xb4
[    3.895214]  el1_interrupt+0x4c/0xe0
[    3.895217]  el1h_64_irq_handler+0x1c/0x40
[    3.895220]  el1h_64_irq+0x78/0x7c
[    3.895222]  __do_softirq+0xd0/0x450
[    3.895223]  __irq_exit_rcu+0xcc/0x120
[    3.895227]  irq_exit_rcu+0x20/0x40
[    3.895229]  el1_interrupt+0x50/0xe0
[    3.895231]  el1h_64_irq_handler+0x1c/0x40
[    3.895233]  el1h_64_irq+0x78/0x7c
[    3.895235]  arch_cpu_idle+0x1c/0x6c
[    3.895238]  default_idle_call+0x4c/0x19c
[    3.895240]  cpuidle_idle_call+0x18c/0x1f0
[    3.895243]  do_idle+0xb0/0x11c
[    3.895245]  cpu_startup_entry+0x34/0x40
[    3.895248]  rest_init+0xec/0x104
[    3.895250]  arch_post_acpi_subsys_init+0x0/0x30
[    3.895254]  start_kernel+0x4d0/0x534
[    3.895256]  __primary_switched+0xc4/0xcc
[    3.895259] handlers:
[    3.895292] [<000000008f5364c7>] arm_cmn_handle_irq [arm_cmn]
[    3.895369] Disabling IRQ #28

This type of kernel error can be reproduced by running perf with
an arm_cmn event active and then forcing a kexec.  On return from
the kexec, this message can appear semi-regularly.


Signed-off-by: Geoff Blake <blakgeof@amazon.com>
---
 drivers/perf/arm-cmn.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index b80a9b74662b..5e661a9aa0fe 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -112,6 +112,7 @@
 #define CMN_DTM_UNIT_INFO		0x0910
 
 #define CMN_DTM_NUM_COUNTERS		4
+#define CMN_DTM_NUM_WPS			4
 /* Want more local counters? Why not replicate the whole DTM! Ugh... */
 #define CMN_DTM_OFFSET(n)		((n) * 0x200)
 
@@ -1797,6 +1798,7 @@ static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_no
 
 static irqreturn_t arm_cmn_handle_irq(int irq, void *dev_id)
 {
+	static int spurious_count = 100;
 	struct arm_cmn_dtc *dtc = dev_id;
 	irqreturn_t ret = IRQ_NONE;
 
@@ -1825,8 +1827,13 @@ static irqreturn_t arm_cmn_handle_irq(int irq, void *dev_id)
 
 		writel_relaxed(status, dtc->base + CMN_DT_PMOVSR_CLR);
 
-		if (!dtc->irq_friend)
-			return ret;
+		if (!dtc->irq_friend) {
+			if (ret != IRQ_HANDLED && spurious_count > 0) {
+				spurious_count--;
+				WARN_ON(ret != IRQ_HANDLED);
+			}
+			return IRQ_HANDLED;
+		}
 		dtc += dtc->irq_friend;
 	}
 }
@@ -1865,7 +1872,7 @@ static void arm_cmn_init_dtm(struct arm_cmn_dtm *dtm, struct arm_cmn_node *xp, i
 
 	dtm->base = xp->pmu_base + CMN_DTM_OFFSET(idx);
 	dtm->pmu_config_low = CMN_DTM_PMU_CONFIG_PMU_EN;
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < CMN_DTM_NUM_WPS; i++) {
 		dtm->wp_event[i] = -1;
 		writeq_relaxed(0, dtm->base + CMN_DTM_WPn_MASK(i));
 		writeq_relaxed(~0ULL, dtm->base + CMN_DTM_WPn_VAL(i));
@@ -2312,11 +2319,18 @@ static int arm_cmn_probe(struct platform_device *pdev)
 	return err;
 }
 
-static int arm_cmn_remove(struct platform_device *pdev)
+static void arm_cmn_shutdown(struct platform_device *pdev)
 {
 	struct arm_cmn *cmn = platform_get_drvdata(pdev);
 
 	writel_relaxed(0, cmn->dtc[0].base + CMN_DT_DTC_CTL);
+}
+
+static int arm_cmn_remove(struct platform_device *pdev)
+{
+	struct arm_cmn *cmn = platform_get_drvdata(pdev);
+	
+	arm_cmn_shutdown(pdev);
 
 	perf_pmu_unregister(&cmn->pmu);
 	cpuhp_state_remove_instance_nocalls(arm_cmn_hp_state, &cmn->cpuhp_node);
@@ -2353,6 +2367,7 @@ static struct platform_driver arm_cmn_driver = {
 	},
 	.probe = arm_cmn_probe,
 	.remove = arm_cmn_remove,
+	.shutdown = arm_cmn_shutdown,
 };
 
 static int __init arm_cmn_init(void)
-- 
2.24.3 (Apple Git-128)