2012-05-29 21:33:54

by Geert Uytterhoeven

[permalink] [raw]
Subject: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

Signed-off-by: Geert Uytterhoeven <[email protected]>
---
Do we also want

select HAVE_EFFICIENT_UNALIGNED_ACCESS if (!COLDFIRE && !M68000)

???

arch/m68k/Kconfig | 2 +
arch/m68k/include/asm/Kbuild | 2 +
arch/m68k/include/asm/uaccess_mm.h | 11 +++--
arch/m68k/lib/uaccess.c | 74 ------------------------------------
4 files changed, 11 insertions(+), 78 deletions(-)

diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index cac5b6b..1471201 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -7,6 +7,8 @@ config M68K
select GENERIC_IRQ_SHOW
select ARCH_HAVE_NMI_SAFE_CMPXCHG if RMW_INSNS
select GENERIC_CPU_DEVICES
+ select GENERIC_STRNCPY_FROM_USER if MMU
+ select GENERIC_STRNLEN_USER if MMU
select FPU if MMU
select ARCH_USES_GETTIMEOFFSET if MMU && !COLDFIRE

diff --git a/arch/m68k/include/asm/Kbuild b/arch/m68k/include/asm/Kbuild
index 1a922fa..eafa253 100644
--- a/arch/m68k/include/asm/Kbuild
+++ b/arch/m68k/include/asm/Kbuild
@@ -1,2 +1,4 @@
include include/asm-generic/Kbuild.asm
header-y += cachectl.h
+
+generic-y += word-at-a-time.h
diff --git a/arch/m68k/include/asm/uaccess_mm.h b/arch/m68k/include/asm/uaccess_mm.h
index 9c80cd5..472c891 100644
--- a/arch/m68k/include/asm/uaccess_mm.h
+++ b/arch/m68k/include/asm/uaccess_mm.h
@@ -379,12 +379,15 @@ __constant_copy_to_user(void __user *to, const void *from, unsigned long n)
#define copy_from_user(to, from, n) __copy_from_user(to, from, n)
#define copy_to_user(to, from, n) __copy_to_user(to, from, n)

-long strncpy_from_user(char *dst, const char __user *src, long count);
-long strnlen_user(const char __user *src, long n);
+#define user_addr_max() \
+ (segment_eq(get_fs(), USER_DS) ? TASK_SIZE : ~0UL)
+
+extern long strncpy_from_user(char *dst, const char __user *src, long count);
+extern __must_check long strlen_user(const char __user *str);
+extern __must_check long strnlen_user(const char __user *str, long n);
+
unsigned long __clear_user(void __user *to, unsigned long n);

#define clear_user __clear_user

