Re: [PATCH] MAINTAINERS: Start using the 'reviewer' (R) tag

[Javier Martinez Canillas]

Hello Lee,

On Wed, Oct 28, 2015 at 11:28 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
> On Wed, 28 Oct 2015, Javier Martinez Canillas wrote:
>> On Wed, Oct 28, 2015 at 9:24 AM, Lee Jones <lee.jones@xxxxxxxxxx> wrote:
>> > On Tue, 2015-10-27 at 18:15 +0000, Lee Jones wrote:
>> >> On Tue, 27 Oct 2015, Sebastian Reichel wrote:
>> >> > On Tue, Oct 27, 2015 at 03:42:37PM +0000, Lee Jones wrote:
>> >> > > Since eafbaac ("MAINTAINERS: Add "R:" designated-reviewers tag") we
>> >> > > have been able to tag specific people as Reviewers.  These are key
>> >> > > individuals who are tasked with or volunteer to review code submitted
>> >> > > to a subsystem or specific file.  However, according to MAINTAINERS
>> >> > > we have 1046 Maintainers and only a mere 22 Reviewers.  I believe
>> >> > > these numbers to be incorrect, as many of these Maintainers are in
>> >> > > fact Reviewers.
>> >
>> > Most entries in MAINTAINERS seem to be vanity entries than actual
>> > active participants.  A person typically writes a driver, adds a
>> > MAINTAINER entry, then forgets about it and/or the hardware becomes
>> > outdated.
>> >
>> > This I agree with.
>> >
>> > On Wed, 28 Oct 2015, Krzysztof Kozlowski wrote:
>> >> 2015-10-28 3:44 GMT+09:00 Joe Perches <joe@xxxxxxxxxxx>:
>> >> > On Tue, 2015-10-27 at 18:15 +0000, Lee Jones wrote:
>> >> > > On Tue, 27 Oct 2015, Sebastian Reichel wrote:> >
>> >> > > > I think you should CC the people, which are changed from "M:" to
>> >> > > > "R:", though.
>> >> > >
>> >> > > Yes, makes sense.
>> >> > >
>> >> > > I'd like to collect some Maintainer Acks first though.
>> >> >
>> >> > I think people from organizations like Samsung are actual
>> >> > maintainers not reviewers.
>> >
>> > So this all hinges on how we are describing Maintainers and Reviewers.
>> >
>> > My personal definition (until convinced otherwise) is that Reviewers
>> > care about their particular subsystem and/or files.  They conduct code
>> > reviews to ensure nothing gets broken and the code base stays in best
>> > possible state of worthiness.  On the other hand Maintainers usually
>> > conduct themselves as Reviewers but also have 'maintainership' duties
>> > as well; such as applying patches, *maintaining*, testing, rebasing,
>> > etc, an upstream branch and ultimately sending pull-requests to higher
>> > level Maintainers i.e. Linus.  Maintainers also have the ultimate say
>> > (unless over-ruled by Linus etc) over what gets applied.
>> >
>> >> > Their drivers are not thrown over a wall and forgotten.
>> >>
>>
>> I've a different definition. For me it depends on much do you care
>> about the component. For example I maintain a couple of drivers in the
>> kernel and Device Tree files for some boards that are important to me
>> but I also care about some other subsystems (i.e: Exynos SoC support)
>> and I act as a reviewer (although I'm not officially listed as
>> reviewer in the MAINTAINERS file).
>
> I wish to make this clear from the out-set.  If you have no obligation
> to review patches but do so occasionally anyway, that does not make
> you the type of Reviewer that we're speaking about here.  Anyone can
> review any patch on the list that they wish to, which is lovely, but
> it won't carry the same authority (for want of a better expression) as
> if you were tagged as an official Reviewer in MAINTAINERS.
>

We agree on that.

>> We do have in fact different tags for each type of involvement so I
>> usually answer with a Reviewed-by tag if I review code for a subsystem
>> I care but I don't maintainer or answer with an Acked-by tag if I
>> review *and agree* with a patch for a component I maintain (so the
>> maintainer knows that is good to apply differently from the list if
>> needed).
>
> I think you need to re-read what those tags mean.
>
>   Documentation/SubmittingPatches
>

I know that document of course but I went and read the tags
description again and I don't see how that document supports your
arguments. Can you please share the paragraphs you are referring to?

