Message-ID: <d8ca9e63-7033-43f0-9a43-304b1a7ded42@linaro.org>
Date: Sat, 6 Apr 2024 13:14:59 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
 Bjorn Andersson <quic_bjorande@...cinc.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Johan Hovold <johan@...nel.org>, Krishna Kurapati
 <quic_kriskura@...cinc.com>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Rob Herring <robh@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
 Wesley Cheng <quic_wcheng@...cinc.com>,
 Konrad Dybcio <konrad.dybcio@...aro.org>, Conor Dooley
 <conor+dt@...nel.org>, Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
 Felipe Balbi <balbi@...nel.org>, devicetree@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
 linux-kernel@...r.kernel.org, quic_ppratap@...cinc.com,
 quic_jackp@...cinc.com, Johan Hovold <johan+linaro@...nel.org>
Subject: Re: [PATCH v19 2/9] usb: dwc3: core: Access XHCI address space
 temporarily to read port info

On 05/04/2024 22:36, Dmitry Baryshkov wrote:
>>>>>
>>>>> Also get those internal reviewers to sign-off on the commits and have
>>>>> that show up when you post them next.  That way they are also
>>>>> responsible for this patchset, it's not fair that they are making you do
>>>>> all the work here :)
>>>>>
>>>>
>>>> I like this idea and I'm open to us changing our way of handling this.
>>>>
>>>> But unless such internal review brings significant input to the
>>>> development I'd say a s-o-b would take the credit from the actual
>>>> author.
>>>
>>> It does not do that at all.  It provides proof that someone else has
>>> reviewed it and agrees with it.  Think of it as a "path of blame" for
>>> when things go bad (i.e. there is a bug in the submission.)  Putting
>>> your name on it makes you take responsibility if that happens.
>>>
>>
>> Right, this is why I like your idea.
>>
>> But as s-o-b either builds a trail of who handled the patch, or reflects
>> that it was co-authored by multiple people, I don't think either one
>> properly reflects reality.
>>
>>>> We've discussed a few times about carrying Reviewed-by et al from the
>>>> internal reviews, but as maintainer I dislike this because I'd have no
>>>> way to know if a r-b on vN means the patch was reviewed, or if it was
>>>> just "accidentally" carried from v(N-1).
>>>> But it might be worth this risk, is this something you think would be
>>>> appropriate?
>>>
>>> For some companies we REQUIRE this to happen due to low-quality
>>> submissions and waste of reviewer's time.  Based on the track record
>>> here for some of these patchsets, hopefully it doesn't become a
>>> requirement for this company as well :)
>>>
>>
>> Interesting, I was under the impression that we (maintainers) didn't
>> want such internally originating tags.
> 
> But why? It just means that the patch has been reviewed. In some rare
> cases we explicitly ask a developer to have all the patches reviewed
> before sending them upstream. In such a case having an R-B tag
> fulfills the expectation of the maintainer: it shows that another
> engineer has reviewed the patch.

Wait, there are two types of internal reviews.

Automatic, +1 from Gerrit or from whatever internal processes require,
which are not useful because these internal reviewers do not actually
review. I have seen a lot of such and I complain. It's easy to spot them
- a patchset consisting of few patches, including trivial ones, all of
them carrying one more more review tags. Even fixing a typo: reviewed
tag. Plus then you see that quality of the patchset is actually poor.

Another are real reviews done internally. If they are real, I find them
useful.

Best regards,
Krzysztof

Powered by blists - more mailing lists