Re: [PATCH] mfd: ucb1x00: remove NO_IRQ check

[Lee Jones]

On Wed, 07 Sep 2016, Russell King - ARM Linux wrote:

> On Wed, Sep 07, 2016 at 11:27:37AM +0100, Lee Jones wrote:
> > On Tue, 06 Sep 2016, Russell King - ARM Linux wrote:
> > 
> > > On Tue, Sep 06, 2016 at 04:45:30PM +0100, Lee Jones wrote:
> > > > On Tue, 06 Sep 2016, Russell King - ARM Linux wrote:
> > > > 
> > > > > You need to send this _to_ me as I need to merge it with my other
> > > > > changes.  This patch on its own does not make sense - it only makes
> > > > > sense with the rest of my SA11x0 patch stack.
> > > > > 
> > > > > NAK for Lee to merge this.
> > > > 
> > > > So if I were to accept this patch, would anything break?  In other
> > > > words, is there an ordering issue where this this change relies on
> > > > something you have in your tree?
> > > > 
> > > 1) This is my driver which I've maintained in the past myself, including
> > >    handling all patches for.  This pre-dates your decision to take over
> > >    the mfd stuff.  I'm still maintaining this driver and I have not
> > >    passed the responsibility to you.
> > 
> > There is no mention of you maintaining this driver in MAINTAINERS.
> > This is the first I've heard of it.  You haven't taken patches for
> > this driver since January 2012 (4 years, 8 months).  Over that period
> > I have accepted 9 patches and conducted more reviews and you haven't
> > taken part or shown any interest whatsoever.
> 
> Maybe I haven't taken any patches because I haven't seen any of these
> patches?
> 
> Yes, you're right that I'm not listed in MAINTAINERS for it, that's
> because it was merged a long time ago when I was maintaining almost
> everything ARM, and listing everything in MAINTAINERS was not really
> viable - there wasn't even the regexp patterns in MAINTAINERS to
> describe what was there.
> 
> Thanks for mentioning it though, I'll add myself so that I get the
> patches in future.

Jolly good.  I'll happily Ack that patch.

I would very much welcome your expertise and review comments, so long
as when finally applied to Mainline, they are merged with the
remainder of the MFD changes.

In the interest of merge-conflict avoidance, this still doesn't mean
you have right to apply patches from a subsystem which is actively
maintained without Maintainer consent though.  If we let everyone do
that there would be chaos, which I am not prepared to tolerate.

> > The *only* logical reason you're making such a fuss now is because of
> > the discussion we had last week.  This behaviour is unacceptable.
> 
> No.  I'm making a fuss because the series you picked it from was not
> ready for you (or anyone) to cherry-pick patches from.  Experienced
> maintainers will _not_ cherry-pick patches from a series, at least

An experienced contributor who is used to working with subsystems
which are maintained by someone other than themselves would have sent
a set which is not ready for anyone to apply as an [RFC] and not a
[PATCH] set.  [PATCH]es are good-to-go.

> without first asking - because there may be non-obvious dependencies
> (which there were) which cherry-picking would break.

You only sent me one patch, and I am not a psychic.  If there were
dependencies, you should have a) sent the whole set, so we could see
what was going on, or b) mentioned that in the cover letter.  We could
have subsequently started to sort something out -- in the form of an
immutable branch.

> Patches are normally sent as a series because there _is_ indeed some
> dependency issue between the patches.  If there is no dependency then
> they would be sent as stand-alone patches.

Wrong.  I receive the majority (almost all in fact) of my patches via
sets which cover multiple subsystems and most of them do not have
*build time* or like this set *API* dependencies and are thus applied
separately.

> Even the sub-series introduction email said:
> 
>   This last part of the series is predominantly a set of cleanup patches
>   removing definitions and files that aren't required, and moving some
>   files out of public view.  The ucb1x00 patch could arguably have
>   been sent in the first set, something that I'll arrange for the next
>   iteration.
> 
> which clearly shows that I did _not_ expect the patch to be picked up -
> if it was picked up, how could I include it in the next iteration of
> the patches.

Then why send it?  That makes no sense whatsoever.

> And the last point is that you were on the Cc line - but I know that
> you take no notice of that what so ever, so maybe I should have omitted
> you completely - but then I'd expect that you'd be whining that you
> didn't receive a personal copy of the patch.

Correct.

> You messed up, plain and simple - I think purposely to make a political
> point given our discussion last week.

This point is tit-for-tat based on what I said.  I'm not going to
substantiate it with an eloquent response.

