[PATCH v4 0/4] Modify die() for MIPS

[PATCH v4 0/4] Modify die() for MIPS

[Tiezhu Yang]

v4:
  -- Add BUG() at the end of nmi_exception_handler()
  -- Return earlier in die() if notify_die() returns NOTIFY_STOP
  -- Update the patch titles and commit messages

v3:
  -- Make each patch can be built without errors and warnings.

v2: 
  -- Update the commit message to give more detailed info, split into
     three individual patches, suggested by Maciej, thank you.

Tiezhu Yang (4):
  MIPS: Add BUG() at the end of nmi_exception_handler()
  MIPS: Do not kill the task in die() if notify_die() returns
    NOTIFY_STOP
  MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP
  MIPS: Add identifier names to arguments of die() declaration

 arch/mips/include/asm/ptrace.h |  2 +-
 arch/mips/kernel/traps.c       | 15 +++++++++------
 2 files changed, 10 insertions(+), 7 deletions(-)

-- 
2.1.0

[PATCH v4 1/4] MIPS: Add BUG() at the end of nmi_exception_handler()

[Tiezhu Yang]

In the later patch, we will remove noreturn attribute for die(), in order
to make each patch can be built without errors and warnings, just add a
noreturn function BUG() at the end of nmi_exception_handler() to make sure
it does not return, otherwise there exists the following build error after
the later patch:

  arch/mips/kernel/traps.c:2001:1: error: 'noreturn' function does return [-Werror]

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/kernel/traps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 246c6a6..b1b68ce 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -1998,6 +1998,7 @@ void __noreturn nmi_exception_handler(struct pt_regs *regs)
 	regs->cp0_epc = read_c0_errorepc();
 	die(str, regs);
 	nmi_exit();
+	BUG();
 }
 
 unsigned long ebase;
-- 
2.1.0

[PATCH v4 2/4] MIPS: Do not kill the task in die() if notify_die() returns NOTIFY_STOP

[Tiezhu Yang]

If notify_die() returns NOTIFY_STOP, honor the return value from the
handler chain invocation in die() and return without killing the task
as, through a debugger, the fault may have been fixed. It makes sense
even if ignoring the event will make the system unstable: by allowing
access through a debugger it has been compromised already anyway. It
makes our port consistent with x86, arm64, riscv and csky.

Commit 20c0d2d44029 ("[PATCH] i386: pass proper trap numbers to die
chain handlers") may be the earliest of similar changes.

Link: https://lore.kernel.org/r/43DDF02E.76F0.0078.0@novell.com/
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/include/asm/ptrace.h |  2 +-
 arch/mips/kernel/traps.c       | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
index daf3cf2..aee8e0a 100644
--- a/arch/mips/include/asm/ptrace.h
+++ b/arch/mips/include/asm/ptrace.h
@@ -159,7 +159,7 @@ static inline long regs_return_value(struct pt_regs *regs)
 extern asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall);
 extern asmlinkage void syscall_trace_leave(struct pt_regs *regs);
 
