From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: James Bottomley <James.Bottomley@hansenpartnership.com>,
ksummit-discuss@lists.linuxfoundation.org
Subject: Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Bug-introducing patches
Date: Wed, 19 Sep 2018 11:26:20 +0300 [thread overview]
Message-ID: <17698287.4NULLKGPAa@avalon> (raw)
In-Reply-To: <alpine.DEB.2.21.1809121531280.9957@nanos.tec.linutronix.de>
Hi Thomas,
On Wednesday, 12 September 2018 17:11:34 EEST Thomas Gleixner wrote:
> On Wed, 12 Sep 2018, Laurent Pinchart wrote:
> > Maintainers are much more than patch juggling monkeys, otherwise they
> > could be replaced by machines. I believe that maintainers are given the
> > huge responsibility of taking care of their community. Fostering a
> > productive work environment, attracting (and keeping) talented developers
> > and reviewers is a huge and honourable task, and gets my full respect. On
> > top of that, if a maintainer has great technical skills, it's even
> > better, and I've learnt a lot from talented maintainers over the time. I
> > however believe that technical skills are not an excuse for not leading
> > by example and showing what the good practices are by applying them.
>
> I surely agree, but reality is different.
>
> I definitely apply my own patches w/o a review tag from time to time. And
> aside of obvious typo cleanups/fixlets, which really can and have to do
> without, all of my patches are posted to LKML and I carefuly respect and
> address review comments.
>
> Though, what am I supposed to do if nothing happens? Repost them five times
> to annoy people? Been there, tried that. Does not help.
Certainly not. I do apply my own patches without Reviewed-by tags from time to
time as well because nobody cared enough about a particular driver to review
the code. When working in corporate environment, or on code that is developed
by a group of people, it gets a bit easier to find interested (or in the
corporate case one could argue coerced) reviewers, but in the general case
it's not always possible. I trust that you will actively try to find a
reviewer for changes you have doubts about yourself, while you won't
necessarily go and ping people for a typo fix.
I don't see anything wrong with this. My point with maintainers privilege was
that maintainers shouldn't bypass the normal review procedure of sending
patches out to public mailing lists, CC'ing the appropriate developers, giving
a bit of time for the review to take place, and incorporating review comments
in new versions. If no reviewer shows up, it's business as usual, but not
different than what happens with other developers than subsystem maintainers,
code can still be merged (I'd love our review rate to be improved, but that's
not specific to maintainers patches, it's a general issue).
As I believe I have mentioned before a case where a maintainer submitted a
patch touching my code, I reviewed it within a few hours, asking for changes,
and the patch was still merged as-is. That's very demotivating for reviewers.
Worse, I've pointed out the problem twice by replying to my original review,
and still haven't received an answer. This is the kind of maintainers
privilege culture that I think isn't acceptable.
> Most of these patches are refactoring and cleanups of the subsystems I
> maintain and I do them for three reasons:
>
> 1) Making the code more maintainable, which in the first place serves the
> egoistic bastard I am, because it makes my life as a maintainer
> simpler in the long run. It also allows others to work easier on top
> of that, which again makes it easier for me to review.
>
> 2) During review of a feature patch submitted by someone else, I notice
> that the code is crap already and the feature adds more crap to it.
>
> So I first try to nudge the submitter to fix that up, but either it's
> outside their expertise level or they are simply telling me: 'I need
> to get this in and cleanup is outside of the scope of my task'.
>
> For the latter, I just refuse to merge it most of the times, but then
> I already identified how it should be done and go back to #1
>
> 3) New hardware, new levels of scale unearth shortcomings in the code. I
> get problem reports and because I deeply care about the stuff I'm
> responsible for, I go and fix it if nobody else cares. Guess what,
> often enough I do not even get a tested-by reply by the people who
> complained in the first place. But with the knowledge of the problem
> and the solution, I would be outright stupid to just put them into
> /dev/null because applying them again makes my life easier.
>
> So again, it's a problem which has to do with the lack of review capacity
> and the lack of people who really care beyond the brim of their teacup.
>
> The 'Make feature X work upstream' task mentality of companies is part of
> the problem along with the expectation, that maintainers will coach,
> educate and babysit their newbies when they have been tasked with problems
> way over their expertise levels. Especially the last part is frustrating
> for everyone. The submitter has worked on this feature for a long time just
> to get it shredded in pieces and then after I got frustrated by the review
> ping pong, I give up and fix it myself in order to have time for other
> things on that ever growing todo list.
>
> This simply cannot scale at all and I'm well aware of it, but I completely
> disagree that this can be fixed by more formalistic processes, gitlab or
> whatever people dream up.
No disagreement here. While gitlab offers interesting features (such as CI
integration), no tool will magically improve our review capacity (a new tool
could cause a marginal influx of new reviewers currently put off by the need
to use e-mail, but I think that in many cases it would be canceled by the
exodus of current reviewers who would be forced to use something else - gerrit
comes to mind, I think that particular tool it could kill the kernel
community).
> It has to be fixed at the mindset level. A code base as large and as
> complex as the kernel needs continous refactoring and cannot be used as
> dumping ground for new features in a drive by mode.
>
> Aside of that, I see people working for large companies doing reviews in
> their spare time, because they care about it. But that's just wrong, they
> should be able to enjoy their spare time as anybody else and get the time
> to review during their work hours.
I've successfully negotiated in the past budget (as in time) with a customer
to review code in subsystems of interest not directly related to the
customer's needs. My main argument was that review was allowing the team to be
recognized as a major actor in the subsystem, and to influence technical
decisions in a direction as favourable as possible for the customer (but not
at the detriment of others of course). This was unfortunately an exception
rather than a rule, but I think that if we could hammer the message in at a
larger scale, there would be hope for improvement.
> I surely encourage people to review things and I offload quite some of the
> work to people who care, but finding them and keeping them on board is hard
> because their daily work just does not allow them to keep up.
>
> I'm definitely open for new ideas and new ways to work, but OTOH I'm not
> interested at all in the 'fix the symptoms' approach and thereby hoping
> that the root cause will cure itself. It simply does not work independent
> of the problem space.
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2018-09-19 8:26 UTC|newest]
Thread overview: 138+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-04 20:16 [Ksummit-discuss] [MAINTAINERS SUMMIT] Bug-introducing patches Sasha Levin
2018-09-04 20:53 ` Daniel Vetter
2018-09-05 14:17 ` Steven Rostedt
2018-09-07 0:51 ` Sasha Levin
2018-09-07 1:09 ` Steven Rostedt
2018-09-07 20:12 ` Greg KH
2018-09-07 21:12 ` Greg KH
2018-09-07 1:09 ` Linus Torvalds
2018-09-07 1:49 ` Sasha Levin
2018-09-07 2:31 ` Linus Torvalds
2018-09-07 2:45 ` Steven Rostedt
2018-09-07 3:43 ` Linus Torvalds
2018-09-07 8:52 ` Daniel Vetter
2018-09-07 8:40 ` Geert Uytterhoeven
2018-09-07 9:07 ` Daniel Vetter
2018-09-07 9:28 ` Geert Uytterhoeven
2018-09-07 17:05 ` Olof Johansson
2018-09-07 14:54 ` Sasha Levin
2018-09-07 15:52 ` Linus Torvalds
2018-09-07 16:17 ` Linus Torvalds
2018-09-07 21:39 ` Mauro Carvalho Chehab
2018-09-09 12:50 ` Stephen Rothwell
2018-09-10 20:05 ` Tony Lindgren
2018-09-10 19:43 ` Sasha Levin
2018-09-10 20:45 ` Steven Rostedt
2018-09-10 21:20 ` Guenter Roeck
2018-09-10 21:46 ` Steven Rostedt
2018-09-10 23:03 ` Eduardo Valentin
2018-09-10 23:13 ` Steven Rostedt
2018-09-11 15:42 ` Steven Rostedt
2018-09-11 17:40 ` Tony Lindgren
2018-09-11 17:47 ` James Bottomley
2018-09-11 18:12 ` Eduardo Valentin
2018-09-11 18:17 ` Geert Uytterhoeven
2018-09-12 15:15 ` Eduardo Valentin
2018-09-11 18:19 ` James Bottomley
2018-09-12 15:17 ` Eduardo Valentin
2018-09-11 18:39 ` Steven Rostedt
2018-09-11 20:09 ` James Bottomley
2018-09-11 20:31 ` Steven Rostedt
2018-09-11 22:53 ` James Bottomley
2018-09-11 23:04 ` Sasha Levin
2018-09-11 23:11 ` James Bottomley
2018-09-11 23:20 ` Sasha Levin
2018-09-12 15:41 ` Eduardo Valentin
2018-09-11 23:22 ` Tony Lindgren
2018-09-11 23:29 ` James Bottomley
2018-09-12 11:55 ` Geert Uytterhoeven
2018-09-12 12:03 ` Laurent Pinchart
2018-09-12 12:29 ` Thomas Gleixner
2018-09-12 12:53 ` Laurent Pinchart
2018-09-12 13:10 ` Alexandre Belloni
2018-09-12 13:30 ` Thomas Gleixner
2018-09-12 23:16 ` Laurent Pinchart
2018-09-12 14:11 ` Thomas Gleixner
2018-09-19 8:26 ` Laurent Pinchart [this message]
2018-09-20 9:02 ` Rafael J. Wysocki
2018-09-20 10:10 ` Laurent Pinchart
2018-09-20 11:00 ` Daniel Vetter
2018-09-20 11:08 ` Laurent Pinchart
2018-09-20 11:49 ` Daniel Vetter
2018-09-12 12:36 ` James Bottomley
2018-09-12 13:38 ` Guenter Roeck
2018-09-12 13:59 ` Tony Lindgren
2018-09-12 10:04 ` Mark Brown
2018-09-12 20:24 ` Steven Rostedt
2018-09-12 20:29 ` Sasha Levin
2018-09-13 0:19 ` Stephen Rothwell
2018-09-13 11:39 ` Mark Brown
2018-09-19 6:27 ` Stephen Rothwell
2018-09-19 17:24 ` Mark Brown
2018-09-19 21:42 ` Stephen Rothwell
2018-09-11 0:49 ` Stephen Rothwell
2018-09-11 1:01 ` Al Viro
2018-09-11 0:47 ` Stephen Rothwell
2018-09-11 17:35 ` Linus Torvalds
2018-09-11 0:43 ` Stephen Rothwell
2018-09-11 16:49 ` Guenter Roeck
2018-09-11 17:47 ` Guenter Roeck
2018-09-11 11:18 ` Mark Brown
2018-09-11 17:02 ` Guenter Roeck
2018-09-11 17:12 ` Jani Nikula
2018-09-11 17:31 ` Mark Brown
2018-09-11 17:41 ` Daniel Vetter
2018-09-11 18:54 ` Mark Brown
2018-09-11 18:03 ` Geert Uytterhoeven
2018-09-11 17:22 ` James Bottomley
2018-09-11 17:56 ` Mark Brown
2018-09-11 18:00 ` James Bottomley
2018-09-11 18:16 ` Mark Brown
2018-09-11 18:07 ` Geert Uytterhoeven
2018-09-12 9:09 ` Dan Carpenter
2018-09-11 17:26 ` Mark Brown
2018-09-11 18:45 ` Steven Rostedt
2018-09-11 18:57 ` Daniel Vetter
2018-09-11 20:15 ` Thomas Gleixner
2018-09-12 9:03 ` Dan Carpenter
2018-09-10 23:01 ` Eduardo Valentin
2018-09-10 23:12 ` Steven Rostedt
2018-09-10 23:32 ` Eduardo Valentin
2018-09-10 23:38 ` Guenter Roeck
2018-09-10 23:38 ` Sasha Levin
2018-09-07 2:33 ` Steven Rostedt
2018-09-07 2:52 ` Guenter Roeck
2018-09-07 14:37 ` Laura Abbott
2018-09-07 15:06 ` Sasha Levin
2018-09-07 15:54 ` Laura Abbott
2018-09-07 16:09 ` Sasha Levin
2018-09-07 20:23 ` Greg KH
2018-09-07 21:13 ` Sasha Levin
2018-09-07 22:27 ` Linus Torvalds
2018-09-07 22:43 ` Guenter Roeck
2018-09-07 22:53 ` Linus Torvalds
2018-09-07 22:57 ` Sasha Levin
2018-09-07 23:52 ` Guenter Roeck
2018-09-08 16:33 ` Greg Kroah-Hartman
2018-09-08 18:35 ` Guenter Roeck
2018-09-10 13:47 ` Mark Brown
2018-09-09 4:36 ` Sasha Levin
2018-09-10 16:20 ` Dan Rue
2018-09-07 21:32 ` Dan Carpenter
2018-09-07 21:43 ` Sasha Levin
2018-09-08 13:20 ` Dan Carpenter
2018-09-10 8:23 ` Jan Kara
2018-09-10 7:53 ` Jan Kara
2018-09-07 3:38 ` Al Viro
2018-09-07 4:27 ` Theodore Y. Ts'o
2018-09-07 5:45 ` Stephen Rothwell
2018-09-07 9:13 ` Daniel Vetter
2018-09-07 11:32 ` Mark Brown
2018-09-07 21:06 ` Mauro Carvalho Chehab
2018-09-08 9:44 ` Laurent Pinchart
2018-09-08 11:48 ` Mauro Carvalho Chehab
2018-09-09 14:26 ` Laurent Pinchart
2018-09-10 22:14 ` Eduardo Valentin
2018-09-07 14:56 ` Sasha Levin
2018-09-07 15:07 ` Jens Axboe
2018-09-07 20:58 ` Mauro Carvalho Chehab
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=17698287.4NULLKGPAa@avalon \
--to=laurent.pinchart@ideasonboard.com \
--cc=James.Bottomley@hansenpartnership.com \
--cc=ksummit-discuss@lists.linuxfoundation.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).