On 5/29/2017 9:56 PM, Michael Ellerman wrote:
> Geert Uytterhoeven <[email protected]> writes:
>
>> Hi Michael,
>>
>> On Mon, May 29, 2017 at 2:07 PM, Michael Ellerman <[email protected]> wrote:
>>> Geert Uytterhoeven <[email protected]> writes:
>>>> On Tue, May 23, 2017 at 11:45 PM, Babu Moger <[email protected]> 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
On Thu, Jun 8, 2017 at 1:07 AM, Babu Moger <[email protected]> wrote:
>
> On 5/29/2017 9:56 PM, Michael Ellerman wrote:
>>
>> Geert Uytterhoeven <[email protected]> writes:
>>
>>> Hi Michael,
>>>
>>> On Mon, May 29, 2017 at 2:07 PM, Michael Ellerman <[email protected]>
>>> wrote:
>>>>
>>>> Geert Uytterhoeven <[email protected]> writes:
>>>>>
>>>>> On Tue, May 23, 2017 at 11:45 PM, Babu Moger <[email protected]>
>>>>> 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
From: Arnd Bergmann <[email protected]>
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?
On Thu, Jun 8, 2017 at 4:02 PM, David Miller <[email protected]> wrote:
> From: Arnd Bergmann <[email protected]>
> 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