[PATCH v2] m68k: Avoid CONFIG_COLDFIRE switch in uapi header

[PATCH v2] m68k: Avoid CONFIG_COLDFIRE switch in uapi header

[Thomas Huth]

We should not use any CONFIG switches in uapi headers since these
only work during kernel compilation; they are not defined for
userspace. Fix it by moving the struct pt_regs to the kernel-internal
header instead - struct pt_regs does not seem to be required for
the userspace headers on m68k at all.

Suggested-by: Greg Ungerer <gerg@linux-m68k.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2: Move the struct instead of changing the #ifdef

 See previous discussion here:
 https://lore.kernel.org/lkml/6e3f2a2e-2430-4b4f-9ead-d9a4d5e42713@linux-m68k.org/

 arch/m68k/include/asm/ptrace.h      | 29 +++++++++++++++++++++++++++++
 arch/m68k/include/uapi/asm/ptrace.h | 28 ----------------------------
 scripts/headers_install.sh          |  1 -
 3 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/arch/m68k/include/asm/ptrace.h b/arch/m68k/include/asm/ptrace.h
index ea5a80ca1ab33..f200712946603 100644
--- a/arch/m68k/include/asm/ptrace.h
+++ b/arch/m68k/include/asm/ptrace.h
@@ -6,6 +6,35 @@
 
 #ifndef __ASSEMBLY__
 
+/*
+ * This struct defines the way the registers are stored on the
+ * stack during a system call.
+ */
+struct pt_regs {
+	long     d1;
+	long     d2;
+	long     d3;
+	long     d4;
+	long     d5;
+	long     a0;
+	long     a1;
+	long     a2;
+	long     d0;
+	long     orig_d0;
+	long     stkadj;
+#ifdef CONFIG_COLDFIRE
+	unsigned format :  4; /* frame format specifier */
+	unsigned vector : 12; /* vector offset */
+	unsigned short sr;
+	unsigned long  pc;
+#else
+	unsigned short sr;
+	unsigned long  pc;
+	unsigned format :  4; /* frame format specifier */
+	unsigned vector : 12; /* vector offset */
+#endif
+};
+
 #ifndef PS_S
 #define PS_S  (0x2000)
 #define PS_M  (0x1000)
diff --git a/arch/m68k/include/uapi/asm/ptrace.h b/arch/m68k/include/uapi/asm/ptrace.h
index 5b50ea592e002..a83bfe36dd10a 100644
--- a/arch/m68k/include/uapi/asm/ptrace.h
+++ b/arch/m68k/include/uapi/asm/ptrace.h
@@ -24,34 +24,6 @@
 
 #ifndef __ASSEMBLY__
 
