From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935840AbcKKSz5 (ORCPT ); Fri, 11 Nov 2016 13:55:57 -0500 Received: from mail.skyhub.de ([78.46.96.112]:51339 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934268AbcKKSzz (ORCPT ); Fri, 11 Nov 2016 13:55:55 -0500 Date: Fri, 11 Nov 2016 19:44:53 +0100 From: Borislav Petkov To: Sebastian Andrzej Siewior Cc: linux-kernel@vger.kernel.org, tglx@linutronix.de, rt@linutronix.de, Tony Luck , linux-edac@vger.kernel.org, x86@kernel.org Subject: Re: [PATCH 5/7] x86/mcheck: reorganize the hotplug callbacks Message-ID: <20161111184453.ax5getrj5y3i4fuc@pd.tnic> References: <20161110091809.vxyf3yiuxtjy3vqv@pd.tnic> <20161110174447.11848-1-bigeasy@linutronix.de> <20161110174447.11848-6-bigeasy@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20161110174447.11848-6-bigeasy@linutronix.de> User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 10, 2016 at 06:44:45PM +0100, Sebastian Andrzej Siewior wrote: > Initially I wanted to remove mcheck_cpu_init() from identify_cpu() and let it > become an independent early hotplug callback. The main problem here was that > the init on the boot CPU may happen too late > (device_initcall_sync(mcheck_init_device)) and nobody wanted to risk receiving > and MCE event at boot time leading to a shutdown (if the MCE feature is not yet > enabled). > > Here is attempt two: the timming stays as-is but the ordering of the functions timing > is changed: > - mcheck_cpu_init() (which is run from identify_cpu()) will setup the timer > struct but won't fire the timer. This is moved to CPU_ONLINE since its > cleanup part is in CPU_DOWN_PREPARE. So if it is okay to stop the timer early > in the shutdown phase, it should be okay to start it late in the bring up phase. > > - CPU_DOWN_PREPARE disables the MCE feature flags for !INTEL CPUs in disables the MCE error reporting... > mce_disable_cpu(). If a failure occures it would be re-enabled on all vendor occurs > CPUs (including Intel where it was not disabled during shutdown). To keep this > working I am moving it to CPU_ONLINE. smp_call_function_single() is dropped > beause the notifier runs nowdays on the target CPU. "... because the notifier runs on the target CPU now." Please run your commit messages text through a spellchecker. > - CPU_ONLINE is invoking mce_device_create() + mce_threshold_create_device() > but its cleanup part is in CPU_DEAD (mce_threshold_remove_device() and > mce_device_remove()). In order to keep this symmetrical I am moving the clean > up from CPU_DEAD to CPU_DOWN_PREPARE. cleanup > > Cc: Tony Luck > Cc: Borislav Petkov > Cc: linux-edac@vger.kernel.org > Cc: x86@kernel.org > Signed-off-by: Sebastian Andrzej Siewior > --- > arch/x86/kernel/cpu/mcheck/mce.c | 31 +++++++++++++++---------------- > 1 file changed, 15 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c > index 052b5e05c3c4..3da6fd94fa2e 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce.c > +++ b/arch/x86/kernel/cpu/mcheck/mce.c > @@ -1771,6 +1771,9 @@ void (*machine_check_vector)(struct pt_regs *, long error_code) = > */ > void mcheck_cpu_init(struct cpuinfo_x86 *c) > { > + struct timer_list *t = this_cpu_ptr(&mce_timer); > + unsigned int cpu = smp_processor_id(); > + > if (mca_cfg.disabled) > return; > > @@ -1796,7 +1799,7 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c) > __mcheck_cpu_init_generic(); > __mcheck_cpu_init_vendor(c); > __mcheck_cpu_init_clear_banks(); > - __mcheck_cpu_init_timer(); > + setup_pinned_timer(t, mce_timer_fn, cpu); Why not leave all that setup stuff in __mcheck_cpu_init_timer() ? ... > @@ -2517,11 +2518,10 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu) > mce_device_remove(cpu); > return NOTIFY_BAD; > } > - > + mce_reenable_cpu(); > + mce_start_timer(cpu, t); > break; > case CPU_DEAD: > - mce_threshold_remove_device(cpu); > - mce_device_remove(cpu); > mce_intel_hcpu_update(cpu); > > /* intentionally ignoring frozen here */ There's another place for cpuhp_tasks_frozen replacement here: /* intentionally ignoring frozen here */ if (!(action & CPU_TASKS_FROZEN)) cmci_rediscover(); into if (!cpuhp_tasks_frozen) cmci_rediscover(); > @@ -2529,12 +2529,11 @@ mce_cpu_callback(struct notifier_block *nfb, unsigned long action, void *hcpu) > cmci_rediscover(); > break; > case CPU_DOWN_PREPARE: > - smp_call_function_single(cpu, mce_disable_cpu, &action, 1); > + mce_disable_cpu(); > del_timer_sync(t); > - break; > - case CPU_DOWN_FAILED: > - smp_call_function_single(cpu, mce_reenable_cpu, &action, 1); > - mce_start_timer(cpu, t); > + > + mce_threshold_remove_device(cpu); > + mce_device_remove(cpu); > break; > } > > -- > 2.10.2 > > -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.