Message-ID: <2215c3f7-9d45-4c7f-a626-e78dbf68a455@xs4all.nl>
Date: Mon, 2 Dec 2024 15:49:08 +0100
From: Hans Verkuil <hverkuil@...all.nl>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 Hans Verkuil <hverkuil@...ll.nl>, Ricardo Ribalda <ribalda@...omium.org>
Subject: Re: [PATCH v3 1/3] docs: media: update maintainer-entry-profile for
 multi-committers

On 02/12/2024 14:54, Mauro Carvalho Chehab wrote:
> Em Mon, 2 Dec 2024 14:17:48 +0100
> Hans Verkuil <hverkuil@...all.nl> escreveu:
> 
>> Hi Mauro,
>>
>> On 02/12/2024 10:26, Mauro Carvalho Chehab 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>  
>>
>> xs4ll.nl -> xs4all.nl
>>
>> (it's probably my typo, but please fix this in the final version)
> 
> Yes, I copied and pasted it. Will fix at the next version.
> 
>>
>>> Reviewed-by: Ricardo Ribalda <ribalda@...omium.org>
>>> ---
>>>  .../media/maintainer-entry-profile.rst        | 208 ++++++++++++++----
>>>  MAINTAINERS                                   |   1 +
>>>  2 files changed, 163 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..dc764163cf1c 100644
>>> --- a/Documentation/driver-api/media/maintainer-entry-profile.rst
>>> +++ b/Documentation/driver-api/media/maintainer-entry-profile.rst
>>> @@ -27,19 +27,139 @@ 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:
>>> +    contributors for one or more drivers within the media subsystem.
>>> +    They can push changes to the tree that do not affect the core or ABI.
>>> +
>>> +- 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.
>>> +
>>> +All media committers shall explicitly agree with the Kernel development process
>>> +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
>>> +linux-media@...r.kernel.org. While subscription is not mandatory, you
>>> +can find details about how to subscribe to it and to see its archives at:
>>> +
>>> +  https://subspace.kernel.org/vger.kernel.org.html
>>> +
>>> +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
>>> +``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.  
>>
>> What is an "released kernel"? Does an -rcX kernel qualify?
>>> +
>>> +3. Fixes for issues not present at the latest released kernel shall
>>> +   be either against a -rc kernel for an upcoming release or
>>> +   against the ``fixes`` branch of the media.git tree.  
>>
>> 2 and 3 remain vague, IMO. Not a blocker, but I think this needs more work
>> at some point.
> 
> Some Kernel documents use "mainline release" to refer to Kernel final
> releases.
> 
> One option to make it clearer would be to add a link to:
> 	https://www.kernel.org/category/releases.html
> 
> And then use the terms:
> 	- "Mainline release" for v6.10, v6.11, v6.12...
> 	- "Prepatch release" for -rc1, -rc2, -rc3...
> 
> What 2 and means are:
> 
> 	1. Features for the next mainline release:
> 
> 	   - baseline shall be media.git ``next`` branch;
> 
> 	2. Fixes bugs at mainline releases:
> 
> 	   - baseline shall be the latest mainline release or media.git ``fixes``  
> 	     if changes depend on a fix already merged;
> 
> 	3. Fixes for the next mainline release:
> 
> 	   - baseline shall be a prepatch release or media.git ``fixes``  
> 	     if changes depend on a fix already merged;

If it is for the next mainline release, then I would also be fine with 'next'
as baseline. That avoids having to modify the patch if it conflicts with
new work that went into next. Having to base a fix on the 'fixes' branch is
rare in this case. You typically only need to do that in case 2.

> 
> 	.. Note:
> 
> 	   See https://www.kernel.org/category/releases.html for an overview
> 	   about Kernel release types.
>   
> Would the above text work best you? If not please propose another
> text.

Yes, that's much better.

> 
> In practice, we'll likely accept other baselines for fixes as
> well, for practical reasons. From my side, I don't have any issues
> if I need to rebase a tested bugfix patch produced against an
> older Kernel send by an occasional contributor, if it only has 
> trivial merge conflicts.

Sure, these three points are really about our preference since following
that reduces the chance of merge conflicts.

We can say something like this:

"To minimize the chance of merge conflicts for your patch series, we recommend
that you use the following baseline for your patch series:"

> 
>>> +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 trusted long-time
>>> +   contributor. If you are not in such group, please don't submit
>>> +   pull requests, as they will not be processed.
>>> +
>>> +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, 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 +167,48 @@ 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  
>>
>> a logic -> logic
>>
>>> +received e-mail contain a valid patch. Any whitespace and new line  
>>
>> contain -> contains
>>
>>> +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. See: :ref:`kernel_org_trust_repository`.  
>>
>> sign -> signing
>>
>> Hmm, merge requests through gitlab are not signed. So this isn't correct, I
>> think.
> 
> No, but they are authenticated based on gitlab's user ID. The authentication
> we'll have is when the new committer sends us an e-mail providing the
> gitlab's user ID.
> 
> See patch 3, as it contains some changes aiming to better explain it. Once
> it get the same people reviewing that also reviewed 1 and 2, I'll fold it.
> 
>>
>>>  
>>> -The media maintainers that work on specific areas of the subsystem are:
>>> +With the pull request workflow, pull requests shall use a PGP-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 +225,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  
>>
>> The ``v4l2-compliance`` utility
>>
>> (add 'utility')
> 
> Ok.
> 
>>
>>>         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 +248,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 +315,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 1e930c7a58b1..c77f56a2e695 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -14510,6 +14510,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/  
>>
>> Shouldn't this point to our patchwork instance instead?
> 
> Good catch! I'll update it.
> 
>>
>>>  T:	git git://linuxtv.org/media.git  

Regards,

	Hans

Powered by blists - more mailing lists