On Sat, Apr 2, 2022 at 3:16 AM Jukka Laitinen <jukka.laiti...@iki.fi> wrote:
> I am not the author there, just feel that the process is not fair for the > author. > > The PR has over 300 comments, and they come in bunches of 20, every day > one bunch more. Most of them are just of type "please replace xxxyyy with > xxx_yyy", or "please move these lines 20 lines above". That is, purely > subjective "I think this looks better if..". > > Yes, some suggestion is to move the code around the different location, but it isn't a simple style change, here is an example: 01: /* Setup arg0(exception cause), arg1(context) */ 02: 03: csrr a0, CSR_CAUSE /* exception cause */ 04: mv a1, sp /* context = sp */ 05: 06:#if CONFIG_ARCH_INTERRUPTSTACK > 15 07: 08: /* Offset to hartid */ 09: 10: mv s0, a0 /* save cause */ 11: jal x1, riscv_mhartid /* get hartid */ 12: 13: /* Switch to interrupt stack */ 14: 15:#if IRQ_NSTACKS > 1 16: li t0, (CONFIG_ARCH_INTERRUPTSTACK & ~15) 17: mul t0, a0, t0 18: la a0, g_intstacktop 19: sub sp, a0, t0 20:#else 21: la sp, g_intstacktop 22:#endif 23: 24: mv a0, s0 /* restore cause */ 25: 26: /* Call interrupt handler in C */ 27: 28: jal x1, riscv_dispatch_irq Line 11 should move after line 15 since one cpu doesn't need to get cpu id. Line 03 could move just before line 27 to avoid save and restore a0 at line 10 and 24. >From the below update, you can see the logic is more clear: 01: /* Setup arg1(context) */ 02: 03: mv a1, sp /* context = sp */ 04: 05:#if CONFIG_ARCH_INTERRUPTSTACK > 15 06: 07: /* Switch to interrupt stack */ 08: 09:#if IRQ_NSTACKS > 1 10: /* Offset to hartid */ 11: 12: jal x1, riscv_mhartid /* get hartid */ 13: 14: li t0, (CONFIG_ARCH_INTERRUPTSTACK & ~15) 15: mul t0, a0, t0 16: la a0, g_intstacktop 17: sub sp, a0, t0 18:#else 19: la sp, g_intstacktop 20:#endif 21: 22: /* Setup arg0(exception cause) */ 23: csrr a0, CSR_CAUSE /* exception cause */ 24: 25: /* Call interrupt handler in C */ 26: 27: jal x1, riscv_dispatch_irq After the review, the commit was reduced from "1,710 additions and 118 deletions" to "967 additions and 192 deletions". If my last comment gets acknowledged, an additional 100~ lines could be eliminated. The patch was merged by Alan and all my comments are ignored. What's the side effect? We can't even pass the compiling if kernel mode is enabled, because this patch forgets to commit riscv_sbi.c(highlighted in my comment). The author (who I know and have very high respect for) has been re-basing > this PR and all the work on top of it now for weeks, since while delaying > this PR, Yes, pussuw <https://github.com/pussuw> and I have a good conversation in PR, and many issues(addrenv, riscv_doirq) get improved quickly. > there are other breaking stuff being pushed in with fast pace by the > company of the main reviewer. > > Do you mean the context switch patch? It doesn't break anything, I think. Earlier this week, Petro K. did some good work with the author and everyone > seemed happy, the PR was approved. But after approval it didn't get merged, > but received another 20 "maybe it looks better this way" from the other > reviewer! > My comment found some more issues not found by Petro, I have shown several before, it isn't a simple style preference but the quality improvement. The term " "maybe it looks better this way" may confuse you, please read the suggestion more closely. Anyway, since many suggestions could improve the performance and reduce the code size, I will provide a patch in the next couple of days. Review is welcome. > > - Jukka > > Alan Carvalho de Assis kirjoitti perjantai 1. huhtikuuta 2022: > > Hi Jukka, > > > > I think everybody want RISC-V with Kernel mode support on NuttX. > > > > Sometimes we get really angry because some comments seem too picky, > > but we need to believe in the "The Wisdom of Crowds" (i.e.: > > https://www.youtube.com/watch?v=Qfh-k9P8ZPI ) > > > > So, instead getting mad about it, please try to implement the > > suggestions. You are an experienced contributor and I'm sure you can > > get this PR accepted as well. :-) > > > > BR, > > > > Alan > > > > On 4/1/22, Jukka Laitinen <jukka.laiti...@iki.fi> wrote: > > > > > > > > > Hi, > > > > > > If RISC-V S-mode and CONFIG_BUILD_KERNEL support is not wanted into > NuttX, > > > please say it out loud instead of playing unfair review games. > > > > > > The team has better things to do than re-write code letter-by letter > via > > > code review, week after week. The code is good quality and working, > and is > > > done according to nuttx standards. > > > > > > We have always the option to keep it all ourselves and branch off. We > do > > > want to contribute, but if there is no will to receive code, we can > change > > > the policy and just bring in small bugfixes in the future and let the > > > fundamental larger improvements stay in our own repositories. > > > > > > If you want to prettify things to look better to your eye, just make > another > > > PR *yourself* *after* merging the working, well inspected and approved > PR. > > > > > > Thanks for your input for this issue, > > > Jukka > > > > >
