Re: [PATCH v3 04/13] arm64: debug: call step handlers statically

On 13/06/2025 08:47, Anshuman Khandual wrote:

On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:

quoted

[...]

diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index b5a3b9c85a65..8f6ce2ea005c 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c

@@ -250,7 +250,7 @@ int kgdb_compiled_brk_handler(struct pt_regs *regs, unsigned long esr)
  }
  NOKPROBE_SYMBOL(kgdb_compiled_brk_handler);
  
-static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned long esr)
+int kgdb_singlestep_handler(struct pt_regs *regs, unsigned long esr)

This rename makes sense but as mentioned later kgdb_single_step_handler()
might save some changes in uprobes callback function.

That's fair. I think I would prefer the `_single_step_` version now as 
well, so I'll go for it.

As per the other patch, would it make sense to split the rename here as 
well ? Would it be OK if it were in the same commit as the breakpoint 
exception handlers ?

quoted

  {
  	if (!kgdb_single_step)
  		return DBG_HOOK_ERROR;

@@ -258,11 +258,7 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned long esr)
  	kgdb_handle_exception(0, SIGTRAP, 0, regs);
  	return DBG_HOOK_HANDLED;
  }
-NOKPROBE_SYMBOL(kgdb_step_brk_fn);
-
-static struct step_hook kgdb_step_hook = {
-	.fn		= kgdb_step_brk_fn
-};
+NOKPROBE_SYMBOL(kgdb_singlestep_handler);
  
  static int __kgdb_notify(struct die_args *args, unsigned long cmd)
  {

@@ -301,13 +297,7 @@ static struct notifier_block kgdb_notifier = {
   */
  int kgdb_arch_init(void)
  {
-	int ret = register_die_notifier(&kgdb_notifier);
-
-	if (ret != 0)
-		return ret;
-
-	register_kernel_step_hook(&kgdb_step_hook);
-	return 0;
+	return register_die_notifier(&kgdb_notifier);
  }
  
  /*

@@ -317,7 +307,6 @@ int kgdb_arch_init(void)
   */
  void kgdb_arch_exit(void)
  {
-	unregister_kernel_step_hook(&kgdb_step_hook);
  	unregister_die_notifier(&kgdb_notifier);
  }

kgdb_arch_init()/_exit() now deals only with kgdb_notifier registration
and un-registration only.

That is correct, however is there something you want me to do/change 
regarding this ? It feels like those would still be the best places for 
the `kgdb_notifier` (un)registration.

quoted

diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index ad68b4a5974d..fefc990860bc 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c

@@ -182,7 +182,7 @@ int uprobe_brk_handler(struct pt_regs *regs,
  	return DBG_HOOK_ERROR;
  }
  
-static int uprobe_single_step_handler(struct pt_regs *regs,
+int uprobe_singlestep_handler(struct pt_regs *regs,
  				      unsigned long esr)

A small nit - if the kgdb handler be changed as kgdb_single_step_handler()
the above rename can be skipped.

ACK above.

quoted

  {

  	struct uprobe_task *utask = current->utask;

@@ -194,15 +194,8 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
  	return DBG_HOOK_ERROR;
  }
  
-/* uprobe single step handler hook */
-static struct step_hook uprobes_step_hook = {
-	.fn = uprobe_single_step_handler,
-};
-
  static int __init arch_init_uprobes(void)
  {
-	register_user_step_hook(&uprobes_step_hook);
-
  	return 0;
  }
  

The arch hook arch_init_uprobes() is redundant now and can be dropped.

static int __init arch_init_uprobes(void)
{
         return 0;
}
device_initcall(arch_init_uprobes);

git grep arch_init_uprobes
arch/arm64/kernel/probes/uprobes.c:static int __init arch_init_uprobes(void)
arch/arm64/kernel/probes/uprobes.c:device_initcall(arch_init_uprobes);

Absolutely right, I did miss that. I will clean up in v4.

Otherwise LGTM.

Thanks for the comments,
Ada