Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1652468 > unrolled thread

Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7] arch/sparc: Define config parameter CPU_BIG_ENDIAN)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7] arch/sparc: Define config parameter CPU_BIG_ENDIAN) Michael Ellerman <mpe@ellerman.id.au> - 2017-05-29 14:10 +0200
    Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7]  arch/sparc: Define config parameter CPU_BIG_ENDIAN) Geert Uytterhoeven <geert@linux-m68k.org> - 2017-05-29 14:20 +0200
      Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7] arch/sparc: Define config parameter CPU_BIG_ENDIAN) Michael Ellerman <mpe@ellerman.id.au> - 2017-05-30 05:00 +0200
        Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7]  arch/sparc: Define config parameter CPU_BIG_ENDIAN) Babu Moger <babu.moger@oracle.com> - 2017-06-08 01:10 +0200
          Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7]  arch/sparc: Define config parameter CPU_BIG_ENDIAN) Arnd Bergmann <arnd@arndb.de> - 2017-06-08 10:10 +0200
            Re: CPU_BIG_ENDIAN in generic code David Miller <davem@davemloft.net> - 2017-06-08 16:10 +0200
              Re: CPU_BIG_ENDIAN in generic code Arnd Bergmann <arnd@arndb.de> - 2017-06-08 16:40 +0200

#1652468 — Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7] arch/sparc: Define config parameter CPU_BIG_ENDIAN)

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Tue, May 23, 2017 at 11:45 PM, Babu Moger <babu.moger@oracle.com> wrote:
>> Found this problem while enabling queued rwlock on SPARC.
>> The parameter CONFIG_CPU_BIG_ENDIAN is used to clear the
>> specific byte in qrwlock structure. Without this parameter,
>> we clear the wrong byte. Here is the code.
>>
>> static inline u8 *__qrwlock_write_byte(struct qrwlock *lock)
>>  {
>>         return (u8 *)lock + 3 * IS_BUILTIN(CONFIG_CPU_BIG_ENDIAN);
>>  }
>>
>> Define CPU_BIG_ENDIAN for SPARC to fix it.
>
>> --- a/arch/sparc/Kconfig
>> +++ b/arch/sparc/Kconfig
>> @@ -92,6 +92,10 @@ config ARCH_DEFCONFIG
>>  config ARCH_PROC_KCORE_TEXT
>>         def_bool y
>>
>> +config CPU_BIG_ENDIAN
>> +       bool
>> +       default y if SPARC
>
> Nice catch!
>
> Traditionally, CPU_BIG_ENDIAN and CPU_LITTLE_ENDIAN were defined only on
> architectures that may support both.  And it was checked in platform code
> and drivers only.
> Hence the symbol is lacking from most architectures. Heck, even
> architectures that support both may default to one endiannes, and declare
> only the symbol for the other endianness:

I guess there's a reason we can't use __BIG_ENDIAN__ / __LITTLE_ENDIAN__ ?

cheers

[toc] | [next] | [standalone]

#1652484 — Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7] arch/sparc: Define config parameter CPU_BIG_ENDIAN)

Hi Michael,

On Mon, May 29, 2017 at 2:07 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>> On Tue, May 23, 2017 at 11:45 PM, Babu Moger <babu.moger@oracle.com> wrote:
>>> Found this problem while enabling queued rwlock on SPARC.
>>> The parameter CONFIG_CPU_BIG_ENDIAN is used to clear the
>>> specific byte in qrwlock structure. Without this parameter,
>>> we clear the wrong byte. Here is the code.
>>>
>>> static inline u8 *__qrwlock_write_byte(struct qrwlock *lock)
>>>  {
>>>         return (u8 *)lock + 3 * IS_BUILTIN(CONFIG_CPU_BIG_ENDIAN);
>>>  }
>>>
>>> Define CPU_BIG_ENDIAN for SPARC to fix it.
>>
>>> --- a/arch/sparc/Kconfig
>>> +++ b/arch/sparc/Kconfig
>>> @@ -92,6 +92,10 @@ config ARCH_DEFCONFIG
>>>  config ARCH_PROC_KCORE_TEXT
>>>         def_bool y
>>>
>>> +config CPU_BIG_ENDIAN
>>> +       bool
>>> +       default y if SPARC
>>
>> Nice catch!
>>
>> Traditionally, CPU_BIG_ENDIAN and CPU_LITTLE_ENDIAN were defined only on
>> architectures that may support both.  And it was checked in platform code
>> and drivers only.
>> Hence the symbol is lacking from most architectures. Heck, even
>> architectures that support both may default to one endiannes, and declare
>> only the symbol for the other endianness:
>
> I guess there's a reason we can't use __BIG_ENDIAN__ / __LITTLE_ENDIAN__ ?

I (C/asm) code we can, in Kconfig we cannot.

So far we tried always doing that, but a few checks for the semi-existing
Kconfig symbol crept in in generic code. Those could be replaced by the __*__
variants, but consistently having the Kconfig symbols would be useful anyway
(e.g. to avoid building the broken-on-big-endian ISDN drivers).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1652784

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Hi Michael,
>
> On Mon, May 29, 2017 at 2:07 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>> On Tue, May 23, 2017 at 11:45 PM, Babu Moger <babu.moger@oracle.com> wrote:
>>>> Found this problem while enabling queued rwlock on SPARC.
>>>> The parameter CONFIG_CPU_BIG_ENDIAN is used to clear the
>>>> specific byte in qrwlock structure. Without this parameter,
>>>> we clear the wrong byte. Here is the code.
>>>>
>>>> static inline u8 *__qrwlock_write_byte(struct qrwlock *lock)
>>>>  {
>>>>         return (u8 *)lock + 3 * IS_BUILTIN(CONFIG_CPU_BIG_ENDIAN);
>>>>  }
>>>>
>>>> Define CPU_BIG_ENDIAN for SPARC to fix it.
>>>
>>>> --- a/arch/sparc/Kconfig
>>>> +++ b/arch/sparc/Kconfig
>>>> @@ -92,6 +92,10 @@ config ARCH_DEFCONFIG
>>>>  config ARCH_PROC_KCORE_TEXT
>>>>         def_bool y
>>>>
>>>> +config CPU_BIG_ENDIAN
>>>> +       bool
>>>> +       default y if SPARC
>>>
>>> Nice catch!
>>>
>>> Traditionally, CPU_BIG_ENDIAN and CPU_LITTLE_ENDIAN were defined only on
>>> architectures that may support both.  And it was checked in platform code
>>> and drivers only.
>>> Hence the symbol is lacking from most architectures. Heck, even
>>> architectures that support both may default to one endiannes, and declare
>>> only the symbol for the other endianness:
>>
>> I guess there's a reason we can't use __BIG_ENDIAN__ / __LITTLE_ENDIAN__ ?
>
> I (C/asm) code we can, in Kconfig we cannot.
>
> So far we tried always doing that, but a few checks for the semi-existing
> Kconfig symbol crept in in generic code. Those could be replaced by the __*__
> variants, but consistently having the Kconfig symbols would be useful anyway
> (e.g. to avoid building the broken-on-big-endian ISDN drivers).

Ah OK, the original mail was citing C code, but yeah I guess it would be
handy in Makefiles etc.

cheers

[toc] | [prev] | [next] | [standalone]

#1660327 — Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7] arch/sparc: Define config parameter CPU_BIG_ENDIAN)

On 5/29/2017 9:56 PM, Michael Ellerman wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>
>> Hi Michael,
>>
>> On Mon, May 29, 2017 at 2:07 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>>> On Tue, May 23, 2017 at 11:45 PM, Babu Moger <babu.moger@oracle.com> wrote:
>>>>> Found this problem while enabling queued rwlock on SPARC.
>>>>> The parameter CONFIG_CPU_BIG_ENDIAN is used to clear the
>>>>> specific byte in qrwlock structure. Without this parameter,
>>>>> we clear the wrong byte. Here is the code.
>>>>>
>>>>> static inline u8 *__qrwlock_write_byte(struct qrwlock *lock)
>>>>>   {
>>>>>          return (u8 *)lock + 3 * IS_BUILTIN(CONFIG_CPU_BIG_ENDIAN);
>>>>>   }
>>>>>
>>>>> Define CPU_BIG_ENDIAN for SPARC to fix it.
>>>>> --- a/arch/sparc/Kconfig
>>>>> +++ b/arch/sparc/Kconfig
>>>>> @@ -92,6 +92,10 @@ config ARCH_DEFCONFIG
>>>>>   config ARCH_PROC_KCORE_TEXT
>>>>>          def_bool y
>>>>>
>>>>> +config CPU_BIG_ENDIAN
>>>>> +       bool
>>>>> +       default y if SPARC
>>>> Nice catch!
>>>>
>>>> Traditionally, CPU_BIG_ENDIAN and CPU_LITTLE_ENDIAN were defined only on
>>>> architectures that may support both.  And it was checked in platform code
>>>> and drivers only.
>>>> Hence the symbol is lacking from most architectures. Heck, even
>>>> architectures that support both may default to one endiannes, and declare
>>>> only the symbol for the other endianness:
>>> I guess there's a reason we can't use __BIG_ENDIAN__ / __LITTLE_ENDIAN__ ?
>> I (C/asm) code we can, in Kconfig we cannot.
>>
>> So far we tried always doing that, but a few checks for the semi-existing
>> Kconfig symbol crept in in generic code. Those could be replaced by the __*__
>> variants, but consistently having the Kconfig symbols would be useful anyway
>> (e.g. to avoid building the broken-on-big-endian ISDN drivers).
> Ah OK, the original mail was citing C code, but yeah I guess it would be
> handy in Makefiles etc.

Thanks for all the responses.  I see couple of options here.

1. Fix the c code in include/asm-generic/qrwlock.h using ifdef 
__BIG_ENDIAN__
     This will fix the issue for us.

2. Define CPU_BIG_ENDIAN for all the missing fixed endian architectures. 
Because the problem is only for fixed big endian archs.

I prefer the option 1.  What do you guys think ?

> cheers

[toc] | [prev] | [next] | [standalone]

#1660904 — Re: CPU_BIG_ENDIAN in generic code (was: Re: [PATCH v3 3/7] arch/sparc: Define config parameter CPU_BIG_ENDIAN)

On Thu, Jun 8, 2017 at 1:07 AM, Babu Moger <babu.moger@oracle.com> wrote:
>
> On 5/29/2017 9:56 PM, Michael Ellerman wrote:
>>
>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>
>>> Hi Michael,
>>>
>>> On Mon, May 29, 2017 at 2:07 PM, Michael Ellerman <mpe@ellerman.id.au>
>>> wrote:
>>>>
>>>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>>>>
>>>>> On Tue, May 23, 2017 at 11:45 PM, Babu Moger <babu.moger@oracle.com>
>>>>> wrote:

>>>
>>> So far we tried always doing that, but a few checks for the semi-existing
>>> Kconfig symbol crept in in generic code. Those could be replaced by the
>>> __*__
>>> variants, but consistently having the Kconfig symbols would be useful
>>> anyway
>>> (e.g. to avoid building the broken-on-big-endian ISDN drivers).
>>
>> Ah OK, the original mail was citing C code, but yeah I guess it would be
>> handy in Makefiles etc.
>
>
> Thanks for all the responses.  I see couple of options here.
>
> 1. Fix the c code in include/asm-generic/qrwlock.h using ifdef
> __BIG_ENDIAN__
>     This will fix the issue for us.
>
> 2. Define CPU_BIG_ENDIAN for all the missing fixed endian architectures.
> Because the problem is only for fixed big endian archs.
>
> I prefer the option 1.  What do you guys think ?

I would prefer option 2. If we leave out CONFIG_CPU_LITTLE_ENDIAN,
then the patch becomes much easier than what we had discussed earlier,
and we just need to patch a few Kconfig files to add

config CPU_BIG_ENDIAN
       def_bool y

I would also suggest adding a sanity check like

diff --git a/include/linux/byteorder/big_endian.h
b/include/linux/byteorder/big_endian.h
index 392041475c72..18a1ab5b0260 100644
--- a/include/linux/byteorder/big_endian.h
+++ b/include/linux/byteorder/big_endian.h
@@ -3,5 +3,9 @@

 #include <uapi/linux/byteorder/big_endian.h>

+#ifndef CONFIG_CPU_BIG_ENDIAN
+#warning inconsistent configuration, need CONFIG_CPU_BIG_ENDIAN
+#endif
+
 #include <linux/byteorder/generic.h>
 #endif /* _LINUX_BYTEORDER_BIG_ENDIAN_H */
diff --git a/include/linux/byteorder/little_endian.h
b/include/linux/byteorder/little_endian.h
index 08057377aa23..ba910bb9aad0 100644
--- a/include/linux/byteorder/little_endian.h
+++ b/include/linux/byteorder/little_endian.h
@@ -3,5 +3,9 @@

 #include <uapi/linux/byteorder/little_endian.h>

+#ifdef CONFIG_CPU_BIG_ENDIAN
+#warning inconsistent configuration, CONFIG_CPU_BIG_ENDIAN is set
+#endif
+
 #include <linux/byteorder/generic.h>
 #endif /* _LINUX_BYTEORDER_LITTLE_ENDIAN_H */

Fixing xtensa properly might still be tricky, but with that change, at least
we detect when things go wrong in this area.

        Arnd

[toc] | [prev] | [next] | [standalone]

#1661310 — Re: CPU_BIG_ENDIAN in generic code

From: Arnd Bergmann <arnd@arndb.de>
Date: Thu, 8 Jun 2017 10:01:59 +0200

> I would also suggest adding a sanity check like

Hmm, but this will kill the build for non-fixed endian architectures
won't it?

[toc] | [prev] | [next] | [standalone]

#1661335 — Re: CPU_BIG_ENDIAN in generic code

On Thu, Jun 8, 2017 at 4:02 PM, David Miller <davem@davemloft.net> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Thu, 8 Jun 2017 10:01:59 +0200
>
>> I would also suggest adding a sanity check like
>
> Hmm, but this will kill the build for non-fixed endian architectures
> won't it?

I think only xtensa, all others already define CONFIG_CPU_BIG_ENDIAN
conditionally, and include the right header depending on that.

For xtensa, the decision is apparently made by the toolchain, and the
kernel just detects the macros set by the compiler, but that is slightly
fragile because it prevents us from making Kconfig decisions based on
endianess.

       Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web