media: add a subsystem profile documentation

Commit Message

Mauro Carvalho Chehab Sept. 13, 2019, 4:19 p.m. UTC

Document the basic policies of the media subsystem profile.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
---

That's basically a modified version of:
    https://patchwork.linuxtv.org/patch/52999/

Applied to the new template

 Documentation/media/index.rst                 |   1 +
 .../media/maintainer-entry-profile.rst        | 140 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 3 files changed, 142 insertions(+)
 create mode 100644 Documentation/media/maintainer-entry-profile.rst

Comments

Kees Cook Sept. 17, 2019, 3:35 a.m. UTC | #1

On Fri, Sep 13, 2019 at 01:19:21PM -0300, Mauro Carvalho Chehab wrote:
> Document the basic policies of the media subsystem profile.
> 
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> ---
> 
> That's basically a modified version of:
>     https://patchwork.linuxtv.org/patch/52999/
> 
> Applied to the new template
> 
>  Documentation/media/index.rst                 |   1 +
>  .../media/maintainer-entry-profile.rst        | 140 ++++++++++++++++++

One idea I proposed to Dan at the Maintainer's Summit was to collect all
the profiles is a single directory in Documentation/, since there are
two ways someone would want to read profiles:

1) a single profile, based on a MAINTAINERS entry which includes the path
2) all of them, to study for various reasons

The #2 case is helped by having them all in one directory with a single
index.rst, etc. Then similar profiles are able to merge, etc.

Mauro Carvalho Chehab Sept. 17, 2019, 1:28 p.m. UTC | #2

Hi Kees,

Em Mon, 16 Sep 2019 20:35:45 -0700
Kees Cook <[e-mail]> escreveu:

> On Fri, Sep 13, 2019 at 01:19:21PM -0300, Mauro Carvalho Chehab wrote:
> > Document the basic policies of the media subsystem profile.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > ---
> > 
> > That's basically a modified version of:
> >     https://patchwork.linuxtv.org/patch/52999/
> > 
> > Applied to the new template
> > 
> >  Documentation/media/index.rst                 |   1 +
> >  .../media/maintainer-entry-profile.rst        | 140 ++++++++++++++++++  
> 
> One idea I proposed to Dan at the Maintainer's Summit was to collect all
> the profiles is a single directory in Documentation/, 

No matter where the profiles will physically be stored, its contents belong 
to subsystem-specific documentation, and should be visible at the same index 
file as the kAPI docs is located, as anyone interested on submitting patches
for a subsystem should be aware about the subsystem specific policies and
details.

So, my vote is to store them at Documentation/*/<subsystem> (together
with the kAPI book).

> since there are
> two ways someone would want to read profiles:
> 
> 1) a single profile, based on a MAINTAINERS entry which includes the path

That is the common case. The problem is that the MAINTAINERS file
currently doesn't generate html output. This is not a problem for
"old school" devs, but a newbie wanting to collaborate to a single
specific subsystem may not notice - or understand - the importance
of the MAINTAINERS file[1].

[1] btw, that's why I submitted a RFC, several years ago, a patch
converting it to ReST - and later - another patch that would be parsing
its contents and producing a ReST output.

That's, by far, the most relevant usecase for the profiles, as the
audience is the ~4k Kernel developers.

> 2) all of them, to study for various reasons

I suspect that only core people will have interest on study them.

Those people are more skilled, and can easily find all those files with
a simple grep:

	$ grep  "^P:\s" MAINTAINERS|cut -d':' -f2-

or

	$ git grep "^P:\s" MAINTAINERS|cut -d: -f3-

If, for whatever reason, he would prefer an HTML output [1], he could even
create an index file with all of those with something like:

	$ for i in $(grep  "^P:\s" MAINTAINERS|cut -d':' -f2-); do j=${i/rst/html}; echo "<a href=\"$j\">$j</a><br/>"; done >Documentation/output/index_profiles.html

We might, instead, teach the Documentation/Makefile to create something
like the above, but, IMHO, that would just add more complexity to the
build without a good reason.

[1] I doubt that core devs would prefer seeing this in html form, but some
variant of the above could be used, for example, to create symlinks for
all profile docs into a "study" directory.

> The #2 case is helped by having them all in one directory with a single
> index.rst, etc. Then similar profiles are able to merge, etc.


Thanks,
Mauro

Kees Cook Sept. 17, 2019, 4:33 p.m. UTC | #3

On Tue, Sep 17, 2019 at 10:28:17AM -0300, Mauro Carvalho Chehab wrote:
> No matter where the profiles will physically be stored, its contents belong 
> to subsystem-specific documentation, and should be visible at the same index 
> file as the kAPI docs is located, as anyone interested on submitting patches
> for a subsystem should be aware about the subsystem specific policies and
> details.

That's a good point. I think your other suggestions below address my
"find them all" case...

> So, my vote is to store them at Documentation/*/<subsystem> (together
> with the kAPI book).
> 
> > since there are
> > two ways someone would want to read profiles:
> > 
> > 1) a single profile, based on a MAINTAINERS entry which includes the path
> 
> That is the common case. The problem is that the MAINTAINERS file
> currently doesn't generate html output. This is not a problem for
> "old school" devs, but a newbie wanting to collaborate to a single
> specific subsystem may not notice - or understand - the importance
> of the MAINTAINERS file[1].
> 
> [1] btw, that's why I submitted a RFC, several years ago, a patch
> converting it to ReST - and later - another patch that would be parsing
> its contents and producing a ReST output.
> 
> That's, by far, the most relevant usecase for the profiles, as the
> audience is the ~4k Kernel developers.

Oh yes, having a .rst of the MAINTAINERS file would be pretty great.
Seems like it could just be a build target (and then dependency) for the
sphinx targets?

> > 2) all of them, to study for various reasons
> 
> I suspect that only core people will have interest on study them.
> 
> Those people are more skilled, and can easily find all those files with
> a simple grep:
> 
> 	$ grep  "^P:\s" MAINTAINERS|cut -d':' -f2-
> 
> or
> 
> 	$ git grep "^P:\s" MAINTAINERS|cut -d: -f3-
> 
> If, for whatever reason, he would prefer an HTML output [1], he could even
> create an index file with all of those with something like:
> 
> 	$ for i in $(grep  "^P:\s" MAINTAINERS|cut -d':' -f2-); do j=${i/rst/html}; echo "<a href=\"$j\">$j</a><br/>"; done >Documentation/output/index_profiles.html
> 
> We might, instead, teach the Documentation/Makefile to create something
> like the above, but, IMHO, that would just add more complexity to the
> build without a good reason.
> 
> [1] I doubt that core devs would prefer seeing this in html form, but some
> variant of the above could be used, for example, to create symlinks for
> all profile docs into a "study" directory.
> 
> > The #2 case is helped by having them all in one directory with a single
> > index.rst, etc. Then similar profiles are able to merge, etc.

Whatever the case, please don't let me distract from the actual content
of these profiles: I think it's awesome to capture these details and
makes my life so much easier. :)

Mauro Carvalho Chehab Sept. 18, 2019, 11:23 a.m. UTC | #4

Em Tue, 17 Sep 2019 09:33:11 -0700
Kees Cook <[e-mail]> escreveu:

> On Tue, Sep 17, 2019 at 10:28:17AM -0300, Mauro Carvalho Chehab wrote:
> > No matter where the profiles will physically be stored, its contents belong 
> > to subsystem-specific documentation, and should be visible at the same index 
> > file as the kAPI docs is located, as anyone interested on submitting patches
> > for a subsystem should be aware about the subsystem specific policies and
> > details.  
> 
> That's a good point. I think your other suggestions below address my
> "find them all" case...
> 
> > So, my vote is to store them at Documentation/*/<subsystem> (together
> > with the kAPI book).
> >   
> > > since there are
> > > two ways someone would want to read profiles:
> > > 
> > > 1) a single profile, based on a MAINTAINERS entry which includes the path  
> > 
> > That is the common case. The problem is that the MAINTAINERS file
> > currently doesn't generate html output. This is not a problem for
> > "old school" devs, but a newbie wanting to collaborate to a single
> > specific subsystem may not notice - or understand - the importance
> > of the MAINTAINERS file[1].
> > 
> > [1] btw, that's why I submitted a RFC, several years ago, a patch
> > converting it to ReST - and later - another patch that would be parsing
> > its contents and producing a ReST output.
> > 
> > That's, by far, the most relevant usecase for the profiles, as the
> > audience is the ~4k Kernel developers.  
> 
> Oh yes, having a .rst of the MAINTAINERS file would be pretty great.
> Seems like it could just be a build target (and then dependency) for the
> sphinx targets?

You can't simply rename MAINTAINERS to .rst and let Sphinx handle it,
as:

	- The instructions at the top will be badly parsed;
	- It will also parse wrong the entries.

On the patches I wrote several years ago, I fixed the instructions
to make them to produce something that would produce a good output.
That's the easiest part.

For the entries contents, the simplest solution would be something like:

	diff --git a/MAINTAINERS b/MAINTAINERS
	index 6b4bc320e6ab..ae2afb14ae01 100644
	--- a/MAINTAINERS
	+++ b/MAINTAINERS
	@@ -147,4 +147,4 @@ F:	drivers/net/ethernet/3com/3c59x.c
	-M:	David Dillow <[e-mail]>
	-L:	[e-mail]
	-S:	Maintained
	-F:	drivers/net/ethernet/3com/typhoon*
	+:M:	David Dillow <[e-mail]>
	+:L:	[e-mail]
	+:S:	Maintained
	+:F:	drivers/net/ethernet/3com/typhoon*

A trivial change for a normal file, but doing this at MAINTAINERS
will be really painful, as it will cause lots of conflicts.

So, IMO, the best way to do it is to have a script (or to teach
get_maintainers.pl) to produce a ReST output that would do the
above.

The latest RFC about that with I sent was this one:

	https://www.mail-archive.com/[e-mail]/msg1238576.html

I would probably address this on a different way those days.

A simple/lazy solution would be to apply the enclosed patch - or a
variant of it that would place the contents of MAINTAINERS outside
process/index.html, and add instructions about how to use
get_maintainers.pl.

Jon,

Please let me know if you're willing to accept something like that.

Thanks,
Mauro

[PATCH RFC] docs: process: add MAINTAINERS file contents

Anyone working with the Kernel need to consider the contents of the
MAINTAINERS file. So, add it to the ReST output.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

diff --git a/Documentation/process/index.rst b/Documentation/process/index.rst
index e2c9ffc682c5..22e5b42b8470 100644
--- a/Documentation/process/index.rst
+++ b/Documentation/process/index.rst
@@ -59,6 +59,9 @@ lack of a better place.
    volatile-considered-harmful
    clang-format
 
+.. include:: ../../MAINTAINERS
+   :literal:
+
 .. only::  subproject and html
 
    Indices

Laurent Pinchart Sept. 18, 2019, 12:36 p.m. UTC | #5

Hi Mauro,

On Fri, Sep 13, 2019 at 01:19:21PM -0300, Mauro Carvalho Chehab wrote:
> Document the basic policies of the media subsystem profile.
> 
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> ---
> 
> That's basically a modified version of:
>     https://patchwork.linuxtv.org/patch/52999/
> 
> Applied to the new template
> 
>  Documentation/media/index.rst                 |   1 +
>  .../media/maintainer-entry-profile.rst        | 140 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  3 files changed, 142 insertions(+)
>  create mode 100644 Documentation/media/maintainer-entry-profile.rst
> 
> diff --git a/Documentation/media/index.rst b/Documentation/media/index.rst
> index 0301c25ff887..ac94685b822a 100644
> --- a/Documentation/media/index.rst
> +++ b/Documentation/media/index.rst
> @@ -12,6 +12,7 @@ Linux Media Subsystem Documentation
>  .. toctree::
>     :maxdepth: 2
>  
> +   maintainer-entry-profile
>     media_uapi
>     media_kapi
>     dvb-drivers/index
> diff --git a/Documentation/media/maintainer-entry-profile.rst b/Documentation/media/maintainer-entry-profile.rst
> new file mode 100644
> index 000000000000..81d3b0f9c36a
> --- /dev/null
> +++ b/Documentation/media/maintainer-entry-profile.rst
> @@ -0,0 +1,140 @@
> +Media Subsystem Profile
> +=======================
> +
> +Overview
> +--------
> +
> +The media subsystem cover support for a variety of devices: stream

s/cover/covers/

> +capture, analog and digital TV, cameras, remote controllers, HDMI CEC
> +and media pipeline control.
> +
> +It covers, mainly, the contents of those directories:
> +
> +  - drivers/media
> +  - drivers/staging/media
> +  - Documentation/media
> +  - include/media
> +
> +Both media userspace and Kernel APIs are documented and should be kept in
> +sync with the API changes. It means that all patches that add new
> +features to the subsystem should also bring changes to the corresponding
> +API files.
> +
> +Also, patches for device drivers that changes the Open Firmware/Device
> +Tree bindings should be reviewed by the Device Tree maintainers.

You may want to make it clear that this covers bindings only, not driver
code that parses the DT. I would just remove "for device drivers", as
the rule should also apply to DT bindings documentation that is not
driver-specific.

> +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 an specific aspect of the subsystem. It is a
> +sub-maintainers task to review the patches, providing feedback to users

s/sub-maintainers/sub-maintainer's/

> +if the patches are following the subsystem rules and are properly using
> +the media internal and external APIs.
> +
> +Patches for the media subsystem should be sent to the media mailing list
> +at [e-mail] as plain text only e-mail. Emails with
> +HTML will be automatically rejected by the mail server. There's no need
> +to copy the maintainer or sub-maintainer(s).

There's too much traffic on mailing lists for me to follow everything, I
much prefer being CC'ed on patches.

> +Media's workflow is heavily based on Patchwork, meaning that, once a patch
> +is submitted, it should appear at:
> +
> +   - https://patchwork.linuxtv.org/project/linux-media/list/
> +
> +If it doesn't automatically appear there after a few minutes, then
> +probably something got wrong on your submission. Please check if the
> +email is in plain text only and if your emailer is not mangling with
> +whitespaces before complaining or submit it again.
> +
> +Sub-maintainers
> ++++++++++++++++
> +
> +At the media subsystem, we have a group of senior developers that are

How about "experienced" instead of "senior" ? Quality doesn't always
come with age, neither for people nor wine :-)

> +responsible for doing the code reviews at the drivers (called
> +sub-maintainers), and another senior developer responsible for the
> +subsystem as a hole. For core changes, whenever possible, multiple
> +media (sub-)maintainers do the review.
> +
> +The sub-maintainers work on specific areas of the subsystem, as
> +described below:
> +
> +Digital TV:
> +  Sean Young <[e-mail]>
> +
> +HDMI CEC:
> +  Hans Verkuil <[e-mail]>
> +
> +Media controller drivers:
> +  Laurent Pinchart <[e-mail]>
> +
> +Remote Controllers:
> +  Sean Young <[e-mail]>
> +
> +Sensor drivers:
> +  Sakari Ailus <[e-mail]>
> +
> +V4L2 drivers:
> +  Hans Verkuil <[e-mail]>
> +
> +Submit Checklist Addendum
> +-------------------------
> +
> +There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
> +that should be used in order to check if the drivers are properly
> +implementing the media APIs.
> +
> +Those tests need to be passed before the patches go upstream.

s/need to be passed/need to pass/

> +
> +Also, please notice that we build the Kernel with::
> +
> +	make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script
> +
> +Where the check script is::
> +
> +	#!/bin/bash
> +	/devel/smatch/smatch -p=kernel $@ >&2
> +	/devel/sparse/sparse $@ >&2
> +
> +Be sure to not introduce new warnings on your patches.

While static analysers deliver value, I fear this is a high barrier to
entry for new contributors.

> +
> +Key Cycle Dates
> +---------------
> +
> +New submissions can be sent at any time, but if they intend to hit the
> +next merge window they should be sent before -rc5, and ideally stabilized
> +in the linux-media branch by -rc6.
> +
> +Coding Style Addendum
> +---------------------
> +
> +Media development uses checkpatch on strict mode to verify the code style, e. g.::
> +
> +	$ ./script/checkpatch.pl --strict

But we still accept some warnings. I think this should be documented.

> +
> +Review Cadence
> +--------------
> +
> +Provided that your patch is at https://patchwork.linuxtv.org, it should
> +be sooner or later handled, so you don't need to re-submit a patch.
> +
> +Except for bug fixes, we don't usually add new patches to the development
> +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 on a couple of weeks or to ask

s/on a/in a/

> +other developers to publicly add Reviewed-by and, more importantly,
> +Tested-by tags.
> +
> +Please notice that we expect a detailed description for Tested-by,

s/notice/note/

> +identifying what boards were used at the test and what it was tested.
> +
> +Style Cleanup Patches
> +---------------------
> +
> +Style-cleanups are welcome when they come together with other changes
> +at the files where the style changes will affect.
> +
> +We may accept pure standalone style-cleanups, but they should be grouped
> +per directory. So, for example, if you're doing a cleanup at drivers
> +under drivers/media, please send a single patch for all drivers under
> +drivers/media/pci, another one for drivers/media/usb and so on.

How about also stating that if the cleanup is low volume, a single patch
for the whole subsystem is preferred ? I think that should actually be
the rule, with a split to ease review only when the patch would be too
big.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7c62b45201d7..e7f44ec05a42 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10077,6 +10077,7 @@ W:	https://linuxtv.org
>  Q:	http://patchwork.kernel.org/project/linux-media/list/
>  T:	git git://linuxtv.org/media_tree.git
>  S:	Maintained
> +P:	Documentation/media/maintainer-entry-profile.rst
>  F:	Documentation/devicetree/bindings/media/
>  F:	Documentation/media/
>  F:	drivers/media/

Mauro Carvalho Chehab Sept. 18, 2019, 1:57 p.m. UTC | #6

Em Wed, 18 Sep 2019 15:36:20 +0300
Laurent Pinchart <[e-mail]> escreveu:

> Hi Mauro,
> 
> On Fri, Sep 13, 2019 at 01:19:21PM -0300, Mauro Carvalho Chehab wrote:
> > Document the basic policies of the media subsystem profile.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > ---
> > 
> > That's basically a modified version of:
> >     https://patchwork.linuxtv.org/patch/52999/
> > 
> > Applied to the new template
> > 
> >  Documentation/media/index.rst                 |   1 +
> >  .../media/maintainer-entry-profile.rst        | 140 ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  3 files changed, 142 insertions(+)
> >  create mode 100644 Documentation/media/maintainer-entry-profile.rst
> > 
> > diff --git a/Documentation/media/index.rst b/Documentation/media/index.rst
> > index 0301c25ff887..ac94685b822a 100644
> > --- a/Documentation/media/index.rst
> > +++ b/Documentation/media/index.rst
> > @@ -12,6 +12,7 @@ Linux Media Subsystem Documentation
> >  .. toctree::
> >     :maxdepth: 2
> >  
> > +   maintainer-entry-profile
> >     media_uapi
> >     media_kapi
> >     dvb-drivers/index
> > diff --git a/Documentation/media/maintainer-entry-profile.rst b/Documentation/media/maintainer-entry-profile.rst
> > new file mode 100644
> > index 000000000000..81d3b0f9c36a
> > --- /dev/null
> > +++ b/Documentation/media/maintainer-entry-profile.rst
> > @@ -0,0 +1,140 @@
> > +Media Subsystem Profile
> > +=======================
> > +
> > +Overview
> > +--------
> > +
> > +The media subsystem cover support for a variety of devices: stream  
> 
> s/cover/covers/
> 
> > +capture, analog and digital TV, cameras, remote controllers, HDMI CEC
> > +and media pipeline control.
> > +
> > +It covers, mainly, the contents of those directories:
> > +
> > +  - drivers/media
> > +  - drivers/staging/media
> > +  - Documentation/media
> > +  - include/media
> > +
> > +Both media userspace and Kernel APIs are documented and should be kept in
> > +sync with the API changes. It means that all patches that add new
> > +features to the subsystem should also bring changes to the corresponding
> > +API files.
> > +
> > +Also, patches for device drivers that changes the Open Firmware/Device
> > +Tree bindings should be reviewed by the Device Tree maintainers.  
> 
> You may want to make it clear that this covers bindings only, not driver
> code that parses the DT. I would just remove "for device drivers", as
> the rule should also apply to DT bindings documentation that is not
> driver-specific.
> 
> > +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 an specific aspect of the subsystem. It is a
> > +sub-maintainers task to review the patches, providing feedback to users  
> 
> s/sub-maintainers/sub-maintainer's/
> 
> > +if the patches are following the subsystem rules and are properly using
> > +the media internal and external APIs.
> > +
> > +Patches for the media subsystem should be sent to the media mailing list
> > +at [e-mail] as plain text only e-mail. Emails with
> > +HTML will be automatically rejected by the mail server. There's no need
> > +to copy the maintainer or sub-maintainer(s).  
> 
> There's too much traffic on mailing lists for me to follow everything, I
> much prefer being CC'ed on patches.

Well, by using patchwork, the best is to take a look on it at least for
the patches that you're interested. You could script something using
pwclient in order to make it easier.

Anyway, not sure if the other sub-maintainers see the same way. From my side,
I prefer not to be c/c, as this is just more noise, as I just rely on
patchwork for media patches. What about changing this to:

	Patches for the media subsystem should be sent to the media mailing list
	at [e-mail] 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's workflow is heavily based on Patchwork, meaning that, once a patch
> > +is submitted, it should appear at:
> > +
> > +   - https://patchwork.linuxtv.org/project/linux-media/list/
> > +
> > +If it doesn't automatically appear there after a few minutes, then
> > +probably something got wrong on your submission. Please check if the
> > +email is in plain text only and if your emailer is not mangling with
> > +whitespaces before complaining or submit it again.
> > +
> > +Sub-maintainers
> > ++++++++++++++++
> > +
> > +At the media subsystem, we have a group of senior developers that are  
> 
> How about "experienced" instead of "senior" ? Quality doesn't always
> come with age, neither for people nor wine :-)
> 
> > +responsible for doing the code reviews at the drivers (called
> > +sub-maintainers), and another senior developer responsible for the
> > +subsystem as a hole. For core changes, whenever possible, multiple
> > +media (sub-)maintainers do the review.
> > +
> > +The sub-maintainers work on specific areas of the subsystem, as
> > +described below:
> > +
> > +Digital TV:
> > +  Sean Young <[e-mail]>
> > +
> > +HDMI CEC:
> > +  Hans Verkuil <[e-mail]>
> > +
> > +Media controller drivers:
> > +  Laurent Pinchart <[e-mail]>
> > +
> > +Remote Controllers:
> > +  Sean Young <[e-mail]>
> > +
> > +Sensor drivers:
> > +  Sakari Ailus <[e-mail]>
> > +
> > +V4L2 drivers:
> > +  Hans Verkuil <[e-mail]>
> > +
> > +Submit Checklist Addendum
> > +-------------------------
> > +
> > +There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
> > +that should be used in order to check if the drivers are properly
> > +implementing the media APIs.
> > +
> > +Those tests need to be passed before the patches go upstream.  
> 
> s/need to be passed/need to pass/
> 
> > +
> > +Also, please notice that we build the Kernel with::
> > +
> > +	make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script
> > +
> > +Where the check script is::
> > +
> > +	#!/bin/bash
> > +	/devel/smatch/smatch -p=kernel $@ >&2
> > +	/devel/sparse/sparse $@ >&2
> > +
> > +Be sure to not introduce new warnings on your patches.  
> 
> While static analysers deliver value, I fear this is a high barrier to
> entry for new contributors.

Will change this to:

	Be sure to not introduce new warnings on your patches without a 
	very good reason.

Especially for new contributors, I really expect patches to not introduce
new warnings, as it is usually a lot more painful to fix them later than
to ask devs to do things right at the first place.

> 
> > +
> > +Key Cycle Dates
> > +---------------
> > +
> > +New submissions can be sent at any time, but if they intend to hit the
> > +next merge window they should be sent before -rc5, and ideally stabilized
> > +in the linux-media branch by -rc6.
> > +
> > +Coding Style Addendum
> > +---------------------
> > +
> > +Media development uses checkpatch on strict mode to verify the code style, e. g.::
> > +
> > +	$ ./script/checkpatch.pl --strict  
> 
> But we still accept some warnings. I think this should be documented.

True, but not sure if we should enter into too specific details here.

What about adding something like:

	Please notice that the goal here is to improve code readability. On a few 
	cases, checkpatch may actually point to	something that would look worse. 

	So, you	should use good	send sense here, being prepared to justify any 
	coding style decision.

	Please also notice that, on some cases,	when you fix one issue,	you may
	receive	warnings about lines longer than 80 columns. It	is fine	to have
	longer lines if	this means that	other warnings will be fixed by	that.

	Yet, if	you're having more than	80 columns on a	line, please consider 
	simplifying the	code - if too idented -	or to use shorter names	for 
	variables.

> 
> > +
> > +Review Cadence
> > +--------------
> > +
> > +Provided that your patch is at https://patchwork.linuxtv.org, it should
> > +be sooner or later handled, so you don't need to re-submit a patch.
> > +
> > +Except for bug fixes, we don't usually add new patches to the development
> > +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 on a couple of weeks or to ask  
> 
> s/on a/in a/
> 
> > +other developers to publicly add Reviewed-by and, more importantly,
> > +Tested-by tags.
> > +
> > +Please notice that we expect a detailed description for Tested-by,  
> 
> s/notice/note/
> 
> > +identifying what boards were used at the test and what it was tested.
> > +
> > +Style Cleanup Patches
> > +---------------------
> > +
> > +Style-cleanups are welcome when they come together with other changes
> > +at the files where the style changes will affect.
> > +
> > +We may accept pure standalone style-cleanups, but they should be grouped
> > +per directory. So, for example, if you're doing a cleanup at drivers
> > +under drivers/media, please send a single patch for all drivers under
> > +drivers/media/pci, another one for drivers/media/usb and so on.  
> 
> How about also stating that if the cleanup is low volume, a single patch
> for the whole subsystem is preferred ? I think that should actually be
> the rule, with a split to ease review only when the patch would be too
> big.
> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 7c62b45201d7..e7f44ec05a42 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10077,6 +10077,7 @@ W:	https://linuxtv.org
> >  Q:	http://patchwork.kernel.org/project/linux-media/list/
> >  T:	git git://linuxtv.org/media_tree.git
> >  S:	Maintained
> > +P:	Documentation/media/maintainer-entry-profile.rst
> >  F:	Documentation/devicetree/bindings/media/
> >  F:	Documentation/media/
> >  F:	drivers/media/  
> 

I'm enclosing the diff against the past version. I'll send a final version
once the profiles patchset arrives upstream.

Thanks,
Mauro


diff --git a/Documentation/media/maintainer-entry-profile.rst b/Documentation/media/maintainer-entry-profile.rst
index 81d3b0f9c36a..68d642abe2c1 100644
--- a/Documentation/media/maintainer-entry-profile.rst
+++ b/Documentation/media/maintainer-entry-profile.rst
@@ -4,7 +4,7 @@ Media Subsystem Profile
 Overview
 --------
 
-The media subsystem cover support for a variety of devices: stream
+The media subsystem covers support for a variety of devices: stream
 capture, analog and digital TV, cameras, remote controllers, HDMI CEC
 and media pipeline control.
 
@@ -20,20 +20,20 @@ sync with the API changes. It means that all patches that add new
 features to the subsystem should also bring changes to the corresponding
 API files.
 
-Also, patches for device drivers that changes the Open Firmware/Device
-Tree bindings should be reviewed by the Device Tree maintainers.
+Also, patches that changes the Open Firmware/Device Tree bindings should
+also be reviewed by the Device Tree maintainers.
 
 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 an specific aspect of the subsystem. It is a
-sub-maintainers task to review the patches, providing feedback to users
+sub-maintainer's task to review the patches, providing feedback to users
 if the patches are following the subsystem rules and are properly using
 the media internal and external APIs.
 
 Patches for the media subsystem should be sent to the media mailing list
 at [e-mail] as plain text only e-mail. Emails with
-HTML will be automatically rejected by the mail server. There's no need
-to copy the maintainer or sub-maintainer(s).
+HTML will be automatically rejected by the mail server. It could be wise
+to also copy the sub-maintainer(s).
 
 Media's workflow is heavily based on Patchwork, meaning that, once a patch
 is submitted, it should appear at:
@@ -48,8 +48,8 @@ whitespaces before complaining or submit it again.
 Sub-maintainers
 +++++++++++++++
 
-At the media subsystem, we have a group of senior developers that are
-responsible for doing the code reviews at the drivers (called
+At the media subsystem, we have a group of experienced developers that
+are responsible for doing the code reviews at the drivers (called
 sub-maintainers), and another senior developer responsible for the
 subsystem as a hole. For core changes, whenever possible, multiple
 media (sub-)maintainers do the review.
@@ -82,7 +82,7 @@ There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
 that should be used in order to check if the drivers are properly
 implementing the media APIs.
 
-Those tests need to be passed before the patches go upstream.
+Those tests need to pass before the patches go upstream.
 
 Also, please notice that we build the Kernel with::
 
@@ -94,7 +94,8 @@ Where the check script is::
 	/devel/smatch/smatch -p=kernel $@ >&2
 	/devel/sparse/sparse $@ >&2
 
-Be sure to not introduce new warnings on your patches.
+Be sure to not introduce new warnings on your patches without a
+very good reason.
 
 Key Cycle Dates
 ---------------
@@ -110,6 +111,20 @@ Media development uses checkpatch on strict mode to verify the code style, e. g.
 
 	$ ./script/checkpatch.pl --strict
 
+Please notice that the goal here is to improve code readability. On a few
+cases, checkpatch may actually point to something that would look worse.
+
+So, you should use good send sense here, being prepared to justify any
+coding style decision.
+
+Please also notice that, on some cases, when you fix one issue, you may
+receive warnings about lines longer than 80 columns. It is fine to have
+longer lines if this means that other warnings will be fixed by that.
+
+Yet, if you're having more than 80 columns on a line, please consider
+simplifying the code - if too idented - or to use shorter names for
+variables.
+
 Review Cadence
 --------------
 
@@ -121,11 +136,11 @@ 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 on a couple of weeks or to ask
+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,
 Tested-by tags.
 
-Please notice that we expect a detailed description for Tested-by,
+Please note that we expect a detailed description for Tested-by,
 identifying what boards were used at the test and what it was tested.
 
 Style Cleanup Patches
@@ -134,7 +149,9 @@ Style Cleanup Patches
 Style-cleanups are welcome when they come together with other changes
 at the files where the style changes will affect.
 
-We may accept pure standalone style-cleanups, but they should be grouped
-per directory. So, for example, if you're doing a cleanup at drivers
-under drivers/media, please send a single patch for all drivers under
-drivers/media/pci, another one for drivers/media/usb and so on.
+We may accept pure standalone style-cleanups, but they should ideally
+be one patch for the hole subsystem (if the cleanup is low volume),
+or at least be grouped per directory. So, for example, if you're doing
+big cleanup changes at drivers under drivers/media, please send a single
+patch for all drivers under drivers/media/pci, another one for
+drivers/media/usb and so on.

Laurent Pinchart Sept. 18, 2019, 5:27 p.m. UTC | #7

Hi Mauro,

On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 18 Sep 2019 15:36:20 +0300 Laurent Pinchart escreveu:
> > On Fri, Sep 13, 2019 at 01:19:21PM -0300, Mauro Carvalho Chehab wrote:
> > > Document the basic policies of the media subsystem profile.
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > > ---
> > > 
> > > That's basically a modified version of:
> > >     https://patchwork.linuxtv.org/patch/52999/
> > > 
> > > Applied to the new template
> > > 
> > >  Documentation/media/index.rst                 |   1 +
> > >  .../media/maintainer-entry-profile.rst        | 140 ++++++++++++++++++
> > >  MAINTAINERS                                   |   1 +
> > >  3 files changed, 142 insertions(+)
> > >  create mode 100644 Documentation/media/maintainer-entry-profile.rst
> > > 
> > > diff --git a/Documentation/media/index.rst b/Documentation/media/index.rst
> > > index 0301c25ff887..ac94685b822a 100644
> > > --- a/Documentation/media/index.rst
> > > +++ b/Documentation/media/index.rst
> > > @@ -12,6 +12,7 @@ Linux Media Subsystem Documentation
> > >  .. toctree::
> > >     :maxdepth: 2
> > >  
> > > +   maintainer-entry-profile
> > >     media_uapi
> > >     media_kapi
> > >     dvb-drivers/index
> > > diff --git a/Documentation/media/maintainer-entry-profile.rst b/Documentation/media/maintainer-entry-profile.rst
> > > new file mode 100644
> > > index 000000000000..81d3b0f9c36a
> > > --- /dev/null
> > > +++ b/Documentation/media/maintainer-entry-profile.rst
> > > @@ -0,0 +1,140 @@
> > > +Media Subsystem Profile
> > > +=======================
> > > +
> > > +Overview
> > > +--------
> > > +
> > > +The media subsystem cover support for a variety of devices: stream  
> > 
> > s/cover/covers/
> > 
> > > +capture, analog and digital TV, cameras, remote controllers, HDMI CEC
> > > +and media pipeline control.
> > > +
> > > +It covers, mainly, the contents of those directories:
> > > +
> > > +  - drivers/media
> > > +  - drivers/staging/media
> > > +  - Documentation/media
> > > +  - include/media
> > > +
> > > +Both media userspace and Kernel APIs are documented and should be kept in
> > > +sync with the API changes. It means that all patches that add new
> > > +features to the subsystem should also bring changes to the corresponding
> > > +API files.
> > > +
> > > +Also, patches for device drivers that changes the Open Firmware/Device
> > > +Tree bindings should be reviewed by the Device Tree maintainers.  
> > 
> > You may want to make it clear that this covers bindings only, not driver
> > code that parses the DT. I would just remove "for device drivers", as
> > the rule should also apply to DT bindings documentation that is not
> > driver-specific.
> > 
> > > +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 an specific aspect of the subsystem. It is a
> > > +sub-maintainers task to review the patches, providing feedback to users  
> > 
> > s/sub-maintainers/sub-maintainer's/
> > 
> > > +if the patches are following the subsystem rules and are properly using
> > > +the media internal and external APIs.
> > > +
> > > +Patches for the media subsystem should be sent to the media mailing list
> > > +at [e-mail] as plain text only e-mail. Emails with
> > > +HTML will be automatically rejected by the mail server. There's no need
> > > +to copy the maintainer or sub-maintainer(s).  
> > 
> > There's too much traffic on mailing lists for me to follow everything, I
> > much prefer being CC'ed on patches.
> 
> Well, by using patchwork, the best is to take a look on it at least for
> the patches that you're interested. You could script something using
> pwclient in order to make it easier.
> 
> Anyway, not sure if the other sub-maintainers see the same way. From my side,
> I prefer not to be c/c, as this is just more noise, as I just rely on
> patchwork for media patches. What about changing this to:
> 
> 	Patches for the media subsystem should be sent to the media mailing list
> 	at [e-mail] 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).

That works for me. As this is really a personal preference, is there a
way it could be encoded in MAINTAINERS in a per-person fashion ?
Something that would allow you to opt-out from CC from linux-media (but
possibly opt-in for other parts of the kernel), and allow me to opt-in
for the drivers I maintain ?

> > > +Media's workflow is heavily based on Patchwork, meaning that, once a patch
> > > +is submitted, it should appear at:
> > > +
> > > +   - https://patchwork.linuxtv.org/project/linux-media/list/
> > > +
> > > +If it doesn't automatically appear there after a few minutes, then
> > > +probably something got wrong on your submission. Please check if the
> > > +email is in plain text only and if your emailer is not mangling with
> > > +whitespaces before complaining or submit it again.
> > > +
> > > +Sub-maintainers
> > > ++++++++++++++++
> > > +
> > > +At the media subsystem, we have a group of senior developers that are  
> > 
> > How about "experienced" instead of "senior" ? Quality doesn't always
> > come with age, neither for people nor wine :-)
> > 
> > > +responsible for doing the code reviews at the drivers (called
> > > +sub-maintainers), and another senior developer responsible for the
> > > +subsystem as a hole. For core changes, whenever possible, multiple
> > > +media (sub-)maintainers do the review.
> > > +
> > > +The sub-maintainers work on specific areas of the subsystem, as
> > > +described below:
> > > +
> > > +Digital TV:
> > > +  Sean Young <[e-mail]>
> > > +
> > > +HDMI CEC:
> > > +  Hans Verkuil <[e-mail]>
> > > +
> > > +Media controller drivers:
> > > +  Laurent Pinchart <[e-mail]>
> > > +
> > > +Remote Controllers:
> > > +  Sean Young <[e-mail]>
> > > +
> > > +Sensor drivers:
> > > +  Sakari Ailus <[e-mail]>
> > > +
> > > +V4L2 drivers:
> > > +  Hans Verkuil <[e-mail]>
> > > +
> > > +Submit Checklist Addendum
> > > +-------------------------
> > > +
> > > +There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
> > > +that should be used in order to check if the drivers are properly
> > > +implementing the media APIs.
> > > +
> > > +Those tests need to be passed before the patches go upstream.  
> > 
> > s/need to be passed/need to pass/
> > 
> > > +
> > > +Also, please notice that we build the Kernel with::
> > > +
> > > +	make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script
> > > +
> > > +Where the check script is::
> > > +
> > > +	#!/bin/bash
> > > +	/devel/smatch/smatch -p=kernel $@ >&2
> > > +	/devel/sparse/sparse $@ >&2
> > > +
> > > +Be sure to not introduce new warnings on your patches.  
> > 
> > While static analysers deliver value, I fear this is a high barrier to
> > entry for new contributors.
> 
> Will change this to:
> 
> 	Be sure to not introduce new warnings on your patches without a 
> 	very good reason.
> 
> Especially for new contributors, I really expect patches to not introduce
> new warnings, as it is usually a lot more painful to fix them later than
> to ask devs to do things right at the first place.

I fully agree with the goal, but asking a drive-by contributor, who
usually has to go through issues with sending patches through e-mail
already, to install smatch and sparse and use them, is setting the bar
high. I think automating those checks is the way to go.

> > > +
> > > +Key Cycle Dates
> > > +---------------
> > > +
> > > +New submissions can be sent at any time, but if they intend to hit the
> > > +next merge window they should be sent before -rc5, and ideally stabilized
> > > +in the linux-media branch by -rc6.
> > > +
> > > +Coding Style Addendum
> > > +---------------------
> > > +
> > > +Media development uses checkpatch on strict mode to verify the code style, e. g.::
> > > +
> > > +	$ ./script/checkpatch.pl --strict  
> > 
> > But we still accept some warnings. I think this should be documented.
> 
> True, but not sure if we should enter into too specific details here.
> 
> What about adding something like:
> 
> 	Please notice that the goal here is to improve code readability. On a few 
> 	cases, checkpatch may actually point to	something that would look worse. 
> 
> 	So, you	should use good	send sense here, being prepared to justify any 

s/send sense/sense/

> 	coding style decision.

"being prepared to justify" sounds a bit harsh :-) But the general
message is good.

> 
> 	Please also notice that, on some cases,	when you fix one issue,	you may
> 	receive	warnings about lines longer than 80 columns. It	is fine	to have
> 	longer lines if	this means that	other warnings will be fixed by	that.
> 
> 	Yet, if	you're having more than	80 columns on a	line, please consider 
> 	simplifying the	code - if too idented -	or to use shorter names	for 
> 	variables.

That's already quite specific for my taste. We can keep it as-is if you
think it's fine, but we clearly shouldn't go into more details.

> > > +
> > > +Review Cadence
> > > +--------------
> > > +
> > > +Provided that your patch is at https://patchwork.linuxtv.org, it should
> > > +be sooner or later handled, so you don't need to re-submit a patch.
> > > +
> > > +Except for bug fixes, we don't usually add new patches to the development
> > > +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 on a couple of weeks or to ask  
> > 
> > s/on a/in a/
> > 
> > > +other developers to publicly add Reviewed-by and, more importantly,
> > > +Tested-by tags.
> > > +
> > > +Please notice that we expect a detailed description for Tested-by,  
> > 
> > s/notice/note/
> > 
> > > +identifying what boards were used at the test and what it was tested.
> > > +
> > > +Style Cleanup Patches
> > > +---------------------
> > > +
> > > +Style-cleanups are welcome when they come together with other changes
> > > +at the files where the style changes will affect.
> > > +
> > > +We may accept pure standalone style-cleanups, but they should be grouped
> > > +per directory. So, for example, if you're doing a cleanup at drivers
> > > +under drivers/media, please send a single patch for all drivers under
> > > +drivers/media/pci, another one for drivers/media/usb and so on.  
> > 
> > How about also stating that if the cleanup is low volume, a single patch
> > for the whole subsystem is preferred ? I think that should actually be
> > the rule, with a split to ease review only when the patch would be too
> > big.
> > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 7c62b45201d7..e7f44ec05a42 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -10077,6 +10077,7 @@ W:	https://linuxtv.org
> > >  Q:	http://patchwork.kernel.org/project/linux-media/list/
> > >  T:	git git://linuxtv.org/media_tree.git
> > >  S:	Maintained
> > > +P:	Documentation/media/maintainer-entry-profile.rst
> > >  F:	Documentation/devicetree/bindings/media/
> > >  F:	Documentation/media/
> > >  F:	drivers/media/  
> > 
> 
> I'm enclosing the diff against the past version. I'll send a final version
> once the profiles patchset arrives upstream.
> 
> diff --git a/Documentation/media/maintainer-entry-profile.rst b/Documentation/media/maintainer-entry-profile.rst
> index 81d3b0f9c36a..68d642abe2c1 100644
> --- a/Documentation/media/maintainer-entry-profile.rst
> +++ b/Documentation/media/maintainer-entry-profile.rst
> @@ -4,7 +4,7 @@ Media Subsystem Profile
>  Overview
>  --------
>  
> -The media subsystem cover support for a variety of devices: stream
> +The media subsystem covers support for a variety of devices: stream
>  capture, analog and digital TV, cameras, remote controllers, HDMI CEC
>  and media pipeline control.
>  
> @@ -20,20 +20,20 @@ sync with the API changes. It means that all patches that add new
>  features to the subsystem should also bring changes to the corresponding
>  API files.
>  
> -Also, patches for device drivers that changes the Open Firmware/Device
> -Tree bindings should be reviewed by the Device Tree maintainers.
> +Also, patches that changes the Open Firmware/Device Tree bindings should
> +also be reviewed by the Device Tree maintainers.
>  
>  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 an specific aspect of the subsystem. It is a
> -sub-maintainers task to review the patches, providing feedback to users
> +sub-maintainer's task to review the patches, providing feedback to users
>  if the patches are following the subsystem rules and are properly using
>  the media internal and external APIs.
>  
>  Patches for the media subsystem should be sent to the media mailing list
>  at [e-mail] as plain text only e-mail. Emails with
> -HTML will be automatically rejected by the mail server. There's no need
> -to copy the maintainer or sub-maintainer(s).
> +HTML will be automatically rejected by the mail server. It could be wise
> +to also copy the sub-maintainer(s).
>  
>  Media's workflow is heavily based on Patchwork, meaning that, once a patch
>  is submitted, it should appear at:
> @@ -48,8 +48,8 @@ whitespaces before complaining or submit it again.
>  Sub-maintainers
>  +++++++++++++++
>  
> -At the media subsystem, we have a group of senior developers that are
> -responsible for doing the code reviews at the drivers (called
> +At the media subsystem, we have a group of experienced developers that
> +are responsible for doing the code reviews at the drivers (called
>  sub-maintainers), and another senior developer responsible for the
>  subsystem as a hole. For core changes, whenever possible, multiple
>  media (sub-)maintainers do the review.
> @@ -82,7 +82,7 @@ There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
>  that should be used in order to check if the drivers are properly
>  implementing the media APIs.
>  
> -Those tests need to be passed before the patches go upstream.
> +Those tests need to pass before the patches go upstream.
>  
>  Also, please notice that we build the Kernel with::
>  
> @@ -94,7 +94,8 @@ Where the check script is::
>  	/devel/smatch/smatch -p=kernel $@ >&2
>  	/devel/sparse/sparse $@ >&2
>  
> -Be sure to not introduce new warnings on your patches.
> +Be sure to not introduce new warnings on your patches without a
> +very good reason.
>  
>  Key Cycle Dates
>  ---------------
> @@ -110,6 +111,20 @@ Media development uses checkpatch on strict mode to verify the code style, e. g.
>  
>  	$ ./script/checkpatch.pl --strict
>  
> +Please notice that the goal here is to improve code readability. On a few
> +cases, checkpatch may actually point to something that would look worse.
> +
> +So, you should use good send sense here, being prepared to justify any
> +coding style decision.
> +
> +Please also notice that, on some cases, when you fix one issue, you may
> +receive warnings about lines longer than 80 columns. It is fine to have
> +longer lines if this means that other warnings will be fixed by that.
> +
> +Yet, if you're having more than 80 columns on a line, please consider
> +simplifying the code - if too idented - or to use shorter names for
> +variables.
> +
>  Review Cadence
>  --------------
>  
> @@ -121,11 +136,11 @@ 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 on a couple of weeks or to ask
> +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,
>  Tested-by tags.
>  
> -Please notice that we expect a detailed description for Tested-by,
> +Please note that we expect a detailed description for Tested-by,
>  identifying what boards were used at the test and what it was tested.
>  
>  Style Cleanup Patches
> @@ -134,7 +149,9 @@ Style Cleanup Patches
>  Style-cleanups are welcome when they come together with other changes
>  at the files where the style changes will affect.
>  
> -We may accept pure standalone style-cleanups, but they should be grouped
> -per directory. So, for example, if you're doing a cleanup at drivers
> -under drivers/media, please send a single patch for all drivers under
> -drivers/media/pci, another one for drivers/media/usb and so on.
> +We may accept pure standalone style-cleanups, but they should ideally
> +be one patch for the hole subsystem (if the cleanup is low volume),

s/hole/whole/

> +or at least be grouped per directory. So, for example, if you're doing
> +big cleanup changes at drivers under drivers/media, please send a single
> +patch for all drivers under drivers/media/pci, another one for
> +drivers/media/usb and so on.

Kees Cook Sept. 18, 2019, 5:39 p.m. UTC | #8

On Wed, Sep 18, 2019 at 08:23:26AM -0300, Mauro Carvalho Chehab wrote:
> You can't simply rename MAINTAINERS to .rst and let Sphinx handle it,

Right! Sorry, I meant what you'd suggested earlier: having a script
convert it at doc-build-time.

> The latest RFC about that with I sent was this one:
> 
> 	https://www.mail-archive.com/[e-mail]/msg1238576.html

Cool, I've found it on Lore now too:
https://lore.kernel.org/lkml/[e-mail]/

I think having a script generate the entire thing might be better
(instead of having duplicated instructions at the top of both files),
which I see is what Joe suggested too.

> +.. include:: ../../MAINTAINERS
> +   :literal:

Nah, let's do a full make target as you'd suggested back in that thread.

I'll give it a shot if you don't beat me to it. :)

Mauro Carvalho Chehab Sept. 18, 2019, 6:35 p.m. UTC | #9

Em Wed, 18 Sep 2019 10:39:32 -0700
Kees Cook <[e-mail]> escreveu:

> On Wed, Sep 18, 2019 at 08:23:26AM -0300, Mauro Carvalho Chehab wrote:
> > You can't simply rename MAINTAINERS to .rst and let Sphinx handle it,  
> 
> Right! Sorry, I meant what you'd suggested earlier: having a script
> convert it at doc-build-time.
> 
> > The latest RFC about that with I sent was this one:
> > 
> > 	https://www.mail-archive.com/[e-mail]/msg1238576.html  
> 
> Cool, I've found it on Lore now too:
> https://lore.kernel.org/lkml/[e-mail]/
> 
> I think having a script generate the entire thing might be better
> (instead of having duplicated instructions at the top of both files),
> which I see is what Joe suggested too.

Yes, I agree that having the instructions duplicated was not the best
idea :-)

I guess I did it, on that time, just to experiment with the approach,
but my plan were to avoid the duplication on some future patch.

The big issue, on that point, was the usage of the programoutput
extension. Some devs didn't like using it.


> 
> > +.. include:: ../../MAINTAINERS
> > +   :literal:  
> 
> Nah, let's do a full make target as you'd suggested back in that thread.
> 
> I'll give it a shot if you don't beat me to it. :)

Yeah, please give it a shot. Feel free to reuse anything from my past
approaches if needed. 

I suspect that, if we convert the first part of MAINTAINERS to ReST
syntax, we could add something similar to this to Documentation/Makefile:

$(BUILDDIR)/maintainers.rst:

	awk '{if (match($0,"----------")) exit; print}' MAINTAINERS >$(BUILDDIR)/maintainers.rst
	awk '/----------/{y=1;next}y' MAINTAINERS|sed -E 's,^(\w:),:\1,' >>$(BUILDDIR)/maintainers.rst


Thanks,
Mauro

Mauro Carvalho Chehab Sept. 18, 2019, 6:48 p.m. UTC | #10

Em Wed, 18 Sep 2019 20:27:05 +0300
Laurent Pinchart <[e-mail]> escreveu:

> > Anyway, not sure if the other sub-maintainers see the same way. From my side,
> > I prefer not to be c/c, as this is just more noise, as I just rely on
> > patchwork for media patches. What about changing this to:
> > 
> > 	Patches for the media subsystem should be sent to the media mailing list
> > 	at [e-mail] 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).  
> 
> That works for me. As this is really a personal preference, is there a
> way it could be encoded in MAINTAINERS in a per-person fashion ?
> Something that would allow you to opt-out from CC from linux-media (but
> possibly opt-in for other parts of the kernel), and allow me to opt-in
> for the drivers I maintain ?

I don't think so. Perhaps we could add, instead, something like that at the
sub-maintainers section of the profile.

> > > > +Submit Checklist Addendum
> > > > +-------------------------
> > > > +
> > > > +There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
> > > > +that should be used in order to check if the drivers are properly
> > > > +implementing the media APIs.
> > > > +
> > > > +Those tests need to be passed before the patches go upstream.    
> > > 
> > > s/need to be passed/need to pass/
> > >   
> > > > +
> > > > +Also, please notice that we build the Kernel with::
> > > > +
> > > > +	make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script
> > > > +
> > > > +Where the check script is::
> > > > +
> > > > +	#!/bin/bash
> > > > +	/devel/smatch/smatch -p=kernel $@ >&2
> > > > +	/devel/sparse/sparse $@ >&2
> > > > +
> > > > +Be sure to not introduce new warnings on your patches.    
> > > 
> > > While static analysers deliver value, I fear this is a high barrier to
> > > entry for new contributors.  
> > 
> > Will change this to:
> > 
> > 	Be sure to not introduce new warnings on your patches without a 
> > 	very good reason.
> > 
> > Especially for new contributors, I really expect patches to not introduce
> > new warnings, as it is usually a lot more painful to fix them later than
> > to ask devs to do things right at the first place.  
> 
> I fully agree with the goal, but asking a drive-by contributor, who
> usually has to go through issues with sending patches through e-mail
> already, to install smatch and sparse and use them, is setting the bar
> high. I think automating those checks is the way to go.

Yeah, I have plans to automate it, the same way we did for pull
requests. I'm planning to do that later, after upgrading patchwork
to the current upstream version (with requires upgrading some libraries
too at the server).

In any case, having this at the profile is a reminder for whomever is 
submitting a patch that it should be clean for static analyzers too.

> > > > +Coding Style Addendum
> > > > +---------------------
> > > > +
> > > > +Media development uses checkpatch on strict mode to verify the code style, e. g.::
> > > > +
> > > > +	$ ./script/checkpatch.pl --strict    
> > > 
> > > But we still accept some warnings. I think this should be documented.  
> > 
> > True, but not sure if we should enter into too specific details here.
> > 
> > What about adding something like:
> > 
> > 	Please notice that the goal here is to improve code readability. On a few 
> > 	cases, checkpatch may actually point to	something that would look worse. 
> > 
> > 	So, you	should use good	send sense here, being prepared to justify any   
> 
> s/send sense/sense/

Gah... where this "send" came from??? :-p

> 
> > 	coding style decision.  
> 
> "being prepared to justify" sounds a bit harsh :-) But the general
> message is good.

Any suggestions for a lighter text with similar meaning? :-)

> > 	Please also notice that, on some cases,	when you fix one issue,	you may
> > 	receive	warnings about lines longer than 80 columns. It	is fine	to have
> > 	longer lines if	this means that	other warnings will be fixed by	that.
> > 
> > 	Yet, if	you're having more than	80 columns on a	line, please consider 
> > 	simplifying the	code - if too idented -	or to use shorter names	for 
> > 	variables.  
> 
> That's already quite specific for my taste. We can keep it as-is if you
> think it's fine, but we clearly shouldn't go into more details.

Yeah, I see. Yet, IMHO, we should either not add anything about checkpatch
warnings that might not be relevant or add something similar like the above.

Thanks,
Mauro

Dan Carpenter Sept. 19, 2019, 6:56 a.m. UTC | #11

On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
> > > +Patches for the media subsystem should be sent to the media mailing list
> > > +at [e-mail] as plain text only e-mail. Emails with
> > > +HTML will be automatically rejected by the mail server. There's no need
> > > +to copy the maintainer or sub-maintainer(s).  
> > 
> > There's too much traffic on mailing lists for me to follow everything, I
> > much prefer being CC'ed on patches.
> 
> Well, by using patchwork, the best is to take a look on it at least for
> the patches that you're interested. You could script something using
> pwclient in order to make it easier.
> 
> Anyway, not sure if the other sub-maintainers see the same way. From my side,
> I prefer not to be c/c, as this is just more noise, as I just rely on
> patchwork for media patches. What about changing this to:
> 
> 	Patches for the media subsystem should be sent to the media mailing list
> 	at [e-mail] 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).

The documentation should say "Use get_maintainer.pl" and do what it
says.  Everything else is too complicated.

Occasionally staging maintainers will complain that they aren't CC'd
even though the staging/driver/README says to CC them and I am tell them
"No, the responsibility is for you to add yourself to MAINTAINERS.  It
doesn't matter what the documentation says, you messed up so you need to
stop getting annoyed with newbies for not reading the documentation when
it's your fault you weren't CC'd."

When I sent a patch, I use get_maintainer.pl then I add whoever the
wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
Cook from the CC list because they worked all over the tree and I know
them.  I also normally remove LKML if there is another mailing list but
at least one subsystem uses LKML for patchwork so this isn't safe.

So the safest instructions are "Use get_matainer.pl and add the person
who wrote the commit in the Fixes tag".

regards,
dan carpenter

Dan Carpenter Sept. 19, 2019, 7:08 a.m. UTC | #12

On Wed, Sep 18, 2019 at 03:48:31PM -0300, Mauro Carvalho Chehab wrote:
> Em Wed, 18 Sep 2019 20:27:05 +0300
> Laurent Pinchart <[e-mail]> escreveu:
> 
> > > Anyway, not sure if the other sub-maintainers see the same way. From my side,
> > > I prefer not to be c/c, as this is just more noise, as I just rely on
> > > patchwork for media patches. What about changing this to:
> > > 
> > > 	Patches for the media subsystem should be sent to the media mailing list
> > > 	at [e-mail] 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).  
> > 
> > That works for me. As this is really a personal preference, is there a
> > way it could be encoded in MAINTAINERS in a per-person fashion ?
> > Something that would allow you to opt-out from CC from linux-media (but
> > possibly opt-in for other parts of the kernel), and allow me to opt-in
> > for the drivers I maintain ?
> 
> I don't think so. Perhaps we could add, instead, something like that at the
> sub-maintainers section of the profile.

Of course there is a way to add yourself as a maintainer for a specific
.c file...  Maybe people feel like MAINTAINERS is too crowded?

We could update get_maintainer.pl to grep the .c files for a specific
tag instead of putting everything in a centralized MAINTAINERS file.
But it doesn't make sense to try store that information MY BRAIN!  I
can't remember anything from one minute to the next so I have no idea
who maintains media submodules...

regards,
dan carpenter

Geert Uytterhoeven Sept. 19, 2019, 7:22 a.m. UTC | #13

On Thu, Sep 19, 2019 at 8:57 AM Dan Carpenter <[e-mail]> wrote:
> On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
> > > > +Patches for the media subsystem should be sent to the media mailing list
> > > > +at [e-mail] as plain text only e-mail. Emails with
> > > > +HTML will be automatically rejected by the mail server. There's no need
> > > > +to copy the maintainer or sub-maintainer(s).
> > >
> > > There's too much traffic on mailing lists for me to follow everything, I
> > > much prefer being CC'ed on patches.
> >
> > Well, by using patchwork, the best is to take a look on it at least for
> > the patches that you're interested. You could script something using
> > pwclient in order to make it easier.
> >
> > Anyway, not sure if the other sub-maintainers see the same way. From my side,
> > I prefer not to be c/c, as this is just more noise, as I just rely on
> > patchwork for media patches. What about changing this to:
> >
> >       Patches for the media subsystem should be sent to the media mailing list
> >       at [e-mail] 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).
>
> The documentation should say "Use get_maintainer.pl" and do what it
> says.  Everything else is too complicated.

+1

> When I sent a patch, I use get_maintainer.pl then I add whoever the
> wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> Cook from the CC list because they worked all over the tree and I know
> them.  I also normally remove LKML if there is another mailing list but
> at least one subsystem uses LKML for patchwork so this isn't safe.
>
> So the safest instructions are "Use get_matainer.pl and add the person
> who wrote the commit in the Fixes tag".

Better: perhaps get_maintainer.pl can be taught to add the author of the
commit pointed to by the Fixes tag, if present?

Gr{oetje,eeting}s,

                        Geert

Jani Nikula Sept. 19, 2019, 8:49 a.m. UTC | #14

On Thu, 19 Sep 2019, Geert Uytterhoeven <[e-mail]> wrote:
> On Thu, Sep 19, 2019 at 8:57 AM Dan Carpenter <[e-mail]> wrote:
>> On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
>> When I sent a patch, I use get_maintainer.pl then I add whoever the
>> wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
>> Cook from the CC list because they worked all over the tree and I know
>> them.  I also normally remove LKML if there is another mailing list but
>> at least one subsystem uses LKML for patchwork so this isn't safe.
>>
>> So the safest instructions are "Use get_matainer.pl and add the person
>> who wrote the commit in the Fixes tag".
>
> Better: perhaps get_maintainer.pl can be taught to add the author of the
> commit pointed to by the Fixes tag, if present?

The drm maintainer tools [1] have that, with Cc's and reviewers picked
up, and appropriate Cc: stable added. On a random commit from v5.3:

$ dim fixes 61f7f7c8f978b1c0d80e43c83b7d110ca0496eb4
Fixes: 61f7f7c8f978 ("gpiolib: acpi: Add gpiolib_acpi_run_edge_events_on_boot option and blacklist")
Cc: [e-mail]
Cc: Daniel Drake <[e-mail]>
Cc: Ian W MORRISON <[e-mail]>
Cc: Hans de Goede <[e-mail]>
Cc: Mika Westerberg <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Cc: Bartosz Golaszewski <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: <[e-mail]> # v5.3+

I'm sure it could be massively improved, but it does give you the
initial list that's easy to trim.

BR,
Jani.


[1] https://gitlab.freedesktop.org/drm/maintainer-tools/blob/master/dim#L2398

Geert Uytterhoeven Sept. 19, 2019, 8:58 a.m. UTC | #15

Hi Jani,

On Thu, Sep 19, 2019 at 10:49 AM Jani Nikula <[e-mail]> wrote:
> On Thu, 19 Sep 2019, Geert Uytterhoeven <[e-mail]> wrote:
> > On Thu, Sep 19, 2019 at 8:57 AM Dan Carpenter <[e-mail]> wrote:
> >> On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
> >> When I sent a patch, I use get_maintainer.pl then I add whoever the
> >> wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> >> Cook from the CC list because they worked all over the tree and I know
> >> them.  I also normally remove LKML if there is another mailing list but
> >> at least one subsystem uses LKML for patchwork so this isn't safe.
> >>
> >> So the safest instructions are "Use get_matainer.pl and add the person
> >> who wrote the commit in the Fixes tag".
> >
> > Better: perhaps get_maintainer.pl can be taught to add the author of the
> > commit pointed to by the Fixes tag, if present?
>
> The drm maintainer tools [1] have that, with Cc's and reviewers picked
> up, and appropriate Cc: stable added. On a random commit from v5.3:

Thanks, but that's not scripts/get_maintainer.pl, and restricted to one out
of N subsystems.  Not so dissimilar from what Dan was complaining about.

Gr{oetje,eeting}s,

                        Geert

Mauro Carvalho Chehab Sept. 19, 2019, 9:52 a.m. UTC | #16

Em Thu, 19 Sep 2019 09:56:44 +0300
Dan Carpenter <[e-mail]> escreveu:

> On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
> > > > +Patches for the media subsystem should be sent to the media mailing list
> > > > +at [e-mail] as plain text only e-mail. Emails with
> > > > +HTML will be automatically rejected by the mail server. There's no need
> > > > +to copy the maintainer or sub-maintainer(s).    
> > > 
> > > There's too much traffic on mailing lists for me to follow everything, I
> > > much prefer being CC'ed on patches.  
> > 
> > Well, by using patchwork, the best is to take a look on it at least for
> > the patches that you're interested. You could script something using
> > pwclient in order to make it easier.
> > 
> > Anyway, not sure if the other sub-maintainers see the same way. From my side,
> > I prefer not to be c/c, as this is just more noise, as I just rely on
> > patchwork for media patches. What about changing this to:
> > 
> > 	Patches for the media subsystem should be sent to the media mailing list
> > 	at [e-mail] 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).  
> 
> The documentation should say "Use get_maintainer.pl" and do what it
> says.  Everything else is too complicated.

Works for me.

> Occasionally staging maintainers will complain that they aren't CC'd
> even though the staging/driver/README says to CC them and I am tell them
> "No, the responsibility is for you to add yourself to MAINTAINERS.  It
> doesn't matter what the documentation says, you messed up so you need to
> stop getting annoyed with newbies for not reading the documentation when
> it's your fault you weren't CC'd."
> 
> When I sent a patch, I use get_maintainer.pl then I add whoever the
> wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> Cook from the CC list because they worked all over the tree and I know
> them.  I also normally remove LKML if there is another mailing list but
> at least one subsystem uses LKML for patchwork so this isn't safe.

I use get_maintainer.pl myself, but when submitting some patches
(like documentation ones), I need to manually clean the list, as
it is not uncommon to have a really long c/c list.

> 
> So the safest instructions are "Use get_matainer.pl and add the person
> who wrote the commit in the Fixes tag".
> 
> regards,
> dan carpenter
> 
> 
> _______________________________________________
> Ksummit-discuss mailing list
> [e-mail]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss



Thanks,
Mauro

Jani Nikula Sept. 19, 2019, 9:52 a.m. UTC | #17

On Thu, 19 Sep 2019, Geert Uytterhoeven <[e-mail]> wrote:
> Hi Jani,
>
> On Thu, Sep 19, 2019 at 10:49 AM Jani Nikula <[e-mail]> wrote:
>> On Thu, 19 Sep 2019, Geert Uytterhoeven <[e-mail]> wrote:
>> > On Thu, Sep 19, 2019 at 8:57 AM Dan Carpenter <[e-mail]> wrote:
>> >> On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
>> >> When I sent a patch, I use get_maintainer.pl then I add whoever the
>> >> wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
>> >> Cook from the CC list because they worked all over the tree and I know
>> >> them.  I also normally remove LKML if there is another mailing list but
>> >> at least one subsystem uses LKML for patchwork so this isn't safe.
>> >>
>> >> So the safest instructions are "Use get_matainer.pl and add the person
>> >> who wrote the commit in the Fixes tag".
>> >
>> > Better: perhaps get_maintainer.pl can be taught to add the author of the
>> > commit pointed to by the Fixes tag, if present?
>>
>> The drm maintainer tools [1] have that, with Cc's and reviewers picked
>> up, and appropriate Cc: stable added. On a random commit from v5.3:
>
> Thanks, but that's not scripts/get_maintainer.pl, and restricted to one out
> of N subsystems.  Not so dissimilar from what Dan was complaining about.

The point was, perhaps poorly conveyed, to provide it as a reference,
and something to steal from. You can think of it as a wrapper around
get_maintainer.pl, it's really not subsystem specific, though part of
our scripts, and it'll take you all of five minutes to make it generic
from the source (MIT):

function dim_cite
{
	local sha1

	sha1=${1:?$usage}

	git --git-dir="$DIM_PREFIX/$DIM_REPO/.git" log -1 $sha1 \
	    "--pretty=format:%H (\"%s\")%n" | \
		sed -e 's/\([0-f]\{12\}\)[0-f]*/\1/'
}

function dim_fixes
{
	local sha1 tag

	sha1=${1:?$usage}

	cd $DIM_PREFIX/$DIM_REPO
	echo "Fixes: $(dim_cite $sha1)"

	(
		git show --no-patch $sha1 | \
			sed -e 's/\(Reviewed\|Acked\|Reported\|Signed\)[a-zA-Z-]*-by:/Cc:/' | \
			sed -e 's/^    C[Cc]: */Cc: /' | grep '^Cc: '
		git show $sha1 | scripts/get_maintainer.pl  --email --norolestats --pattern-depth 1 | sed -e "s/^/Cc: /"
	) | awk '!x[$0]++'

	tag=$(git tag --contains $sha1 | grep ^v | sort -V | head -n 1)
	if [[ -n "$tag" ]]; then
		if ! echo "$tag" | grep -q -e "-rc"; then
			echo "Cc: <[e-mail]> # ${tag}+"
		fi
	fi
}


HTH,
Jani.

Joe Perches Sept. 20, 2019, 5:29 a.m. UTC | #18

On Thu, 2019-09-19 at 10:08 +0300, Dan Carpenter wrote:
> On Wed, Sep 18, 2019 at 03:48:31PM -0300, Mauro Carvalho Chehab wrote:
> > Em Wed, 18 Sep 2019 20:27:05 +0300
> > Laurent Pinchart <[e-mail]> escreveu:
> > 
> > > > Anyway, not sure if the other sub-maintainers see the same way. From my side,
> > > > I prefer not to be c/c, as this is just more noise, as I just rely on
> > > > patchwork for media patches. What about changing this to:
> > > > 
> > > > 	Patches for the media subsystem should be sent to the media mailing list
> > > > 	at [e-mail] 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).  
> > > 
> > > That works for me. As this is really a personal preference, is there a
> > > way it could be encoded in MAINTAINERS in a per-person fashion ?
> > > Something that would allow you to opt-out from CC from linux-media (but
> > > possibly opt-in for other parts of the kernel), and allow me to opt-in
> > > for the drivers I maintain ?
> > 
> > I don't think so. Perhaps we could add, instead, something like that at the
> > sub-maintainers section of the profile.
> 
> Of course there is a way to add yourself as a maintainer for a specific
> .c file...  Maybe people feel like MAINTAINERS is too crowded?
> 
> We could update get_maintainer.pl to grep the .c files for a specific
> tag instead of putting everything in a centralized MAINTAINERS file.

Another option is to split the MAINTAINERS file into multiple
distributed files.  get_maintainer.pl already supports that.

https://lwn.net/Articles/730509/
https://lore.kernel.org/lkml/[e-mail]/

> But it doesn't make sense to try store that information MY BRAIN!  I
> can't remember anything from one minute to the next so I have no idea
> who maintains media submodules...

Nor I.  Nor should I have to.

Daniel Vetter Sept. 20, 2019, 2:09 p.m. UTC | #19

On Fri, Sep 20, 2019 at 1:27 PM Joe Perches <[e-mail]> wrote:
>
> On Thu, 2019-09-19 at 10:08 +0300, Dan Carpenter wrote:
> > On Wed, Sep 18, 2019 at 03:48:31PM -0300, Mauro Carvalho Chehab wrote:
> > > Em Wed, 18 Sep 2019 20:27:05 +0300
> > > Laurent Pinchart <[e-mail]> escreveu:
> > >
> > > > > Anyway, not sure if the other sub-maintainers see the same way. From my side,
> > > > > I prefer not to be c/c, as this is just more noise, as I just rely on
> > > > > patchwork for media patches. What about changing this to:
> > > > >
> > > > >         Patches for the media subsystem should be sent to the media mailing list
> > > > >         at [e-mail] 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).
> > > >
> > > > That works for me. As this is really a personal preference, is there a
> > > > way it could be encoded in MAINTAINERS in a per-person fashion ?
> > > > Something that would allow you to opt-out from CC from linux-media (but
> > > > possibly opt-in for other parts of the kernel), and allow me to opt-in
> > > > for the drivers I maintain ?
> > >
> > > I don't think so. Perhaps we could add, instead, something like that at the
> > > sub-maintainers section of the profile.
> >
> > Of course there is a way to add yourself as a maintainer for a specific
> > .c file...  Maybe people feel like MAINTAINERS is too crowded?
> >
> > We could update get_maintainer.pl to grep the .c files for a specific
> > tag instead of putting everything in a centralized MAINTAINERS file.
>
> Another option is to split the MAINTAINERS file into multiple
> distributed files.  get_maintainer.pl already supports that.
>
> https://lwn.net/Articles/730509/
> https://lore.kernel.org/lkml/[e-mail]/

Oh I missed that entirely. Can we roll this out now in subsystems? I'd
really like to move all the gpu related MAINTAINERS entries into
drivers/gpu. The one file in the root is unmangeable big and git blame
takes forever. Ofc splitting it isn't an immediate fix, but long term
should be easier. I thought Linus planned to just do that split (at
least the first level or so) after an -rc1?
-Daniel

> > But it doesn't make sense to try store that information MY BRAIN!  I
> > can't remember anything from one minute to the next so I have no idea
> > who maintains media submodules...
>
> Nor I.  Nor should I have to.
>
>
> _______________________________________________
> Ksummit-discuss mailing list
> [e-mail]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss

Laurent Pinchart Sept. 20, 2019, 2:53 p.m. UTC | #20

On Thu, Sep 19, 2019 at 09:22:45AM +0200, Geert Uytterhoeven wrote:
> On Thu, Sep 19, 2019 at 8:57 AM Dan Carpenter <[e-mail]> wrote:
> > On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
> > > > > +Patches for the media subsystem should be sent to the media mailing list
> > > > > +at [e-mail] as plain text only e-mail. Emails with
> > > > > +HTML will be automatically rejected by the mail server. There's no need
> > > > > +to copy the maintainer or sub-maintainer(s).
> > > >
> > > > There's too much traffic on mailing lists for me to follow everything, I
> > > > much prefer being CC'ed on patches.
> > >
> > > Well, by using patchwork, the best is to take a look on it at least for
> > > the patches that you're interested. You could script something using
> > > pwclient in order to make it easier.
> > >
> > > Anyway, not sure if the other sub-maintainers see the same way. From my side,
> > > I prefer not to be c/c, as this is just more noise, as I just rely on
> > > patchwork for media patches. What about changing this to:
> > >
> > >       Patches for the media subsystem should be sent to the media mailing list
> > >       at [e-mail] 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).
> >
> > The documentation should say "Use get_maintainer.pl" and do what it
> > says.  Everything else is too complicated.
> 
> +1
> 
> > When I sent a patch, I use get_maintainer.pl then I add whoever the
> > wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> > Cook from the CC list because they worked all over the tree and I know
> > them.  I also normally remove LKML if there is another mailing list but
> > at least one subsystem uses LKML for patchwork so this isn't safe.
> >
> > So the safest instructions are "Use get_matainer.pl and add the person
> > who wrote the commit in the Fixes tag".
> 
> Better: perhaps get_maintainer.pl can be taught to add the author of the
> commit pointed to by the Fixes tag, if present?

And remove Kees Cook and Colin King ? :-) Jokes aside, brushing up
get_maintainer.pl a bit is a good idea. I'm for instance not sure adding
LKML automatically is a good idea if other mailing lists are already
CC'ed, as it's a bit of a /dev/null (albeit with logging, so CC'ing it
when no other mailing list is appropriate certainly makes sense).

Doug Anderson Sept. 20, 2019, 2:59 p.m. UTC | #21

Hi,

On Fri, Sep 20, 2019 at 7:54 AM Laurent Pinchart
<[e-mail]> wrote:
>
> On Thu, Sep 19, 2019 at 09:22:45AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Sep 19, 2019 at 8:57 AM Dan Carpenter <[e-mail]> wrote:
> > > On Wed, Sep 18, 2019 at 10:57:28AM -0300, Mauro Carvalho Chehab wrote:
> > > > > > +Patches for the media subsystem should be sent to the media mailing list
> > > > > > +at [e-mail] as plain text only e-mail. Emails with
> > > > > > +HTML will be automatically rejected by the mail server. There's no need
> > > > > > +to copy the maintainer or sub-maintainer(s).
> > > > >
> > > > > There's too much traffic on mailing lists for me to follow everything, I
> > > > > much prefer being CC'ed on patches.
> > > >
> > > > Well, by using patchwork, the best is to take a look on it at least for
> > > > the patches that you're interested. You could script something using
> > > > pwclient in order to make it easier.
> > > >
> > > > Anyway, not sure if the other sub-maintainers see the same way. From my side,
> > > > I prefer not to be c/c, as this is just more noise, as I just rely on
> > > > patchwork for media patches. What about changing this to:
> > > >
> > > >       Patches for the media subsystem should be sent to the media mailing list
> > > >       at [e-mail] 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).
> > >
> > > The documentation should say "Use get_maintainer.pl" and do what it
> > > says.  Everything else is too complicated.
> >
> > +1
> >
> > > When I sent a patch, I use get_maintainer.pl then I add whoever the
> > > wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> > > Cook from the CC list because they worked all over the tree and I know
> > > them.  I also normally remove LKML if there is another mailing list but
> > > at least one subsystem uses LKML for patchwork so this isn't safe.
> > >
> > > So the safest instructions are "Use get_matainer.pl and add the person
> > > who wrote the commit in the Fixes tag".
> >
> > Better: perhaps get_maintainer.pl can be taught to add the author of the
> > commit pointed to by the Fixes tag, if present?
>
> And remove Kees Cook and Colin King ? :-) Jokes aside, brushing up
> get_maintainer.pl a bit is a good idea. I'm for instance not sure adding
> LKML automatically is a good idea if other mailing lists are already
> CC'ed, as it's a bit of a /dev/null (albeit with logging, so CC'ing it
> when no other mailing list is appropriate certainly makes sense).

Please don't do this, as it means the patch won't be findable on the
"LKML" patchwork instance at:

https://lore.kernel.org/patchwork/project/lkml/list/

Having LKML copied on all patches is also nice because it makes it
easier to respond to a patch that was posted to a list you didn't
subscribe to.  I subscribe to LKML and have it redirected to a folder
that I never look at.  Then if I want to find an email thread I can
search that folder and easily respond from within my normal email
client.

Is there any downside to CCing LKML?

-Doug

Jani Nikula Sept. 21, 2019, 8:56 a.m. UTC | #22

On Fri, 20 Sep 2019, Doug Anderson <[e-mail]> wrote:
> On Fri, Sep 20, 2019 at 7:54 AM Laurent Pinchart <[e-mail]> wrote:
>> And remove Kees Cook and Colin King ? :-) Jokes aside, brushing up
>> get_maintainer.pl a bit is a good idea. I'm for instance not sure adding
>> LKML automatically is a good idea if other mailing lists are already
>> CC'ed, as it's a bit of a /dev/null (albeit with logging, so CC'ing it
>> when no other mailing list is appropriate certainly makes sense).
>
> Please don't do this, as it means the patch won't be findable on the
> "LKML" patchwork instance at:
>
> https://lore.kernel.org/patchwork/project/lkml/list/
>
> Having LKML copied on all patches is also nice because it makes it
> easier to respond to a patch that was posted to a list you didn't
> subscribe to.  I subscribe to LKML and have it redirected to a folder
> that I never look at.  Then if I want to find an email thread I can
> search that folder and easily respond from within my normal email
> client.
>
> Is there any downside to CCing LKML?

I think the question becomes, do we want *everything* posted to LKML?

For example, based on the last 30 days, the kernel the monthly addition
to LKML traffic from my corner of the kernel would be in this ballpark:

$ notmuch count date:30d.. to:[e-mail] or to:[e-mail] and not to [e-mail] and subject:PATCH
96904

OTOH LKML is already a firehose that's impossible to drink from, so not
much difference there...

BR,
Jani.

Jonathan Corbet Sept. 21, 2019, 7:13 p.m. UTC | #23

On Wed, 18 Sep 2019 08:23:26 -0300
Mauro Carvalho Chehab <[e-mail]> wrote:

> A simple/lazy solution would be to apply the enclosed patch - or a
> variant of it that would place the contents of MAINTAINERS outside
> process/index.html, and add instructions about how to use
> get_maintainers.pl.
> 
> Jon,
> 
> Please let me know if you're willing to accept something like that.

[Sorry for the slowness, I'm kind of tuned out this week]

I guess we could do that as a short-term thing.

In truth, though, this thing is a database; printing it out linearly is
perhaps not the best thing we could do.  There should be better ways we
could provide access to it.

Also, that file is nearly 18K lines long.  If some unsuspecting person
generates a PDF and prints it, they're going to get something along the
lines of 300 pages of MAINTAINERS, which may not quite be what they had
in mind.  It costs (almost) nothing to put that into HTML output, but
other formats could be painful.

So I dunno, we need to think this through a bit...

jon

Mauro Carvalho Chehab Sept. 21, 2019, 7:45 p.m. UTC | #24

Em Sat, 21 Sep 2019 13:13:07 -0600
Jonathan Corbet <[e-mail]> escreveu:

> On Wed, 18 Sep 2019 08:23:26 -0300
> Mauro Carvalho Chehab <[e-mail]> wrote:
> 
> > A simple/lazy solution would be to apply the enclosed patch - or a
> > variant of it that would place the contents of MAINTAINERS outside
> > process/index.html, and add instructions about how to use
> > get_maintainers.pl.
> > 
> > Jon,
> > 
> > Please let me know if you're willing to accept something like that.  
> 
> [Sorry for the slowness, I'm kind of tuned out this week]
> 
> I guess we could do that as a short-term thing.
> 
> In truth, though, this thing is a database; printing it out linearly is
> perhaps not the best thing we could do.  There should be better ways we
> could provide access to it.

Yeah, as this is a database, instead of just outputting it on a
formatted way, it is possible to generate other types of output.

We could, for example, have an extension with would implement something like:

	.. maintainers:: <subdir>

Which would call get-maintainers in order to parse a subsystem-specific
set of entries and printing the maintainership details.

This could be added at the subsystem-specific profile, for the subsystems
that have it.

> 
> Also, that file is nearly 18K lines long.  If some unsuspecting person
> generates a PDF and prints it, they're going to get something along the
> lines of 300 pages of MAINTAINERS, which may not quite be what they had
> in mind.  It costs (almost) nothing to put that into HTML output, but
> other formats could be painful.

Even if we go for adding a Sphinx tag that would produce a parsed
output for a system-specific profile, we'll still have several other
subsystems that won't have a profile for a while, so I would still 
consider having somewhere an output with its contents. Yeah, someone
might be tempted to print it, but we could place it on a separate PDF,
in order to minimize the risks of someone printing the 300+ pages.

> 
> So I dunno, we need to think this through a bit...


> 
> jon



Thanks,
Mauro

Doug Anderson Sept. 23, 2019, 3:58 p.m. UTC | #25

Hi,

On Sat, Sep 21, 2019 at 1:56 AM Jani Nikula <[e-mail]> wrote:
>
> On Fri, 20 Sep 2019, Doug Anderson <[e-mail]> wrote:
> > On Fri, Sep 20, 2019 at 7:54 AM Laurent Pinchart <[e-mail]> wrote:
> >> And remove Kees Cook and Colin King ? :-) Jokes aside, brushing up
> >> get_maintainer.pl a bit is a good idea. I'm for instance not sure adding
> >> LKML automatically is a good idea if other mailing lists are already
> >> CC'ed, as it's a bit of a /dev/null (albeit with logging, so CC'ing it
> >> when no other mailing list is appropriate certainly makes sense).
> >
> > Please don't do this, as it means the patch won't be findable on the
> > "LKML" patchwork instance at:
> >
> > https://lore.kernel.org/patchwork/project/lkml/list/
> >
> > Having LKML copied on all patches is also nice because it makes it
> > easier to respond to a patch that was posted to a list you didn't
> > subscribe to.  I subscribe to LKML and have it redirected to a folder
> > that I never look at.  Then if I want to find an email thread I can
> > search that folder and easily respond from within my normal email
> > client.
> >
> > Is there any downside to CCing LKML?
>
> I think the question becomes, do we want *everything* posted to LKML?

I swear that I was involved in a conversation in the past where
someone suggested to stop CCing LKML on patches and Jonathan Corbet
jumped in and said that he supported CCing LKML on all patches.  I
searched for that conversation and couldn't find it, so it's possible
I dreamed it.  Maybe he can confirm?


> For example, based on the last 30 days, the kernel the monthly addition
> to LKML traffic from my corner of the kernel would be in this ballpark:
>
> $ notmuch count date:30d.. to:[e-mail] or to:[e-mail] and not to [e-mail] and subject:PATCH
> 96904
>
> OTOH LKML is already a firehose that's impossible to drink from, so not
> much difference there...

Right.  At this point I think LKML is just useful as a dumping ground
and not a place to look for patches or conversations without filters.
It feels fine to keep using it that way.  Having another list (like
ksummit-discuss) for conversations with a higher signal-to-noise ratio
seems like a fine way forward to me.


-Doug

Jonathan Corbet Sept. 23, 2019, 4:04 p.m. UTC | #26

On Mon, 23 Sep 2019 08:58:28 -0700
Doug Anderson <[e-mail]> wrote:

> I swear that I was involved in a conversation in the past where
> someone suggested to stop CCing LKML on patches and Jonathan Corbet
> jumped in and said that he supported CCing LKML on all patches.

I don't *think* I said that, I have no particular reason to argue for
doing that...?  There are people out there who feel that absolutely
everything needs to be on LKML, but I don't really have a strong opinion
on that matter.

jon

Kees Cook Sept. 23, 2019, 10:45 p.m. UTC | #27

On Sat, Sep 21, 2019 at 01:13:07PM -0600, Jonathan Corbet wrote:
> Also, that file is nearly 18K lines long.  If some unsuspecting person
> generates a PDF and prints it, they're going to get something along the
> lines of 300 pages of MAINTAINERS, which may not quite be what they had
> in mind.  It costs (almost) nothing to put that into HTML output, but
> other formats could be painful.

Is this something that can be specifically excluded from the non-HTML
outputs? (Or rather, specifically included in only the HTML output?) I
don't see a way to do that exactly... maybe in my RFC only the html
target would get the "real" file?

Joe Perches Sept. 25, 2019, 5:13 p.m. UTC | #28

On Thu, 2019-09-19 at 09:56 +0300, Dan Carpenter wrote:
> When I sent a patch, I use get_maintainer.pl then I add whoever the
> wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> Cook from the CC list because they worked all over the tree and I know
> them.  I also normally remove LKML if there is another mailing list but
> at least one subsystem uses LKML for patchwork so this isn't safe.
> 
> So the safest instructions are "Use get_matainer.pl and add the person
> who wrote the commit in the Fixes tag".

Maybe add this:

Add the signers of any commit referenced in a "Fixes: <commit>" line
of a patch description.

---
 scripts/get_maintainer.pl | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 5ef59214c555..34085d146fa2 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -26,6 +26,7 @@ my $email = 1;
 my $email_usename = 1;
 my $email_maintainer = 1;
 my $email_reviewer = 1;
+my $email_fixes = 1;
 my $email_list = 1;
 my $email_moderated_list = 1;
 my $email_subscriber_list = 0;
@@ -249,6 +250,7 @@ if (!GetOptions(
 		'r!' => \$email_reviewer,
 		'n!' => \$email_usename,
 		'l!' => \$email_list,
+		'fixes!' => \$email_fixes,
 		'moderated!' => \$email_moderated_list,
 		's!' => \$email_subscriber_list,
 		'multiline!' => \$output_multiline,
@@ -503,6 +505,7 @@ sub read_mailmap {
 ## use the filenames on the command line or find the filenames in the patchfiles
 
 my @files = ();
+my @fixes = ();			# If a patch description includes Fixes: lines
 my @range = ();
 my @keyword_tvi = ();
 my @file_emails = ();
@@ -568,6 +571,8 @@ foreach my $file (@ARGV) {
 		my $filename2 = $2;
 		push(@files, $filename1);
 		push(@files, $filename2);
+	    } elsif (m/^Fixes:\s+([0-9a-fA-F]{6,40})/) {
+		push(@fixes, $1) if ($email_fixes);
 	    } elsif (m/^\+\+\+\s+(\S+)/ or m/^---\s+(\S+)/) {
 		my $filename = $1;
 		$filename =~ s@^[^/]*/@@;
@@ -598,6 +603,7 @@ foreach my $file (@ARGV) {
 }
 
 @file_emails = uniq(@file_emails);
+@fixes = uniq(@fixes);
 
 my %email_hash_name;
 my %email_hash_address;
@@ -612,7 +618,6 @@ my %deduplicate_name_hash = ();
 my %deduplicate_address_hash = ();
 
 my @maintainers = get_maintainers();
-
 if (@maintainers) {
     @maintainers = merge_email(@maintainers);
     output(@maintainers);
@@ -927,6 +932,10 @@ sub get_maintainers {
 	}
     }
 
+    foreach my $fix (@fixes) {
+	vcs_add_commit_signers($fix, "blamed_fixes");
+    }
+
     foreach my $email (@email_to, @list_to) {
 	$email->[0] = deduplicate_email($email->[0]);
     }
@@ -1031,6 +1040,7 @@ MAINTAINER field selection options:
     --roles => show roles (status:subsystem, git-signer, list, etc...)
     --rolestats => show roles and statistics (commits/total_commits, %)
     --file-emails => add email addresses found in -f file (default: 0 (off))
+    --fixes => for patches, add signatures of commits with 'Fixes: <commit>' (default: 1 (on))
   --scm => print SCM tree(s) if any
   --status => print status if any
   --subsystem => print subsystem name if any
@@ -1730,6 +1740,32 @@ sub vcs_is_hg {
     return $vcs_used == 2;
 }
 
+sub vcs_add_commit_signers {
+    return if (!vcs_exists());
+
+    my ($commit, $desc) = @_;
+    my $commit_count = 0;
+    my $commit_authors_ref;
+    my $commit_signers_ref;
+    my $stats_ref;
+    my @commit_authors = ();
+    my @commit_signers = ();
+    my $cmd;
+
+    $cmd = $VCS_cmds{"find_commit_signers_cmd"};
+    $cmd =~ s/(\$\w+)/$1/eeg;	#substitute variables in $cmd
+
+    ($commit_count, $commit_signers_ref, $commit_authors_ref, $stats_ref) = vcs_find_signers($cmd, "");
+    @commit_authors = @{$commit_authors_ref} if defined $commit_authors_ref;
+    @commit_signers = @{$commit_signers_ref} if defined $commit_signers_ref;
+
+    foreach my $signer (@commit_signers) {
+	$signer = deduplicate_email($signer);
+    }
+
+    vcs_assign($desc, 1, @commit_signers);
+}
+
 sub interactive_get_maintainers {
     my ($list_ref) = @_;
     my @list = @$list_ref;

Kees Cook Sept. 25, 2019, 6:40 p.m. UTC | #29

On Wed, Sep 25, 2019 at 10:13:37AM -0700, Joe Perches wrote:
> On Thu, 2019-09-19 at 09:56 +0300, Dan Carpenter wrote:
> > When I sent a patch, I use get_maintainer.pl then I add whoever the
> > wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> > Cook from the CC list because they worked all over the tree and I know
> > them.  I also normally remove LKML if there is another mailing list but
> > at least one subsystem uses LKML for patchwork so this isn't safe.
> > 
> > So the safest instructions are "Use get_matainer.pl and add the person
> > who wrote the commit in the Fixes tag".
> 
> Maybe add this:
> 
> Add the signers of any commit referenced in a "Fixes: <commit>" line
> of a patch description.

Oh yes please! I've always done this manually, so that's a nice bit of
automation. :)

> 
> ---
>  scripts/get_maintainer.pl | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 5ef59214c555..34085d146fa2 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -26,6 +26,7 @@ my $email = 1;
>  my $email_usename = 1;
>  my $email_maintainer = 1;
>  my $email_reviewer = 1;
> +my $email_fixes = 1;
>  my $email_list = 1;
>  my $email_moderated_list = 1;
>  my $email_subscriber_list = 0;
> @@ -249,6 +250,7 @@ if (!GetOptions(
>  		'r!' => \$email_reviewer,
>  		'n!' => \$email_usename,
>  		'l!' => \$email_list,
> +		'fixes!' => \$email_fixes,
>  		'moderated!' => \$email_moderated_list,
>  		's!' => \$email_subscriber_list,
>  		'multiline!' => \$output_multiline,
> @@ -503,6 +505,7 @@ sub read_mailmap {
>  ## use the filenames on the command line or find the filenames in the patchfiles
>  
>  my @files = ();
> +my @fixes = ();			# If a patch description includes Fixes: lines
>  my @range = ();
>  my @keyword_tvi = ();
>  my @file_emails = ();
> @@ -568,6 +571,8 @@ foreach my $file (@ARGV) {
>  		my $filename2 = $2;
>  		push(@files, $filename1);
>  		push(@files, $filename2);
> +	    } elsif (m/^Fixes:\s+([0-9a-fA-F]{6,40})/) {

Is "6" a safe lower bound here? I thought 12 was the way to go?

$ git log | egrep 'Fixes: [a-f0-9]{1,40}' | col2 | awk '{print length }' | sort | uniq -c | sort -n | tail
    238 8
    300 7
    330 14
    344 6
    352 11
    408 40
    425 10
    735 16
   1866 13
  31446 12

Hmpf, 6 is pretty high up there...

> +		push(@fixes, $1) if ($email_fixes);
>  	    } elsif (m/^\+\+\+\s+(\S+)/ or m/^---\s+(\S+)/) {
>  		my $filename = $1;
>  		$filename =~ s@^[^/]*/@@;
> @@ -598,6 +603,7 @@ foreach my $file (@ARGV) {
>  }
>  
>  @file_emails = uniq(@file_emails);
> +@fixes = uniq(@fixes);
>  
>  my %email_hash_name;
>  my %email_hash_address;
> @@ -612,7 +618,6 @@ my %deduplicate_name_hash = ();
>  my %deduplicate_address_hash = ();
>  
>  my @maintainers = get_maintainers();
> -
>  if (@maintainers) {
>      @maintainers = merge_email(@maintainers);
>      output(@maintainers);
> @@ -927,6 +932,10 @@ sub get_maintainers {
>  	}
>      }
>  
> +    foreach my $fix (@fixes) {
> +	vcs_add_commit_signers($fix, "blamed_fixes");
> +    }
> +
>      foreach my $email (@email_to, @list_to) {
>  	$email->[0] = deduplicate_email($email->[0]);
>      }
> @@ -1031,6 +1040,7 @@ MAINTAINER field selection options:
>      --roles => show roles (status:subsystem, git-signer, list, etc...)
>      --rolestats => show roles and statistics (commits/total_commits, %)
>      --file-emails => add email addresses found in -f file (default: 0 (off))
> +    --fixes => for patches, add signatures of commits with 'Fixes: <commit>' (default: 1 (on))

Should "Tested-by" and "Co-developed-by" get added to @signature_tags ?

>    --scm => print SCM tree(s) if any
>    --status => print status if any
>    --subsystem => print subsystem name if any
> @@ -1730,6 +1740,32 @@ sub vcs_is_hg {
>      return $vcs_used == 2;
>  }
>  
> +sub vcs_add_commit_signers {
> +    return if (!vcs_exists());
> +
> +    my ($commit, $desc) = @_;
> +    my $commit_count = 0;
> +    my $commit_authors_ref;
> +    my $commit_signers_ref;
> +    my $stats_ref;
> +    my @commit_authors = ();
> +    my @commit_signers = ();
> +    my $cmd;
> +
> +    $cmd = $VCS_cmds{"find_commit_signers_cmd"};
> +    $cmd =~ s/(\$\w+)/$1/eeg;	#substitute variables in $cmd
> +
> +    ($commit_count, $commit_signers_ref, $commit_authors_ref, $stats_ref) = vcs_find_signers($cmd, "");
> +    @commit_authors = @{$commit_authors_ref} if defined $commit_authors_ref;
> +    @commit_signers = @{$commit_signers_ref} if defined $commit_signers_ref;
> +
> +    foreach my $signer (@commit_signers) {
> +	$signer = deduplicate_email($signer);
> +    }
> +
> +    vcs_assign($desc, 1, @commit_signers);
> +}

@commit_authors is unused?

> +
>  sub interactive_get_maintainers {
>      my ($list_ref) = @_;
>      my @list = @$list_ref;
> 
> 
> _______________________________________________
> Ksummit-discuss mailing list
> [e-mail]
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss

Yay! :)

Geert Uytterhoeven Sept. 26, 2019, 10:25 a.m. UTC | #30

Hi Joe,

On Wed, Sep 25, 2019 at 7:32 PM Joe Perches <[e-mail]> wrote:
> On Thu, 2019-09-19 at 09:56 +0300, Dan Carpenter wrote:
> > When I sent a patch, I use get_maintainer.pl then I add whoever the
> > wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> > Cook from the CC list because they worked all over the tree and I know
> > them.  I also normally remove LKML if there is another mailing list but
> > at least one subsystem uses LKML for patchwork so this isn't safe.
> >
> > So the safest instructions are "Use get_matainer.pl and add the person
> > who wrote the commit in the Fixes tag".
>
> Maybe add this:
>
> Add the signers of any commit referenced in a "Fixes: <commit>" line
> of a patch description.
>
> ---
>  scripts/get_maintainer.pl | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)

Thanks! I gave it a quick try for my first fix after returning from ER, and it
did the right thing.

Gr{oetje,eeting}s,

                        Geert

Joe Perches Sept. 26, 2019, 3:14 p.m. UTC | #31

On Wed, 2019-09-25 at 11:40 -0700, Kees Cook wrote:
> On Wed, Sep 25, 2019 at 10:13:37AM -0700, Joe Perches wrote:
> > On Thu, 2019-09-19 at 09:56 +0300, Dan Carpenter wrote:
> > > When I sent a patch, I use get_maintainer.pl then I add whoever the
> > > wrote the commit from the Fixes tag.  Then I remove Colin King and Kees
> > > Cook from the CC list because they worked all over the tree and I know
> > > them.  I also normally remove LKML if there is another mailing list but
> > > at least one subsystem uses LKML for patchwork so this isn't safe.
> > > 
> > > So the safest instructions are "Use get_matainer.pl and add the person
> > > who wrote the commit in the Fixes tag".
> > 
> > Maybe add this:
> > 
> > Add the signers of any commit referenced in a "Fixes: <commit>" line
> > of a patch description.
> 
> Oh yes please! I've always done this manually, so that's a nice bit of
> automation. :)
> 
> Is "6" a safe lower bound here? I thought 12 was the way to go?
[]
> $ git log | egrep 'Fixes: [a-f0-9]{1,40}' | col2 | awk '{print length }' | sort | uniq -c | sort -n | tail
>     238 8
>     300 7
>     330 14
>     344 6
>     352 11
>     408 40
>     425 10
>     735 16
>    1866 13
>   31446 12
> 
> Hmpf, 6 is pretty high up there...

Yes, but your grep then col2 isn't right.
You are counting all the 'Fixes: commit <foo>' output
as 6 because that's the length of 'commit'.

I also think the length of the hex commit value doesn't
matter much as it's got to be a specific single commit
SHA1 anyway, otherwise the commit id lookup will fail.

> > > @@ -1031,6 +1040,7 @@ MAINTAINER field selection options:
> >      --roles => show roles (status:subsystem, git-signer, list, etc...)
> >      --rolestats => show roles and statistics (commits/total_commits, %)
> >      --file-emails => add email addresses found in -f file (default: 0 (off))
> > +    --fixes => for patches, add signatures of commits with 'Fixes: <commit>' (default: 1 (on))
> 
> Should "Tested-by" and "Co-developed-by" get added to @signature_tags ?

All "<foo>-by:" signatures are added.

> @commit_authors is unused?

Yes, authors are already required to sign-off so
it's just duplicating already existing signatures.

Kees Cook Sept. 26, 2019, 3:53 p.m. UTC | #32

On Thu, Sep 26, 2019 at 08:14:03AM -0700, Joe Perches wrote:
> On Wed, 2019-09-25 at 11:40 -0700, Kees Cook wrote:
> > Is "6" a safe lower bound here? I thought 12 was the way to go?
> []
> > $ git log | egrep 'Fixes: [a-f0-9]{1,40}' | col2 | awk '{print length }' | sort | uniq -c | sort -n | tail
> >     238 8
> >     300 7
> >     330 14
> >     344 6
> >     352 11
> >     408 40
> >     425 10
> >     735 16
> >    1866 13
> >   31446 12
> > 
> > Hmpf, 6 is pretty high up there...
> 
> Yes, but your grep then col2 isn't right.
> You are counting all the 'Fixes: commit <foo>' output
> as 6 because that's the length of 'commit'.

the [a-f0-9]{1,40} already excludes "commit".

> I also think the length of the hex commit value doesn't
> matter much as it's got to be a specific single commit
> SHA1 anyway, otherwise the commit id lookup will fail.

Fail enough. We do already have 6-digit SHA1 collisions, so it seemed
like using more than 6 would be nicer? *shrug* I don't have a strong
opinion. :)

> 
> > > > @@ -1031,6 +1040,7 @@ MAINTAINER field selection options:
> > >      --roles => show roles (status:subsystem, git-signer, list, etc...)
> > >      --rolestats => show roles and statistics (commits/total_commits, %)
> > >      --file-emails => add email addresses found in -f file (default: 0 (off))
> > > +    --fixes => for patches, add signatures of commits with 'Fixes: <commit>' (default: 1 (on))
> > 
> > Should "Tested-by" and "Co-developed-by" get added to @signature_tags ?
> 
> All "<foo>-by:" signatures are added.

Ah, I'd missed where that happened. I do note that's only when
git-all-signature-types is set, which is default 0. (/me goes to add
this to his invocations...)

my $email_git_all_signature_types = 0;
...
    if ($email_git_all_signature_types) {
        $signature_pattern = "(.+?)[Bb][Yy]:";
    } else {
        $signature_pattern = "\(" . join("|", @signature_tags) . "\)";
    }

> > @commit_authors is unused?
> 
> Yes, authors are already required to sign-off so
> it's just duplicating already existing signatures.

Sure, it just seemed odd to populate it if it wasn't going to be used.

Joe Perches Sept. 26, 2019, 4:02 p.m. UTC | #33

On Thu, 2019-09-26 at 08:53 -0700, Kees Cook wrote:
> On Thu, Sep 26, 2019 at 08:14:03AM -0700, Joe Perches wrote:
> > On Wed, 2019-09-25 at 11:40 -0700, Kees Cook wrote:
> > > Is "6" a safe lower bound here? I thought 12 was the way to go?
> > []
> > > $ git log | egrep 'Fixes: [a-f0-9]{1,40}' | col2 | awk '{print length }' | sort | uniq -c | sort -n | tail
> > >     238 8
> > >     300 7
> > >     330 14
> > >     344 6
> > >     352 11
> > >     408 40
> > >     425 10
> > >     735 16
> > >    1866 13
> > >   31446 12
> > > 
> > > Hmpf, 6 is pretty high up there...
> > 
> > Yes, but your grep then col2 isn't right.
> > You are counting all the 'Fixes: commit <foo>' output
> > as 6 because that's the length of 'commit'.
> 
> the [a-f0-9]{1,40} already excludes "commit".

No it doesn't as commit starts with c which matches [a-f0-9]{1,40}

Try your original egrep command line yourself.

Maybe use:

$ git log | egrep 'Fixes: [a-f0-9]{1,40}' | awk '{ if (length($2) == 6) { print $0;} }'

The first few matches are

    the commit referenced in Fixes: below replaced the call to
    Fixes: commit 18a992787896 ("ARM: ux500: move soc_id driver to drivers/soc")
    Fixes: commit 0580dde59438 ("ASoC: simple-card-utils: add asoc_simple_debug_info()")
    Since Fixes: 8c5421c016a4 ("perf pmu: Display pmu name when printing
    Fixes: commit 961fb3c206dc ("ASoC: rockchip: rk3399_gru_sound: don't select unnecessary Platform")

> > I also think the length of the hex commit value doesn't
> > matter much as it's got to be a specific single commit
> > SHA1 anyway, otherwise the commit id lookup will fail.
> 
> Fail enough. We do already have 6-digit SHA1 collisions, so it seemed
> like using more than 6 would be nicer? *shrug* I don't have a strong
> opinion. :)
> 
> > > > > @@ -1031,6 +1040,7 @@ MAINTAINER field selection options:
> > > >      --roles => show roles (status:subsystem, git-signer, list, etc...)
> > > >      --rolestats => show roles and statistics (commits/total_commits, %)
> > > >      --file-emails => add email addresses found in -f file (default: 0 (off))
> > > > +    --fixes => for patches, add signatures of commits with 'Fixes: <commit>' (default: 1 (on))
> > > 
> > > Should "Tested-by" and "Co-developed-by" get added to @signature_tags ?
> > 
> > All "<foo>-by:" signatures are added.
> 
> Ah, I'd missed where that happened. I do note that's only when
> git-all-signature-types is set, which is default 0. (/me goes to add
> this to his invocations...)
> 
> my $email_git_all_signature_types = 0;
> ...
>     if ($email_git_all_signature_types) {
>         $signature_pattern = "(.+?)[Bb][Yy]:";
>     } else {
>         $signature_pattern = "\(" . join("|", @signature_tags) . "\)";
>     }
> 
> > > @commit_authors is unused?
> > 
> > Yes, authors are already required to sign-off so
> > it's just duplicating already existing signatures.
> 
> Sure, it just seemed odd to populate it if it wasn't going to be used.

It's a generic function.

Kees Cook Sept. 26, 2019, 4:24 p.m. UTC | #34

On Thu, Sep 26, 2019 at 09:02:07AM -0700, Joe Perches wrote:
> > the [a-f0-9]{1,40} already excludes "commit".
> 
> No it doesn't as commit starts with c which matches [a-f0-9]{1,40}

Whoops! Yes, sorry, you're right. I needed a trailing whitespace in the
regex.

Patch

diff --git a/Documentation/media/index.rst b/Documentation/media/index.rst
index 0301c25ff887..ac94685b822a 100644
--- a/Documentation/media/index.rst
+++ b/Documentation/media/index.rst
@@ -12,6 +12,7 @@  Linux Media Subsystem Documentation
 .. toctree::
    :maxdepth: 2
 
+   maintainer-entry-profile
    media_uapi
    media_kapi
    dvb-drivers/index
diff --git a/Documentation/media/maintainer-entry-profile.rst b/Documentation/media/maintainer-entry-profile.rst
new file mode 100644
index 000000000000..81d3b0f9c36a
--- /dev/null
+++ b/Documentation/media/maintainer-entry-profile.rst
@@ -0,0 +1,140 @@ 
+Media Subsystem Profile
+=======================
+
+Overview
+--------
+
+The media subsystem cover support for a variety of devices: stream
+capture, analog and digital TV, cameras, remote controllers, HDMI CEC
+and media pipeline control.
+
+It covers, mainly, the contents of those directories:
+
+  - drivers/media
+  - drivers/staging/media
+  - Documentation/media
+  - include/media
+
+Both media userspace and Kernel APIs are documented and should be kept in
+sync with the API changes. It means that all patches that add new
+features to the subsystem should also bring changes to the corresponding
+API files.
+
+Also, patches for device drivers that changes the Open Firmware/Device
+Tree bindings should be reviewed by the Device Tree maintainers.
+
+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 an specific aspect of the subsystem. It is a
+sub-maintainers task to review the patches, providing feedback to users
+if the patches are following the subsystem rules and are properly using
+the media internal and external APIs.
+
+Patches for the media subsystem should be sent to the media mailing list
+at [e-mail] as plain text only e-mail. Emails with
+HTML will be automatically rejected by the mail server. There's no need
+to copy the maintainer or sub-maintainer(s).
+
+Media's workflow is heavily based on Patchwork, meaning that, once a patch
+is submitted, it should appear at:
+
+   - https://patchwork.linuxtv.org/project/linux-media/list/
+
+If it doesn't automatically appear there after a few minutes, then
+probably something got wrong on your submission. Please check if the
+email is in plain text only and if your emailer is not mangling with
+whitespaces before complaining or submit it again.
+
+Sub-maintainers
++++++++++++++++
+
+At the media subsystem, we have a group of senior developers that are
+responsible for doing the code reviews at the drivers (called
+sub-maintainers), and another senior developer responsible for the
+subsystem as a hole. For core changes, whenever possible, multiple
+media (sub-)maintainers do the review.
+
+The sub-maintainers work on specific areas of the subsystem, as
+described below:
+
+Digital TV:
+  Sean Young <[e-mail]>
+
+HDMI CEC:
+  Hans Verkuil <[e-mail]>
+
+Media controller drivers:
+  Laurent Pinchart <[e-mail]>
+
+Remote Controllers:
+  Sean Young <[e-mail]>
+
+Sensor drivers:
+  Sakari Ailus <[e-mail]>
+
+V4L2 drivers:
+  Hans Verkuil <[e-mail]>
+
+Submit Checklist Addendum
+-------------------------
+
+There is a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/
+that should be used in order to check if the drivers are properly
+implementing the media APIs.
+
+Those tests need to be passed before the patches go upstream.
+
+Also, please notice that we build the Kernel with::
+
+	make CF=-D__CHECK_ENDIAN__ CONFIG_DEBUG_SECTION_MISMATCH=y C=1 W=1 CHECK=check_script
+
+Where the check script is::
+
+	#!/bin/bash
+	/devel/smatch/smatch -p=kernel $@ >&2
+	/devel/sparse/sparse $@ >&2
+
+Be sure to not introduce new warnings on your patches.
+
+Key Cycle Dates
+---------------
+
+New submissions can be sent at any time, but if they intend to hit the
+next merge window they should be sent before -rc5, and ideally stabilized
+in the linux-media branch by -rc6.
+
+Coding Style Addendum
+---------------------
+
+Media development uses checkpatch on strict mode to verify the code style, e. g.::
+
+	$ ./script/checkpatch.pl --strict
+
+Review Cadence
+--------------
+
+Provided that your patch is at https://patchwork.linuxtv.org, it should
+be sooner or later handled, so you don't need to re-submit a patch.
+
+Except for bug fixes, we don't usually add new patches to the development
+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 on a couple of weeks or to ask
+other developers to publicly add Reviewed-by and, more importantly,
+Tested-by tags.
+
+Please notice that we expect a detailed description for Tested-by,
+identifying what boards were used at the test and what it was tested.
+
+Style Cleanup Patches
+---------------------
+
+Style-cleanups are welcome when they come together with other changes
+at the files where the style changes will affect.
+
+We may accept pure standalone style-cleanups, but they should be grouped
+per directory. So, for example, if you're doing a cleanup at drivers
+under drivers/media, please send a single patch for all drivers under
+drivers/media/pci, another one for drivers/media/usb and so on.
diff --git a/MAINTAINERS b/MAINTAINERS
index 7c62b45201d7..e7f44ec05a42 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10077,6 +10077,7 @@  W:	https://linuxtv.org
 Q:	http://patchwork.kernel.org/project/linux-media/list/
 T:	git git://linuxtv.org/media_tree.git
 S:	Maintained
+P:	Documentation/media/maintainer-entry-profile.rst
 F:	Documentation/devicetree/bindings/media/
 F:	Documentation/media/
 F:	drivers/media/