[PATCH 0/3] More signal clean up

[PATCH 0/3] More signal clean up

[Franck Bui-Huu]

Hi Ralf,

Sorry for forgetting these 3 trivial patches. It shouldn't
conflict with Atsushi's last patch:

	Check FCSR for pending interrupts, alternative version

Please consider,

		Franck
---

 arch/mips/kernel/signal-common.h |    2 ++
 arch/mips/kernel/signal.c        |   10 ++++------
 arch/mips/kernel/signal32.c      |    2 --
 arch/mips/kernel/signal_n32.c    |    2 --
 4 files changed, 6 insertions(+), 10 deletions(-)

[PATCH 1/3] signal: avoid useless test in do_signal()

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

Indeed we can simply clear the flag whatever its value

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 arch/mips/kernel/signal.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 8dfb7b1..464d34b 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -552,10 +552,8 @@ void do_signal(struct pt_regs *regs)
 			 * and will be restored by sigreturn, so we can simply
 			 * clear the TIF_RESTORE_SIGMASK flag.
 			 */
-			if (test_thread_flag(TIF_RESTORE_SIGMASK))
-				clear_thread_flag(TIF_RESTORE_SIGMASK);
+			clear_thread_flag(TIF_RESTORE_SIGMASK);
 		}
-
 		return;
 	}
 
-- 
1.4.4.3.ge6d4

[PATCH 2/3] signals: make common _BLOCKABLE macro

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 arch/mips/kernel/signal-common.h |    2 ++
 arch/mips/kernel/signal.c        |    2 --
 arch/mips/kernel/signal32.c      |    2 --
 arch/mips/kernel/signal_n32.c    |    2 --
 4 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/mips/kernel/signal-common.h b/arch/mips/kernel/signal-common.h
index 9a8abd6..23fffb4 100644
--- a/arch/mips/kernel/signal-common.h
+++ b/arch/mips/kernel/signal-common.h
@@ -19,6 +19,8 @@
 #  define DEBUGP(fmt, args...)
 #endif
 
+#define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
+
 /*
  * Horribly complicated - with the bloody RM9000 workarounds enabled
  * the signal trampolines is moving to the end of the structure so we can
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 464d34b..a3c04d0 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -34,8 +34,6 @@
 
 #include "signal-common.h"
 
-#define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
-
 #if ICACHE_REFILLS_WORKAROUND_WAR == 0
 
 struct rt_sigframe {
diff --git a/arch/mips/kernel/signal32.c b/arch/mips/kernel/signal32.c
index 183fc7e..f603ff4 100644
--- a/arch/mips/kernel/signal32.c
+++ b/arch/mips/kernel/signal32.c
@@ -104,8 +104,6 @@ typedef struct compat_siginfo {
 #define __NR_O32_rt_sigreturn		4193
 #define __NR_O32_restart_syscall	4253
 
-#define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
-
 /* 32-bit compatibility types */
 
 #define _NSIG_BPW32	32
diff --git a/arch/mips/kernel/signal_n32.c b/arch/mips/kernel/signal_n32.c
index 57456e6..51b114f 100644
--- a/arch/mips/kernel/signal_n32.c
+++ b/arch/mips/kernel/signal_n32.c
@@ -47,8 +47,6 @@
 #define __NR_N32_rt_sigreturn		6211
 #define __NR_N32_restart_syscall	6214
 