>> Now, that doesn't mean that I provide a pull request for the drivers
>> or boards I maintain on every release since that will depend on the
>> number of patches posted for that component per release. So if there
>> are only a couple of patches, I think is easier for the subsystem
>> maintainer to pick those directly from the list but if there are a lot
>> of them, then the maintainer may ask me to prepare a branch to pull
>> and I've done in the past for drivers I maintain to be sure that the
>> patches in the list are applied in the right order, no needed patches
>> were missed, etc.
>
> I have also submitted patches via a pull-request as a Submitter to
> different subsystems.  That does not mean I should automatically be
> classed as a Maintainer.
>

Yes I agree that preparing pull requests doesn't make you a maintainer
but you are the one using maintain a branch / sending pull requests as
classification method. My point is that this is orthogonal to being a
maintainer or reviewer.

>> Another difference is that when I'm listed as a maintainer, I feel an
>> obligation to answer to the patches touching that component but that's
>> not the case for components I usually act as a reviewer, I may review
>> it if I have time but if I don't, I let other people to review it.
>
> Then, in the latter case you shouldn't be listed as a Reviewer in my
> example.  Anyone listed as a Reviewer in MAINTAINERS *does* have that
> obligation.  That's what it means.  If Reviewers don't review, they
> should be removed from MAINTAINERS.
>

Again we agree on that, that's why I said that I'm *not* officially
listed as a reviewer for Exynos SoC patches since even when I'm
interested on that, I don't have time to review every single patch.

>> >> At least for Samsung Multifunction PMIC drivers (and some of Maxim
>> >> MUICs and PMICs) these are actively used by us in existing and new
>> >> products. They are also continuously extended and actually maintained.
>> >> This means that it is not only about review of new patches but also
>> >> about caring that nothing will become broken.
>> >
>> > Exactly.  This what I expect of any good code Reviewer.
>> >
>> >> I would prefer to leave the "SAMSUNG MULTIFUNCTION PMIC DEVICE
>> >> DRIVERS" entry as is - maintainers.
>>
>> I agree with Krzysztof here, I would prefer to keep them as
>> maintainers if they are maintaining the drivers.
>
> But they're not.  They're reviewing and caring like a good Reviewer
> should.
>

That's your opinion, as I said my opinion is that they are maintaining
it because they care that the drivers are in good shape, testing that
no regressions are introduced, fixing bugs, etc.

>> > But you aren't maintaining the driver i.e. you don't collect patches
>> > and *maintain* them on an upstream branch.  Granted some of you guys
>> > are doing a great job of maintaining branches on your downstream or
>> > BSP kernels, but conduct a Reviewer type role for upstream.
>> >
>> > You guys are pushing back like this is some kind of demotion.  That's
>> > not the case at all.  All it does is better describe the (very worthy)
>> > function you *actually* provide.
>> >
>>
>> But I think it makes description less accurate in fact, since without
>> $SUBJECT get_maintainers.pl reports for example:
>>
>> Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx> (supporter:MAXIM PMIC
>> AND MUIC DRIVERS FOR EXYNOS BASED BO...)
>> Lee Jones <lee.jones@xxxxxxxxxx> (supporter:MULTIFUNCTION DEVICES (MFD))
>>
>> and after the change:
>>
>> Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx> (reviewer)
>> Lee Jones <lee.jones@xxxxxxxxxx> (supporter:MULTIFUNCTION DEVICES (MFD))
>>
>> He also works for Samsung so the driver is not only maintained but
>> supported since he can actually take care of it as a part of his day
>> job (if I understood correctly).
>
> It's not the person that's supported, it's the driver.  The driver
> doesn't need to change state.
>

Yes, is the driver that is supported but by whom? In my opinion the
supporter should be the maintainer of the driver and that is what
get_maintainer.pl thinks as well.

So in summary, you think that the difference between a maintainer and
a reviewer is if a branch with fixes / new features are kept and pull
requests sent while I think that the difference is the level of
involvement someone has with a driver regardless of how patches ends
in the subsystem tree (picked directly by subsystem maintainers or
sent through pull requests).

Is the first time I heard your definition but maybe I'm the one that
is wrong so it would be great to get a consensus on that and get it
documented somewhere.

> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org â Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/