-extern void die(const char *, struct pt_regs *) __noreturn;
+extern void die(const char *, struct pt_regs *);
 
 static inline void die_if_kernel(const char *str, struct pt_regs *regs)
 {
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index b1b68ce..8e528a8 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -391,16 +391,15 @@ void show_registers(struct pt_regs *regs)
 
 static DEFINE_RAW_SPINLOCK(die_lock);
 
-void __noreturn die(const char *str, struct pt_regs *regs)
+void die(const char *str, struct pt_regs *regs)
 {
 	static int die_counter;
-	int sig = SIGSEGV;
+	int ret;
 
 	oops_enter();
 
-	if (notify_die(DIE_OOPS, str, regs, 0, current->thread.trap_nr,
-		       SIGSEGV) == NOTIFY_STOP)
-		sig = 0;
+	ret = notify_die(DIE_OOPS, str, regs, 0,
+			 current->thread.trap_nr, SIGSEGV);
 
 	console_verbose();
 	raw_spin_lock_irq(&die_lock);
@@ -422,7 +421,8 @@ void __noreturn die(const char *str, struct pt_regs *regs)
 	if (regs && kexec_should_crash(current))
 		crash_kexec(regs);
 
-	make_task_dead(sig);
+	if (ret != NOTIFY_STOP)
+		make_task_dead(SIGSEGV);
 }
 
 extern struct exception_table_entry __start___dbe_table[];
-- 
2.1.0

[PATCH v4 3/4] MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP

[Tiezhu Yang]

After the call to oops_exit(), it should not panic or execute
the crash kernel if the oops is to be suppressed.

Suggested-by: Maciej W. Rozycki <macro@orcam.me.uk>
Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/kernel/traps.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 8e528a8..fd770dc 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -412,6 +412,9 @@ void die(const char *str, struct pt_regs *regs)
 
 	oops_exit();
 
+	if (ret == NOTIFY_STOP)
+		return;
+
 	if (in_interrupt())
 		panic("Fatal exception in interrupt");
 
@@ -421,8 +424,7 @@ void die(const char *str, struct pt_regs *regs)
 	if (regs && kexec_should_crash(current))
 		crash_kexec(regs);
 
-	if (ret != NOTIFY_STOP)
-		make_task_dead(SIGSEGV);
+	make_task_dead(SIGSEGV);
 }
 
 extern struct exception_table_entry __start___dbe_table[];
-- 
2.1.0

Re: [PATCH v4 3/4] MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP

[Huacai Chen]

Hi, Tiezhu,

On Sun, Aug 20, 2023 at 7:21 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
> After the call to oops_exit(), it should not panic or execute
> the crash kernel if the oops is to be suppressed.
>
> Suggested-by: Maciej W. Rozycki <macro@orcam.me.uk>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>  arch/mips/kernel/traps.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> index 8e528a8..fd770dc 100644
> --- a/arch/mips/kernel/traps.c
> +++ b/arch/mips/kernel/traps.c
> @@ -412,6 +412,9 @@ void die(const char *str, struct pt_regs *regs)
>
>         oops_exit();
>
> +       if (ret == NOTIFY_STOP)
> +               return;
> +
>         if (in_interrupt())
>                 panic("Fatal exception in interrupt");
>
> @@ -421,8 +424,7 @@ void die(const char *str, struct pt_regs *regs)
>         if (regs && kexec_should_crash(current))
>                 crash_kexec(regs);
>
> -       if (ret != NOTIFY_STOP)
> -               make_task_dead(SIGSEGV);
> +       make_task_dead(SIGSEGV);
Then you call make_task_dead() at the end unconditionally, and die()
becomes a noreturn function again.

Huacai
>  }
>
>  extern struct exception_table_entry __start___dbe_table[];
> --
> 2.1.0
>

Re: [PATCH v4 3/4] MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP

[Tiezhu Yang]

On 08/20/2023 04:53 PM, Huacai Chen wrote:
> Hi, Tiezhu,
>
> On Sun, Aug 20, 2023 at 7:21 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>>
>> After the call to oops_exit(), it should not panic or execute
>> the crash kernel if the oops is to be suppressed.
>>
>> Suggested-by: Maciej W. Rozycki <macro@orcam.me.uk>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>  arch/mips/kernel/traps.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
>> index 8e528a8..fd770dc 100644
>> --- a/arch/mips/kernel/traps.c
>> +++ b/arch/mips/kernel/traps.c
>> @@ -412,6 +412,9 @@ void die(const char *str, struct pt_regs *regs)
>>
>>         oops_exit();
>>
>> +       if (ret == NOTIFY_STOP)
>> +               return;
>> +
>>         if (in_interrupt())
>>                 panic("Fatal exception in interrupt");
>>
>> @@ -421,8 +424,7 @@ void die(const char *str, struct pt_regs *regs)
>>         if (regs && kexec_should_crash(current))
>>                 crash_kexec(regs);
>>
>> -       if (ret != NOTIFY_STOP)
>> -               make_task_dead(SIGSEGV);
>> +       make_task_dead(SIGSEGV);
> Then you call make_task_dead() at the end unconditionally, and die()
> becomes a noreturn function again.

No, it can return if (ret == NOTIFY_STOP), so die() is a return
function now, please see objdump -d arch/mips/kernel/traps.o.

Thanks,
Tiezhu

Re: [PATCH v4 3/4] MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP

[Huacai Chen]

On Mon, Aug 21, 2023 at 10:29 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
>
>
> On 08/20/2023 04:53 PM, Huacai Chen wrote:
> > Hi, Tiezhu,
> >
> > On Sun, Aug 20, 2023 at 7:21 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> >>
> >> After the call to oops_exit(), it should not panic or execute
> >> the crash kernel if the oops is to be suppressed.
> >>
> >> Suggested-by: Maciej W. Rozycki <macro@orcam.me.uk>
> >> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> >> ---
> >>  arch/mips/kernel/traps.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> >> index 8e528a8..fd770dc 100644
> >> --- a/arch/mips/kernel/traps.c
> >> +++ b/arch/mips/kernel/traps.c
> >> @@ -412,6 +412,9 @@ void die(const char *str, struct pt_regs *regs)
> >>
> >>         oops_exit();
> >>
> >> +       if (ret == NOTIFY_STOP)
> >> +               return;
> >> +
> >>         if (in_interrupt())
> >>                 panic("Fatal exception in interrupt");
> >>
> >> @@ -421,8 +424,7 @@ void die(const char *str, struct pt_regs *regs)
> >>         if (regs && kexec_should_crash(current))
> >>                 crash_kexec(regs);
> >>
> >> -       if (ret != NOTIFY_STOP)
> >> -               make_task_dead(SIGSEGV);
> >> +       make_task_dead(SIGSEGV);
> > Then you call make_task_dead() at the end unconditionally, and die()
> > becomes a noreturn function again.
>
> No, it can return if (ret == NOTIFY_STOP), so die() is a return
> function now, please see objdump -d arch/mips/kernel/traps.o.
Then should loongarch patches need to be updated, too?

Huacai
>
> Thanks,
> Tiezhu
>

Re: [PATCH v4 3/4] MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP

[Tiezhu Yang]

On 08/22/2023 03:38 PM, Huacai Chen wrote:
> On Mon, Aug 21, 2023 at 10:29 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>>
>>
>>
>> On 08/20/2023 04:53 PM, Huacai Chen wrote:
>>> Hi, Tiezhu,
>>>
>>> On Sun, Aug 20, 2023 at 7:21 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>>>>
>>>> After the call to oops_exit(), it should not panic or execute
>>>> the crash kernel if the oops is to be suppressed.
>>>>
>>>> Suggested-by: Maciej W. Rozycki <macro@orcam.me.uk>
>>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>>>> ---
>>>>  arch/mips/kernel/traps.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
>>>> index 8e528a8..fd770dc 100644
>>>> --- a/arch/mips/kernel/traps.c
>>>> +++ b/arch/mips/kernel/traps.c
>>>> @@ -412,6 +412,9 @@ void die(const char *str, struct pt_regs *regs)
>>>>
>>>>         oops_exit();
>>>>
>>>> +       if (ret == NOTIFY_STOP)
>>>> +               return;
>>>> +
>>>>         if (in_interrupt())
>>>>                 panic("Fatal exception in interrupt");
>>>>
>>>> @@ -421,8 +424,7 @@ void die(const char *str, struct pt_regs *regs)
>>>>         if (regs && kexec_should_crash(current))
>>>>                 crash_kexec(regs);
>>>>
>>>> -       if (ret != NOTIFY_STOP)
>>>> -               make_task_dead(SIGSEGV);
>>>> +       make_task_dead(SIGSEGV);
>>> Then you call make_task_dead() at the end unconditionally, and die()
>>> becomes a noreturn function again.
>>
>> No, it can return if (ret == NOTIFY_STOP), so die() is a return
>> function now, please see objdump -d arch/mips/kernel/traps.o.
> Then should loongarch patches need to be updated, too?

Yes, I think so, will do it.

Thanks,
Tiezhu

[PATCH v4 4/4] MIPS: Add identifier names to arguments of die() declaration

[Tiezhu Yang]

Add identifier names to arguments of die() declaration in ptrace.h
to fix the following checkpatch warnings:

  WARNING: function definition argument 'const char *' should also have an identifier name
  WARNING: function definition argument 'struct pt_regs *' should also have an identifier name

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/include/asm/ptrace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/ptrace.h b/arch/mips/include/asm/ptrace.h
index aee8e0a..d05844e 100644
--- a/arch/mips/include/asm/ptrace.h
+++ b/arch/mips/include/asm/ptrace.h
@@ -159,7 +159,7 @@ static inline long regs_return_value(struct pt_regs *regs)
 extern asmlinkage long syscall_trace_enter(struct pt_regs *regs, long syscall);
 extern asmlinkage void syscall_trace_leave(struct pt_regs *regs);
 
-extern void die(const char *, struct pt_regs *);
+extern void die(const char *str, struct pt_regs *regs);
 
 static inline void die_if_kernel(const char *str, struct pt_regs *regs)
 {
-- 
2.1.0

Re: [PATCH v4 4/4] MIPS: Add identifier names to arguments of die() declaration

[Philippe Mathieu-Daudé]

On 19/8/23 10:36, Tiezhu Yang wrote:
> Add identifier names to arguments of die() declaration in ptrace.h
> to fix the following checkpatch warnings:
> 
>    WARNING: function definition argument 'const char *' should also have an identifier name
>    WARNING: function definition argument 'struct pt_regs *' should also have an identifier name
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   arch/mips/include/asm/ptrace.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v4 0/4] Modify die() for MIPS

[Thomas Bogendoerfer]

On Sat, Aug 19, 2023 at 04:36:19PM +0800, Tiezhu Yang wrote:
> v4:
>   -- Add BUG() at the end of nmi_exception_handler()
>   -- Return earlier in die() if notify_die() returns NOTIFY_STOP
>   -- Update the patch titles and commit messages
> 
> v3:
>   -- Make each patch can be built without errors and warnings.
> 
> v2: 
>   -- Update the commit message to give more detailed info, split into
>      three individual patches, suggested by Maciej, thank you.
> 
> Tiezhu Yang (4):
>   MIPS: Add BUG() at the end of nmi_exception_handler()
>   MIPS: Do not kill the task in die() if notify_die() returns
>     NOTIFY_STOP
>   MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP
>   MIPS: Add identifier names to arguments of die() declaration
> 
>  arch/mips/include/asm/ptrace.h |  2 +-
>  arch/mips/kernel/traps.c       | 15 +++++++++------
>  2 files changed, 10 insertions(+), 7 deletions(-)

series applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v4 0/4] Modify die() for MIPS

[Thomas Bogendoerfer]

On Mon, Aug 28, 2023 at 11:11:19AM +0200, Thomas Bogendoerfer wrote:
> On Sat, Aug 19, 2023 at 04:36:19PM +0800, Tiezhu Yang wrote:
> > v4:
> >   -- Add BUG() at the end of nmi_exception_handler()
> >   -- Return earlier in die() if notify_die() returns NOTIFY_STOP
> >   -- Update the patch titles and commit messages
> > 
> > v3:
> >   -- Make each patch can be built without errors and warnings.
> > 
> > v2: 
> >   -- Update the commit message to give more detailed info, split into
> >      three individual patches, suggested by Maciej, thank you.
> > 
> > Tiezhu Yang (4):
> >   MIPS: Add BUG() at the end of nmi_exception_handler()
> >   MIPS: Do not kill the task in die() if notify_die() returns
> >     NOTIFY_STOP
> >   MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP
> >   MIPS: Add identifier names to arguments of die() declaration
> > 
> >  arch/mips/include/asm/ptrace.h |  2 +-
> >  arch/mips/kernel/traps.c       | 15 +++++++++------
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> series applied to mips-next.

I've dropped the series again after feedback from Maciej, that this
still needs more changes.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v4 0/4] Modify die() for MIPS

[Huacai Chen]

Hi, Thomas,

On Tue, Aug 29, 2023 at 11:44 PM Thomas Bogendoerfer
<tsbogend@alpha.franken.de> wrote:
>
> On Mon, Aug 28, 2023 at 11:11:19AM +0200, Thomas Bogendoerfer wrote:
> > On Sat, Aug 19, 2023 at 04:36:19PM +0800, Tiezhu Yang wrote:
> > > v4:
> > >   -- Add BUG() at the end of nmi_exception_handler()
> > >   -- Return earlier in die() if notify_die() returns NOTIFY_STOP
> > >   -- Update the patch titles and commit messages
> > >
> > > v3:
> > >   -- Make each patch can be built without errors and warnings.
> > >
> > > v2:
> > >   -- Update the commit message to give more detailed info, split into
> > >      three individual patches, suggested by Maciej, thank you.
> > >
> > > Tiezhu Yang (4):
> > >   MIPS: Add BUG() at the end of nmi_exception_handler()
> > >   MIPS: Do not kill the task in die() if notify_die() returns
> > >     NOTIFY_STOP
> > >   MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP
> > >   MIPS: Add identifier names to arguments of die() declaration
> > >
> > >  arch/mips/include/asm/ptrace.h |  2 +-
> > >  arch/mips/kernel/traps.c       | 15 +++++++++------
> > >  2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > series applied to mips-next.
>
> I've dropped the series again after feedback from Maciej, that this
> still needs more changes.
I feel a little surprised. This series has appeared for more than ten
days and received some R-b, and we haven't seen any objections from
Maciej. If there are really some bugs that need to be fixed, I think
the normal operation is making additional patches...

Huacai

>
> Thomas.
>
> --
> Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
> good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH v4 0/4] Modify die() for MIPS

[Maciej W. Rozycki]

On Wed, 30 Aug 2023, Huacai Chen wrote:

> > > series applied to mips-next.
> >
> > I've dropped the series again after feedback from Maciej, that this
> > still needs more changes.
> I feel a little surprised. This series has appeared for more than ten
> days and received some R-b, and we haven't seen any objections from
> Maciej. If there are really some bugs that need to be fixed, I think
> the normal operation is making additional patches...

 You haven't received any ack from me either, and I stopped reviewing the 
series as it was taking too much of my time and mental effort and yet 
changes were going in the wrong direction.  Silence never means an ack.

 It's up to the submitter to get things right and not to expect from the
reviewer to get issues pointed at by finger one by one, effectively 
demanding someone else's effort to get their own objectives complete even 
with the most obvious things.

 And then for a hypothetical case only that the submitter is not able to 
verify.  For such cases the usual approach is to do nothing until an 
actual real case is found.

 Very simple such a change that one can verify to an acceptable degree
that it is correct by just proofreading might be accepted anyway, but it 
cannot be guaranteed.

 The missed NMI case only proved the submitter didn't do their homework 
and didn't track down all the call sites as expected with such a change, 
and instead relied on reviewer's vigilance.

 As to the changes, specifically:

- 1/4 is bogus, the kernel must not BUG on user activities.  Most simply
  die() should be told by the NMI caller that it must not return in this 
  case and then it should ignore the NOTIFY_STOP condition.

  I realise we may not be able to just return from the NMI handler to the 
  location at CP0.ErrorEPC and continue, because owing to the privileged 
  ISA design we won't be able to make such an NMI handler reentrant, let 
  alone SMP-safe.  But it should have been given in the change description 
  as rationale for not handling the NOTIFY_STOP condition for the NMI.

  I leave it as an exercise to the reader to figure out why a returning 
  NMI handler cannot be made reentrant.

- 2/4 should be a one-liner to handle the NOTIFY_STOP condition just as 
  with the x86 port, which I already (!) communicated, and which was (!!!) 
  ignored.  There is no need to rewrite the rest of die() and make it more 
  complex too just because it can be done.