-#define _BLOCKABLE (~(sigmask(SIGKILL) | sigmask(SIGSTOP)))
-
 /* IRIX compatible stack_t  */
 typedef struct sigaltstack32 {
 	s32 ss_sp;
-- 
1.4.4.3.ge6d4

[PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Franck Bui-Huu]

From: Franck Bui-Huu <fbuihuu@gmail.com>

This hack prevents gcc to produce the following warning:

  CC      arch/mips/kernel/signal.o
arch/mips/kernel/signal.c: In function `sys_sigaction':
arch/mips/kernel/signal.c:266: warning: cast to pointer from integer of different size

This warning is due to the following line:

	__get_user(new_ka.sa.sa_handler, &act->sa_handler);

Indeed when __get_user() is expanded it produces the following
code:

	switch (sizeof(*(&act->sa_handler))) {
	... [snip] ...
	case 8: {
		unsigned long long __gu_tmp;
		__asm__ __volatile__(
			"1: lw      %1, (%3)                  \n"
			"2: lw      %D1, 4(%3)                \n"
			"   move    %0, $0                    \n"
			"3: .section        .fixup,\"ax\"     \n"
			"4: li      %0, %4                    \n"
			"   move    %1, $0                    \n"
			"   move    %D1, $0                   \n"
			"   j       3b                        \n"
			"   .previous                         \n"
			"   .section        __ex_table,\"a\"  \n"
			"   " ".word" "     1b, 4b            \n"
			"   " ".word" "     2b, 4b            \n"
			"   .previous                         \n"
			: "=r" (__gu_err), "=&r" (__gu_tmp)
			: "0" (0), "r" ((&act->sa_handler)), "i" (-14));

		(((new_ka.sa.sa_handler))) = (__typeof__(*((&act->sa_handler)))) __gu_tmp;
		};
		break;
	default:
		__get_user_unknown();
		break;
	}

which actually try to do:

	new_ka.sa.sa_handler = (__sighandler_t) __gu_tmp;

Here we try to cast an 'unsigned long long' into a 32 bits pointer and
that's the reason of the warning.

Signed-off-by: Franck Bui-Huu <fbuihuu@gmail.com>
---
 arch/mips/kernel/signal.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index a3c04d0..ac8a05a 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -260,15 +260,17 @@ asmlinkage int sys_sigaction(int sig, const struct sigaction __user *act,
 
 	if (act) {
 		old_sigset_t mask;
+		unsigned long tmp; /* fix a gcc warning */
 
 		if (!access_ok(VERIFY_READ, act, sizeof(*act)))
 			return -EFAULT;
-		err |= __get_user(new_ka.sa.sa_handler, &act->sa_handler);
+		err |= __get_user(tmp, (unsigned long *)&act->sa_handler);
 		err |= __get_user(new_ka.sa.sa_flags, &act->sa_flags);
 		err |= __get_user(mask, &act->sa_mask.sig[0]);
 		if (err)
 			return -EFAULT;
 
+		new_ka.sa.sa_handler = (__sighandler_t)tmp;
 		siginitset(&new_ka.sa.sa_mask, mask);
 	}
 
-- 
1.4.4.3.ge6d4

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Atsushi Nemoto]

On Fri,  9 Feb 2007 16:07:38 +0100, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
>   CC      arch/mips/kernel/signal.o
> arch/mips/kernel/signal.c: In function `sys_sigaction':
> arch/mips/kernel/signal.c:266: warning: cast to pointer from integer of different size
> 
> This warning is due to the following line:
> 
> 	__get_user(new_ka.sa.sa_handler, &act->sa_handler);

This usage of __get_user() should be absolutely legal.

> 	new_ka.sa.sa_handler = (__sighandler_t) __gu_tmp;
> 
> Here we try to cast an 'unsigned long long' into a 32 bits pointer and
> that's the reason of the warning.

This line is never executed on 32bit kernel and gcc optimize out.  On
64-bit kernel, this line is executed without any problem without
warning.

I think this is a problem of __get_user() implementation or gcc
itself.  Though I can not find better solution yet, hacking the caller
to avoid the warning would not be right things to to.

---
Atsushi Nemoto

Re: [PATCH 1/3] signal: avoid useless test in do_signal()

[Ralf Baechle]

On Fri, Feb 09, 2007 at 04:07:36PM +0100, Franck Bui-Huu wrote:

> -			if (test_thread_flag(TIF_RESTORE_SIGMASK))
> -				clear_thread_flag(TIF_RESTORE_SIGMASK);

This is a microoptimization.  The assumption here is TIF_RESTORE_SIGMASK
will rarely need to be cleared and atomic operations are somewhat
expensive if as in this case we have to assume the cacheline isn't
held exclusive yet.

  Ralf

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Franck Bui-Huu]

On 2/9/07, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> On Fri,  9 Feb 2007 16:07:38 +0100, Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> >       new_ka.sa.sa_handler = (__sighandler_t) __gu_tmp;
> >
> > Here we try to cast an 'unsigned long long' into a 32 bits pointer and
> > that's the reason of the warning.
>
> This line is never executed on 32bit kernel and gcc optimize out.  On

yes I agree but it seems that gcc compiles this line before optimizing out...

>
> I think this is a problem of __get_user() implementation or gcc
> itself.  Though I can not find better solution yet, hacking the caller
> to avoid the warning would not be right things to to.

I agree too but I haven't found something else.

BTW, my version of gcc is: mipsel-linux-gcc (GCC) 3.4.4 mipssde-6.05.00-20061023

thanks
-- 
                Franck

Re: [PATCH 1/3] signal: avoid useless test in do_signal()

[Franck Bui-Huu]

On 2/9/07, Ralf Baechle <ralf@linux-mips.org> wrote:
Ralf Baechle wrote:
> On Fri, Feb 09, 2007 at 04:07:36PM +0100, Franck Bui-Huu wrote:
>
>> -			if (test_thread_flag(TIF_RESTORE_SIGMASK))
>> -				clear_thread_flag(TIF_RESTORE_SIGMASK);
>
> This is a microoptimization.  The assumption here is TIF_RESTORE_SIGMASK
> will rarely need to be cleared and atomic operations are somewhat
> expensive if as in this case we have to assume the cacheline isn't
> held exclusive yet.
>

I missed that. You can forget this patch or maybe something like this
is more appropriate ?

	if (unlikely(test_thread_flag(TIF_RESTORE_SIGMASK)))
		clear_thread_flag(TIF_RESTORE_SIGMASK);
-- 
               Franck

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Ralf Baechle]

On Fri, Feb 09, 2007 at 05:34:16PM +0100, Franck Bui-Huu wrote:
> Date:	Fri, 9 Feb 2007 17:34:16 +0100
> From:	"Franck Bui-Huu" <fbuihuu@gmail.com>
> To:	"Atsushi Nemoto" <anemo@mba.ocn.ne.jp>
> Subject: Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel
> Cc:	vagabon.xyz@gmail.com, ralf@linux-mips.org,
> 	linux-mips@linux-mips.org
> Content-Type: text/plain; charset=ISO-8859-1; format=flowed
> 
> On 2/9/07, Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> >On Fri,  9 Feb 2007 16:07:38 +0100, Franck Bui-Huu <vagabon.xyz@gmail.com> 
> >wrote:
> >>       new_ka.sa.sa_handler = (__sighandler_t) __gu_tmp;
> >>
> >> Here we try to cast an 'unsigned long long' into a 32 bits pointer and
> >> that's the reason of the warning.
> >
> >This line is never executed on 32bit kernel and gcc optimize out.  On
> 
> yes I agree but it seems that gcc compiles this line before optimizing 
> out...
> 
> >
> >I think this is a problem of __get_user() implementation or gcc
> >itself.  Though I can not find better solution yet, hacking the caller
> >to avoid the warning would not be right things to to.
> 
> I agree too but I haven't found something else.

All gcc versions produce this warning and no, it's not a gcc bug but a
kernel bug.  __get_user expands into:

        case 8: {
                unsigned long long __gu_tmp;
[...]
		new_ka.sa.sa_handler =
			(__typeof__(*((&act->sa_handler)))) __gu_tmp;
	}

Which is quite a funny C problem to solve :-)

  Ralf

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Franck Bui-Huu]

On 2/9/07, Ralf Baechle <ralf@linux-mips.org> wrote:
> Which is quite a funny C problem to solve :-)
>

How about this instead ?

-- >8 --

