Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1524694 > unrolled thread

[PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-17 19:40 +0100
    Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine Will Deacon <will.deacon@arm.com> - 2016-11-18 13:10 +0100
      Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state  machine Thomas Gleixner <tglx@linutronix.de> - 2016-11-18 14:20 +0100
        Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine Will Deacon <will.deacon@arm.com> - 2016-11-18 14:40 +0100
          Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine Will Deacon <will.deacon@arm.com> - 2016-11-18 14:50 +0100
            Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state  machine Thomas Gleixner <tglx@linutronix.de> - 2016-11-18 15:10 +0100
              Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine Will Deacon <will.deacon@arm.com> - 2016-11-18 15:20 +0100
          Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state  machine Thomas Gleixner <tglx@linutronix.de> - 2016-11-18 14:50 +0100
    [tip:smp/hotplug] ARM/hw_breakpoint: Convert to hotplug state  machine tip-bot for Sebastian Andrzej Siewior <tipbot@zytor.com> - 2016-11-21 17:00 +0100
    [tip:smp/hotplug] ARM/hw_breakpoint: Convert to hotplug state  machine tip-bot for Sebastian Andrzej Siewior <tipbot@zytor.com> - 2016-11-22 23:50 +0100

#1524694 — [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine

Install the callbacks via the state machine and let the core invoke
the callbacks on the already online CPUs.

smp_call_function_single() has been removed because the function is already
invoked on the target CPU.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/arm/kernel/hw_breakpoint.c | 44 ++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index b8df45883cf7..51cff5a8feff 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -925,9 +925,9 @@ static bool core_has_os_save_restore(void)
 	}
 }
 
-static void reset_ctrl_regs(void *unused)
+static void reset_ctrl_regs(unsigned int cpu)
 {
-	int i, raw_num_brps, err = 0, cpu = smp_processor_id();
+	int i, raw_num_brps, err = 0;
 	u32 val;
 
 	/*
@@ -1020,25 +1020,20 @@ static void reset_ctrl_regs(void *unused)
 		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
 }
 
-static int dbg_reset_notify(struct notifier_block *self,
-				      unsigned long action, void *cpu)
+static int dbg_reset_online(unsigned int cpu)
 {
-	if ((action & ~CPU_TASKS_FROZEN) == CPU_ONLINE)
-		smp_call_function_single((int)cpu, reset_ctrl_regs, NULL, 1);
-
-	return NOTIFY_OK;
+	local_irq_disable();
+	reset_ctrl_regs(cpu);
+	local_irq_enable();
+	return 0;
 }
 
-static struct notifier_block dbg_reset_nb = {
-	.notifier_call = dbg_reset_notify,
-};
-
 #ifdef CONFIG_CPU_PM
 static int dbg_cpu_pm_notify(struct notifier_block *self, unsigned long action,
 			     void *v)
 {
 	if (action == CPU_PM_EXIT)
-		reset_ctrl_regs(NULL);
+		reset_ctrl_regs(smp_processor_id());
 
 	return NOTIFY_OK;
 }
@@ -1059,6 +1054,8 @@ static inline void pm_init(void)
 
 static int __init arch_hw_breakpoint_init(void)
 {
+	int ret;
+
 	debug_arch = get_debug_arch();
 
 	if (!debug_arch_supported()) {
@@ -1072,8 +1069,6 @@ static int __init arch_hw_breakpoint_init(void)
 	core_num_brps = get_num_brps();
 	core_num_wrps = get_num_wrps();
 
-	cpu_notifier_register_begin();
-
 	/*
 	 * We need to tread carefully here because DBGSWENABLE may be
 	 * driven low on this core and there isn't an architected way to
@@ -1082,15 +1077,18 @@ static int __init arch_hw_breakpoint_init(void)
 	register_undef_hook(&debug_reg_hook);
 
 	/*
-	 * Reset the breakpoint resources. We assume that a halting
-	 * debugger will leave the world in a nice state for us.
+	 * Register CPU notifier which resets the breakpoint resources. We
+	 * assume that a halting debugger will leave the world in a nice state
+	 * for us.
 	 */
-	on_each_cpu(reset_ctrl_regs, NULL, 1);
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
+				dbg_reset_online, NULL);
 	unregister_undef_hook(&debug_reg_hook);