- 3/4 is not needed if 2/4 was done properly.  And as it stands it should 
  have been folded into 2/4, because fixes to an own pending submission 
  mustn't be made with a separate patch: the original change has to be 
  corrected instead.

- 4/4 is OK (and I believe the only one that actually got a Reviewed-by: 
  tag).

Most of these issues would have been avoided if the submitter made 
themselves familiar with Documentation/process/submitting-patches.rst and 
followed the rules specified there.

 Otherwise this takes valuable reviewer resources that would best be used 
elsewhere and it puts submitters of quality changes at a disadvantage, 
which is not fair.

 It is not our policy to accept known-broken changes and then fix them up 
afterwards.  Changes are expected to be technically sound to the best of 
everyone's involved knowledge and it's up to the submitter to prove that 
it is the case and that a change is worth including.  You would have 
learnt it from the document referred.  Nobody's perfect and issues may 
slip through, but we need to make every effort so as to avoid it.

 Mind that we're doing reviews as volunteers entirely in our free time we 
might instead want to spend with friends or in another enjoyable way.  It 
is not my day job to review random MIPS/Linux patches posted to a mailing 
list.  Even composing this reply took a considerable amount of time and 
effort, which would best be spent elsewhere, because I am talking obvious 
things here and repeating Documentation/process/submitting-patches.rst 
stuff.

  Maciej

Re: [PATCH v4 0/4] Modify die() for MIPS

[YunQiang Su]

Maciej W. Rozycki <macro@orcam.me.uk> 于2023年8月31日周四 02:56写道：
>
> On Wed, 30 Aug 2023, Huacai Chen wrote:
>
> > > > series applied to mips-next.
> > >
> > > I've dropped the series again after feedback from Maciej, that this
> > > still needs more changes.
> > I feel a little surprised. This series has appeared for more than ten
> > days and received some R-b, and we haven't seen any objections from
> > Maciej. If there are really some bugs that need to be fixed, I think
> > the normal operation is making additional patches...
>
>  You haven't received any ack from me either, and I stopped reviewing the
> series as it was taking too much of my time and mental effort and yet
> changes were going in the wrong direction.  Silence never means an ack.
>

In my opinion, the position of `reviewer` normally means more
obligation instead of
power.
The world is always changing, I don't think that the world needs to
wait for anybody.

In the open source community, if a person has some position, if
they/he/she has little
time to play that position well, he should do something, for example, seek help
or transfer the position to somebody who can work well.

>  It's up to the submitter to get things right and not to expect from the
> reviewer to get issues pointed at by finger one by one, effectively
> demanding someone else's effort to get their own objectives complete even
> with the most obvious things.
>
>  And then for a hypothetical case only that the submitter is not able to
> verify.  For such cases the usual approach is to do nothing until an
> actual real case is found.
>

I think that it depends. If the patch can solve a part of this problem, and
won't leave some problem hard to solve in the future or serious
security problems.
I think that it is worth considering.

Yes, I agree with a sentence from you "Nobody's perfect", and the same goes for
software, even Linux kernel.

PS: I never read the code of this thread, it is just common sense, I guess.

>  Very simple such a change that one can verify to an acceptable degree
> that it is correct by just proofreading might be accepted anyway, but it
> cannot be guaranteed.
>
>  The missed NMI case only proved the submitter didn't do their homework
> and didn't track down all the call sites as expected with such a change,
> and instead relied on reviewer's vigilance.
>
>  As to the changes, specifically:
>
> - 1/4 is bogus, the kernel must not BUG on user activities.  Most simply
>   die() should be told by the NMI caller that it must not return in this
>   case and then it should ignore the NOTIFY_STOP condition.
>
>   I realise we may not be able to just return from the NMI handler to the
>   location at CP0.ErrorEPC and continue, because owing to the privileged
>   ISA design we won't be able to make such an NMI handler reentrant, let
>   alone SMP-safe.  But it should have been given in the change description
>   as rationale for not handling the NOTIFY_STOP condition for the NMI.
>
>   I leave it as an exercise to the reader to figure out why a returning
>   NMI handler cannot be made reentrant.
>
> - 2/4 should be a one-liner to handle the NOTIFY_STOP condition just as
>   with the x86 port, which I already (!) communicated, and which was (!!!)
>   ignored.  There is no need to rewrite the rest of die() and make it more
>   complex too just because it can be done.
>
> - 3/4 is not needed if 2/4 was done properly.  And as it stands it should
>   have been folded into 2/4, because fixes to an own pending submission
>   mustn't be made with a separate patch: the original change has to be
>   corrected instead.
>
> - 4/4 is OK (and I believe the only one that actually got a Reviewed-by:
>   tag).
>
> Most of these issues would have been avoided if the submitter made
> themselves familiar with Documentation/process/submitting-patches.rst and
> followed the rules specified there.
>
>  Otherwise this takes valuable reviewer resources that would best be used
> elsewhere and it puts submitters of quality changes at a disadvantage,
> which is not fair.
>
>  It is not our policy to accept known-broken changes and then fix them up
> afterwards.  Changes are expected to be technically sound to the best of
> everyone's involved knowledge and it's up to the submitter to prove that
> it is the case and that a change is worth including.  You would have
> learnt it from the document referred.  Nobody's perfect and issues may
> slip through, but we need to make every effort so as to avoid it.
>
>  Mind that we're doing reviews as volunteers entirely in our free time we
> might instead want to spend with friends or in another enjoyable way.  It
> is not my day job to review random MIPS/Linux patches posted to a mailing
> list.  Even composing this reply took a considerable amount of time and
> effort, which would best be spent elsewhere, because I am talking obvious
> things here and repeating Documentation/process/submitting-patches.rst
> stuff.
>

I have noticed that in the note of this series of patches:
     v3:
        -- Make each patch can be built without errors and warnings.

I think that how to split the patches is a trade off, not a principle.
For me, the reasons to split patches are:
   1. Make the problem obviously.
   2. Show the problem one by one, and it is easy to understand.
   3. Make life easier for distributors.
   4. And maybe more

While if a splitting conflicts with these purposes, it will be another story.

>   Maciej



-- 
YunQiang Su

Re: [PATCH v4 0/4] Modify die() for MIPS

[Jiaxun Yang]

在 2023/8/29 23:19, Thomas Bogendoerfer 写道:
> On Mon, Aug 28, 2023 at 11:11:19AM +0200, Thomas Bogendoerfer wrote:
>> On Sat, Aug 19, 2023 at 04:36:19PM +0800, Tiezhu Yang wrote:
>>> v4:
>>>    -- Add BUG() at the end of nmi_exception_handler()
>>>    -- Return earlier in die() if notify_die() returns NOTIFY_STOP
>>>    -- Update the patch titles and commit messages
>>>
>>> v3:
>>>    -- Make each patch can be built without errors and warnings.
>>>
>>> v2:
>>>    -- Update the commit message to give more detailed info, split into
>>>       three individual patches, suggested by Maciej, thank you.
>>>
>>> Tiezhu Yang (4):
>>>    MIPS: Add BUG() at the end of nmi_exception_handler()
>>>    MIPS: Do not kill the task in die() if notify_die() returns
>>>      NOTIFY_STOP
>>>    MIPS: Return earlier in die() if notify_die() returns NOTIFY_STOP
>>>    MIPS: Add identifier names to arguments of die() declaration
>>>
>>>   arch/mips/include/asm/ptrace.h |  2 +-
>>>   arch/mips/kernel/traps.c       | 15 +++++++++------
>>>   2 files changed, 10 insertions(+), 7 deletions(-)
>> series applied to mips-next.
> I've dropped the series again after feedback from Maciej, that this
> still needs more changes.
Was the feedback made off-list?

I think it's important to make submitter aware of the problem before
action being taken.

 From my perspective Teizhu was working hard to address review comments
all the way so it looks unfair to neglect his work without transparent 
communication.

Thanks
- Jiaxun
> Thomas.