On Sat, Sep 19, 2020 at 7:32 AM Christoph Hellwig [off-list ref] wrote:

quoted

index 855aa7cc9b8e..156880943c16 100644

--- a/arch/arm/include/asm/syscall.h
+++ b/arch/arm/include/asm/syscall.h

@@ -28,6 +28,17 @@ static inline int syscall_get_nr(struct task_struct *task,
      return task_thread_info(task)->syscall & ~__NR_OABI_SYSCALL_BASE;
 }

+static inline bool __in_oabi_syscall(struct task_struct *task)
+{
+     return IS_ENABLED(CONFIG_OABI_COMPAT) &&
+             (task_thread_info(task)->syscall & __NR_OABI_SYSCALL_BASE);
+}
+
+static inline bool in_oabi_syscall(void)
+{
+     return __in_oabi_syscall(current);
+}
+

Maybe split these infrastructure additions into a separate helper?

Sorry, I'm not following what you mean by this. Both of the above
are pretty minimal helpers already, in what way could they be split
further?

So after you argued for this variant I still have minor nitpicks:

I alway find positive ifdefs better where possible, e.g.

#if defined(CONFIG_ARM) && defined(CONFIG_OABI_COMPAT)
external declaration here
#else
the real thing
#endif

Ok.

but I still find the fact that the native case goes into the arch
helper a little weird.

Would you prefer something like this:

static inline struct epoll_event __user *
epoll_put_uevent(__poll_t revents, __u64 data,
                 struct epoll_event __user *uevent)
{
#if defined(CONFIG_ARM) && defined(CONFIG_OABI_COMPAT)
        /* ARM OABI has an incompatible struct layout and needs a
special handler */
        extern struct epoll_event __user *
        epoll_oabi_put_uevent(__poll_t revents, __u64 data,
                              struct epoll_event __user *uevent);

        if (in_oabi_syscall())
                return epoll_oabi_put_uevent(revents, data, uevent);
#endif
        if (__put_user(revents, &uevent->events) ||
            __put_user(data, &uevent->data))
                return NULL;

        return uevent+1;
}

That would keep the native case in one place, but also mix in
more architecture specific stuff into the common source location,
which again seems worse to me.

     Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel