* [PATCH] perf/arm-cmn: Add shutdown routine
@ 2022-11-25 23:01 Geoff Blake
2022-11-29 19:52 ` Will Deacon
0 siblings, 1 reply; 10+ messages in thread
From: Geoff Blake @ 2022-11-25 23:01 UTC (permalink / raw)
To: linux-kernel; +Cc: blakgeof, Will Deacon, Mark Rutland, linux-arm-kernel
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)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] perf/arm-cmn: Add shutdown routine
2022-11-25 23:01 [PATCH] perf/arm-cmn: Add shutdown routine Geoff Blake
@ 2022-11-29 19:52 ` Will Deacon
2022-11-30 12:28 ` Robin Murphy
0 siblings, 1 reply; 10+ messages in thread
From: Will Deacon @ 2022-11-29 19:52 UTC (permalink / raw)
To: Geoff Blake, linux-kernel
Cc: catalin.marinas, kernel-team, Will Deacon, linux-arm-kernel,
Mark Rutland
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] perf/arm-cmn: Add shutdown routine
2022-11-29 19:52 ` Will Deacon
@ 2022-11-30 12:28 ` Robin Murphy
2022-11-30 13:29 ` Will Deacon
0 siblings, 1 reply; 10+ messages in thread
From: Robin Murphy @ 2022-11-30 12:28 UTC (permalink / raw)
To: Will Deacon, Geoff Blake, linux-kernel
Cc: catalin.marinas, kernel-team, linux-arm-kernel, Mark Rutland
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf/arm-cmn: Add shutdown routine
2022-11-30 12:28 ` Robin Murphy
@ 2022-11-30 13:29 ` Will Deacon
0 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2022-11-30 13:29 UTC (permalink / raw)
To: Robin Murphy
Cc: Geoff Blake, linux-kernel, catalin.marinas, kernel-team,
linux-arm-kernel, Mark Rutland
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH 1/2] perf/arm-cmn: Cope with spurious IRQs better
@ 2022-12-05 15:38 Geoff Blake
2022-12-15 18:00 ` [PATCH] perf/arm-cmn: Add shutdown routine Geoff Blake
0 siblings, 1 reply; 10+ messages in thread
From: Geoff Blake @ 2022-12-05 15:38 UTC (permalink / raw)
To: Robin Murphy; +Cc: will, mark.rutland, linux-arm-kernel, linux-kernel
> > > > From my perspective, this is a worse solution as now we're sweeping an
> > > > issue under the rug and consuming CPU cycles handling IRQs we should not
> > > > be getting in the first place. While an overflow IRQ from the cmn
> > > > should
> > > > not be high frequency, there is a non-zero chance in the future it could
> > > > be and this could lead to a very hard to debug performance issue instead
> > > > of the current problem, which is discovering we need to clean up better
> > > > from a noisy kernel message.
> > >
> > > Kexec is not the only possible source of spurious IRQs. If they cause a
> > > problem for this driver, that cannot be robustly addressed by trying to
> > > rely on whatever software might happen to run before this driver.
> >
> > Sure, I can agree with the assertion a spurious IRQ could come from
> > anywhere, in that case though, shouldn't the behavior still be to log
> > spurious IRQs as a warning instead of silently sinking them?
>
> We still have to handle the interrupt anyway to avoid it getting
> disabled behind our back, and beyond that it's not really something
> that's actionable by the user. What would we say?
>
> dev_warn(dev, "Something harmless, and in some cases expected,
> happened! If you've just rebooted after a kernel panic, maybe try having
> the kernel not panic?");
>
> Perhaps that should be a core IRQ helper so that many other drivers can
> also call it too?
>
> Furthermore if you're worried about performance implications from a
> theoretical interrupt storm, I can tell you from experience that logging
> to a serial console from a high-frequency interrupt handler is one of
> the best ways to cripple a system to the point where reaching for the
> power switch is the only option.
Logging unexpected events is necessary to give clues of what is going
wrong before they implode on fully remote machines. If you prefer to
handle the IRQ here rather than in the bad_irq section, then can we at
least have a WARN_ON() in the case where a spurious IRQ happens but no
overflow bit is set.
> The DTC_CTL documentation seems fairly unambiguous:
>
> [0] dt_en Enables debug, trace, and PMU features
>
> The design intent is that the PMU counters do not count when the entire
> PMU feature is disabled. I'm pretty sure I did confirm that empirically
> during development too (I recall the sheer number of different "enable"
> bits baffled me at the beginning, and there was actually one that did
> nothing, which I think did eventually get removed from the documentation).
>
> Of course clearing PMCR_PMU_EN is sufficient to simply stop counting,
> which we also depend on for correct operation, but I believe clearing
> DT_EN allows it to put all of the DT logic into a quiescent state.
I took the other patch that writes 0 to DTC_CTL.dt_en only and put it in a
loop of kexec'ing when the PMU is active for a few hours, I did not see
anymore spurious IRQs (whereas with the stock driver I could reproduce in under 10 tries).
You are correct Robin, that is all that is needed, and my code was overly
cautious.
- Geoff
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] perf/arm-cmn: Add shutdown routine
2022-12-05 15:38 [PATCH 1/2] perf/arm-cmn: Cope with spurious IRQs better Geoff Blake
@ 2022-12-15 18:00 ` Geoff Blake
2023-01-04 15:55 ` Geoff Blake
0 siblings, 1 reply; 10+ messages in thread
From: Geoff Blake @ 2022-12-15 18:00 UTC (permalink / raw)
To: Robin.Murphy
Cc: blakgeof, Will Deacon, Mark Rutland, linux-arm-kernel,
linux-kernel
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)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] perf/arm-cmn: Add shutdown routine
2022-12-15 18:00 ` [PATCH] perf/arm-cmn: Add shutdown routine Geoff Blake
@ 2023-01-04 15:55 ` Geoff Blake
2023-01-19 15:30 ` Geoff Blake
2023-01-19 15:32 ` Robin Murphy
0 siblings, 2 replies; 10+ messages in thread
From: Geoff Blake @ 2023-01-04 15:55 UTC (permalink / raw)
To: Robin.Murphy; +Cc: Will Deacon, Mark Rutland, linux-arm-kernel, linux-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf/arm-cmn: Add shutdown routine
2023-01-04 15:55 ` Geoff Blake
@ 2023-01-19 15:30 ` Geoff Blake
2023-01-19 15:32 ` Robin Murphy
1 sibling, 0 replies; 10+ messages in thread
From: Geoff Blake @ 2023-01-19 15:30 UTC (permalink / raw)
To: Robin.Murphy; +Cc: Will Deacon, Mark Rutland, linux-arm-kernel, linux-kernel
Robin, Will,
Still looking for a follow-up on this patch, would appreciate another
review cycle.
-Geoff
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf/arm-cmn: Add shutdown routine
2023-01-04 15:55 ` Geoff Blake
2023-01-19 15:30 ` Geoff Blake
@ 2023-01-19 15:32 ` Robin Murphy
2023-01-19 17:10 ` Geoff Blake
1 sibling, 1 reply; 10+ messages in thread
From: Robin Murphy @ 2023-01-19 15:32 UTC (permalink / raw)
To: Geoff Blake; +Cc: Will Deacon, Mark Rutland, linux-arm-kernel, linux-kernel
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] perf/arm-cmn: Add shutdown routine
2023-01-19 15:32 ` Robin Murphy
@ 2023-01-19 17:10 ` Geoff Blake
2023-01-19 18:14 ` Will Deacon
0 siblings, 1 reply; 10+ messages in thread
From: Geoff Blake @ 2023-01-19 17:10 UTC (permalink / raw)
To: Robin Murphy; +Cc: Will Deacon, Mark Rutland, linux-arm-kernel, linux-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] perf/arm-cmn: Add shutdown routine
2023-01-19 17:10 ` Geoff Blake
@ 2023-01-19 18:14 ` Will Deacon
0 siblings, 0 replies; 10+ messages in thread
From: Will Deacon @ 2023-01-19 18:14 UTC (permalink / raw)
To: Geoff Blake, robin.murphy; +Cc: Mark Rutland, linux-arm-kernel, linux-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-01-19 18:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-25 23:01 [PATCH] perf/arm-cmn: Add shutdown routine Geoff Blake
2022-11-29 19:52 ` Will Deacon
2022-11-30 12:28 ` Robin Murphy
2022-11-30 13:29 ` Will Deacon
-- strict thread matches above, loose matches on Subject: below --
2022-12-05 15:38 [PATCH 1/2] perf/arm-cmn: Cope with spurious IRQs better Geoff Blake
2022-12-15 18:00 ` [PATCH] perf/arm-cmn: Add shutdown routine Geoff Blake
2023-01-04 15:55 ` Geoff Blake
2023-01-19 15:30 ` Geoff Blake
2023-01-19 15:32 ` Robin Murphy
2023-01-19 17:10 ` Geoff Blake
2023-01-19 18:14 ` Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox