Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Sven Schnelle <svens@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, hca@linux.ibm.com,
        Sven Schnelle <svens@linux.ibm.com>
Subject: Re: [PATCH] entry: split lockdep and syscall work functions
In-Reply-To: <20201201083553.50996-2-svens@linux.ibm.com>
References: <20201201083553.50996-1-svens@linux.ibm.com> <20201201083553.50996-2-svens@linux.ibm.com>
Date:   Tue, 01 Dec 2020 12:09:45 +0100
Message-ID: <87lfehhjeu.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Sven,

On Tue, Dec 01 2020 at 09:35, Sven Schnelle wrote:
> On s390, we can not call one function which sets lockdep
> state and do the syscall work at the same time. There add
> make enter_from_user_mode() and exit_to_user_mode() public, and
> add syscall_exit_to_user_mode1() which does the same as
> syscall_exit_to_user_mode() but skips the final exit_to_user_mode().

the explanation in the "cover letter" made at least sense, but the above
is unparseable word salad.

> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>  include/linux/entry-common.h |  4 +++-
>  kernel/entry/common.c        | 35 +++++++++++++++++++++++++++--------
>  2 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/entry-common.h b/include/linux/entry-common.h
> index 474f29638d2c..496c9a47eab4 100644
> --- a/include/linux/entry-common.h
> +++ b/include/linux/entry-common.h
> @@ -124,7 +124,7 @@ static inline __must_check int arch_syscall_enter_tracehook(struct pt_regs *regs
>   * to be done between establishing state and handling user mode entry work.
>   */
>  void syscall_enter_from_user_mode_prepare(struct pt_regs *regs);
> -
> +void enter_from_user_mode(struct pt_regs *regs);

You might have noticed, that all of these function prototypes have
proper kernel documentation. So just glueing this on to the previous
prototype does not cut it. enter_from/exit_to_user_mode() want to go
together into a seperate section.

>  /**
>   * syscall_enter_from_user_mode_work - Check and handle work before invoking
>   *				       a syscall
> @@ -311,6 +311,8 @@ static inline void arch_syscall_exit_tracehook(struct pt_regs *regs, bool step)
>   *     arch_exit_to_user_mode() to handle e.g. speculation mitigations
>   */
>  void syscall_exit_to_user_mode(struct pt_regs *regs);
> +void syscall_exit_to_user_mode1(struct pt_regs *regs);

Same here and as you mentioned ...mode1() is a pretty horrible name.

     syscall_exit_to_user_mode_work() perhaps?

> +void exit_to_user_mode(void);
>  
>  /**
>   * irqentry_enter_from_user_mode - Establish state before invoking the irq handler
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index e9e2df3f3f9e..3ad462ebfa15 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -18,7 +18,7 @@
>   * 2) Invoke context tracking if enabled to reactivate RCU
>   * 3) Trace interrupts off state
>   */
> -static __always_inline void enter_from_user_mode(struct pt_regs *regs)
> +static __always_inline void __enter_from_user_mode(struct pt_regs
>  *regs)

Can you please split the renaming into a seperate preparatory patch?

> +__visible noinstr void syscall_exit_to_user_mode1(struct pt_regs *regs)

What's the point of marking this function noinstr? Everything it does is
instrumentable.

Thanks,

        tglx