-	if (!cpumask_empty(&debug_err_mask)) {
+	if (WARN_ON(ret < 0) || !cpumask_empty(&debug_err_mask)) {
 		core_num_brps = 0;
 		core_num_wrps = 0;
-		cpu_notifier_register_done();
+		if (ret > 0)
+			cpuhp_remove_state_nocalls(ret);
 		return 0;
 	}
 
@@ -1109,11 +1107,7 @@ static int __init arch_hw_breakpoint_init(void)
 	hook_ifault_code(FAULT_CODE_DEBUG, hw_breakpoint_pending, SIGTRAP,
 			TRAP_HWBKPT, "breakpoint debug exception");
 
-	/* Register hotplug and PM notifiers. */
-	__register_cpu_notifier(&dbg_reset_nb);
-
-	cpu_notifier_register_done();
-
+	/* Register PM notifiers. */
 	pm_init();
 	return 0;
 }
-- 
2.10.2

[toc] | [next] | [standalone]

#1525245

On Thu, Nov 17, 2016 at 07:35:36PM +0100, Sebastian Andrzej Siewior wrote:
> Install the callbacks via the state machine and let the core invoke
> the callbacks on the already online CPUs.
> 
> smp_call_function_single() has been removed because the function is already
> invoked on the target CPU.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/arm/kernel/hw_breakpoint.c | 44 ++++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 25 deletions(-)

[...]

