[PATCH 0/2] Minor signal clean up

[PATCH 0/2] Minor signal clean up

[Markos Chandras]

Hi,

The following two patches contain a minor clean up in the signal setup code.
The first one makes o32 behave the same on 64-bit kernels as with the 32-bit
ones. The second one drops the extra arguments from traditional signal
handlers.

Markos Chandras (2):
  MIPS: asm: signal.h: Fix traditional signal handling for o32 on 64-bit
    kernels
  MIPS: kernel: signal: Drop unused arguments for traditional signal
    handlers

 arch/mips/include/asm/signal.h | 8 +++-----
 arch/mips/kernel/signal.c      | 8 ++------
 arch/mips/kernel/signal32.c    | 6 +-----
 arch/mips/kernel/signal_n32.c  | 2 +-
 4 files changed, 7 insertions(+), 17 deletions(-)

-- 
2.5.0

[PATCH 0/2] Minor signal clean up

[Markos Chandras]

Hi,

The following two patches contain a minor clean up in the signal setup code.
The first one makes o32 behave the same on 64-bit kernels as with the 32-bit
ones. The second one drops the extra arguments from traditional signal
handlers.

Markos Chandras (2):
  MIPS: asm: signal.h: Fix traditional signal handling for o32 on 64-bit
    kernels
  MIPS: kernel: signal: Drop unused arguments for traditional signal
    handlers

 arch/mips/include/asm/signal.h | 8 +++-----
 arch/mips/kernel/signal.c      | 8 ++------
 arch/mips/kernel/signal32.c    | 6 +-----
 arch/mips/kernel/signal_n32.c  | 2 +-
 4 files changed, 7 insertions(+), 17 deletions(-)

-- 
2.5.0

[PATCH 1/2] MIPS: asm: signal.h: Fix traditional signal handling for o32 on 64-bit kernels

[Markos Chandras]

CONFIG_TRAD_SIGNALS is used to denote that legacy signal handlers are
supported (ie !SA_SIGINFO). However, this symbol is only available on
32-bit kernels, so o32 running on 64-bit kernels with !SA_SIGINFO was
treated as if SA_SIGINFO was set so there was extra overhead setting up
the signal handling code within the kernel. o32 should work in the same
way for both 32-bit and 64-bit kernels so we fix the sig_uses_siginfo
definition to allow traditional signal handling for o32 even on 64-bit
kernels. This has been tested booting a MIPS32R6 userland on a 64-bit
MIPS64R6 kernel.

Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/signal.h | 8 +++-----
 arch/mips/kernel/signal.c      | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/signal.h b/arch/mips/include/asm/signal.h
index 003e273eff4c..cedc53b0ab69 100644
--- a/arch/mips/include/asm/signal.h
+++ b/arch/mips/include/asm/signal.h
@@ -12,11 +12,9 @@
 #include <uapi/asm/signal.h>
 
 
-#ifdef CONFIG_TRAD_SIGNALS
-#define sig_uses_siginfo(ka)	((ka)->sa.sa_flags & SA_SIGINFO)
-#else
-#define sig_uses_siginfo(ka)	(1)
-#endif
+#define sig_uses_siginfo(abi, ka)		\
+	(((ka)->sa.sa_flags & SA_SIGINFO) ||	\
+	 (!config_enabled(CONFIG_TRAD_SIGNALS) && !(abi)->setup_frame))
 
 #include <asm/sigcontext.h>
 #include <asm/siginfo.h>
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 359fb5829f66..be3ac5f7cbbb 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -801,7 +801,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		regs->regs[0] = 0;		/* Don't deal with this again.	*/
 	}
 
-	if (sig_uses_siginfo(&ksig->ka))
+	if (sig_uses_siginfo(abi, &ksig->ka))
 		ret = abi->setup_rt_frame(vdso + abi->rt_signal_return_offset,
 					  ksig, regs, oldset);
 	else
-- 
2.5.0

[PATCH 1/2] MIPS: asm: signal.h: Fix traditional signal handling for o32 on 64-bit kernels

[Markos Chandras]

CONFIG_TRAD_SIGNALS is used to denote that legacy signal handlers are
supported (ie !SA_SIGINFO). However, this symbol is only available on
32-bit kernels, so o32 running on 64-bit kernels with !SA_SIGINFO was
treated as if SA_SIGINFO was set so there was extra overhead setting up
the signal handling code within the kernel. o32 should work in the same
way for both 32-bit and 64-bit kernels so we fix the sig_uses_siginfo
definition to allow traditional signal handling for o32 even on 64-bit
kernels. This has been tested booting a MIPS32R6 userland on a 64-bit
MIPS64R6 kernel.

Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/include/asm/signal.h | 8 +++-----
 arch/mips/kernel/signal.c      | 2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/mips/include/asm/signal.h b/arch/mips/include/asm/signal.h
index 003e273eff4c..cedc53b0ab69 100644
--- a/arch/mips/include/asm/signal.h
+++ b/arch/mips/include/asm/signal.h
@@ -12,11 +12,9 @@
 #include <uapi/asm/signal.h>
 
 
-#ifdef CONFIG_TRAD_SIGNALS
-#define sig_uses_siginfo(ka)	((ka)->sa.sa_flags & SA_SIGINFO)
-#else
-#define sig_uses_siginfo(ka)	(1)
-#endif
+#define sig_uses_siginfo(abi, ka)		\
+	(((ka)->sa.sa_flags & SA_SIGINFO) ||	\
+	 (!config_enabled(CONFIG_TRAD_SIGNALS) && !(abi)->setup_frame))
 
 #include <asm/sigcontext.h>
 #include <asm/siginfo.h>
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index 359fb5829f66..be3ac5f7cbbb 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -801,7 +801,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		regs->regs[0] = 0;		/* Don't deal with this again.	*/
 	}
 
-	if (sig_uses_siginfo(&ksig->ka))
+	if (sig_uses_siginfo(abi, &ksig->ka))
 		ret = abi->setup_rt_frame(vdso + abi->rt_signal_return_offset,
 					  ksig, regs, oldset);
 	else
-- 
2.5.0

[PATCH 2/2] MIPS: kernel: signal: Drop unused arguments for traditional signal handlers

[Markos Chandras]

Traditional signal handlers (ie !SA_SIGINFO) only need only argument
holding the signal number so we drop the additional arguments and fix
the related comments. We also update the comments for the SA_SIGINFO
case where the second argument is a pointer to a siginfo_t structure.

Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/kernel/signal.c     | 6 +-----
 arch/mips/kernel/signal32.c   | 6 +-----
 arch/mips/kernel/signal_n32.c | 2 +-
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index be3ac5f7cbbb..3a125331bf8b 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -683,15 +683,11 @@ static int setup_frame(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
-	 *   a2 = pointer to struct sigcontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to the
 	 * struct sigframe.
 	 */
 	regs->regs[ 4] = ksig->sig;
-	regs->regs[ 5] = 0;
-	regs->regs[ 6] = (unsigned long) &frame->sf_sc;
 	regs->regs[29] = (unsigned long) frame;
 	regs->regs[31] = (unsigned long) sig_return;
 	regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
@@ -730,7 +726,7 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
+	 *   a1 = pointer to siginfo_t
 	 *   a2 = pointer to ucontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to
diff --git a/arch/mips/kernel/signal32.c b/arch/mips/kernel/signal32.c
index 3059f36bfc89..6f6f79435edf 100644
--- a/arch/mips/kernel/signal32.c
+++ b/arch/mips/kernel/signal32.c
@@ -336,15 +336,11 @@ static int setup_frame_32(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
-	 *   a2 = pointer to struct sigcontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to the
 	 * struct sigframe.
 	 */
 	regs->regs[ 4] = ksig->sig;
-	regs->regs[ 5] = 0;
-	regs->regs[ 6] = (unsigned long) &frame->sf_sc;
 	regs->regs[29] = (unsigned long) frame;
 	regs->regs[31] = (unsigned long) sig_return;
 	regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
@@ -383,7 +379,7 @@ static int setup_rt_frame_32(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
+	 *   a1 = pointer to siginfo_t
 	 *   a2 = pointer to ucontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to
diff --git a/arch/mips/kernel/signal_n32.c b/arch/mips/kernel/signal_n32.c
index 0d017fdcaf07..04c4a2a7df13 100644
--- a/arch/mips/kernel/signal_n32.c
+++ b/arch/mips/kernel/signal_n32.c
@@ -129,7 +129,7 @@ static int setup_rt_frame_n32(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
+	 *   a1 = pointer to siginfo_t
 	 *   a2 = pointer to ucontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to
-- 
2.5.0

[PATCH 2/2] MIPS: kernel: signal: Drop unused arguments for traditional signal handlers

[Markos Chandras]

Traditional signal handlers (ie !SA_SIGINFO) only need only argument
holding the signal number so we drop the additional arguments and fix
the related comments. We also update the comments for the SA_SIGINFO
case where the second argument is a pointer to a siginfo_t structure.

Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
---
 arch/mips/kernel/signal.c     | 6 +-----
 arch/mips/kernel/signal32.c   | 6 +-----
 arch/mips/kernel/signal_n32.c | 2 +-
 3 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index be3ac5f7cbbb..3a125331bf8b 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -683,15 +683,11 @@ static int setup_frame(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
-	 *   a2 = pointer to struct sigcontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to the
 	 * struct sigframe.
 	 */
 	regs->regs[ 4] = ksig->sig;
-	regs->regs[ 5] = 0;
-	regs->regs[ 6] = (unsigned long) &frame->sf_sc;
 	regs->regs[29] = (unsigned long) frame;
 	regs->regs[31] = (unsigned long) sig_return;
 	regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
@@ -730,7 +726,7 @@ static int setup_rt_frame(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
+	 *   a1 = pointer to siginfo_t
 	 *   a2 = pointer to ucontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to
diff --git a/arch/mips/kernel/signal32.c b/arch/mips/kernel/signal32.c
index 3059f36bfc89..6f6f79435edf 100644
--- a/arch/mips/kernel/signal32.c
+++ b/arch/mips/kernel/signal32.c
@@ -336,15 +336,11 @@ static int setup_frame_32(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
-	 *   a2 = pointer to struct sigcontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to the
 	 * struct sigframe.
 	 */
 	regs->regs[ 4] = ksig->sig;
-	regs->regs[ 5] = 0;
-	regs->regs[ 6] = (unsigned long) &frame->sf_sc;
 	regs->regs[29] = (unsigned long) frame;
 	regs->regs[31] = (unsigned long) sig_return;
 	regs->cp0_epc = regs->regs[25] = (unsigned long) ksig->ka.sa.sa_handler;
@@ -383,7 +379,7 @@ static int setup_rt_frame_32(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
+	 *   a1 = pointer to siginfo_t
 	 *   a2 = pointer to ucontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to
diff --git a/arch/mips/kernel/signal_n32.c b/arch/mips/kernel/signal_n32.c
index 0d017fdcaf07..04c4a2a7df13 100644
--- a/arch/mips/kernel/signal_n32.c
+++ b/arch/mips/kernel/signal_n32.c
@@ -129,7 +129,7 @@ static int setup_rt_frame_n32(void *sig_return, struct ksignal *ksig,
 	 * Arguments to signal handler:
 	 *
 	 *   a0 = signal number
-	 *   a1 = 0 (should be cause)
+	 *   a1 = pointer to siginfo_t
 	 *   a2 = pointer to ucontext
 	 *
 	 * $25 and c0_epc point to the signal handler, $29 points to
-- 
2.5.0

Re: [PATCH 2/2] MIPS: kernel: signal: Drop unused arguments for traditional signal handlers

[David Daney]

On 08/20/2015 04:45 AM, Markos Chandras wrote:
> Traditional signal handlers (ie !SA_SIGINFO) only need only argument
> holding the signal number so we drop the additional arguments and fix
> the related comments. We also update the comments for the SA_SIGINFO
> case where the second argument is a pointer to a siginfo_t structure.
>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---
>   arch/mips/kernel/signal.c     | 6 +-----
>   arch/mips/kernel/signal32.c   | 6 +-----
>   arch/mips/kernel/signal_n32.c | 2 +-
>   3 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> index be3ac5f7cbbb..3a125331bf8b 100644
> --- a/arch/mips/kernel/signal.c
> +++ b/arch/mips/kernel/signal.c
> @@ -683,15 +683,11 @@ static int setup_frame(void *sig_return, struct ksignal *ksig,
>   	 * Arguments to signal handler:
>   	 *
>   	 *   a0 = signal number
> -	 *   a1 = 0 (should be cause)
> -	 *   a2 = pointer to struct sigcontext
>   	 *
>   	 * $25 and c0_epc point to the signal handler, $29 points to the
>   	 * struct sigframe.
>   	 */
>   	regs->regs[ 4] = ksig->sig;
> -	regs->regs[ 5] = 0;
> -	regs->regs[ 6] = (unsigned long) &frame->sf_sc;

This changes the kernel ABI.

Have you tested this change against all userspace applications that use 
signals to make sure it doesn't break anything?

David Daney

Re: [PATCH 2/2] MIPS: kernel: signal: Drop unused arguments for traditional signal handlers

[Markos Chandras]

On 08/21/2015 06:03 PM, David Daney wrote:
> On 08/20/2015 04:45 AM, Markos Chandras wrote:
>> Traditional signal handlers (ie !SA_SIGINFO) only need only argument
>> holding the signal number so we drop the additional arguments and fix
>> the related comments. We also update the comments for the SA_SIGINFO
>> case where the second argument is a pointer to a siginfo_t structure.
>>
>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>> ---
>>   arch/mips/kernel/signal.c     | 6 +-----
>>   arch/mips/kernel/signal32.c   | 6 +-----
>>   arch/mips/kernel/signal_n32.c | 2 +-
>>   3 files changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
>> index be3ac5f7cbbb..3a125331bf8b 100644
>> --- a/arch/mips/kernel/signal.c
>> +++ b/arch/mips/kernel/signal.c
>> @@ -683,15 +683,11 @@ static int setup_frame(void *sig_return, struct
>> ksignal *ksig,
>>        * Arguments to signal handler:
>>        *
>>        *   a0 = signal number
>> -     *   a1 = 0 (should be cause)
>> -     *   a2 = pointer to struct sigcontext
>>        *
>>        * $25 and c0_epc point to the signal handler, $29 points to the
>>        * struct sigframe.
>>        */
>>       regs->regs[ 4] = ksig->sig;
>> -    regs->regs[ 5] = 0;
>> -    regs->regs[ 6] = (unsigned long) &frame->sf_sc;
> 
> This changes the kernel ABI.
> 
> Have you tested this change against all userspace applications that use
> signals to make sure it doesn't break anything?
> 
> David Daney

i am confident there is no userland application that uses inline asm to
fetch additional arguments from (*sa_handler) when using !SA_SIGINFO

-- 
markos

Re: [PATCH 2/2] MIPS: kernel: signal: Drop unused arguments for traditional signal handlers

[Markos Chandras]

On 08/21/2015 06:03 PM, David Daney wrote:
> On 08/20/2015 04:45 AM, Markos Chandras wrote:
>> Traditional signal handlers (ie !SA_SIGINFO) only need only argument
>> holding the signal number so we drop the additional arguments and fix
>> the related comments. We also update the comments for the SA_SIGINFO
>> case where the second argument is a pointer to a siginfo_t structure.
>>
>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>> ---
>>   arch/mips/kernel/signal.c     | 6 +-----
>>   arch/mips/kernel/signal32.c   | 6 +-----
>>   arch/mips/kernel/signal_n32.c | 2 +-
>>   3 files changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
>> index be3ac5f7cbbb..3a125331bf8b 100644
>> --- a/arch/mips/kernel/signal.c
>> +++ b/arch/mips/kernel/signal.c
>> @@ -683,15 +683,11 @@ static int setup_frame(void *sig_return, struct
>> ksignal *ksig,
>>        * Arguments to signal handler:
>>        *
>>        *   a0 = signal number
>> -     *   a1 = 0 (should be cause)
>> -     *   a2 = pointer to struct sigcontext
>>        *
>>        * $25 and c0_epc point to the signal handler, $29 points to the
>>        * struct sigframe.
>>        */
>>       regs->regs[ 4] = ksig->sig;
>> -    regs->regs[ 5] = 0;
>> -    regs->regs[ 6] = (unsigned long) &frame->sf_sc;
> 
> This changes the kernel ABI.
> 
> Have you tested this change against all userspace applications that use
> signals to make sure it doesn't break anything?
> 
> David Daney

i am confident there is no userland application that uses inline asm to
fetch additional arguments from (*sa_handler) when using !SA_SIGINFO

-- 
markos

Re: [PATCH 2/2] MIPS: kernel: signal: Drop unused arguments for traditional signal handlers

[Paul Burton]

On Mon, Aug 24, 2015 at 09:01:09AM +0100, Markos Chandras wrote:
> On 08/21/2015 06:03 PM, David Daney wrote:
> > On 08/20/2015 04:45 AM, Markos Chandras wrote:
> >> Traditional signal handlers (ie !SA_SIGINFO) only need only argument
> >> holding the signal number so we drop the additional arguments and fix
> >> the related comments. We also update the comments for the SA_SIGINFO
> >> case where the second argument is a pointer to a siginfo_t structure.
> >>
> >> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> >> ---
> >>   arch/mips/kernel/signal.c     | 6 +-----
> >>   arch/mips/kernel/signal32.c   | 6 +-----
> >>   arch/mips/kernel/signal_n32.c | 2 +-
> >>   3 files changed, 3 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> >> index be3ac5f7cbbb..3a125331bf8b 100644
> >> --- a/arch/mips/kernel/signal.c
> >> +++ b/arch/mips/kernel/signal.c
> >> @@ -683,15 +683,11 @@ static int setup_frame(void *sig_return, struct
> >> ksignal *ksig,
> >>        * Arguments to signal handler:
> >>        *
> >>        *   a0 = signal number
> >> -     *   a1 = 0 (should be cause)
> >> -     *   a2 = pointer to struct sigcontext
> >>        *
> >>        * $25 and c0_epc point to the signal handler, $29 points to the
> >>        * struct sigframe.
> >>        */
> >>       regs->regs[ 4] = ksig->sig;
> >> -    regs->regs[ 5] = 0;
> >> -    regs->regs[ 6] = (unsigned long) &frame->sf_sc;
> > 
> > This changes the kernel ABI.
> > 
> > Have you tested this change against all userspace applications that use
> > signals to make sure it doesn't break anything?
> > 
> > David Daney
> 
> i am confident there is no userland application that uses inline asm to
> fetch additional arguments from (*sa_handler) when using !SA_SIGINFO
> 
> -- 
> markos

I'm not sure where you get the idea that you'd need inline asm to use
the a1/a2 values - you'd just need to declare the extra parameters to
your signal handling function.

This does seem like a scary change! I'm pretty confident you haven't
actually checked every bit of userland code. Would we perhaps be better
off leaving the registers set (which is a trivial cost) and document the
behaviour instead?

Thanks,
    Paul

Re: [PATCH 2/2] MIPS: kernel: signal: Drop unused arguments for traditional signal handlers

[Paul Burton]

On Mon, Aug 24, 2015 at 09:01:09AM +0100, Markos Chandras wrote:
> On 08/21/2015 06:03 PM, David Daney wrote:
> > On 08/20/2015 04:45 AM, Markos Chandras wrote:
> >> Traditional signal handlers (ie !SA_SIGINFO) only need only argument
> >> holding the signal number so we drop the additional arguments and fix
> >> the related comments. We also update the comments for the SA_SIGINFO
> >> case where the second argument is a pointer to a siginfo_t structure.
> >>
> >> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> >> ---
> >>   arch/mips/kernel/signal.c     | 6 +-----
> >>   arch/mips/kernel/signal32.c   | 6 +-----
> >>   arch/mips/kernel/signal_n32.c | 2 +-
> >>   3 files changed, 3 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> >> index be3ac5f7cbbb..3a125331bf8b 100644
> >> --- a/arch/mips/kernel/signal.c
> >> +++ b/arch/mips/kernel/signal.c
> >> @@ -683,15 +683,11 @@ static int setup_frame(void *sig_return, struct
> >> ksignal *ksig,
> >>        * Arguments to signal handler:
> >>        *
> >>        *   a0 = signal number
> >> -     *   a1 = 0 (should be cause)
> >> -     *   a2 = pointer to struct sigcontext
> >>        *
> >>        * $25 and c0_epc point to the signal handler, $29 points to the
> >>        * struct sigframe.
> >>        */
> >>       regs->regs[ 4] = ksig->sig;
> >> -    regs->regs[ 5] = 0;
> >> -    regs->regs[ 6] = (unsigned long) &frame->sf_sc;
> > 
> > This changes the kernel ABI.
> > 
> > Have you tested this change against all userspace applications that use
> > signals to make sure it doesn't break anything?
> > 
> > David Daney
> 
> i am confident there is no userland application that uses inline asm to
> fetch additional arguments from (*sa_handler) when using !SA_SIGINFO
> 
> -- 
> markos

I'm not sure where you get the idea that you'd need inline asm to use
the a1/a2 values - you'd just need to declare the extra parameters to
your signal handling function.

This does seem like a scary change! I'm pretty confident you haven't
actually checked every bit of userland code. Would we perhaps be better
off leaving the registers set (which is a trivial cost) and document the
behaviour instead?

Thanks,
    Paul

Re: [PATCH 2/2] MIPS: kernel: signal: Drop unused arguments for traditional signal handlers

[David Daney]

On 08/24/2015 01:01 AM, Markos Chandras wrote:
> On 08/21/2015 06:03 PM, David Daney wrote:
>> On 08/20/2015 04:45 AM, Markos Chandras wrote:
>>> Traditional signal handlers (ie !SA_SIGINFO) only need only argument
>>> holding the signal number so we drop the additional arguments and fix
>>> the related comments. We also update the comments for the SA_SIGINFO
>>> case where the second argument is a pointer to a siginfo_t structure.
>>>
>>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>>> ---
>>>    arch/mips/kernel/signal.c     | 6 +-----
>>>    arch/mips/kernel/signal32.c   | 6 +-----
>>>    arch/mips/kernel/signal_n32.c | 2 +-
>>>    3 files changed, 3 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
>>> index be3ac5f7cbbb..3a125331bf8b 100644
>>> --- a/arch/mips/kernel/signal.c
>>> +++ b/arch/mips/kernel/signal.c
>>> @@ -683,15 +683,11 @@ static int setup_frame(void *sig_return, struct
>>> ksignal *ksig,
>>>         * Arguments to signal handler:
>>>         *
>>>         *   a0 = signal number
>>> -     *   a1 = 0 (should be cause)
>>> -     *   a2 = pointer to struct sigcontext
>>>         *
>>>         * $25 and c0_epc point to the signal handler, $29 points to the
>>>         * struct sigframe.
>>>         */
>>>        regs->regs[ 4] = ksig->sig;
>>> -    regs->regs[ 5] = 0;
>>> -    regs->regs[ 6] = (unsigned long) &frame->sf_sc;
>>
>> This changes the kernel ABI.
>>
>> Have you tested this change against all userspace applications that use
>> signals to make sure it doesn't break anything?
>>
>> David Daney
>
> i am confident there is no userland application that uses inline asm to
> fetch additional arguments from (*sa_handler) when using !SA_SIGINFO

You don't need inline asm.  All you have to do is use a C cast on the 
function pointer.

I think your confidence is misplaced.  I have a program that this change 
will break.

You are changing the de facto kernel ABI in such a manner that it could 
break existing programs without a strong reason to do so.

Answer this:  Why is it important to you to change this?  The change log 
gives no justification other than we can, and it doesn't seem to break 
limited set of test cases.

If you are fixing some real bug, then OK.  But if you don't have a 
reason to do this, then: NAK.

David Daney


>