Message-ID: <CAPybu_2O9GVNCroLoFPGgrhs_UB97p6ng1dHgtVwbGhyr9LruA@mail.gmail.com>
Date: Fri, 29 Nov 2024 16:09:41 +0100
From: Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, 
	Hans Verkuil <hverkuil@...ll.nl>
Subject: Re: [PATCH v2 1/2] docs: media: update maintainer-entry-profile for multi-committers

Thanks for putting this together.

I have marked some minor nits here and there. You can put my
Reviewed-by: Ricardo Ribalda <ribalda@...omium.org>

The only thing that is not a nit: is changing responsible with
contributor. But if we agree on the meaning (and I think that we do)
we can always improve this doc later.

On Fri, Nov 29, 2024 at 12:15 PM Mauro Carvalho Chehab
<mchehab+huawei@...nel.org> wrote:
>
> As the media subsystem will experiment with a multi-committers model,
> update the Maintainer's entry profile to the new rules.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> Signed-off-by: Hans Verkuil <hverkuil@...ll.nl>
> ---
>  .../media/maintainer-entry-profile.rst        | 203 ++++++++++++++----
>  MAINTAINERS                                   |   1 +
>  2 files changed, 158 insertions(+), 46 deletions(-)
>
> diff --git a/Documentation/driver-api/media/maintainer-entry-profile.rst b/Documentation/driver-api/media/maintainer-entry-profile.rst
> index ffc712a5f632..47f15fad7f9f 100644
> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
> @@ -27,19 +27,133 @@ It covers, mainly, the contents of those directories:
>  Both media userspace and Kernel APIs are documented and the documentation
>  must be kept in sync with the API changes. It means that all patches that
>  add new features to the subsystem must also bring changes to the
> -corresponding API files.
> +corresponding API documentation files.
>
> -Due to the size and wide scope of the media subsystem, media's
> -maintainership model is to have sub-maintainers that have a broad
> -knowledge of a specific aspect of the subsystem. It is the sub-maintainers'
> -task to review the patches, providing feedback to users if the patches are
> +Due to the size and wide scope of the media subsystem, the media's
> +maintainership model is to have committers that have a broad knowledge of
> +a specific aspect of the subsystem. It is the committers' task to
> +review the patches, providing feedback to users if the patches are
>  following the subsystem rules and are properly using the media kernel and
>  userspace APIs.
>
> -Patches for the media subsystem must be sent to the media mailing list
> -at linux-media@...r.kernel.org as plain text only e-mail. Emails with
> -HTML will be automatically rejected by the mail server. It could be wise
> -to also copy the sub-maintainer(s).
> +Media committers
> +----------------
> +
> +In the media subsystem, there are experienced developers who can push
> +patches directly to the development tree. These developers are called
> +Media committers and are divided into the following categories:
> +
> +- Committers: responsible for one or more drivers within the media subsystem.
> +  They can push changes to the tree that do not affect the core or ABI.

Can we say contributor instead of responsible? For me responsible
means maintainer.

I would like to land patches that have been properly reviewed to the
committers tree for areas that I do not maintiain:

For example:
- Laurent has reviewed a uvc patch that I want to land asap to avoid
conflicts with other patchsets that I am working with.
- I want to land a patch for a ci breakage that has been reviewed by
another person, it is trivial, and none has a bad comment about it.
- I want to land a fix for a driver that has been properly reviewed by
the maintainer and none has a bad comment about it.


> +
> +- Core committers: responsible for part of the media core. They are typically
> +  responsible for one or more drivers within the media subsystem, but, besides
> +  that, they can also merge patches that change the code common to multiple
> +  drivers, including the kernel internal API.
> +
> +- Subsystem maintainers: responsible for the subsystem as a whole, with
> +  access to the entire subsystem.
> +
> +  Only subsystem maintainers can push changes that affect the userspace
> +  API/ABI.
> +
> +Media committers shall explicitly agree with the Kernel development process
s/Media committers/All
> +as described at Documentation/process/index.rst and to the Kernel
> +development rules inside the Kernel documentation, including its code of
> +conduct.
> +
> +Media development tree
> +----------------------
> +
> +The main development tree used by the media subsystem is hosted at LinuxTV.org,
> +where we also maintain news about the subsystem, wiki pages and a patchwork
> +instance where we track patches though their lifetime.
> +
> +The main tree used by media developers is at:
> +
> +https://git.linuxtv.org/media.git/
> +
> +.. _Media development workflow:
> +
> +Media development workflow
> +++++++++++++++++++++++++++
> +
> +All changes for the media subsystem must be sent first as e-mails to the
> +media mailing list, following the process documented at
> +Documentation/process/index.rst.
> +
> +It means that patches shall be submitted as plain text only via e-mail to:
> +
> +  `https://subspace.kernel.org/vger.kernel.org.html <linux-media@...r.kernel.org>`_

