netdev - Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56B8A6F5.9060409@laposte.net>
Date:	Mon, 08 Feb 2016 15:32:21 +0100
From:	Sebastian Frias <sf84@...oste.net>
To:	Måns Rullgård <mans@...sr.com>
CC:	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>, mason <slash.tmp@...e.fr>
Subject: Re: [PATCH v3] net: ethernet: support "fixed-link" DT node on nb8800
 driver

On 02/08/2016 02:37 PM, Måns Rullgård wrote:
> Sebastian Frias <sf84@...oste.net> writes:
>
>> On 02/05/2016 04:26 PM, Måns Rullgård wrote:
>>> Sebastian Frias <sf84@...oste.net> writes:
>>>
>>>> On 02/05/2016 04:08 PM, Måns Rullgård wrote:
>>>>> Sebastian Frias <sf84@...oste.net> writes:
>>>>>
>>>>>> On 02/05/2016 03:34 PM, Måns Rullgård wrote:
>>>>>>> Sebastian Frias <sf84@...oste.net> writes:
>>>>>>>
>>>>>>>> Signed-off-by: Sebastian Frias <sf84@...oste.net>
>>>>>>>
>>>>>>> Please change the subject to something like "net: ethernet: nb8800:
>>>>>>> support fixed-link DT node" and add a comment body.
>>>>>>
>>>>>> The subject is pretty explicit for such a simple patch, what else
>>>>>> could I add that wouldn't be unnecessary chat?
>>>>>
>>>>> It's customary to include a description body even if it's little more
>>>>> than a restatement of the subject.  Also, while the subject usually only
>>>>> says _what_ the patch does, the body should additionally state _why_ it
>>>>> is needed.
>>>>
>>>> I understand, but _why_ it is needed is also obvious in this case; I
>>>> mean, without the patch "fixed-link" cannot be used.
>>>
>>> Then say so.
>>>
>>>> Other patches may not be as obvious/simple and thus justify and
>>>> require more details.
>>>>
>>>> Anyway, I added "Properly handles the case where the PHY is not connected
>>>> to the real MDIO bus" would that be ok?
>>>
>>> Have you read Documentation/SubmittingPatches?  Do so (again) and pay
>>> special attention to section 2 "Describe your changes."
>>
>> I just sent v5.
>
> Thanks for your patience.

:-)

>
>> If for whatever reason, you or anybody else think that the comment is
>> not good, would you mind proposing a comment that would make everybody
>> happy so that the patch goes thru?
>> And if you or anybody else does not want the patch, could you please
>> say so as well?
>>
>> I have to admit this process (sending patches and getting it reviewed)
>> could benefit from more clarifications.
>> For example, the process could say that at least 2 reviewers must
>> agree on it (on the comments made to the patch and on the patch
>> itself).
>> I could also say that reviewers are to express not only their opinion
>> but to clearly and unequivocally accept or reject.
>>
>> For instance, right now, it is not clear to me if your comments are
>> "nice to have" or "blocking" the patch.
>> I don't know if the patch is welcome or not, etc.
>> So I submitted v5, but maybe it was not even necessary, it's hard to
>> know where in the submission process we are.
>
> In this case, it's ultimately up to Dave Miller.  He'll take into
> account whatever comments others have made and decide whether he wants
> to accept it.

Ok, thanks.

>
>> By the way, I know some people like the command line, email, etc. but
>> there ought to be other tools better suited for patch review...
>
> Some kernel subsystems use http://patchwork.ozlabs.org/ to track status
> of various patches.
>

Thanks, I see that netdev is part of it, and that the patches are there:

https://patchwork.ozlabs.org/patch/580217/

seems like a slight layer over plain email and mailinglists; I was 
thinking of something more in the line of https://www.gerritcodereview.com/
I believe Google uses Gerrit for Android.
I think Gerrit would probably be too big (and being written in Java, 
using Prolog and other DSLs, implementing its own Git server in Java, 
etc, may make some -or lots?- of kernel developers cry :-) )
However, in Gerrit it is easier to know where in the "review" process we 
are, because people have to explicitly give a score "+/- X" when 
commenting on a patch.
Also, the diff can operate between different versions of the patches 
themselves to see if the inlined comments were addressed.