Message-ID: <20160907150846.GA4921@dell>
Date:   Wed, 7 Sep 2016 16:08:46 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Hans-Christian Egtvedt <egtvedt@...fundet.no>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mfd: ucb1x00: remove NO_IRQ check

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

> On Wed, Sep 07, 2016 at 01:48:18PM +0100, Lee Jones wrote:
> > On Wed, 07 Sep 2016, Russell King - ARM Linux wrote:
> > 
> > > On Wed, Sep 07, 2016 at 11:27:37AM +0100, Lee Jones wrote:
> > > > 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.
> 
> Right, and with the default being [PATCH], and not [RFC], it got sent
> as [PATCH] BY MISTAKE which is something I already covered in the email
> you are replying to. I'm not going to repeat that yet again, just
> because you think that repeating the same point several times in your
> reply makes it any more valid than the first time.

Mistakes happen.  I'm fine with that.  I'm only taking exception
because you are trying to put the blame on me, when clearly you are
the one at fault.

> > > 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.
> 
> You got the cover letter, and I quoted a bit from it which clearly
> indicated that I was not intending the patch to be applied.

That's my point.  You did *not* do that.

> Only
> someone with a political point to make, or someone being obtuse
> would ignore that.

Tit-for-tat - I already accused you of that.  I had no agenda.  Just
maintaining MFD the same was as I always do.  I think we know who's
trying to make the political point here!

> > > 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.
> 
> That is where you are totally and utterly wrong.  Sorry, but you are
> definitely completely wrong on that.
> 
> If they are stand-alone, then they should not be sent as a series.

I think the issue here is the definition of dependant.  Because this
is MFD, and it's entire reason for being is to register and interact
with leaf drivers, almost all of my submitters send patches as sets
which contain changes to said leaf drivers.  A lot of times they are
"related" but not necessarily "dependant" (as described before,
remember; *build* and *API* are the only type of dependency I  will
accept for cross-subsystem patches/sets).

More often than not when people send sets, it's because they patches
are functionally related, but do not depend on each other per say.
Another example of this is when people send changes to files in
drivers/ and also send their DTS counterparts in the same set.  It's
*very* common, but certainly does not mean that all the patches should
go through one repo.

Disagree all you like, but that's just the way it is.

> > > 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.
> 
> Oh for fuck sake, what the hell is up with you.

Take control of yourself Russell.  It's just an email.

> It got sent for REVIEW COMMENTS and TESTING for people like Robert
> Jarzmik and Adam, to get some sense as to the _entire_ series
> acceptability to people.  This is a _massive_ series, and it's still
> growing.  The series is now at more than 100 patches.

We've already covered the fact that you should have sent it as an
[RFC].  None of this would have happened if you'd done so.  Let's
leave it at that.

OOI, why are you doing this massive overhaul for next to zero users?
Isn't that a massive waste of your valuable time?

> > > 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.
> 
> So you've just admitted that I can't win - if I don't send you a copy
> of a patch you should be notified about, you whine.  If I send you a
> copy, it's liable to get applied even when it shouldn't be.
> 
> Sorry, I can't work with you if that's how you're going to operate.

That's not what I've been saying is it?

What I'm saying is; follow the processes;

[A]
 - Send me the patch
 - Be forthcoming with your intentions
 - We'll work something out between us using commonly used processes

If the only option for you is to make changes all over the tree and
only you take them in, then that is not going to work!

> > > > > 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.
> 
> It was _NOT_ sent on it's own.  You're making this up, trying to
> stupidly justify your idiotic position.  What's really funny is
> that everyone on linux-arm-kernel can see that you're talking
> total crap here.

I'm telling you outright -- you sent *me* one patch with a
cover-letter which did not state your intentions and was not an RFC:

30 2016 Russell King - AR (  0) [PATCH 0/8] SA11x0/PXA remainder & cleanups
30 2016 Russell King      (  0) └>[PATCH 1/8] mfd: ucb1x00: allow IRQ probing to work with IRQs > 32

Take responsibility for your own actions.

> So the fact that (a) you were copied on the cover letter to the
> sub-series, (b) that the patch had a numeric patch number in the
> subject line with a numeric total of patches didn't make you
> _think_ for one bit that it was part of a series?
> 
> How could such a change be "on its own"?

We've been over this.  As I already mentioned, most people send me
sets and most of the time they aren't *build* or *api* dependent,
so they can normally be applied on their own.

I also looked at the subjects of the other patches (because that's all
you sent me), and noticed that all of the remaining patches were ARM
patches.  Which do not normally have decencies with drivers/.  And
when they do, the contributor mentions the fact so we can deal with
that accordingly.  You did not do that.

As I'm sure you can appreciate, this is a very abnormal submission and
you supplied me with no valid information to that fact, so I treated
it as a normal submission.  I would have never let you apply the patch
on your own, for reasons I've already explained.

> > > 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:
> 
> Look, (and I've had enough of your crap) the cover letter was _very_
> clear that this particular patch should have been included _EARLIER_
> in the overall series, and that I was _GOING_ to resend it.
> 
> That's not cryptic.  You're making this up as you're going along.

That's the problem, it was not clear, at all.  You said you "could
have arguably applied it earlier in the set".  But without knowing
that this wasn't a stand-alone set (how could I, you didn't mention
that), what does the really mean?

As a comparison, a *clear* message would be like the one I quoted to
you in the last message.  I believe it's further down from here.  Take
a look.

> > > 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?
> 
> Because you've proven to be incompetent in this instance.

Insults aren't going to get you anywhere with me I'm afraid.

It is you who have admitted making mistakes.  The fallout are a direct
result of *your* incompetence and failure to understand the rules.

Other Maintainers might allow you to subvert their processes and do
your own thing, but I'm not willing to tolerate that kind of nonsense.
Not even from you!

> > > 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.
> 
> You clearly have not read it.  Subsequent?  No, none of the cover
> messages made any reference to a follow-on set of patches.
> 
> Let me re-quote it again, for the hard of understanding (aka thick)

Yawn!

> with underlining for the relevant part:
> 
>    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.
>    ^^^^^^^^^^
> 
> The reason that was added is because by the time I'd got to posting
> the series, I realised that I should have moved it earlier in the
> overall series, near to the GPIO patch which it depended on.  However,

  -------

This is the point you are missing!  *You* made no reference to a
larger "overall" series, or the fact that this was a "sub-set".  If
you can get your head around that, you can start to understand where
*you* went wrong.

The fact of the matter is, because *you* didn't provide the relevant
information with regards to your intent, it made your comment
cryptic.

I sincerely believe that that if you made it clear what was going on,
there would be no crossed wires and we wouldn't be having this lovely
conversation.

> as that had been sent the previous day, I wasn't about to reshuffle
> the patches and start the process from the very beginning again.
> 
> Now, to help your English comprehension:
> 
> - "The ucb1x00 patch could arguably have been sent in the first set"
>    -- oops I made a mistake, I'm acknowledging that I made a mistake
>       here
> 
> - "something that I'll arrange for the next iteration."
>    -- an indication that I will fix the mistake when I resend the
>       _entire_ series.
> 
> There's nothing cryptic there.  You're just being difficult, trying
> to protect your position with absurd arguments.

Wrong again.  Read up to save me for labouring points.

> > > 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.
> 
> Stop trying to protect your position with absurd arguments.

Nothing absurd about them.

I am happy with the way I have operated given the information I had
at the time.  I'm pretty sure there is little I could have
*reasonably* done to prevent this, besides reading L(A)KML.

> > > 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.
> 
> You are right and you are wrong.  The _cover_ letter only had RFC,
> not the patches - which was an oversight and a mistake on my part.
> 
> However, rather than making stuff up, you ought to actually read
> some emails.  Linus didn't just offer an ack, Linus asked _how_
> the series was going to be dealt with - something that you failed
> to do.  So that actually makes you wrong.

See my list above, I'm going to mark it with a [A] so you can find
it.  If you would have completed those items, as any reasonable
contributor would, asking you what your plans are is *exactly* what I
would have done as part of that process.

You completed that list with Linus, and he acted accordingly.  You did
*not* do that in the MFD case, thus mine and Linus' actions would be
different, wouldn't they?  I'm sure Linus' would have done the same as
me if he received the same submission.

> In fact, Linus realised (as you will see if you read his reply) that
> the patches were probably going to have to go through a tree other
> than his own.

Are you reading what I write, because this statement would assume not.

The submission you sent me what not at the same *quality* level as the
one you sent Linus.  I guarantee you Linus and I would have acted the
same way with the same submission.

If there is any doubt, I always ask submitters what their intentions
are.  That level of doubt was not evident in your somewhat lacking
submission.

> > > 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!
> 
> No, you need to change your behaviour.
> 
> I acknowledge that I made mistakes while posting the patch set (I
> even acknowledged there were mistakes _while_ posting the set) but
> I still maintain that this entire situation is of your making
> because you think that you have a right to cherry-pick from patch
> series as you please without first talking to the submitter or
> reading the cover message.

Sorry, you're wrong here.  I can reference 100's of patches which were
part of sets that were merged on their own without asking. That's just
the nature of MFD and their leaf drivers.

I grant you the submissions you see are different to the ones I have
to deal with, but take my word for it, I receive *way* more patch sets
that are "related", but not {build,API} "dependant", than I do the
other way around.

> Everyone I have worked with has asked how the series is going to be
> delt with before merging any of it.  If people have wanted to take
> a patch from a series, they have always asked first.

We've covered this.  If you with to divert from the accepted process,
you need be more forthcoming in the description.

> Anyway, I'm starting to repeat myself, so it's time to draw this
> exchange to a close.  No, I'm not changing my position, so don't
> try to ask me to "digest" what you've said - as far as I'm concerned
> you are in the wrong on this subject, and you're not going to change
> my view on that, sorry.

Then I guess we'll have to agree to disagree.

> What I want from you is an acknowledgement that you will avoid
> cherry-picking from any of my patch series again without first
> asking the question - like everyone else does.

If you allude to dependencies in the set like everyone else does,
absolutely!

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

Powered by blists - more mailing lists