nit: Maybe this is a better url? https://lore.kernel.org/linux-media/

> +
> +Emails with HTML will be automatically rejected by the mail server.
> +
> +It could be wise to also copy the media committer(s). You should use
nit: How does someone know who the committers are? I think sending to
the ML and to ./get_maintainers.pl is enough


> +``scripts/get_maintainers.pl`` to identify whom else needs to be copied.
> +Please always copy driver's authors and maintainers.
> +
> +Such patches need to be based against a public branch or tag as follows:
> +
> +1. Patches for new features need to be based at the ``next`` branch of
> +   media.git tree;
> +
> +2. Fixes against an already released kernel should preferably be against
> +   the latest released Kernel. If they require a previously-applied
> +   change at media.git tree, they need to be against its ``fixes`` branch.

2. Fixes against an already released kernel should preferably be against
   the ``fixes`` branch of the media.git tree;

> +
> +3. Fixes for issues not present at the latest released kernel should
> +   preferably be against the latest -rc1 Kernel. If they require a
> +   previously-applied change at media.git tree, they need to be against
> +   its ``fixes`` branch.

Can we get rid of this third type? It is a bit confusing.  My mental model is:
- Things for the next kernel version: next
- Things for this kernel version: fixes

We will make sure to close the next tree when needed, and that fixes
and next are upreved accordingly.



> +
> +Patches with fixes shall have:
> +- a ``Fixes:`` tag pointing to the first commit that introduced the bug;
> +- when applicable, a ``Cc: stable@...r.kernel.org``.
> +
> +Patches that were fixing bugs publicly reported by someone at the
> +linux-media@...r.kernel.org mailing list shall have:
> +- a ``Reported-by:`` tag immediately followed by a ``Closes:`` tag.
> +
> +Patches that change API shall update documentation accordingly at the
> +same patch series.
> +
> +See Documentation/process/index.rst for more details about e-mail submission.
> +
> +Once a patch is submitted, it may follow either one of the following
> +workflows:
> +
> +a. Pull request workflow: patches are handled by subsystem maintainers::
> +
> +     +------+   +---------+   +-------+   +-----------------------+   +---------+
> +     |e-mail|-->|patchwork|-->|pull   |-->|maintainers merge      |-->|media.git|
> +     +------+   |picks it |   |request|   |in media-committers.git|   +---------+
> +                +---------+   +-------+   +-----------------------+
> +
> +   For this workflow, pull requests can be generated by a committer,
> +   a previous committer, subsystem maintainers or by a couple of trusted

I guess you mean a trusted long-time contributor, not a couple. How
can you send a PR from two people?

> +   long-time contributors. If you are not in such group, please don't submit
> +   pull requests, as they will likely be ignored.
s/be ignored/not processed/.

Sounds a bit better :).
Maybe you could even say: not processed, and the author notified.

> +
> +b. Committers' workflow: patches are handled by media committers::
> +
> +     +------+   +---------+   +--------------------+   +-----------+   +---------+
> +     |e-mail|-->|patchwork|-->|committers merge at |-->|maintainers|-->|media.git|
> +     +------+   |picks it |   |media-committers.git|   |approval   |   +---------+
> +                +---------+   +--------------------+   +-----------+
> +
> +On both workflows, all patches shall be properly reviewed at
> +linux-media@...r.kernel.org before being merged at media-committers.git.
> +
> +When patches are picked by patchwork and when merged at media-committers,
> +CI bots will check for errors and may provide e-mail feedback about
> +patch problems. When this happens, the patch submitter must fix them
> +and send another version of the patch.

must fix them, or explain why the errors are false positives.