-/* this struct defines the way the registers are stored on the
-   stack during a system call. */
-
-struct pt_regs {
-  long     d1;
-  long     d2;
-  long     d3;
-  long     d4;
-  long     d5;
-  long     a0;
-  long     a1;
-  long     a2;
-  long     d0;
-  long     orig_d0;
-  long     stkadj;
-#ifdef CONFIG_COLDFIRE
-  unsigned format :  4; /* frame format specifier */
-  unsigned vector : 12; /* vector offset */
-  unsigned short sr;
-  unsigned long  pc;
-#else
-  unsigned short sr;
-  unsigned long  pc;
-  unsigned format :  4; /* frame format specifier */
-  unsigned vector : 12; /* vector offset */
-#endif
-};
-
 /*
  * This is the extended stack used by signal handlers and the context
  * switcher: it's pushed after the normal "struct pt_regs".
diff --git a/scripts/headers_install.sh b/scripts/headers_install.sh
index f7d9b114de8f7..6bbccb43f7e72 100755
--- a/scripts/headers_install.sh
+++ b/scripts/headers_install.sh
@@ -74,7 +74,6 @@ arch/arc/include/uapi/asm/page.h:CONFIG_ARC_PAGE_SIZE_16K
 arch/arc/include/uapi/asm/page.h:CONFIG_ARC_PAGE_SIZE_4K
 arch/arc/include/uapi/asm/swab.h:CONFIG_ARC_HAS_SWAPE
 arch/arm/include/uapi/asm/ptrace.h:CONFIG_CPU_ENDIAN_BE8
-arch/m68k/include/uapi/asm/ptrace.h:CONFIG_COLDFIRE
 arch/nios2/include/uapi/asm/swab.h:CONFIG_NIOS2_CI_SWAB_NO
 arch/nios2/include/uapi/asm/swab.h:CONFIG_NIOS2_CI_SWAB_SUPPORT
 arch/x86/include/uapi/asm/auxvec.h:CONFIG_IA32_EMULATION
-- 
2.43.2

Re: [PATCH v2] m68k: Avoid CONFIG_COLDFIRE switch in uapi header

[Greg Ungerer]

On 20/2/24 02:01, Thomas Huth wrote:
> We should not use any CONFIG switches in uapi headers since these
> only work during kernel compilation; they are not defined for
> userspace. Fix it by moving the struct pt_regs to the kernel-internal
> header instead - struct pt_regs does not seem to be required for
> the userspace headers on m68k at all.
> 
> Suggested-by: Greg Ungerer <gerg@linux-m68k.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   v2: Move the struct instead of changing the #ifdef
> 
>   See previous discussion here:
>   https://lore.kernel.org/lkml/6e3f2a2e-2430-4b4f-9ead-d9a4d5e42713@linux-m68k.org/

I am fine with this. FWIW the following architectures do
not define pt_regs in their uapi/ptrace.h header either:
arc, arm64, loongarch, nios2, openrisc, riscv, s390, xtensa
Though quite a few of them have a user_pt_regs instead.

So for me:

Acked-by: Greg Ungerer <gerg@linux-m68k.org>

Geert, Arnd, do you have any thoughts on this?

Regards
Greg



>   arch/m68k/include/asm/ptrace.h      | 29 +++++++++++++++++++++++++++++
>   arch/m68k/include/uapi/asm/ptrace.h | 28 ----------------------------
>   scripts/headers_install.sh          |  1 -
>   3 files changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/m68k/include/asm/ptrace.h b/arch/m68k/include/asm/ptrace.h
> index ea5a80ca1ab33..f200712946603 100644
> --- a/arch/m68k/include/asm/ptrace.h
> +++ b/arch/m68k/include/asm/ptrace.h
> @@ -6,6 +6,35 @@
>   
>   #ifndef __ASSEMBLY__
>   
> +/*
> + * This struct defines the way the registers are stored on the
> + * stack during a system call.
> + */
> +struct pt_regs {
> +	long     d1;
> +	long     d2;
> +	long     d3;
> +	long     d4;
> +	long     d5;
> +	long     a0;
> +	long     a1;
> +	long     a2;
> +	long     d0;
> +	long     orig_d0;
> +	long     stkadj;
> +#ifdef CONFIG_COLDFIRE
> +	unsigned format :  4; /* frame format specifier */
> +	unsigned vector : 12; /* vector offset */
> +	unsigned short sr;
> +	unsigned long  pc;
> +#else
> +	unsigned short sr;
> +	unsigned long  pc;
> +	unsigned format :  4; /* frame format specifier */
> +	unsigned vector : 12; /* vector offset */
> +#endif
> +};
> +
>   #ifndef PS_S
>   #define PS_S  (0x2000)
>   #define PS_M  (0x1000)
> diff --git a/arch/m68k/include/uapi/asm/ptrace.h b/arch/m68k/include/uapi/asm/ptrace.h
> index 5b50ea592e002..a83bfe36dd10a 100644
> --- a/arch/m68k/include/uapi/asm/ptrace.h
> +++ b/arch/m68k/include/uapi/asm/ptrace.h
> @@ -24,34 +24,6 @@
>   
>   #ifndef __ASSEMBLY__
>   
> -/* this struct defines the way the registers are stored on the
> -   stack during a system call. */
> -
> -struct pt_regs {
> -  long     d1;
> -  long     d2;
> -  long     d3;
> -  long     d4;
> -  long     d5;
> -  long     a0;
> -  long     a1;
> -  long     a2;
> -  long     d0;
> -  long     orig_d0;
> -  long     stkadj;
> -#ifdef CONFIG_COLDFIRE
> -  unsigned format :  4; /* frame format specifier */
> -  unsigned vector : 12; /* vector offset */
> -  unsigned short sr;
> -  unsigned long  pc;
> -#else
> -  unsigned short sr;
> -  unsigned long  pc;
> -  unsigned format :  4; /* frame format specifier */
> -  unsigned vector : 12; /* vector offset */
> -#endif
> -};
> -
>   /*
>    * This is the extended stack used by signal handlers and the context
>    * switcher: it's pushed after the normal "struct pt_regs".
> diff --git a/scripts/headers_install.sh b/scripts/headers_install.sh
> index f7d9b114de8f7..6bbccb43f7e72 100755
> --- a/scripts/headers_install.sh
> +++ b/scripts/headers_install.sh
> @@ -74,7 +74,6 @@ arch/arc/include/uapi/asm/page.h:CONFIG_ARC_PAGE_SIZE_16K
>   arch/arc/include/uapi/asm/page.h:CONFIG_ARC_PAGE_SIZE_4K
>   arch/arc/include/uapi/asm/swab.h:CONFIG_ARC_HAS_SWAPE
>   arch/arm/include/uapi/asm/ptrace.h:CONFIG_CPU_ENDIAN_BE8
> -arch/m68k/include/uapi/asm/ptrace.h:CONFIG_COLDFIRE
>   arch/nios2/include/uapi/asm/swab.h:CONFIG_NIOS2_CI_SWAB_NO
>   arch/nios2/include/uapi/asm/swab.h:CONFIG_NIOS2_CI_SWAB_SUPPORT
>   arch/x86/include/uapi/asm/auxvec.h:CONFIG_IA32_EMULATION

Re: [PATCH v2] m68k: Avoid CONFIG_COLDFIRE switch in uapi header

[Arnd Bergmann]

On Tue, Feb 20, 2024, at 15:13, Greg Ungerer wrote:
> On 20/2/24 02:01, Thomas Huth wrote:
>> We should not use any CONFIG switches in uapi headers since these
>> only work during kernel compilation; they are not defined for
>> userspace. Fix it by moving the struct pt_regs to the kernel-internal
>> header instead - struct pt_regs does not seem to be required for
>> the userspace headers on m68k at all.
>> 
>> Suggested-by: Greg Ungerer <gerg@linux-m68k.org>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   v2: Move the struct instead of changing the #ifdef
>> 
>>   See previous discussion here:
>>   https://lore.kernel.org/lkml/6e3f2a2e-2430-4b4f-9ead-d9a4d5e42713@linux-m68k.org/
>
> I am fine with this. FWIW the following architectures do
> not define pt_regs in their uapi/ptrace.h header either:
> arc, arm64, loongarch, nios2, openrisc, riscv, s390, xtensa
> Though quite a few of them have a user_pt_regs instead.
>
> So for me:
>
> Acked-by: Greg Ungerer <gerg@linux-m68k.org>
>
> Geert, Arnd, do you have any thoughts on this?

It clearly doesn't change the ABI, so that part is fine.

If asm/ptrace.h is included by some userspace tool to
get the definition, it might cause a compile-time error
that needs a trivial source change.

This could be needed for ptrace (gdb, strace) or signal
handling and setjmp (libc), though it's more likely that these
already have their own copies.

     Arnd

Re: [PATCH v2] m68k: Avoid CONFIG_COLDFIRE switch in uapi header

[Thomas Huth]

On 20/02/2024 16.09, Arnd Bergmann wrote:
> On Tue, Feb 20, 2024, at 15:13, Greg Ungerer wrote:
>> On 20/2/24 02:01, Thomas Huth wrote:
>>> We should not use any CONFIG switches in uapi headers since these
>>> only work during kernel compilation; they are not defined for
>>> userspace. Fix it by moving the struct pt_regs to the kernel-internal
>>> header instead - struct pt_regs does not seem to be required for
>>> the userspace headers on m68k at all.
>>>
>>> Suggested-by: Greg Ungerer <gerg@linux-m68k.org>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>    v2: Move the struct instead of changing the #ifdef
>>>
>>>    See previous discussion here:
>>>    https://lore.kernel.org/lkml/6e3f2a2e-2430-4b4f-9ead-d9a4d5e42713@linux-m68k.org/
>>
>> I am fine with this. FWIW the following architectures do
>> not define pt_regs in their uapi/ptrace.h header either:
>> arc, arm64, loongarch, nios2, openrisc, riscv, s390, xtensa
>> Though quite a few of them have a user_pt_regs instead.
>>
>> So for me:
>>
>> Acked-by: Greg Ungerer <gerg@linux-m68k.org>
>>
>> Geert, Arnd, do you have any thoughts on this?
> 
> It clearly doesn't change the ABI, so that part is fine.
> 
> If asm/ptrace.h is included by some userspace tool to
> get the definition, it might cause a compile-time error
> that needs a trivial source change.
> 
> This could be needed for ptrace (gdb, strace) or signal
> handling and setjmp (libc), though it's more likely that these
> already have their own copies.

If we still feel unsure, we should maybe rather go with v1:

  https://lore.kernel.org/lkml/20231110103120.387517-1-thuth@redhat.com/

?

  Thomas

Re: [PATCH v2] m68k: Avoid CONFIG_COLDFIRE switch in uapi header

[Greg Ungerer]

Hi Thomas,

On 23/2/24 18:13, Thomas Huth wrote:
> On 20/02/2024 16.09, Arnd Bergmann wrote:
>> On Tue, Feb 20, 2024, at 15:13, Greg Ungerer wrote:
>>> On 20/2/24 02:01, Thomas Huth wrote:
>>>> We should not use any CONFIG switches in uapi headers since these
>>>> only work during kernel compilation; they are not defined for
>>>> userspace. Fix it by moving the struct pt_regs to the kernel-internal
>>>> header instead - struct pt_regs does not seem to be required for
>>>> the userspace headers on m68k at all.
>>>>
>>>> Suggested-by: Greg Ungerer <gerg@linux-m68k.org>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    v2: Move the struct instead of changing the #ifdef
>>>>
>>>>    See previous discussion here:
>>>>    https://lore.kernel.org/lkml/6e3f2a2e-2430-4b4f-9ead-d9a4d5e42713@linux-m68k.org/
>>>
>>> I am fine with this. FWIW the following architectures do
>>> not define pt_regs in their uapi/ptrace.h header either:
>>> arc, arm64, loongarch, nios2, openrisc, riscv, s390, xtensa
>>> Though quite a few of them have a user_pt_regs instead.
>>>
>>> So for me:
>>>
>>> Acked-by: Greg Ungerer <gerg@linux-m68k.org>
>>>
>>> Geert, Arnd, do you have any thoughts on this?
>>
>> It clearly doesn't change the ABI, so that part is fine.
>>
>> If asm/ptrace.h is included by some userspace tool to
>> get the definition, it might cause a compile-time error
>> that needs a trivial source change.
>>
>> This could be needed for ptrace (gdb, strace) or signal
>> handling and setjmp (libc), though it's more likely that these
>> already have their own copies.
> 
> If we still feel unsure, we should maybe rather go with v1:
> 
>   https://lore.kernel.org/lkml/20231110103120.387517-1-thuth@redhat.com/
> 
> ?

We have not had much movement on this.
So... I am confidant that v2 is good, but lets err on the side of caution first up.
I have applied v1 to the m68knommu git tree, for-next branch.

Thanks
Greg