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