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
