<div dir="ltr">Okay, sir.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sat, Jul 11, 2020 at 2:46 AM Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com">lukas.bulwahn@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Jul 10, 2020 at 10:46 PM Shuah Khan <<a href="mailto:skhan@linuxfoundation.org" target="_blank">skhan@linuxfoundation.org</a>> wrote:<br>
><br>
> On 7/10/20 5:26 AM, Mrinal Pandey wrote:<br>
> > On Thu, Jul 9, 2020 at 10:33 AM Lukas Bulwahn <<a href="mailto:lukas.bulwahn@gmail.com" target="_blank">lukas.bulwahn@gmail.com</a><br>
> > <mailto:<a href="mailto:lukas.bulwahn@gmail.com" target="_blank">lukas.bulwahn@gmail.com</a>>> wrote:<br>
> ><br>
> > On Thu, Jul 9, 2020 at 6:31 AM Mrinal Pandey <<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a><br>
> > <mailto:<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a>>> wrote:<br>
> > ><br>
> > > On Tue, Jul 7, 2020 at 10:24 PM Lukas Bulwahn<br>
> > <<a href="mailto:lukas.bulwahn@gmail.com" target="_blank">lukas.bulwahn@gmail.com</a> <mailto:<a href="mailto:lukas.bulwahn@gmail.com" target="_blank">lukas.bulwahn@gmail.com</a>>> wrote:<br>
> > >><br>
> > >> On Tue, Jul 7, 2020 at 10:22 AM Mrinal Pandey<br>
> > <<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a> <mailto:<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a>>> wrote:<br>
> > >> ><br>
> > >> ><br>
> > >> ><br>
> > >> > On Tue, Jul 7, 2020 at 1:20 AM Lukas Bulwahn<br>
> > <<a href="mailto:lukas.bulwahn@gmail.com" target="_blank">lukas.bulwahn@gmail.com</a> <mailto:<a href="mailto:lukas.bulwahn@gmail.com" target="_blank">lukas.bulwahn@gmail.com</a>>> wrote:<br>
> > >> >><br>
> > >> >> On Mon, Jul 6, 2020 at 10:08 AM Mrinal Pandey<br>
> > <<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a> <mailto:<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a>>> wrote:<br>
> > >> >> ><br>
> > >> >> > <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> <<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">http://checkpatch.pl</a>> issues warnings on the<br>
> > commits<br>
> > >> >> > made to scripts/spelling.txt for new entries<br>
> > >> >> > of typos and their fixes. This commit adjusts<br>
> > >> >> > checkpatch not to complain about the same.<br>
> > >> >> ><br>
> > >> >> > Signed-off-by: Mrinal Pandey <<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a><br>
> > <mailto:<a href="mailto:mrinalmni@gmail.com" target="_blank">mrinalmni@gmail.com</a>>><br>
> > >> >> > ---<br>
> > >> >><br>
> > >> >> How often does that issue appear? Can you use your checkpatch<br>
> > >> >> evaluation to show that it is relevant?<br>
> > >> ><br>
> > >><br>
> > >> How many commits to spelling.txt happened within the last year?<br>
> > ><br>
> > ><br>
> > > Sir,<br>
> > ><br>
> > > I could find only commit to the file in the range 5.7 to 5.8.rc-1.<br>
> > >><br>
> > >><br>
> > >> The patch might be accepted, but the reason is not that convincing.<br>
> > ><br>
> > ><br>
> > > What do you suggest? Should I send it or not?<br>
> > ><br>
> ><br>
> > Let us keep that in the backlog for now, but not send it. If it is<br>
> > only one single case among hundreds false positives, it is maybe not<br>
> > the best to start with.<br>
> > We might get to that one case here eventually, but let us start with<br>
> > the more important and critical cases first.<br>
> ><br>
> ><br>
> > >> Maybe you can find another class of false positives that happen more<br>
> > >> often?<br>
> > ><br>
> > ><br>
> > > Yes, I have a few other suggestions that I found occurring often<br>
> > and I'm still evaluating to find more:<br>
> > > 1. In `.h` files, when we write a function prototype, the name of<br>
> > the function parameters are<br>
> > > not required, only the data type is enough, checkpatch says to<br>
> > define the name of the parameters too.<br>
> > > Issues a warning like - function definition argument '<arg>'<br>
> > should also have an identifier name<br>
> > ><br>
> ><br>
> > Okay, we need to discuss if that is a convention that developers care<br>
> > about or not.<br>
> ><br>
> ><br>
> > > 2. A very common warning is - Macros with complex values should<br>
> > be enclosed in parentheses<br>
> > > which is correct sometimes but a false positive many times, for<br>
> > macros ending with `)` or<br>
> > > macros like `#define var value` we probably don't need another<br>
> > pair of `()`<br>
> > ><br>
> ><br>
> > Agree, this might be worth refining in checkpatch as you described.<br>
> ><br>
> > > 3. checkpatch complains about breaking a quoted string across<br>
> > lines but this is many a time<br>
> > > necessary for readability and in most of the patches I saw the<br>
> > strings broken.<br>
> > ><br>
> ><br>
> > Tricky to really know what the best solution is here. It is a tradeoff<br>
> > in both directions.<br>
> > Let us put that aside for now.<br>
> ><br>
> > > 4. There are many patches where checkpatch issues false positives<br>
> > regarding spaces before<br>
> > > and after lines.<br>
> > ><br>
> > Why are they false positives?<br>
> ><br>
> ><br>
> > Sir,<br>
> ><br>
> > The warning by checkpatch says - please, no spaces at the start of a line<br>
> > but there are indeed no spaces before the line where this warning is issued.<br>
> > There are multiple commits having this issue, two of them are<br>
> > `acaab7335bd6` and `372b38ea5911`.<br>
> ><br>
> ><br>
> > > 5. The warning - EXPORT_SYMBOL(foo); should immediately follow<br>
> > its function/variable<br>
> > > is falsely positive in many cases where the statement is correct<br>
> > but the script fails to identify it.<br>
> > ><br>
> ><br>
> > If the script does not detect that, it sounds like a bug.<br>
> > This can be improved for <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> <<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">http://checkpatch.pl</a>>.<br>
> ><br>
> > > 6. While running checkpatch on a patch the following error was<br>
> > thrown to the console -<br>
> > > Use of uninitialized value $1 in regexp compilation at<br>
> > ./scripts/<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> <<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">http://checkpatch.pl</a>> line 2653.<br>
> > > This could be fixed.<br>
> > ><br>
> ><br>
> > That looks pretty sure like a bug.<br>
> ><br>
> > > Please let me know your views on these ideas.<br>
> ><br>
> > I suggest we look into issue 5 and 6.<br>
> ><br>
> > For Issue 5: Can you provide me (and the CC: the list) the list of<br>
> > false positives (the commit hashes) you found for issue 5 on<br>
> > EXPORT_SYMBOL?<br>
> ><br>
> ><br>
> > Here are the commit hashes for which the warning is issued:<br>
> > 54505a1e2083<br>
> > 75d75b7a4d54<br>
> > 8084c99b9af6<br>
> > bfdaf029c9c9<br>
> > dfd402a4c4ba<br>
> ><br>
> > Can you also provide a short rationale/explanation for<br>
> > each case that you considered a false positive?<br>
> ><br>
> ><br>
> > In each case the `EXPORT_SYMBOL()` is correctly written and the<br>
> > variable/function to be exported<br>
> > is also inside the parentheses, still, we get the warning. Please let me<br>
> > know if I am wrong here.<br>
> ><br>
> ><br>
> > For Issue 6: Can you provide me the commit hash that caused this<br>
> > <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> <<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">http://checkpatch.pl</a>> error? Then, we can reproduce<br>
> > and confirm that issue<br>
> > probably simply with `git format-patch -1 $SHA |<br>
> > ./scripts/<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> <<a href="http://checkpatch.pl" rel="noreferrer" target="_blank">http://checkpatch.pl</a>>` and observe the bug<br>
> > and crash ourselves?<br>
> ><br>
> ><br>
> > These are the commit hashes that crashed the checkpatch:<br>
> > 6b3e0e2e0461<br>
> > 19ce2321739d<br>
> > 059c6d68cfc5<br>
> ><br>
> ><br>
> > (I added <a href="mailto:linux-kernel-mentees@lists.linuxfoundation.org" target="_blank">linux-kernel-mentees@lists.linuxfoundation.org</a><br>
> > <mailto:<a href="mailto:linux-kernel-mentees@lists.linuxfoundation.org" target="_blank">linux-kernel-mentees@lists.linuxfoundation.org</a>> back to the<br>
> > recipient list.)<br>
> > Also, on sending emails: you started the thread on<br>
> > <a href="mailto:linux-kernel-mentees@lists.linuxfoundation.org" target="_blank">linux-kernel-mentees@lists.linuxfoundation.org</a><br>
> > <mailto:<a href="mailto:linux-kernel-mentees@lists.linuxfoundation.org" target="_blank">linux-kernel-mentees@lists.linuxfoundation.org</a>>. All further<br>
> > replies<br>
> > shall always include that list in To or CC, so that the email thread<br>
> > is complete on the list.<br>
> ><br>
> > At some point in this mail thread, you only replied to me but did not<br>
> > have the list in the recipient list (in To or CC). That was wrong;<br>
> > Please follow the rule stated above. I hope this point was already<br>
> > taught on the LF Kernel Development Introduction course. Maybe you can<br>
> > check the material once again and see if and where that was pointed<br>
> > out in the course material?<br>
> ><br>
> ><br>
> > Sir, I apologize for not including the list in my previous replies.<br>
> > Unfortunately, it slipped out of my mind.<br>
> > I assure you it would not happen again. Also, Linux Kernel Mentorship<br>
> > wiki says to CC the overall<br>
> > program mentor Shuah Khan Ma'am on each contribution. Should I do it<br>
> > only on the final patches or on<br>
> > every mail I send?<br>
> ><br>
><br>
> No worries. You are new and this is a learning process.<br>
><br>
> Please cc me on emails. You have to reply to the list when you respond<br>
> to patch reviews.<br>
><br>
> Please run <a href="http://get_maintainers.pl" rel="noreferrer" target="_blank">get_maintainers.pl</a> and include everybody <a href="http://get_maintainers.pl" rel="noreferrer" target="_blank">get_maintainers.pl</a><br>
> suggests. Without doing so will add more work for you when you send<br>
> it to the community.<br>
><br>
<br>
Mrinal, please first send these suggested patches only to me, Shuah<br>
and the linux-kernel-mentees list for reviewing.<br>
<br>
If I am okay with a specific patch, I will let you know to then send<br>
the patch to everybody <a href="http://get_maintainers.pl" rel="noreferrer" target="_blank">get_maintainers.pl</a> suggest, which will be for<br>
the patches we discuss:<br>
<br>
Andy Whitcroft <<a href="mailto:apw@canonical.com" target="_blank">apw@canonical.com</a>> (maintainer:CHECKPATCH)<br>
Joe Perches <<a href="mailto:joe@perches.com" target="_blank">joe@perches.com</a>> (maintainer:CHECKPATCH)<br>
<a href="mailto:linux-kernel@vger.kernel.org" target="_blank">linux-kernel@vger.kernel.org</a> (open list)<br>
<br>
I want to make sure that I agree with the patch before sending it to<br>
Andy and Joe.<br>
<br>
Lukas<br>
</blockquote></div>