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

[Stephane Eranian]

On Sun, Apr 26, 2015 at 8:43 PM, Liang, Kan <kan.liang@xxxxxxxxx> wrote:
>
>>
>> > This leads me to believe that this patch:
>> >
>> > commit c05199e5a57a579fea1e8fa65e2b511ceb524ffc
>> > Author: Kan Liang <kan.liang@xxxxxxxxx>
>> > 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@xxxxxxxxx>
> 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@xxxxxxxxx>

Ok this works for me now. Thanks.

Tested-by: Stephane Eranian <eranian@xxxxxxxxxx>

> ---
>  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@xxxxxxxxxxxxxxx -- Speaking for myself only
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/