-#define strlen_user(str) strnlen_user(str, 32767)
-
#endif /* _M68K_UACCESS_H */
diff --git a/arch/m68k/lib/uaccess.c b/arch/m68k/lib/uaccess.c
index 5664386..5e97f2e 100644
--- a/arch/m68k/lib/uaccess.c
+++ b/arch/m68k/lib/uaccess.c
@@ -104,80 +104,6 @@ unsigned long __generic_copy_to_user(void __user *to, const void *from,
EXPORT_SYMBOL(__generic_copy_to_user);

/*
- * Copy a null terminated string from userspace.
- */
-long strncpy_from_user(char *dst, const char __user *src, long count)
-{
- long res;
- char c;
-
- if (count <= 0)
- return count;
-
- asm volatile ("\n"
- "1: "MOVES".b (%2)+,%4\n"
- " move.b %4,(%1)+\n"
- " jeq 2f\n"
- " subq.l #1,%3\n"
- " jne 1b\n"
- "2: sub.l %3,%0\n"
- "3:\n"
- " .section .fixup,\"ax\"\n"
- " .even\n"
- "10: move.l %5,%0\n"
- " jra 3b\n"
- " .previous\n"
- "\n"
- " .section __ex_table,\"a\"\n"
- " .align 4\n"
- " .long 1b,10b\n"
- " .previous"
- : "=d" (res), "+a" (dst), "+a" (src), "+r" (count), "=&d" (c)
- : "i" (-EFAULT), "0" (count));
-
- return res;
-}
-EXPORT_SYMBOL(strncpy_from_user);
-
-/*
- * Return the size of a string (including the ending 0)
- *
- * Return 0 on exception, a value greater than N if too long
- */
-long strnlen_user(const char __user *src, long n)
-{
- char c;
- long res;
-
- asm volatile ("\n"
- "1: subq.l #1,%1\n"
- " jmi 3f\n"
- "2: "MOVES".b (%0)+,%2\n"
- " tst.b %2\n"
- " jne 1b\n"
- " jra 4f\n"
- "\n"
- "3: addq.l #1,%0\n"
- "4: sub.l %4,%0\n"
- "5:\n"
- " .section .fixup,\"ax\"\n"
- " .even\n"
- "20: sub.l %0,%0\n"
- " jra 5b\n"
- " .previous\n"
- "\n"
- " .section __ex_table,\"a\"\n"
- " .align 4\n"
- " .long 2b,20b\n"
- " .previous\n"
- : "=&a" (res), "+d" (n), "=&d" (c)
- : "0" (src), "r" (src));
-
- return res;
-}
-EXPORT_SYMBOL(strnlen_user);
-
-/*
* Zero Userspace
*/

--
1.7.0.4


2012-05-30 03:08:15

by Greg Ungerer

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

Hi Geert,

On 30/05/12 07:33, Geert Uytterhoeven wrote:
> Signed-off-by: Geert Uytterhoeven<[email protected]>

Looks good:

Acked-by: Greg Ungerer <[email protected]>


> ---
> Do we also want
>
> select HAVE_EFFICIENT_UNALIGNED_ACCESS if (!COLDFIRE&& !M68000)

Maybe yes.

Regards
Greg


> arch/m68k/Kconfig | 2 +
> arch/m68k/include/asm/Kbuild | 2 +
> arch/m68k/include/asm/uaccess_mm.h | 11 +++--
> arch/m68k/lib/uaccess.c | 74 ------------------------------------
> 4 files changed, 11 insertions(+), 78 deletions(-)
>
> diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
> index cac5b6b..1471201 100644
> --- a/arch/m68k/Kconfig
> +++ b/arch/m68k/Kconfig
> @@ -7,6 +7,8 @@ config M68K
> select GENERIC_IRQ_SHOW
> select ARCH_HAVE_NMI_SAFE_CMPXCHG if RMW_INSNS
> select GENERIC_CPU_DEVICES
> + select GENERIC_STRNCPY_FROM_USER if MMU
> + select GENERIC_STRNLEN_USER if MMU
> select FPU if MMU
> select ARCH_USES_GETTIMEOFFSET if MMU&& !COLDFIRE
>
> diff --git a/arch/m68k/include/asm/Kbuild b/arch/m68k/include/asm/Kbuild
> index 1a922fa..eafa253 100644
> --- a/arch/m68k/include/asm/Kbuild
> +++ b/arch/m68k/include/asm/Kbuild
> @@ -1,2 +1,4 @@
> include include/asm-generic/Kbuild.asm
> header-y += cachectl.h
> +
> +generic-y += word-at-a-time.h
> diff --git a/arch/m68k/include/asm/uaccess_mm.h b/arch/m68k/include/asm/uaccess_mm.h
> index 9c80cd5..472c891 100644
> --- a/arch/m68k/include/asm/uaccess_mm.h
> +++ b/arch/m68k/include/asm/uaccess_mm.h
> @@ -379,12 +379,15 @@ __constant_copy_to_user(void __user *to, const void *from, unsigned long n)
> #define copy_from_user(to, from, n) __copy_from_user(to, from, n)
> #define copy_to_user(to, from, n) __copy_to_user(to, from, n)
>
> -long strncpy_from_user(char *dst, const char __user *src, long count);
> -long strnlen_user(const char __user *src, long n);
> +#define user_addr_max() \
> + (segment_eq(get_fs(), USER_DS) ? TASK_SIZE : ~0UL)
> +
> +extern long strncpy_from_user(char *dst, const char __user *src, long count);
> +extern __must_check long strlen_user(const char __user *str);
> +extern __must_check long strnlen_user(const char __user *str, long n);
> +
> unsigned long __clear_user(void __user *to, unsigned long n);
>
> #define clear_user __clear_user
>
> -#define strlen_user(str) strnlen_user(str, 32767)
> -
> #endif /* _M68K_UACCESS_H */
> diff --git a/arch/m68k/lib/uaccess.c b/arch/m68k/lib/uaccess.c
> index 5664386..5e97f2e 100644
> --- a/arch/m68k/lib/uaccess.c
> +++ b/arch/m68k/lib/uaccess.c
> @@ -104,80 +104,6 @@ unsigned long __generic_copy_to_user(void __user *to, const void *from,
> EXPORT_SYMBOL(__generic_copy_to_user);
>
> /*
> - * Copy a null terminated string from userspace.
> - */
> -long strncpy_from_user(char *dst, const char __user *src, long count)
> -{
> - long res;
> - char c;
> -
> - if (count<= 0)
> - return count;
> -
> - asm volatile ("\n"
> - "1: "MOVES".b (%2)+,%4\n"
> - " move.b %4,(%1)+\n"
> - " jeq 2f\n"
> - " subq.l #1,%3\n"
> - " jne 1b\n"
> - "2: sub.l %3,%0\n"
> - "3:\n"
> - " .section .fixup,\"ax\"\n"
> - " .even\n"
> - "10: move.l %5,%0\n"
> - " jra 3b\n"
> - " .previous\n"
> - "\n"
> - " .section __ex_table,\"a\"\n"
> - " .align 4\n"
> - " .long 1b,10b\n"
> - " .previous"
> - : "=d" (res), "+a" (dst), "+a" (src), "+r" (count), "=&d" (c)
> - : "i" (-EFAULT), "0" (count));
> -
> - return res;
> -}
> -EXPORT_SYMBOL(strncpy_from_user);
> -
> -/*
> - * Return the size of a string (including the ending 0)
> - *
> - * Return 0 on exception, a value greater than N if too long
> - */
> -long strnlen_user(const char __user *src, long n)
> -{
> - char c;
> - long res;
> -
> - asm volatile ("\n"
> - "1: subq.l #1,%1\n"
> - " jmi 3f\n"
> - "2: "MOVES".b (%0)+,%2\n"
> - " tst.b %2\n"
> - " jne 1b\n"
> - " jra 4f\n"
> - "\n"
> - "3: addq.l #1,%0\n"
> - "4: sub.l %4,%0\n"
> - "5:\n"
> - " .section .fixup,\"ax\"\n"
> - " .even\n"
> - "20: sub.l %0,%0\n"
> - " jra 5b\n"
> - " .previous\n"
> - "\n"
> - " .section __ex_table,\"a\"\n"
> - " .align 4\n"
> - " .long 2b,20b\n"
> - " .previous\n"
> - : "=&a" (res), "+d" (n), "=&d" (c)
> - : "0" (src), "r" (src));
> -
> - return res;
> -}
> -EXPORT_SYMBOL(strnlen_user);
> -
> -/*
> * Zero Userspace
> */
>


--
------------------------------------------------------------------------
Greg Ungerer -- Principal Engineer EMAIL: [email protected]
SnapGear Group, McAfee PHONE: +61 7 3435 2888
8 Gardner Close FAX: +61 7 3217 5323
Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com

2012-05-30 10:22:32

by Philippe De Muyter

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

On Tue, May 29, 2012 at 11:33:36PM +0200, Geert Uytterhoeven wrote:
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> ---
> Do we also want
>
> select HAVE_EFFICIENT_UNALIGNED_ACCESS if (!COLDFIRE && !M68000)

Sorry, I did not follow what happened to unaligned accesses, but
CPU32 family (at least 68340) crashes on unaligned accesses.

Philippe

2012-05-30 11:20:06

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

On Wed, May 30, 2012 at 12:22 PM, Philippe De Muyter <[email protected]> wrote:
> On Tue, May 29, 2012 at 11:33:36PM +0200, Geert Uytterhoeven wrote:
>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> ---
>> Do we also want
>>
>>     select HAVE_EFFICIENT_UNALIGNED_ACCESS if (!COLDFIRE && !M68000)
>
> Sorry, I did not follow what happened to unaligned accesses, but
> CPU32 family (at least 68340) crashes on unaligned accesses.

We don't seem to have CONFIG_M68340 in arch/m68k/Kconfig.cpu?
But Freescale's website confirms both 68340 and 68360 are CPU32.

arch/m68k/include/asm/unaligned.h assumes CPU32 (CONFIG_MCPU32)
can do unaligned accesses:

#if defined(CONFIG_COLDFIRE) || defined(CONFIG_M68000)
#include <linux/unaligned/be_struct.h>
#include <linux/unaligned/le_byteshift.h>
#include <linux/unaligned/generic.h>

#define get_unaligned __get_unaligned_be
#define put_unaligned __put_unaligned_be

#else
/*
* The m68k can do unaligned accesses itself.
*/
#include <linux/unaligned/access_ok.h>
#include <linux/unaligned/generic.h>

#define get_unaligned __get_unaligned_be
#define put_unaligned __put_unaligned_be

#endif

Is this wrong?

However, for CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS,
the question is not whether unaligned accesses are supported, but
whether they are more efficient than byte copies when copying larger blocks.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2012-05-30 14:05:24

by Philippe De Muyter

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

On Wed, May 30, 2012 at 01:20:02PM +0200, Geert Uytterhoeven wrote:
> On Wed, May 30, 2012 at 12:22 PM, Philippe De Muyter <[email protected]> wrote:
> > On Tue, May 29, 2012 at 11:33:36PM +0200, Geert Uytterhoeven wrote:
> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
> >> ---
> >> Do we also want
> >>
> >> ? ? select HAVE_EFFICIENT_UNALIGNED_ACCESS if (!COLDFIRE && !M68000)
> >
> > Sorry, I did not follow what happened to unaligned accesses, but
> > CPU32 family (at least 68340) crashes on unaligned accesses.
>
> We don't seem to have CONFIG_M68340 in arch/m68k/Kconfig.cpu?

I have a local port here (but based on an ancient linux kernel, 2.6.2 IIRC)

> But Freescale's website confirms both 68340 and 68360 are CPU32.
>
> arch/m68k/include/asm/unaligned.h assumes CPU32 (CONFIG_MCPU32)
> can do unaligned accesses:

That's not true. Accessing a 16- or 32-bit word at an odd address
with a 68340 generates an Address Error Exception. I remember
discovering a bug in the ppp kernel code because of that.

>
> #if defined(CONFIG_COLDFIRE) || defined(CONFIG_M68000)
> #include <linux/unaligned/be_struct.h>
> #include <linux/unaligned/le_byteshift.h>
> #include <linux/unaligned/generic.h>
>
> #define get_unaligned __get_unaligned_be
> #define put_unaligned __put_unaligned_be
>
> #else
> /*
> * The m68k can do unaligned accesses itself.
> */
> #include <linux/unaligned/access_ok.h>
> #include <linux/unaligned/generic.h>
>
> #define get_unaligned __get_unaligned_be
> #define put_unaligned __put_unaligned_be
>
> #endif
>
> Is this wrong?

I can't tell from reading just the lines above, but I think one should add
"|| defined(CONFIG_MCPU32)" at the end of the if condition.

I also think that the Coldfire 5272 can do unaligned accesses, but I
cannot test that at the moment.

>
> However, for CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS,
> the question is not whether unaligned accesses are supported, but
> whether they are more efficient than byte copies when copying larger blocks.

OK, thanks

Philippe

--
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

2012-05-30 14:43:11

by Andreas Schwab

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

Geert Uytterhoeven <[email protected]> writes:

> On Wed, May 30, 2012 at 12:22 PM, Philippe De Muyter <[email protected]> wrote:
>> On Tue, May 29, 2012 at 11:33:36PM +0200, Geert Uytterhoeven wrote:
>>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>>> ---
>>> Do we also want
>>>
>>>     select HAVE_EFFICIENT_UNALIGNED_ACCESS if (!COLDFIRE && !M68000)
>>
>> Sorry, I did not follow what happened to unaligned accesses, but
>> CPU32 family (at least 68340) crashes on unaligned accesses.
>
> We don't seem to have CONFIG_M68340 in arch/m68k/Kconfig.cpu?

But we have CONFIG_MCPU32.

Andreas.

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2012-06-05 20:57:33

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

On Wed, May 30, 2012 at 4:04 PM, Philippe De Muyter <[email protected]> wrote:
> On Wed, May 30, 2012 at 01:20:02PM +0200, Geert Uytterhoeven wrote:
>> On Wed, May 30, 2012 at 12:22 PM, Philippe De Muyter <[email protected]> wrote:
>> > On Tue, May 29, 2012 at 11:33:36PM +0200, Geert Uytterhoeven wrote:
>> >> Signed-off-by: Geert Uytterhoeven <[email protected]>
>> >> ---
>> >> Do we also want
>> >>
>> >>     select HAVE_EFFICIENT_UNALIGNED_ACCESS if (!COLDFIRE && !M68000)
>> >
>> > Sorry, I did not follow what happened to unaligned accesses, but
>> > CPU32 family (at least 68340) crashes on unaligned accesses.
>>
>> We don't seem to have CONFIG_M68340 in arch/m68k/Kconfig.cpu?
>
> I have a local port here (but based on an ancient linux kernel, 2.6.2 IIRC)
>
>> But Freescale's website confirms both 68340 and 68360 are CPU32.
>>
>> arch/m68k/include/asm/unaligned.h assumes CPU32 (CONFIG_MCPU32)
>> can do unaligned accesses:
>
> That's not true.  Accessing a 16- or 32-bit word at an odd address
> with a 68340 generates an Address Error Exception.  I remember
> discovering a bug in the ppp kernel code because of that.
>
>>
>> #if defined(CONFIG_COLDFIRE) || defined(CONFIG_M68000)
>> #include <linux/unaligned/be_struct.h>
>> #include <linux/unaligned/le_byteshift.h>
>> #include <linux/unaligned/generic.h>
>>
>> #define get_unaligned   __get_unaligned_be
>> #define put_unaligned   __put_unaligned_be
>>
>> #else
>> /*
>>  * The m68k can do unaligned accesses itself.
>>  */
>> #include <linux/unaligned/access_ok.h>
>> #include <linux/unaligned/generic.h>
>>
>> #define get_unaligned   __get_unaligned_be
>> #define put_unaligned   __put_unaligned_be
>>
>> #endif
>>
>> Is this wrong?
>
> I can't tell from reading just the lines above, but I think one should add
> "|| defined(CONFIG_MCPU32)" at the end of the if condition.

Greg?

If more CPUs cannot handle unaligned accesses, I propose to add
CONFIG_CPU_HAS_NO_UNALIGNED.

> I also think that the Coldfire 5272 can do unaligned accesses, but I
> cannot test that at the moment.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2012-06-06 06:35:53

by Greg Ungerer

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

Hi Geert,

On 06/06/12 06:57, Geert Uytterhoeven wrote:
> On Wed, May 30, 2012 at 4:04 PM, Philippe De Muyter<[email protected]> wrote:
>> On Wed, May 30, 2012 at 01:20:02PM +0200, Geert Uytterhoeven wrote:
>>> On Wed, May 30, 2012 at 12:22 PM, Philippe De Muyter<[email protected]> wrote:
>>>> On Tue, May 29, 2012 at 11:33:36PM +0200, Geert Uytterhoeven wrote:
>>>>> Signed-off-by: Geert Uytterhoeven<[email protected]>
>>>>> ---
>>>>> Do we also want
>>>>>
>>>>> select HAVE_EFFICIENT_UNALIGNED_ACCESS if (!COLDFIRE&& !M68000)
>>>>
>>>> Sorry, I did not follow what happened to unaligned accesses, but
>>>> CPU32 family (at least 68340) crashes on unaligned accesses.
>>>
>>> We don't seem to have CONFIG_M68340 in arch/m68k/Kconfig.cpu?
>>
>> I have a local port here (but based on an ancient linux kernel, 2.6.2 IIRC)
>>
>>> But Freescale's website confirms both 68340 and 68360 are CPU32.
>>>
>>> arch/m68k/include/asm/unaligned.h assumes CPU32 (CONFIG_MCPU32)
>>> can do unaligned accesses:
>>
>> That's not true. Accessing a 16- or 32-bit word at an odd address
>> with a 68340 generates an Address Error Exception. I remember
>> discovering a bug in the ppp kernel code because of that.
>>
>>>
>>> #if defined(CONFIG_COLDFIRE) || defined(CONFIG_M68000)
>>> #include<linux/unaligned/be_struct.h>
>>> #include<linux/unaligned/le_byteshift.h>
>>> #include<linux/unaligned/generic.h>
>>>
>>> #define get_unaligned __get_unaligned_be
>>> #define put_unaligned __put_unaligned_be
>>>
>>> #else
>>> /*
>>> * The m68k can do unaligned accesses itself.
>>> */
>>> #include<linux/unaligned/access_ok.h>
>>> #include<linux/unaligned/generic.h>
>>>
>>> #define get_unaligned __get_unaligned_be
>>> #define put_unaligned __put_unaligned_be
>>>
>>> #endif
>>>
>>> Is this wrong?
>>
>> I can't tell from reading just the lines above, but I think one should add
>> "|| defined(CONFIG_MCPU32)" at the end of the if condition.
>
> Greg?
>
> If more CPUs cannot handle unaligned accesses, I propose to add
> CONFIG_CPU_HAS_NO_UNALIGNED.

Yes, looks like that should have a "|| defined(CONFIG_CPU32)".
(According to the CPU32 reference manual words and long words must
be aligned on word boundaries.)

I think something like CONFIG_CPU_HAS_NO_UNALIGNED makes sense.


>> I also think that the Coldfire 5272 can do unaligned accesses, but I
>> cannot test that at the moment.

According to the MCF5272 User Manual, "it supports misaligned data
accesses ...". So it looks like it does.

Having a CONFIG_CPU_HAS_NO_UNALIGNED looks like a really good solution
then. We need to be able select it as required on individual CPU types.

Regards
Greg



------------------------------------------------------------------------
Greg Ungerer -- Principal Engineer EMAIL: [email protected]
SnapGear Group, McAfee PHONE: +61 7 3435 2888
8 Gardner Close FAX: +61 7 3217 5323
Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com

2012-06-06 13:44:07

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

On Wed, May 30, 2012 at 4:04 PM, Philippe De Muyter <[email protected]> wrote:
>> > Sorry, I did not follow what happened to unaligned accesses, but
>> > CPU32 family (at least 68340) crashes on unaligned accesses.
>>
>> We don't seem to have CONFIG_M68340 in arch/m68k/Kconfig.cpu?
>
> I have a local port here (but based on an ancient linux kernel, 2.6.2 IIRC)

Just to be sure: basically include/asm-m68knommu/unaligned.h has always
been wrong (also in 2.6.2), so you had to fix this locally?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2012-06-06 15:20:38

by Philippe De Muyter

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

Hi Geert,

On Wed, Jun 06, 2012 at 03:44:03PM +0200, Geert Uytterhoeven wrote:
> On Wed, May 30, 2012 at 4:04 PM, Philippe De Muyter <[email protected]> wrote:
> >> > Sorry, I did not follow what happened to unaligned accesses, but
> >> > CPU32 family (at least 68340) crashes on unaligned accesses.
> >>
> >> We don't seem to have CONFIG_M68340 in arch/m68k/Kconfig.cpu?
> >
> > I have a local port here (but based on an ancient linux kernel, 2.6.2 IIRC)
>
> Just to be sure: basically include/asm-m68knommu/unaligned.h has always
> been wrong (also in 2.6.2), so you had to fix this locally?

I have just verified, and you are right :

include/asm-m68knommu/unaligned.h was wrong in 2.6.2

But I did not have to fix it locally.

I remember the ppp driver kernel doing by error unaligned accesses
(of course without using unaligned macros) and crashing my kernel.
I fixed the error in the ppp driver and the fix went into the
mainline kernel.

I am not aware that unaligned macros were used in the parts of the
kernel I used on this board. Just to be sure I made a quick list :

$ find -name \*.o | sed -e 's/\.o$/.c/' | xargs grep t_unaligned 2> /dev/null
./fs/partitions/msdos.c:#define SYS_IND(p) (get_unaligned(&p->sys_ind))
./fs/partitions/msdos.c: get_unaligned(&p->nr_sects); \
./fs/partitions/msdos.c: get_unaligned(&p->start_sect); \
./drivers/net/slhc.c: put_unaligned(ip_fast_csum(icp, ip->ihl),

I am 100% sure I did not use msdos partitions on that board. I don't
know why msdos.c was compiled in. For the slhc case, I surmise that it
was not used because its usage depends on some option we did not use.

Best regards

Philippe

--
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

2012-06-06 17:46:10

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

Hi Greg,

On Wed, Jun 6, 2012 at 8:31 AM, Greg Ungerer <[email protected]> wrote:
> Yes, looks like that should have a "|| defined(CONFIG_CPU32)".
> (According to the CPU32 reference manual words and long words must
> be aligned on word boundaries.)
>
> I think something like CONFIG_CPU_HAS_NO_UNALIGNED makes sense.

OK, doing that now...

Then I saw arch/m68k/lib/memcpy.c.

commit f230e80b423f6cb002015ab4771c06a53d5a2287
("m68k: fix memcpy to unmatched/unaligned source and dest on 68000")

| The original 68000 processors cannot copy 16bit or larger quantities from
| odd addresses. All newer members of the 68k family (including ColdFire)
| can do this.

So all Coldfires can do unaligned _reads_, but not unaligned _writes_
(exceptions below)?

>>> I also think that the Coldfire 5272 can do unaligned accesses, but I
>>> cannot test that at the moment.
>
>
> According to the MCF5272 User Manual, "it supports misaligned data
> accesses ...". So it looks like it does.
>
> Having a CONFIG_CPU_HAS_NO_UNALIGNED looks like a really good solution
> then. We need to be able select it as required on individual CPU types.

For now, I just make COLDFIRE select it, but we can move it to the individual
CPU types later.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

2012-06-07 11:12:51

by Greg Ungerer

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

Hi Geert,

On 06/07/2012 03:46 AM, Geert Uytterhoeven wrote:
> On Wed, Jun 6, 2012 at 8:31 AM, Greg Ungerer<[email protected]> wrote:
>> Yes, looks like that should have a "|| defined(CONFIG_CPU32)".
>> (According to the CPU32 reference manual words and long words must
>> be aligned on word boundaries.)
>>
>> I think something like CONFIG_CPU_HAS_NO_UNALIGNED makes sense.
>
> OK, doing that now...
>
> Then I saw arch/m68k/lib/memcpy.c.
>
> commit f230e80b423f6cb002015ab4771c06a53d5a2287
> ("m68k: fix memcpy to unmatched/unaligned source and dest on 68000")
>
> | The original 68000 processors cannot copy 16bit or larger quantities from
> | odd addresses. All newer members of the 68k family (including ColdFire)
> | can do this.
>
> So all Coldfires can do unaligned _reads_, but not unaligned _writes_
> (exceptions below)?

This strikes me as odd. Maybe this has been wrong all along. I need
to check further but in a little testing I did today I think it may
well be that all ColdFire support unaligned reads and writes.


>>>> I also think that the Coldfire 5272 can do unaligned accesses, but I
>>>> cannot test that at the moment.
>>
>>
>> According to the MCF5272 User Manual, "it supports misaligned data
>> accesses ...". So it looks like it does.
>>
>> Having a CONFIG_CPU_HAS_NO_UNALIGNED looks like a really good solution
>> then. We need to be able select it as required on individual CPU types.
>
> For now, I just make COLDFIRE select it, but we can move it to the individual
> CPU types later.

No matter what I find I still think this is the way to go.

Thanks
Greg


------------------------------------------------------------------------
Greg Ungerer -- Principal Engineer EMAIL: [email protected]
SnapGear Group, McAfee PHONE: +61 7 3435 2888
8 Gardner Close, FAX: +61 7 3891 3630
Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com

2012-06-07 13:15:12

by Andreas Schwab

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

Greg Ungerer <[email protected]> writes:

> This strikes me as odd. Maybe this has been wrong all along. I need
> to check further but in a little testing I did today I think it may
> well be that all ColdFire support unaligned reads and writes.

The CFPRM doesn't indicate otherwise (the address error exception is
only documented to be raised for a word index addressing mode).

Andreas.

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2012-06-08 04:15:34

by Greg Ungerer

[permalink] [raw]
Subject: Re: [PATCH] m68k: Use generic strncpy_from_user(), strlen_user(), and strnlen_user()

On 07/06/12 23:14, Andreas Schwab wrote:
> Greg Ungerer<[email protected]> writes:
>
>> This strikes me as odd. Maybe this has been wrong all along. I need
>> to check further but in a little testing I did today I think it may
>> well be that all ColdFire support unaligned reads and writes.
>
> The CFPRM doesn't indicate otherwise (the address error exception is
> only documented to be raised for a word index addressing mode).

Yeah, I can't find anything that says otherwise in any of the CPU
specific data sheets either. I suspect the internals of unaligned.h
have just been carried from the original non-mmu 68k support to
ColdFire.

Testing on the ColdFires I have at hand here shows unaligned reads
and writes working on all. And that includes testing with a modified
unaligned.h that allows unaligned accesses.

Geert, I'll put a patch together that changes this over. I'll make
that on top of your recent patch series.

Regards
Greg


------------------------------------------------------------------------
Greg Ungerer -- Principal Engineer EMAIL: [email protected]
SnapGear Group, McAfee PHONE: +61 7 3435 2888
8 Gardner Close FAX: +61 7 3217 5323
Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com