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

From: Javier Martinez Canillas
Date: Wed Oct 28 2015 - 06:53:48 EST

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 reports for example:
>> Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx> (supporter:MAXIM PMIC
>> 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 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
> â Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Best regards,
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at