Re: [PATCH -next v7 7/7] arm64: entry: Switch to generic IRQ entry

[Ada Couprie Diaz <ada.coupriediaz@arm.com>]

Hi Jinjie,

The code changes look good to me, just a small missing clean up I believe.

On 29/07/2025 02:54, Jinjie Ruan wrote:

> Currently, x86, Riscv, Loongarch use the generic entry. Convert arm64
> to use the generic entry infrastructure from kernel/entry/*.
> The generic entry makes maintainers' work easier and codes
> more elegant.
>
> Switch arm64 to generic IRQ entry first, which removed duplicate 100+
> LOC. The next patch serise will switch to generic entry completely later.
> Switch to generic entry in two steps according to Mark's suggestion
> will make it easier to review.

I think the commit message could be clearer, especially since now this 
series
only moves arm64 to generic IRQ entry and not the complete generic entry.

What do you think of something like below ? It repeats a bit less and I 
think
it helps understanding what is going on in this specific commit, as you 
already
have details on the larger plans in the cover.

	Currently, x86, Riscv and Loongarch use the generic entry code, which makes
	maintainer's work easier and code more elegant.
	Start converting arm64 to use the generic entry infrastructure
	from kernel/entry/* by switching it to generic IRQ entry, which removes 100+
	lines of duplicate code.
	arm64 will completely switch to generic entry in a later series.

> The changes are below:
>   - Remove *enter_from/exit_to_kernel_mode(), and wrap with generic
>     irqentry_enter/exit(). Also remove *enter_from/exit_to_user_mode(),
>     and wrap with generic enter_from/exit_to_user_mode() because they
>     are exactly the same so far.
Nit : "so far" can be removed
>   - Remove arm64_enter/exit_nmi() and use generic irqentry_nmi_enter/exit()
>     because they're exactly the same, so the temporary arm64 version
>     irqentry_state can also be removed.
>
>   - Remove PREEMPT_DYNAMIC code, as generic entry do the same thing
>     if arm64 implement arch_irqentry_exit_need_resched().
This feels unrelated, given that the part that needs 
`arch_irqentry_exit_need_resched()`
is called whether or not PREEMPT_DYNAMIC is enabled ?

Given my comments on patch 5, I feel that the commit message should mention
explicitly the implementation of `arch_irqentry_exit_need_resched()` and 
why,
even though it was already mentioned in patch 5.
(This is what I was referencing in patch 5 : as I feel it's useful to 
mention again
the reasons when implementing it, it doesn't feel too out of place to 
introduce
the generic part at the same time. But again, I might be wrong here.)

Then you can have another point explaining that 
`raw_irqentry_exit_cond_resched()`
and the PREEMPT_DYNAMIC code is removed because they are identical to the
generic entry code, similarly to your other points.
> Tested ok with following test cases on Qemu virt platform:
>   - Perf tests.
>   - Different `dynamic preempt` mode switch.
>   - Pseudo NMI tests.
>   - Stress-ng CPU stress test.
>   - MTE test case in Documentation/arch/arm64/memory-tagging-extension.rst
>     and all test cases in tools/testing/selftests/arm64/mte/*.
Nit : I'm not sure if the commit message is the best place for this, 
given you
already gave some details in the cover ?
But I don't have much experience here, so I'll leave it up to you and 
others !
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> [...]
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index db3f972f8cd9..1110eeb21f57 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -9,6 +9,7 @@
>   #include <linux/cache.h>
>   #include <linux/compat.h>
>   #include <linux/errno.h>
> +#include <linux/irq-entry-common.h>
>   #include <linux/kernel.h>
>   #include <linux/signal.h>
>   #include <linux/freezer.h>
> @@ -1576,7 +1577,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
>    * the kernel can handle, and then we build all the user-level signal handling
>    * stack-frames in one go after that.
>    */
> -void do_signal(struct pt_regs *regs)
> +void arch_do_signal_or_restart(struct pt_regs *regs)
Given that `do_signal(struct pt_regs *regs)` is defined in 
`arch/arm64/include/asm/exception.h`,
and that there remains no users of `do_signal()`, I think it should be 
removed there.

Thanks,
Ada