> +
> +Patches will only be moved to the next stage in those two workflows if they
> +don't fail on CI or if there are false-positives in the CI reports.
> +
> +Failures during e-mail submission
> ++++++++++++++++++++++++++++++++++
>
>  Media's workflow is heavily based on Patchwork, meaning that, once a patch
>  is submitted, the e-mail will first be accepted by the mailing list
> @@ -47,51 +161,49 @@ server, and, after a while, it should appear at:
>
>     - https://patchwork.linuxtv.org/project/linux-media/list/
>
> -If it doesn't automatically appear there after a few minutes, then
> +If it doesn't automatically appear there after some time [2]_, then
>  probably something went wrong on your submission. Please check if the
> -email is in plain text\ [2]_ only and if your emailer is not mangling
> +email is in plain text\ [3]_ only and if your emailer is not mangling
>  whitespaces before complaining or submitting them again.
>
> -You can check if the mailing list server accepted your patch, by looking at:
> +To troubleshoot problems, you should first check if the mailing list
> +server has accepted your patch, by looking at:
>
>     - https://lore.kernel.org/linux-media/
>
> -.. [2] If your email contains HTML, the mailing list server will simply
> +If the patch is there and not at patchwork, it is likely that your e-mailer
> +mangled the patch. Patchwork internally has a logic that checks if the
> +received e-mail contain a valid patch. Any whitespace and new line
> +breakages mangling the patch won't be recognized by patchwork, thus such
> +patch will be rejected.
> +
> +.. [2] It usually takes a few minutes for the patch to arrive, but
> +       the e-mail server may be busy, so it may take up to a few hours
> +       for a patch to be picked by patchwork.
> +
> +.. [3] If your email contains HTML, the mailing list server will simply
>         drop it, without any further notice.
>
> +.. _media-developers-gpg:
>
> -Media maintainers
> -+++++++++++++++++
> +Authentication for pull and merge requests
> +++++++++++++++++++++++++++++++++++++++++++
>
> -At the media subsystem, we have a group of senior developers that
> -are responsible for doing the code reviews at the drivers (also known as
> -sub-maintainers), and another senior developer responsible for the
> -subsystem as a whole. For core changes, whenever possible, multiple
> -media maintainers do the review.
> +The authenticity of developers submitting pull requests and merge requests
> +shall be validated by using PGP sign, via the
> +:ref:`kernel_org_trust_repository`.
>
> -The media maintainers that work on specific areas of the subsystem are:
> +With the pull request workflow, pull requests shall use a GPG-signed tag.
>
> -- Remote Controllers (infrared):
> -    Sean Young <sean@...s.org>
> +For more details about PGP sign, please read
> +Documentation/process/maintainer-pgp-guide.rst.
>
> -- HDMI CEC:
> -    Hans Verkuil <hverkuil@...all.nl>
> +Subsystem maintainers
> +---------------------
>
> -- Media controller drivers:
> -    Laurent Pinchart <laurent.pinchart@...asonboard.com>
> -
> -- ISP, v4l2-async, v4l2-fwnode, v4l2-flash-led-class and Sensor drivers:
> -    Sakari Ailus <sakari.ailus@...ux.intel.com>
> -
> -- V4L2 drivers and core V4L2 frameworks:
> -    Hans Verkuil <hverkuil@...all.nl>
> -
> -The subsystem maintainer is:
> -  Mauro Carvalho Chehab <mchehab@...nel.org>
> -
> -Media maintainers may delegate a patch to other media maintainers as needed.
> -On such case, checkpatch's ``delegate`` field indicates who's currently
> -responsible for reviewing a patch.
> +The subsystem maintainers are:
> +  - Mauro Carvalho Chehab <mchehab@...nel.org> and
> +  - Hans Verkuil <hverkuil@...all.nl>
>
>  Submit Checklist Addendum
>  -------------------------
> @@ -108,17 +220,14 @@ implementing the media APIs:
>  ====================   =======================================================
>  Type                   Tool
>  ====================   =======================================================
> -V4L2 drivers\ [3]_     ``v4l2-compliance``
> +V4L2 drivers\ [4]_     ``v4l2-compliance``
>  V4L2 virtual drivers   ``contrib/test/test-media``
>  CEC drivers            ``cec-compliance``
>  ====================   =======================================================
>
> -.. [3] The ``v4l2-compliance`` also covers the media controller usage inside
> +.. [4] The ``v4l2-compliance`` also covers the media controller usage inside
>         V4L2 drivers.
>
> -Other compilance tools are under development to check other parts of the
> -subsystem.
> -
>  Those tests need to pass before the patches go upstream.
>
>  Also, please notice that we build the Kernel with::
> @@ -134,6 +243,8 @@ Where the check script is::
>  Be sure to not introduce new warnings on your patches without a
>  very good reason.
>
> +Please see `Media development workflow`_ for e-mail submission rules.
> +
>  Style Cleanup Patches
>  +++++++++++++++++++++
>
> @@ -199,7 +310,7 @@ tree between -rc6 and the next -rc1.
>  Please notice that the media subsystem is a high traffic one, so it
>  could take a while for us to be able to review your patches. Feel free
>  to ping if you don't get a feedback in a couple of weeks or to ask
> -other developers to publicly add Reviewed-by and, more importantly,
> +other developers to publicly add ``Reviewed-by:`` and, more importantly,
>  ``Tested-by:`` tags.
>
>  Please note that we expect a detailed description for ``Tested-by:``,
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6db07b8fa215..f9bdef1b5966 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14193,6 +14193,7 @@ MEDIA INPUT INFRASTRUCTURE (V4L/DVB)
>  M:     Mauro Carvalho Chehab <mchehab@...nel.org>
>  L:     linux-media@...r.kernel.org
>  S:     Maintained
> +P:     Documentation/driver-api/media/maintainer-entry-profile.rst
>  W:     https://linuxtv.org
>  Q:     http://patchwork.kernel.org/project/linux-media/list/
>  T:     git git://linuxtv.org/media_tree.git
> --
> 2.47.0
>
>


-- 
Ricardo Ribalda

Powered by blists - more mailing lists