| Message ID | 1692434183-2054-1-git-send-email-yangtiezhu@loongson.cn (mailing list archive) |
|---|---|
| Headers | show |
| Series | Modify die() for MIPS | expand |
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.
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.
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 ]
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
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
在 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.
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(-)