> > > 2) If you review the driver and consider the effect of the change (which,
> > >    as you don't know the driver, you should have done before replying if
> > >    you're wanting to claim to be responsible for it) you would have
> > 
> > This is why I asked the question.  I doubt any subsystem Maintainer
> > knows the intricacies of every driver they maintain.  We rely on
> > original driver writers and other experts (e.g. assigned vendor
> > engineers and the like) for guidance on the specifics.
> 
> Right, which is why you decided to take my patch because you thought
> it was simple, rather than deferring to me - the author.  You are
> being arrogant.

I took your patch because you send it to me, on its own, with no
*clear* indication of its dependencies or that it shouldn't be taken
as a normal patch.  When decent contributors do this, they normally
send as an [RFC] or a forthcoming message of their intentions.  This
set contained neither.

I took your upstream credibility into account and assumed you knew
what you were doing, (and yes the patch looked trivial,) as I do with
quite a few of my quality contributors.  I'll try not to make that
mistake with you in future.

> > > 4) I find it annoying that you've picked up on this patch in the way
> > >    that you have (as a result of my NAK), yet you have failed to make
> > >    any comment what so ever on _my_ patch, which you were copied with
> > >    on the 30th August.
> > 
> > I'd seriously consider checking your mail filters if I were you (the
> > reply was even addressed To: you, just how you like it.  It took me
> > exactly an hour from you sending the patch to me reviewing and
> > accepting it:
> > 
> >   http://www.spinics.net/lists/arm-kernel/msg527414.html
> 
> Yea, soo quick that you failed to read even the covering message on the
> sub-series.  Maybe you need to slow down a bit?

I certainly don't have time to decrypt hidden meanings in
cover-letters, no.  If you have a request or a message, make it
clear:

"Lee, I intend to take patch X through my tree because Y."

"Okay Russell, no problem.  Can you send me a pull-request when you
do, which I can rebase on in the case of merge-conflict?"

Or, even better in this case, since there is only MFD patch:

"Certainly Russell.  I'll stick the patch on an immutable branch for
you and provide you with a pull-request."

This is how it works for *all* the other Maintainers I work with,
which is quite a few.  I know you have some pretty odd systems and
processes in place, but working with you shouldn't be different to
working with anyone else.

> > It's even in -next, if you'd cared enough to look:
> > 
> >   http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=159aed02b0074bac1c21544befb773dce39b9fcb
> 
> I don't look at -next, sorry.  Anything that needs me to go off and do
> extra work is pretty much a no-no at the moment (and is a no-no at the
> best of times.)  I'm still pretty much working solidly on this series,
> and it remains a work-in-progress - as the purpose of posting it is to
> get feedback and testing - and I've been getting results from people
> testing, which has been provoking further changes to the whole series.
> That's pretty much a full-time job at the moment.

I can certainly empathise with that.

> > > This is part of the problem I have here with your attitude with mfd:
> > > you decided yourself to become maintainer of everything in drivers/mfd,
> > > riding rough-shod over your fellow maintainers.  And when your fellow
> > > maintainers try to reassert control, you get upset about it.  Sorry,
> > > you can't have it both ways.
> > 
> > Not sure what you're trying to say here to be honest.  I offered my
> > help to Sam, the then MFD Maintainer [0], which was subsequently
> > accepted.  He then became busy with NFC, so I took the driver's seat.
> > 
> >   [0] http://www.spinics.net/lists/kernel/msg1525957.html
> > 
> > I try to Maintain MFD in a simple, effective manner, where my
> > priorities are letting good code easily pass through, keeping bad code
> > out and most relevant here, trying to prevent merge-conflicts so Linus
> > has an easy time during the merge-window.
> > 
> > It doesn't make sense for drivers which reside in the same subsystem
> > to be applied to multiple trees.  I like for all changes to MFD to be
> > reflected in the MFD pull-request.  If they *need* to go in via other
> > repos *as well*, that's fine too.  I am usually very happy to provide
> > pull-requests when those needs arise.
> > 
> > If you are insistent on applying this yourself, so long as you have a
> > *technical* (rather than territorial/spiteful) reason for doing so, I,
> > as always, will be happy to oblige you with a pull-request too.
> 
> I think I've already pointed out the technical argument here, and why
> your attitude towards total ownership of MFD is broken.

I disagree.  It's been going well for over 3 years now.

> The fact of the matter is that the ucb1x00 drivers (by your own statement
> above) receive very few patches, so the argument of avoiding conflicts
> doesn't really apply.

With *this* driver maybe.  But providing exceptions to the rule is a
slippery slope.  It's simpler if we work as a team.  Give the driver
experts a chance to comment, in cases of in-depth issues.  Then funnel
the changes through one repo.

Why do you insist of being different to everyone else?

> > > Now, if you had discussed my patch from the 30th _first_ then I would
> > > not be NAKing this patch, and I probably would not be making such a
> > > big deal about this.  But let's put that aside and stick to the
> > > technicalities.
> > 
> > I think this *rant* of yours might be based on very shakey foundations
> > then, because I *did* reply to your patch.  From what I saw, it was a
> > reasonable patch, so I took it.
> > 
> > Maybe if you would have known the driver as well as you profess to,
> > you would have fixed the issue properly and there would be no need for
> > this patch in the first place.  I tease of course!  ;)
> 
> As I said above, the whole series (of some 60-90 patches) is still very
> much a work in progress.  It took two days to post the whole series
> (29th and 30th August) so getting all the details into every message
> wasn't practical.  However, there's enough of a clue there: the size
> of the overall series,

The size of the over-all series, as far as my vision goes was 8.  And
the other 7 patches were based in arm/*.  It's *very* rare for patches
to be required to to into MFD and ARM simultaneously.

> the comments in the sub-series cover message,

... were cryptic.

> the fact that earlier cover message had RFC in, over sub-series cover
> messages were asking for acks, etc.

You didn't send anything to me with "RFC", and no mention of this
being part of a collection of series'.  I think you've made way too
many assumptions here and not been forthcoming or clear at all.  You
only have yourself to blame!

> I don't expect you to read all those, but I do expect you to read at
> least the cover message to which the patch you took was attached to.

I did.  It made one cryptic reference to a subsequent set, and that
was it.  I'll not reiterate, please see my comments above.

> > > There are two dependencies here.  Right now, the probe_irq_on()
> > > returning zero on SA11x0 platforms, causing ucb1x00_detect_irq() to
> > > return NO_IRQ, which then causes the probe function to fail.  Whether
> > > that happens on PXA or not, I don't know and I have no way to test
> > > anymore as I donated all my PXA platforms to Robert.
> > > 
> > > Applying Arnd's patch on its own, or applying my patch on its own will
> > > cause ucb1x00 to initialise with either NO_IRQ or 0 in ucb->irq, which
> > > causes the device to have no interrupts - or worse, it screws up any
> > > configuration of IRQ0 that the platform may have (eg, PXA).  Applying
> > > both together results in the probe function continuing to fail.
> > > 
> > > My patch is not supposed to be applied on its own; it is supposed to
> > > be applied with the third patch already in place - a GPIO patch.  So
> > > yes, there _were_ dependencies here.
> > > 
> > > That dependency can be solved by taking _both_ my patch (first) and
> > > Arnd's patch.  However, given that each patch introduces its own
> > > different form of breakage, it makes sense to combine the two patches
> > > to avoid that breakage.  So, when I'm less busy sorting out other
> > > SA11x0 stuff, I want to discuss with Arnd about combining them into
> > > a single patch.
> > > 
> > > _Then_ we need to have a discussion about how to handle the patch.
> > 
> > Good explanation.  Just the sort of information I'd expect during a
> > patch review.  Much more helpful than "this patch doesn't make
> > sense.  Give it to me.  NACK".  Now I can work with you both to
> > resolve this issue properly and professionally.
> 
> That was to stop you blindly taking it based on our conversation last
> week - and at that point I hadn't realised you'd inappropriately
> cherry-picked my previous patch.  I haven't really been following
> email because I've been working on this series.

Who's fault is that?  I think you need to start taking resposibilty
for your own lack for forthcomingness and lack of clarity with regards
to your intentions.

> > So, since I already have half of the resolution, all I need to do is
> > squash this into it, right?  Or do we need to carry this GPIO patch
> > too?  If so, where is that currently?
> 
> So, let's apply your maintainership style to GPIO... and let's say
> that LinusW just applied the patch within hours because it "looked
> simple enough".  Then what?  The dependencies are split between two
> trees, potentially with other patches behind them.
> 
> No, LinusW didn't do that.  Linus read the cover message, realised
> that it was part of a larger series, and decided not to take the
> patch, because there was a huge number of dependent patches on top
> of it touching _many_ different subsystems.
> 
> Now, consider that difference between two maintainer behaviours -
> yours and LinusW's.  One is co-operative, working with the patch
> submitter, eyes open to issues that may be caused by taking patches.
> The other is arrogant, with an attitude of the maintainer knows
> better than the submitter.  I can work with LinusW, but I seemingly
> can't work with you.

That's because you didn't cock-up when you sent your set to Linus.
You had "[RFC]" in the subject line and you were *very* forthcoming
with the fact that this is a set of sets and with your intentions.

Go read them again and take a look at the differences.

You have no one to blame but yourself.

> As a result of your actions, I'm now having to ask LinusW to take
> the GPIO patch and get it into mainline ASAP, preferably for -rc6,
> so at least the straight-line tips of LinusT's tree don't end up
> with weirdness - and so I can rebase my entire series on top of
> -rc6 (or maybe later) to avoid conflicts.

That's your choice.  It's not the only (or best) way to solve this.

I work with Linus all the time.  We use the methods I described.
Works well.

> It's all far from ideal, and it all stems from your arrogant actions.

Wrong!  Read up!

> I hope, in future, that you'll discuss with your submitters if you
> want to cherry-pick patches out of a series before doing so to
> avoid creating problems like this, and avoiding giving other people
> more work.

This is the first issue I've had like this, and I totally put blame in
the way you submitted the set.  Read up, inwardly digest and hopefully
it will help you to provide better submissions in the future!

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog