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

[Ada Couprie Diaz]

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

> On 09/06/25 11:04 PM, Ada Couprie Diaz wrote:
>> [...]
>>
>> 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 ?

>>   {
>>   	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.
>> 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.
>>   {
>>
>>   	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