diff --git a/include/asm-mips/uaccess.h b/include/asm-mips/uaccess.h
index 1cdd4ee..ab7fe1c 100644
--- a/include/asm-mips/uaccess.h
+++ b/include/asm-mips/uaccess.h
@@ -265,7 +265,7 @@ do {									\
  */
 #define __get_user_asm_ll32(val, addr)					\
 {									\
-        unsigned long long __gu_tmp;					\
+        __typeof__(*(addr)) __gu_tmp;					\
 									\
 	__asm__ __volatile__(						\
 	"1:	lw	%1, (%3)				\n"	\
@@ -283,7 +283,7 @@ do {									\
 	"	.previous					\n"	\
 	: "=r" (__gu_err), "=&r" (__gu_tmp)				\
 	: "0" (0), "r" (addr), "i" (-EFAULT));				\
-	(val) = (__typeof__(*(addr))) __gu_tmp;				\
+	(val) = __gu_tmp;						\
 }

 /*

-- 
               Franck

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Ralf Baechle]

On Mon, Feb 12, 2007 at 10:01:18AM +0100, Franck Bui-Huu wrote:

> How about this instead ?

Won't work for a pointer to some const thing.

  Ralf

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Atsushi Nemoto]

On Mon, 12 Feb 2007 14:04:59 +0000, Ralf Baechle <ralf@linux-mips.org> wrote:
> > How about this instead ?
> 
> Won't work for a pointer to some const thing.

And recent commit 4ed3a77f38c023658784804cb39a7ce18063dc88 reverts
commit 3218357c94af92478ef39163163a81e654385320.

Round and round and round and ...

---
Atsushi Nemoto

Re: [PATCH 2/3] signals: make common _BLOCKABLE macro

[Ralf Baechle]

On Fri, Feb 09, 2007 at 04:07:37PM +0100, Franck Bui-Huu wrote:

Thanks, applied.

  Ralf

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Ralf Baechle]

On Tue, Feb 13, 2007 at 12:25:45AM +0900, Atsushi Nemoto wrote:

> On Mon, 12 Feb 2007 14:04:59 +0000, Ralf Baechle <ralf@linux-mips.org> wrote:
> > > How about this instead ?
> > 
> > Won't work for a pointer to some const thing.
> 
> And recent commit 4ed3a77f38c023658784804cb39a7ce18063dc88 reverts
> commit 3218357c94af92478ef39163163a81e654385320.
> 
> Round and round and round and ...

Well, I reverted that the old state of a warning is definately preferable
until we found a proper solution.

  Ralf

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Ralf Baechle]

On Tue, Feb 13, 2007 at 01:43:45AM +0000, Ralf Baechle wrote:

> Well, I reverted that the old state of a warning is definately preferable
> until we found a proper solution.

Type-punning should do the trick.

  Ralf

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/include/asm-mips/uaccess.h b/include/asm-mips/uaccess.h
index c12ebc5..36b3a42 100644
--- a/include/asm-mips/uaccess.h
+++ b/include/asm-mips/uaccess.h
@@ -265,7 +265,10 @@ do {									\
  */
 #define __get_user_asm_ll32(val, addr)					\
 {									\
-        unsigned long long __gu_tmp;					\
+	union {								\
+		unsigned long long	l;				\
+		__typeof__(*(addr))	t;				\
+	} __gu_tmp;							\
 									\
 	__asm__ __volatile__(						\
 	"1:	lw	%1, (%3)				\n"	\
@@ -281,9 +284,10 @@ do {									\
 	"	" __UA_ADDR "	1b, 4b				\n"	\
 	"	" __UA_ADDR "	2b, 4b				\n"	\
 	"	.previous					\n"	\
-	: "=r" (__gu_err), "=&r" (__gu_tmp)				\
+	: "=r" (__gu_err), "=&r" (__gu_tmp.l)				\
 	: "0" (0), "r" (addr), "i" (-EFAULT));				\
-	(val) = (__typeof__(*(addr))) __gu_tmp;				\
+									\
+	(val) = __gu_tmp.t;						\
 }
 
 /*

Re: [PATCH 3/3] signal.c: fix gcc warning on 32 bits kernel

[Franck Bui-Huu]

On 2/13/07, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Tue, Feb 13, 2007 at 01:43:45AM +0000, Ralf Baechle wrote:
>
> > Well, I reverted that the old state of a warning is definately preferable
> > until we found a proper solution.
>
> Type-punning should do the trick.
>
yes it does.

thanks.
-- 
               Franck