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