>  static int __init arch_hw_breakpoint_init(void)
>  {
> +	int ret;
> +
>  	debug_arch = get_debug_arch();
>  
>  	if (!debug_arch_supported()) {
> @@ -1072,8 +1069,6 @@ static int __init arch_hw_breakpoint_init(void)
>  	core_num_brps = get_num_brps();
>  	core_num_wrps = get_num_wrps();
>  
> -	cpu_notifier_register_begin();
> -
>  	/*
>  	 * We need to tread carefully here because DBGSWENABLE may be
>  	 * driven low on this core and there isn't an architected way to
> @@ -1082,15 +1077,18 @@ static int __init arch_hw_breakpoint_init(void)
>  	register_undef_hook(&debug_reg_hook);
>  
>  	/*
> -	 * Reset the breakpoint resources. We assume that a halting
> -	 * debugger will leave the world in a nice state for us.
> +	 * Register CPU notifier which resets the breakpoint resources. We
> +	 * assume that a halting debugger will leave the world in a nice state
> +	 * for us.
>  	 */
> -	on_each_cpu(reset_ctrl_regs, NULL, 1);
> +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
> +				dbg_reset_online, NULL);

I'm slightly unsure about this. The dbg_reset_online callback can execute
undefined instructions (unfortunately, there's no way to probe the presence
of some of the debug registers), so it absolutely has to run within the 
register_undef_hook/unregister_undef_hook calls that are in this function.

With this patch, I worry that the callback can be postponed to ONLINE time
for other CPUs, and then the kernel will panic.

Will

[toc] | [prev] | [next] | [standalone]

#1525310 — Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine

On Fri, 18 Nov 2016, Will Deacon wrote:
> On Thu, Nov 17, 2016 at 07:35:36PM +0100, Sebastian Andrzej Siewior wrote:
> > Install the callbacks via the state machine and let the core invoke
> > the callbacks on the already online CPUs.
> > 
> > smp_call_function_single() has been removed because the function is already
> > invoked on the target CPU.
> > 
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  arch/arm/kernel/hw_breakpoint.c | 44 ++++++++++++++++++-----------------------
> >  1 file changed, 19 insertions(+), 25 deletions(-)
> 
> [...]
> 
> >  static int __init arch_hw_breakpoint_init(void)
> >  {
> > +	int ret;
> > +
> >  	debug_arch = get_debug_arch();
> >  
> >  	if (!debug_arch_supported()) {
> > @@ -1072,8 +1069,6 @@ static int __init arch_hw_breakpoint_init(void)
> >  	core_num_brps = get_num_brps();
> >  	core_num_wrps = get_num_wrps();
> >  
> > -	cpu_notifier_register_begin();
> > -
> >  	/*
> >  	 * We need to tread carefully here because DBGSWENABLE may be
> >  	 * driven low on this core and there isn't an architected way to
> > @@ -1082,15 +1077,18 @@ static int __init arch_hw_breakpoint_init(void)
> >  	register_undef_hook(&debug_reg_hook);
> >  
> >  	/*
> > -	 * Reset the breakpoint resources. We assume that a halting
> > -	 * debugger will leave the world in a nice state for us.
> > +	 * Register CPU notifier which resets the breakpoint resources. We
> > +	 * assume that a halting debugger will leave the world in a nice state
> > +	 * for us.
> >  	 */
> > -	on_each_cpu(reset_ctrl_regs, NULL, 1);
> > +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
> > +				dbg_reset_online, NULL);
> 
> I'm slightly unsure about this. The dbg_reset_online callback can execute
> undefined instructions (unfortunately, there's no way to probe the presence
> of some of the debug registers), so it absolutely has to run within the 
> register_undef_hook/unregister_undef_hook calls that are in this function.
> 
> With this patch, I worry that the callback can be postponed to ONLINE time
> for other CPUs, and then the kernel will panic.

No. The flow is the following:

  	register_undef_hook(&debug_reg_hook);

	ret = cpuhp_setup_state(.., dbg_reset_online, NULL);
	      {
		for_each_online_cpu(cpu) {
			ret = call_on_cpu(cpu, dbg_reset_online);
			if (ret)
			      return ret:
		}
	      }

  	unregister_undef_hook(&debug_reg_hook);
	
The only difference to the current code is that the call is not invoked via
a smp function call (on_each_cpu), it's pushed to the hotplug thread
context of each cpu and executed there.

But it's guaranteed that cpuhp_setup_state() will not return before the
callback has been invoked on each online cpu.

If cpus are not yet online when that code is invoked, then it's the same
behaviour as before. It will be invoked when the cpu comes online.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1525330

On Fri, Nov 18, 2016 at 02:11:58PM +0100, Thomas Gleixner wrote:
> On Fri, 18 Nov 2016, Will Deacon wrote:
> > On Thu, Nov 17, 2016 at 07:35:36PM +0100, Sebastian Andrzej Siewior wrote:
> > > @@ -1082,15 +1077,18 @@ static int __init arch_hw_breakpoint_init(void)
> > >  	register_undef_hook(&debug_reg_hook);
> > >  
> > >  	/*
> > > -	 * Reset the breakpoint resources. We assume that a halting
> > > -	 * debugger will leave the world in a nice state for us.
> > > +	 * Register CPU notifier which resets the breakpoint resources. We
> > > +	 * assume that a halting debugger will leave the world in a nice state
> > > +	 * for us.
> > >  	 */
> > > -	on_each_cpu(reset_ctrl_regs, NULL, 1);
> > > +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
> > > +				dbg_reset_online, NULL);
> > 
> > I'm slightly unsure about this. The dbg_reset_online callback can execute
> > undefined instructions (unfortunately, there's no way to probe the presence
> > of some of the debug registers), so it absolutely has to run within the 
> > register_undef_hook/unregister_undef_hook calls that are in this function.
> > 
> > With this patch, I worry that the callback can be postponed to ONLINE time
> > for other CPUs, and then the kernel will panic.
> 
> No. The flow is the following:
> 
>   	register_undef_hook(&debug_reg_hook);
> 
> 	ret = cpuhp_setup_state(.., dbg_reset_online, NULL);
> 	      {
> 		for_each_online_cpu(cpu) {
> 			ret = call_on_cpu(cpu, dbg_reset_online);
> 			if (ret)
> 			      return ret:
> 		}
> 	      }
> 
>   	unregister_undef_hook(&debug_reg_hook);
> 	
> The only difference to the current code is that the call is not invoked via
> a smp function call (on_each_cpu), it's pushed to the hotplug thread
> context of each cpu and executed there.
> 
> But it's guaranteed that cpuhp_setup_state() will not return before the
> callback has been invoked on each online cpu.

Ok, that's good.

> If cpus are not yet online when that code is invoked, then it's the same
> behaviour as before. It will be invoked when the cpu comes online.

Just to check, but what stops a CPU from coming online between the call
to cpuhp_setup_state and the call to cpuhp_remove_state_nocalls in the
case of failure (debug_err_mask isn't empty)?

Will

[toc] | [prev] | [next] | [standalone]

#1525342

On Fri, Nov 18, 2016 at 02:42:15PM +0100, Thomas Gleixner wrote:
> On Fri, 18 Nov 2016, Will Deacon wrote:
> > On Fri, Nov 18, 2016 at 02:11:58PM +0100, Thomas Gleixner wrote:
> > > But it's guaranteed that cpuhp_setup_state() will not return before the
> > > callback has been invoked on each online cpu.
> > 
> > Ok, that's good.
> > 
> > > If cpus are not yet online when that code is invoked, then it's the same
> > > behaviour as before. It will be invoked when the cpu comes online.
> > 
> > Just to check, but what stops a CPU from coming online between the call
> > to cpuhp_setup_state and the call to cpuhp_remove_state_nocalls in the
> > case of failure (debug_err_mask isn't empty)?
> 
> Indeed! I missed that part. So we still need a get/put_online_cpus()
> protection around all of this.

Yes, that should do it.

> Just for curiosity sake. Wouldn't it be simpler and less error prone to
> make the ARM_DBG_READ/WRITE macros use the exception table and handle that
> in the undefined instruction handler to avoid this hook dance?

That would be an option, but it's only the reset sequence that could
generate this fault so it's simpler to isolate it there. We'd also have
to take into account SMP if we toggle the handler in the READ/WRITE
accessors, since the fault handler framework is system-wide as opposed
to per-cpu. The whole thing is grotty as hell.

Will

[toc] | [prev] | [next] | [standalone]

#1525352 — Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine

On Fri, 18 Nov 2016, Will Deacon wrote:
> On Fri, Nov 18, 2016 at 02:42:15PM +0100, Thomas Gleixner wrote:
> > On Fri, 18 Nov 2016, Will Deacon wrote:
> > > On Fri, Nov 18, 2016 at 02:11:58PM +0100, Thomas Gleixner wrote:
> > > > But it's guaranteed that cpuhp_setup_state() will not return before the
> > > > callback has been invoked on each online cpu.
> > > 
> > > Ok, that's good.
> > > 
> > > > If cpus are not yet online when that code is invoked, then it's the same
> > > > behaviour as before. It will be invoked when the cpu comes online.
> > > 
> > > Just to check, but what stops a CPU from coming online between the call
> > > to cpuhp_setup_state and the call to cpuhp_remove_state_nocalls in the
> > > case of failure (debug_err_mask isn't empty)?
> > 
> > Indeed! I missed that part. So we still need a get/put_online_cpus()
> > protection around all of this.
> 
> Yes, that should do it.
> 
> > Just for curiosity sake. Wouldn't it be simpler and less error prone to
> > make the ARM_DBG_READ/WRITE macros use the exception table and handle that
> > in the undefined instruction handler to avoid this hook dance?
> 
> That would be an option, but it's only the reset sequence that could
> generate this fault so it's simpler to isolate it there. 

ARM_DBG_READ/WRITE_SAFE() then for reset_ctrl_regs()

> We'd also have to take into account SMP if we toggle the handler in the
> READ/WRITE accessors, since the fault handler framework is system-wide as
> opposed to per-cpu. The whole thing is grotty as hell.

The exception table is not toggling anything. It's just providing an entry
in the exception tables, which is scanned by fixup_exception(), which then
moves PC to the exception code. See __get_user_asm().

So the whole thing becomes:

static int reset_ctrl_regs(unsigned cpu)
{
	....
	if (ARM_DBG_READ_SAFE(c1, c5, 4, val))
		return -ENODEV;
	....
	return 0;
}

All you need is the extra

    	if (fixup_exception(regs))
		return;

in do_undefinstr() like it is there in do_kernel_fault(). No hooks, no
scope issues, just works.

I just mention this because that's how x86 implements rdmsr/wrmsr_safe() so
it can probe msr access. The difference though it that this results in a
#GP and not in #UD, but that's not a show stopper :)

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1525364

On Fri, Nov 18, 2016 at 02:59:04PM +0100, Thomas Gleixner wrote:
> On Fri, 18 Nov 2016, Will Deacon wrote:
> > On Fri, Nov 18, 2016 at 02:42:15PM +0100, Thomas Gleixner wrote:
> > > On Fri, 18 Nov 2016, Will Deacon wrote:
> > > > On Fri, Nov 18, 2016 at 02:11:58PM +0100, Thomas Gleixner wrote:
> > > > > But it's guaranteed that cpuhp_setup_state() will not return before the
> > > > > callback has been invoked on each online cpu.
> > > > 
> > > > Ok, that's good.
> > > > 
> > > > > If cpus are not yet online when that code is invoked, then it's the same
> > > > > behaviour as before. It will be invoked when the cpu comes online.
> > > > 
> > > > Just to check, but what stops a CPU from coming online between the call
> > > > to cpuhp_setup_state and the call to cpuhp_remove_state_nocalls in the
> > > > case of failure (debug_err_mask isn't empty)?
> > > 
> > > Indeed! I missed that part. So we still need a get/put_online_cpus()
> > > protection around all of this.
> > 
> > Yes, that should do it.
> > 
> > > Just for curiosity sake. Wouldn't it be simpler and less error prone to
> > > make the ARM_DBG_READ/WRITE macros use the exception table and handle that
> > > in the undefined instruction handler to avoid this hook dance?
> > 
> > That would be an option, but it's only the reset sequence that could
> > generate this fault so it's simpler to isolate it there. 
> 
> ARM_DBG_READ/WRITE_SAFE() then for reset_ctrl_regs()
> 
> > We'd also have to take into account SMP if we toggle the handler in the
> > READ/WRITE accessors, since the fault handler framework is system-wide as
> > opposed to per-cpu. The whole thing is grotty as hell.
> 
> The exception table is not toggling anything. It's just providing an entry
> in the exception tables, which is scanned by fixup_exception(), which then
> moves PC to the exception code. See __get_user_asm().

Oooh, now I see what you mean. I thought you were on about toggling
using register_undef_hook, but you're actually on about the extable stuff
that we already use for handling faults on user addresses in the kernel.

That's not a bad idea at all.

Will

[toc] | [prev] | [next] | [standalone]

#1525344 — Re: [PATCH 15/20] ARM/hw_breakpoint: Convert to hotplug state machine

On Fri, 18 Nov 2016, Will Deacon wrote:
> On Fri, Nov 18, 2016 at 02:11:58PM +0100, Thomas Gleixner wrote:
> > But it's guaranteed that cpuhp_setup_state() will not return before the
> > callback has been invoked on each online cpu.
> 
> Ok, that's good.
> 
> > If cpus are not yet online when that code is invoked, then it's the same
> > behaviour as before. It will be invoked when the cpu comes online.
> 
> Just to check, but what stops a CPU from coming online between the call
> to cpuhp_setup_state and the call to cpuhp_remove_state_nocalls in the
> case of failure (debug_err_mask isn't empty)?

Indeed! I missed that part. So we still need a get/put_online_cpus()
protection around all of this.

Just for curiosity sake. Wouldn't it be simpler and less error prone to
make the ARM_DBG_READ/WRITE macros use the exception table and handle that
in the undefined instruction handler to avoid this hook dance?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1526832 — [tip:smp/hotplug] ARM/hw_breakpoint: Convert to hotplug state machine

Commit-ID:  3b341295030a327d7c763630f865fbf20a1df1c8
Gitweb:     http://git.kernel.org/tip/3b341295030a327d7c763630f865fbf20a1df1c8
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Thu, 17 Nov 2016 19:35:36 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 21 Nov 2016 16:37:08 +0100

ARM/hw_breakpoint: Convert to hotplug state machine

Install the callbacks via the state machine and let the core invoke
the callbacks on the already online CPUs.

smp_call_function_single() has been removed because the function is already
invoked on the target CPU.

[ tglx: Added protection agaist hotplug back according to discussion with Will ]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: rt@linuxtronix.de
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20161117183541.8588-16-bigeasy@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/arm/kernel/hw_breakpoint.c | 47 +++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index b8df458..188180b 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -925,9 +925,9 @@ static bool core_has_os_save_restore(void)
 	}
 }
 
-static void reset_ctrl_regs(void *unused)
+static void reset_ctrl_regs(unsigned int cpu)
 {
-	int i, raw_num_brps, err = 0, cpu = smp_processor_id();
+	int i, raw_num_brps, err = 0;
 	u32 val;
 
 	/*
@@ -1020,25 +1020,20 @@ out_mdbgen:
 		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
 }
 
-static int dbg_reset_notify(struct notifier_block *self,
-				      unsigned long action, void *cpu)
+static int dbg_reset_online(unsigned int cpu)
 {
-	if ((action & ~CPU_TASKS_FROZEN) == CPU_ONLINE)
-		smp_call_function_single((int)cpu, reset_ctrl_regs, NULL, 1);
-
-	return NOTIFY_OK;
+	local_irq_disable();
+	reset_ctrl_regs(cpu);
+	local_irq_enable();
+	return 0;
 }
 
-static struct notifier_block dbg_reset_nb = {
-	.notifier_call = dbg_reset_notify,
-};
-
 #ifdef CONFIG_CPU_PM
 static int dbg_cpu_pm_notify(struct notifier_block *self, unsigned long action,
 			     void *v)
 {
 	if (action == CPU_PM_EXIT)
-		reset_ctrl_regs(NULL);
+		reset_ctrl_regs(smp_processor_id());
 
 	return NOTIFY_OK;
 }
@@ -1059,6 +1054,8 @@ static inline void pm_init(void)
 
 static int __init arch_hw_breakpoint_init(void)
 {
+	int ret;
+
 	debug_arch = get_debug_arch();
 
 	if (!debug_arch_supported()) {
@@ -1072,25 +1069,28 @@ static int __init arch_hw_breakpoint_init(void)
 	core_num_brps = get_num_brps();
 	core_num_wrps = get_num_wrps();
 
-	cpu_notifier_register_begin();
-
 	/*
 	 * We need to tread carefully here because DBGSWENABLE may be
 	 * driven low on this core and there isn't an architected way to
 	 * determine that.
 	 */
+	get_online_cpus();
 	register_undef_hook(&debug_reg_hook);
 
 	/*
-	 * Reset the breakpoint resources. We assume that a halting
-	 * debugger will leave the world in a nice state for us.
+	 * Register CPU notifier which resets the breakpoint resources. We
+	 * assume that a halting debugger will leave the world in a nice state
+	 * for us.
 	 */
-	on_each_cpu(reset_ctrl_regs, NULL, 1);
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
+				dbg_reset_online, NULL);
 	unregister_undef_hook(&debug_reg_hook);
-	if (!cpumask_empty(&debug_err_mask)) {
+	if (WARN_ON(ret < 0) || !cpumask_empty(&debug_err_mask)) {
 		core_num_brps = 0;
 		core_num_wrps = 0;
-		cpu_notifier_register_done();
+		if (ret > 0)
+			cpuhp_remove_state_nocalls(ret);
+		put_online_cpus();
 		return 0;
 	}
 
@@ -1108,12 +1108,9 @@ static int __init arch_hw_breakpoint_init(void)
 			TRAP_HWBKPT, "watchpoint debug exception");
 	hook_ifault_code(FAULT_CODE_DEBUG, hw_breakpoint_pending, SIGTRAP,
 			TRAP_HWBKPT, "breakpoint debug exception");
+	put_online_cpus();
 
-	/* Register hotplug and PM notifiers. */
-	__register_cpu_notifier(&dbg_reset_nb);
-
-	cpu_notifier_register_done();
-
+	/* Register PM notifiers. */
 	pm_init();
 	return 0;
 }

[toc] | [prev] | [next] | [standalone]

#1528010 — [tip:smp/hotplug] ARM/hw_breakpoint: Convert to hotplug state machine

Commit-ID:  9b377e217f0bd07f972d89ed0963df92818beffd
Gitweb:     http://git.kernel.org/tip/9b377e217f0bd07f972d89ed0963df92818beffd
Author:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
AuthorDate: Thu, 17 Nov 2016 19:35:36 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 22 Nov 2016 23:34:41 +0100

ARM/hw_breakpoint: Convert to hotplug state machine

Install the callbacks via the state machine and let the core invoke
the callbacks on the already online CPUs.

smp_call_function_single() has been removed because the function is already
invoked on the target CPU.

[ tglx: Added protection agaist hotplug back according to discussion with Will ]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: rt@linuxtronix.de
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org
Link: http://lkml.kernel.org/r/20161117183541.8588-16-bigeasy@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/arm/kernel/hw_breakpoint.c | 47 +++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index b8df458..188180b 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -925,9 +925,9 @@ static bool core_has_os_save_restore(void)
 	}
 }
 
-static void reset_ctrl_regs(void *unused)
+static void reset_ctrl_regs(unsigned int cpu)
 {
-	int i, raw_num_brps, err = 0, cpu = smp_processor_id();
+	int i, raw_num_brps, err = 0;
 	u32 val;
 
 	/*
@@ -1020,25 +1020,20 @@ out_mdbgen:
 		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
 }
 
-static int dbg_reset_notify(struct notifier_block *self,
-				      unsigned long action, void *cpu)
+static int dbg_reset_online(unsigned int cpu)
 {
-	if ((action & ~CPU_TASKS_FROZEN) == CPU_ONLINE)
-		smp_call_function_single((int)cpu, reset_ctrl_regs, NULL, 1);
-
-	return NOTIFY_OK;
+	local_irq_disable();
+	reset_ctrl_regs(cpu);
+	local_irq_enable();
+	return 0;
 }
 
-static struct notifier_block dbg_reset_nb = {
-	.notifier_call = dbg_reset_notify,
-};
-
 #ifdef CONFIG_CPU_PM
 static int dbg_cpu_pm_notify(struct notifier_block *self, unsigned long action,
 			     void *v)
 {
 	if (action == CPU_PM_EXIT)
-		reset_ctrl_regs(NULL);
+		reset_ctrl_regs(smp_processor_id());
 
 	return NOTIFY_OK;
 }
@@ -1059,6 +1054,8 @@ static inline void pm_init(void)
 
 static int __init arch_hw_breakpoint_init(void)
 {
+	int ret;
+
 	debug_arch = get_debug_arch();
 
 	if (!debug_arch_supported()) {
@@ -1072,25 +1069,28 @@ static int __init arch_hw_breakpoint_init(void)
 	core_num_brps = get_num_brps();
 	core_num_wrps = get_num_wrps();
 
-	cpu_notifier_register_begin();
-
 	/*
 	 * We need to tread carefully here because DBGSWENABLE may be
 	 * driven low on this core and there isn't an architected way to
 	 * determine that.
 	 */
+	get_online_cpus();
 	register_undef_hook(&debug_reg_hook);
 
 	/*
-	 * Reset the breakpoint resources. We assume that a halting
-	 * debugger will leave the world in a nice state for us.
+	 * Register CPU notifier which resets the breakpoint resources. We
+	 * assume that a halting debugger will leave the world in a nice state
+	 * for us.
 	 */
-	on_each_cpu(reset_ctrl_regs, NULL, 1);
+	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "arm/hw_breakpoint:online",
+				dbg_reset_online, NULL);
 	unregister_undef_hook(&debug_reg_hook);
-	if (!cpumask_empty(&debug_err_mask)) {
+	if (WARN_ON(ret < 0) || !cpumask_empty(&debug_err_mask)) {
 		core_num_brps = 0;
 		core_num_wrps = 0;
-		cpu_notifier_register_done();
+		if (ret > 0)
+			cpuhp_remove_state_nocalls(ret);
+		put_online_cpus();
 		return 0;
 	}
 
@@ -1108,12 +1108,9 @@ static int __init arch_hw_breakpoint_init(void)
 			TRAP_HWBKPT, "watchpoint debug exception");
 	hook_ifault_code(FAULT_CODE_DEBUG, hw_breakpoint_pending, SIGTRAP,
 			TRAP_HWBKPT, "breakpoint debug exception");
+	put_online_cpus();
 
-	/* Register hotplug and PM notifiers. */
-	__register_cpu_notifier(&dbg_reset_nb);
-
-	cpu_notifier_register_done();
-
+	/* Register PM notifiers. */
 	pm_init();
 	return 0;
 }

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web