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

[YunQiang Su]

Maciej W. Rozycki <macro@xxxxxxxxxxx> 于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