Re: [PATCH v2] PCI: imx6: Check for link training status in link up check

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Nov 02, 2018 at 12:21:13PM +0000, Lorenzo Pieralisi wrote:
> On Fri, Nov 02, 2018 at 12:23:12AM +0000, Trent Piepho wrote:
> > On Thu, 2018-11-01 at 13:55 +0000, Lorenzo Pieralisi wrote:
> > > On Wed, Oct 31, 2018 at 07:49:59PM +0000, Trent Piepho wrote:
> > > > This fixes a regression introduced in merge 562df5c8521e.
> > > 
> > > A merge being a commits collection, the regression was certainly
> > > introduced by a specific commit in it, not the merge itself.
> > 
> > In this case it really is merge commit.
> > 
> > While the problem and fix are relatively obvious, finding how it came
> > to be broken was a challenge.  Most of the commit message is my proof
> > that this is a bug and not something done intentionally, by tracking
> > back the complicated route that caused the code to be in its current
> > state.
> > 
> > Basically there are two commits, on both branches of the merge, neither
> > of which caused a problem nor were they incorrect in any way at the
> > time they were committed.  When that merge combined this collection of
> > multiple commits, it did not do so correctly and that is the point a
> > bug appeared.
> > 
> > Or put another way, this problem was already fixed some time ago, but
> > in the merge commit the fix was dropped.
> > 
> > It seems like the proper way to attribute my fix is to the merge, as
> > that is what caused the regression.  There is no prior commit where one
> > can observe the regression.
> 
> You are right, I went through git history and I agree with your
> summary, that's what happened.
> 
> > > Also for the Fixes: tag and all references.
> > 
> > Would it be ok to refer to the commit this way the first time, then use
> > a shortened method for subsequent usages?  Otherwise talking about two
> > commits becomes very long.  This it what I have mostly done, other than
> > the "Fixes" line.  I'm surprised checkpatch didn't catch that.
> 
> I would like to ask Bjorn's opinion on this, I do not know if adding
> a Fixes: tag with a merge commit in it is common practice but that
> summarizes what happened so I assume it should be fine.

That sounds good to me.

> As for the shortened commit format I understand your point, I do not
> know if that's "official" from a kernel process standpoint.

It took me a while to figure out what '886 meant because it's unusual and
the "'" usually goes where the *missing* characters are.  Personally I
would use the full 

  886bc5ceb5cc ("PCI: designware: Add generic dw_pcie_wait_for_link()")

at the first reference and just 886bc5ceb5cc for subsequent references.

Thanks for chasing this down!  It's really a hassle to figure out things
that work separately but not together.

Bjorn