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

[Russell King - ARM Linux]

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.

> 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
without first asking - because there may be non-obvious dependencies
(which there were) which cherry-picking would break.

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.

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.

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.

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

> > 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.

> > 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?

> 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.

> > 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.

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.

> > 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 comments in the sub-series cover message,
the fact that earlier cover message had RFC in, over sub-series cover
messages were asking for acks, etc.

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.

> > 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.

> 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.

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.

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

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.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.