RE: [PATCH] perf/x86/intel/uncore: fix IMC missing box initialization

["Liang, Kan" <kan.liang@intel.com>]

>
> > This leads me to believe that this patch:
> >
> > commit c05199e5a57a579fea1e8fa65e2b511ceb524ffc
> > Author: Kan Liang <kan.liang@intel.com>
> > Date:   Tue Jan 20 04:54:25 2015 +0000
> >
> >     perf/x86/intel/uncore: Move uncore_box_init() out of driver
> initialization
> >
> > If I revert it, I bet things will work again.
>
> Yes the initialization needs to be moved out of the IPI context.
>

Maybe we can move them to event init, which is not in IPI context.

What do you think of this patch?

---

>From 8a61c48144921e9d1c841656829c3bae9bfb4408 Mon Sep 17 00:00:00 2001
From: Kan Liang <kan.liang@intel.com>
Date: Sun, 26 Apr 2015 16:24:59 -0400
Subject: [PATCH 1/1] perf/x86/intel/uncore: move uncore_box_init to uncore
 event init

commit c05199e5a57a("perf/x86/intel/uncore: Move uncore_box_init() out
of driver initialization") moves uncore_box_init into uncore_enable_box
to prevent potential boot failures. However, uncore_enable_box is not
called on some client platforms (SNB/IVB/HSW) for counting IMC event.
When it is not called, the box is not initialized, which hard locks the
system.

Additionally, uncore_enable_box along with the initialization code in it
is always called in uncore event start functions, which are in IPI
context. But the initizlization code should not be in IPI context. This
is because, for example, the IMC box initialization codes for client
platforms include ioremap, which is not allowed to be called in IPI
context.

This patch moves uncore_box_init out of IPI context, to uncore event
init. The box is initialized only when it has not yet been initialized.

Signed-off-by: Kan Liang <kan.liang@intel.com>
---
 arch/x86/kernel/cpu/perf_event_intel_uncore.c     | 4 ++++
 arch/x86/kernel/cpu/perf_event_intel_uncore.h     | 2 --
 arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c | 3 +++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.c b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
index c635b8b..cbc1a93 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c
@@ -623,6 +623,10 @@ static int uncore_pmu_event_init(struct perf_event *event)
        box = uncore_pmu_to_box(pmu, event->cpu);
        if (!box || box->cpu < 0)
                return -EINVAL;
+
+       /* Init box if it's not initialized yet */
+       uncore_box_init(box);
+
        event->cpu = box->cpu;

        event->hw.idx = -1;
diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore.h b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
index 6c8c1e7..1fb2905 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore.h
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.h
@@ -273,8 +273,6 @@ static inline void uncore_disable_box(struct intel_uncore_box *box)

 static inline void uncore_enable_box(struct intel_uncore_box *box)
 {
-       uncore_box_init(box);
-
        if (box->pmu->type->ops->enable_box)
                box->pmu->type->ops->enable_box(box);
 }
diff --git a/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c b/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c
index 4562e9e..ead70a6 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c
@@ -279,6 +279,9 @@ static int snb_uncore_imc_event_init(struct perf_event *event)
        if (!box || box->cpu < 0)
                return -EINVAL;

+       /* Init box if it's not initialized yet */
+       uncore_box_init(box);
+
        event->cpu = box->cpu;

        event->hw.idx = -1;

Thanks,
Kan

> -Andi
>
>
> --
> ak@linux.intel.com -- Speaking for myself only