Re: [PATCH v19 2/9] usb: dwc3: core: Access XHCI address space temporarily to read port info

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux